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", "