Skip to content

Prevent script injection in the public task form and bound access to it - #3

Open
Klaas-Ritense wants to merge 2 commits into
mainfrom
bugfix/584-publictask-form-xss-and-unbounded-access
Open

Prevent script injection in the public task form and bound access to it#3
Klaas-Ritense wants to merge 2 commits into
mainfrom
bugfix/584-publictask-form-xss-and-unbounded-access

Conversation

@Klaas-Ritense

Copy link
Copy Markdown
Member

https://github.com/generiekzaakafhandelcomponent/atlas-internal/issues/584

A value from the case data could contain markup that the public task page ran as script, because the
prefilled form was written straight into a script block. The form definition is now placed in a JSON
data block that the page reads instead of executes, and the page escapes its content by default.

The page that shows the form also had no end date: anyone with the link could keep retrieving the
prefilled form after the task was submitted and after the end date had passed, while submitting the
form was already blocked. Showing and submitting now apply the same check.

The public task id moved from the query string into the address path. Links that were already sent out
keep working through the old form, which is deprecated.

On the escaping, since it is easy to get wrong

Three independent layers, not alternatives:

  1. The payload moved into <script id="form-io-form" type="application/json"> and is read with
    JSON.parse. It is emitted with every < escaped as <, not just </ — in JSON a <
    can only occur inside a string literal, where < means the same thing, so this is loss-free
    and also closes the <!-- script-data-escaped variant.
  2. FreemarkerConfig now sets an HTML output format, so every other interpolation in this and any
    future template is escaped by default.
  3. public_task_url sits in a JavaScript string literal, where HTML escaping would be wrong (an &
    would become &amp; inside raw text), so it uses ?js_string.

?json_string was deliberately not used on the payload: the value is a whole JSON document, not a
string being placed inside a literal, so it would escape the document's own quotes and break
JSON.parse.

Verified in a browser

A regex assertion does not prove how an HTML parser behaves, so the rendered page was checked directly:
before the fix document.querySelectorAll('img') returned 1 — a live <img src=x onerror=alert(1)>
with the rest of the script leaking into the body text. After, 0 images, 4 intact script elements, and
the value round-trips to exactly </script><img src=x onerror=alert(1)> as inert data, with Formio
running against the parsed JSON. 8 of the 11 new tests fail against the unpatched code; the 3 that pass
are the correct controls.

Affected releases: all 6 published com.ritense.valtimoplugins:publictask versions (1.0.0, 1.1.0,
2.0.0, 2.0.1, 2.1.0, 2.1.1). No released version contains a fix.

Left for a product decision: the render path is not single-use, only bounded by completion and
expiry. Making it single-use would lock out a recipient who reloads, loses their connection mid-form or
opens the link on a second device. The residual risk is that a leaked link stays usable until the task
is submitted or expires — now a bounded window rather than forever.

Also note the deprecated query-string route keeps the Referer/proxy-log exposure alive for links
already issued; that is the deliberate cost of not breaking them.

backend/plugin/build.gradle.kts gains three testImplementation entries. The main sources use
compileOnly, which test compilation does not inherit, so the service could not be unit-tested at all
before this. Those jars were already on the test runtime classpath, so runtime behaviour is unchanged.

Not verified: the full flow was not exercised against a running stack; the escaping is confirmed in a
browser against the rendered page, the rest by unit and integration tests.

The generated public task page interpolated the prefilled Form.io definition
straight into a script block, with no auto-escaping configured, so a field
value containing "</script>" could execute script on the Valtimo origin. The
form definition is now emitted as a JSON data block, FreeMarker escapes every
other interpolation by default, and the submit URL is encoded for the
JavaScript string literal it sits in.

The GET that renders the form also applied no expiry or completion check,
while the submit path did, so the prefilled form stayed retrievable
indefinitely by anyone holding the link. Both paths now share one availability
check, the public task id travels in the path instead of the query string
(the query form is kept, deprecated, so links already sent out keep working),
and the public task id is no longer written to the debug log.
Backend and frontend versions are released together, so they must match even when
the change itself is backend-only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant