Prevent script injection in the public task form and bound access to it - #3
Open
Klaas-Ritense wants to merge 2 commits into
Open
Prevent script injection in the public task form and bound access to it#3Klaas-Ritense wants to merge 2 commits into
Klaas-Ritense wants to merge 2 commits into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
<script id="form-io-form" type="application/json">and is read withJSON.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-freeand also closes the
<!--script-data-escaped variant.FreemarkerConfignow sets an HTML output format, so every other interpolation in this and anyfuture template is escaped by default.
public_task_urlsits in a JavaScript string literal, where HTML escaping would be wrong (an&would become
&inside raw text), so it uses?js_string.?json_stringwas deliberately not used on the payload: the value is a whole JSON document, not astring 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 Formiorunning 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:publictaskversions (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 linksalready issued; that is the deliberate cost of not breaking them.
backend/plugin/build.gradle.ktsgains threetestImplementationentries. The main sources usecompileOnly, which test compilation does not inherit, so the service could not be unit-tested at allbefore 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.