From 2aa0f66fca84813fe646c8038bd57e07742cb8ca Mon Sep 17 00:00:00 2001 From: Klaas Schuijtemaker Date: Tue, 11 Aug 2026 15:33:22 +0200 Subject: [PATCH 1/2] Prevent script injection in the public task form and bound access to it 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 "" 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/plugin/build.gradle.kts | 3 + backend/plugin/plugin.properties | 2 +- .../htmlrenderer/config/FreemarkerConfig.kt | 4 + .../publictask/service/PublicTaskService.kt | 44 +++-- .../publictask/web/rest/PublicTaskResource.kt | 25 ++- .../config/template/public_task_html.ftl | 13 +- .../service/PublicTaskHtmlTemplateTest.kt | 120 ++++++++++++++ .../service/PublicTaskServiceTest.kt | 151 ++++++++++++++++++ .../web/rest/PublicTaskResourceIT.kt | 9 ++ documentation/plugin.md | 16 ++ documentation/release-notes.md | 3 + 11 files changed, 372 insertions(+), 18 deletions(-) create mode 100644 backend/plugin/src/test/kotlin/com/ritense/valtimoplugins/publictask/htmlrenderer/service/PublicTaskHtmlTemplateTest.kt create mode 100644 backend/plugin/src/test/kotlin/com/ritense/valtimoplugins/publictask/service/PublicTaskServiceTest.kt diff --git a/backend/plugin/build.gradle.kts b/backend/plugin/build.gradle.kts index 80c5807..ef7413e 100644 --- a/backend/plugin/build.gradle.kts +++ b/backend/plugin/build.gradle.kts @@ -53,10 +53,13 @@ dependencies { // Testing testImplementation("com.ritense.valtimo:building-block") + testImplementation("com.ritense.valtimo:core") + testImplementation("com.ritense.valtimo:form") testImplementation("com.ritense.valtimo:plugin") testImplementation("com.ritense.valtimo:temporary-resource-storage") testImplementation("com.ritense.valtimo:test-utils-common") + testImplementation("org.springframework.boot:spring-boot-starter-data-jpa") testImplementation("org.springframework.boot:spring-boot-starter-test") testImplementation("org.postgresql:postgresql") diff --git a/backend/plugin/plugin.properties b/backend/plugin/plugin.properties index 753b277..213ef0c 100644 --- a/backend/plugin/plugin.properties +++ b/backend/plugin/plugin.properties @@ -1,3 +1,3 @@ pluginGroupId=com.ritense.valtimoplugins pluginArtifactId=publictask -pluginVersion=2.1.1 +pluginVersion=2.1.2 diff --git a/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/htmlrenderer/config/FreemarkerConfig.kt b/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/htmlrenderer/config/FreemarkerConfig.kt index c8e4ff2..8107f58 100644 --- a/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/htmlrenderer/config/FreemarkerConfig.kt +++ b/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/htmlrenderer/config/FreemarkerConfig.kt @@ -17,6 +17,7 @@ package com.ritense.valtimoplugins.publictask.htmlrenderer.config import freemarker.cache.ClassTemplateLoader +import freemarker.core.HTMLOutputFormat import freemarker.template.Configuration import freemarker.template.TemplateExceptionHandler @@ -25,5 +26,8 @@ class FreemarkerConfig : Configuration(VERSION_2_3_31) { templateLoader = ClassTemplateLoader(javaClass, "/config/template") defaultEncoding = Charsets.UTF_8.toString() templateExceptionHandler = TemplateExceptionHandler.RETHROW_HANDLER + // Templates render HTML that is served to the public, so every interpolation is HTML escaped unless the + // template asks for a different encoder. Without this, a value taken from case data can inject markup. + outputFormat = HTMLOutputFormat.INSTANCE } } diff --git a/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/service/PublicTaskService.kt b/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/service/PublicTaskService.kt index 0384bf6..1024409 100644 --- a/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/service/PublicTaskService.kt +++ b/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/service/PublicTaskService.kt @@ -34,6 +34,7 @@ import org.springframework.http.HttpStatus import org.springframework.http.ResponseEntity import org.springframework.web.util.UriComponentsBuilder import java.time.LocalDate +import java.time.format.DateTimeParseException import java.util.UUID class PublicTaskService( @@ -66,9 +67,7 @@ class PublicTaskService( } fun createPublicTaskHtml(publicTaskId: UUID): ResponseEntity { - val publicTaskEntity = - publicTaskRepository.findById(publicTaskId).orElse(null) - ?: return TASK_NOT_AVAILABLE_ERROR + val publicTaskEntity = findAvailablePublicTask(publicTaskId) ?: return TASK_NOT_AVAILABLE_ERROR val formHtml = try { @@ -96,13 +95,7 @@ class PublicTaskService( publicTaskId: UUID, submission: JsonNode, ): ResponseEntity { - val publicTaskEntity = - publicTaskRepository.findById(publicTaskId).orElse(null) - ?: return TASK_NOT_AVAILABLE_ERROR - - if (LocalDate.parse(publicTaskEntity.taskExpirationDate).isBefore(LocalDate.now())) { - return TASK_NOT_AVAILABLE_ERROR - } + val publicTaskEntity = findAvailablePublicTask(publicTaskId) ?: return TASK_NOT_AVAILABLE_ERROR val operatonTask = try { @@ -137,11 +130,36 @@ class PublicTaskService( return ResponseEntity("Your response has been submitted", HttpStatus.OK) } + /** + * Returns the public task only while it is still available: not completed through the public form and not past + * its expiration date. Both showing the form and submitting it go through this, so that the two cannot drift + * apart and leave the form - which contains case data - retrievable for longer than the task itself lives. + */ + private fun findAvailablePublicTask(publicTaskId: UUID): PublicTaskEntity? = + publicTaskRepository + .findById(publicTaskId) + .orElse(null) + ?.takeIf { it.isAvailable() } + + private fun PublicTaskEntity.isAvailable(): Boolean { + if (isCompletedByPublicTask) { + return false + } + val expirationDate = + try { + LocalDate.parse(taskExpirationDate) + } catch (e: DateTimeParseException) { + logger.warn(e) { "Public task has an unusable expiration date and is treated as expired" } + return false + } + return !expirationDate.isBefore(LocalDate.now()) + } + private fun publicTaskUrl(publicTaskId: UUID): String = UriComponentsBuilder .fromUriString(baseUrl.removeSuffix("/")) .path(PUBLIC_TASK_URL) - .queryParam("publicTaskId", publicTaskId) + .pathSegment(publicTaskId.toString()) .build() .toUriString() @@ -157,7 +175,9 @@ class PublicTaskService( isCompletedByPublicTask = publicTaskData.isCompletedByPublicTask, ), ).also { - logger.debug { "Saved public task entity $it" } + // Only the user task is logged: the public task id is what grants access to the form, so it must + // not end up in log files. + logger.debug { "Saved public task entity for user task ${it.userTaskId}" } } } diff --git a/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/web/rest/PublicTaskResource.kt b/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/web/rest/PublicTaskResource.kt index ab47a45..ccf5a1b 100644 --- a/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/web/rest/PublicTaskResource.kt +++ b/backend/plugin/src/main/kotlin/com/ritense/valtimoplugins/publictask/web/rest/PublicTaskResource.kt @@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.JsonNode import com.ritense.valtimoplugins.publictask.service.PublicTaskService import org.springframework.http.ResponseEntity import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.PathVariable import org.springframework.web.bind.annotation.PostMapping import org.springframework.web.bind.annotation.RequestBody import org.springframework.web.bind.annotation.RequestMapping @@ -32,13 +33,31 @@ import java.util.UUID class PublicTaskResource( private val publicTaskService: PublicTaskService, ) { - @GetMapping + @GetMapping("/{publicTaskId}") fun sendPublicTaskHtml( - @RequestParam publicTaskId: UUID, + @PathVariable publicTaskId: UUID, ): ResponseEntity = publicTaskService.createPublicTaskHtml(publicTaskId) - @PostMapping + @PostMapping("/{publicTaskId}") fun completeUserTask( + @PathVariable publicTaskId: UUID, + @RequestBody submission: JsonNode, + ): ResponseEntity = publicTaskService.completeUserTaskWithPublicTaskSubmission(publicTaskId, submission) + + /** + * Kept so that public task links which were sent out before the id moved into the path keep working. New links + * use the path form, because an id in the query string ends up in Referer headers, proxy logs and browser + * history. + */ + @Deprecated("Use GET /api/v1/public-task/{publicTaskId}") + @GetMapping(params = ["publicTaskId"]) + fun sendPublicTaskHtmlForQueryParameter( + @RequestParam publicTaskId: UUID, + ): ResponseEntity = publicTaskService.createPublicTaskHtml(publicTaskId) + + @Deprecated("Use POST /api/v1/public-task/{publicTaskId}") + @PostMapping(params = ["publicTaskId"]) + fun completeUserTaskForQueryParameter( @RequestParam publicTaskId: UUID, @RequestBody submission: JsonNode, ): ResponseEntity = publicTaskService.completeUserTaskWithPublicTaskSubmission(publicTaskId, submission) diff --git a/backend/plugin/src/main/resources/config/template/public_task_html.ftl b/backend/plugin/src/main/resources/config/template/public_task_html.ftl index 92b5441..9faa94c 100644 --- a/backend/plugin/src/main/resources/config/template/public_task_html.ftl +++ b/backend/plugin/src/main/resources/config/template/public_task_html.ftl @@ -50,16 +50,25 @@
+<#-- + The form definition is emitted as a data block instead of as a JavaScript literal, so that its content is never + parsed as script. HTML escaping would be the wrong escaping here - the content of a script element is raw text, + where entities are not decoded - so the value is marked as no_esc and encoded for this context instead: in JSON a + '<' can only occur inside a string literal, where "\u003C" means exactly the same thing. Replacing every '<' + therefore keeps the JSON intact while removing every sequence (" + " } + ] + } + """.trimIndent() + + val html = render(formIoForm) + + // No '<' survives in the data block, so nothing in it can close the element or open an HTML comment. + assertThat(jsonDataBlockOf(html)).doesNotContain("<") + assertThat(html).doesNotContain("\\u003Cimg src=x onerror=alert(1)>") + } + + @Test + fun `a valid form definition is still readable as json after escaping`() { + val formIoForm = + objectMapper + .createObjectNode() + .put("quotes", """He said "hi" and \ left""") + .put("unicode", "Ruben ë ç 😀") + .put("newlines", "line one\nline two\ttabbed") + .put("slashes", "https://example.org/a/b?c=d&e=f") + .put("markup", "