From c423d7cd8eeb7672f01d295d5ccf5cf6b511c236 Mon Sep 17 00:00:00 2001 From: Gauthier Roebroeck Date: Fri, 7 Feb 2025 16:34:30 +0800 Subject: [PATCH] refactor(api): add validation on client-settings api --- komga/build.gradle.kts | 1 + .../api/rest/ClientSettingsController.kt | 31 ++- .../api/rest/dto/ClientSettingDto.kt | 3 + .../api/rest/ClientSettingsControllerTest.kt | 206 ++++++++++++++++++ 4 files changed, 237 insertions(+), 4 deletions(-) diff --git a/komga/build.gradle.kts b/komga/build.gradle.kts index 11073a928..c4ac82784 100644 --- a/komga/build.gradle.kts +++ b/komga/build.gradle.kts @@ -145,6 +145,7 @@ tasks { freeCompilerArgs = listOf( "-Xjsr305=strict", + "-Xemit-jvm-type-annotations", "-opt-in=kotlin.time.ExperimentalTime", ) } diff --git a/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/ClientSettingsController.kt b/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/ClientSettingsController.kt index 3423d385e..b0921248a 100644 --- a/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/ClientSettingsController.kt +++ b/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/ClientSettingsController.kt @@ -1,5 +1,8 @@ package org.gotson.komga.interfaces.api.rest +import jakarta.validation.Valid +import jakarta.validation.constraints.NotNull +import jakarta.validation.constraints.Pattern import org.gotson.komga.infrastructure.jooq.main.ClientSettingsDtoDao import org.gotson.komga.infrastructure.security.KomgaPrincipal import org.gotson.komga.interfaces.api.rest.dto.ClientSettingDto @@ -9,6 +12,7 @@ import org.springframework.http.HttpStatus import org.springframework.http.MediaType import org.springframework.security.access.prepost.PreAuthorize import org.springframework.security.core.annotation.AuthenticationPrincipal +import org.springframework.validation.annotation.Validated import org.springframework.web.bind.annotation.DeleteMapping import org.springframework.web.bind.annotation.GetMapping import org.springframework.web.bind.annotation.PatchMapping @@ -17,8 +21,11 @@ import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.ResponseStatus import org.springframework.web.bind.annotation.RestController +private const val KEY_REGEX = """[a-z]+(?:\.[a-z]+)*""" + @RestController @RequestMapping(value = ["api/v1/client-settings"], produces = [MediaType.APPLICATION_JSON_VALUE]) +@Validated class ClientSettingsController( private val clientSettingsDtoDao: ClientSettingsDtoDao, ) { @@ -36,7 +43,12 @@ class ClientSettingsController( @ResponseStatus(HttpStatus.NO_CONTENT) @PreAuthorize("hasRole('ADMIN')") fun saveGlobalSetting( - @RequestBody newSettings: Map, + @RequestBody newSettings: Map< + @Pattern(regexp = KEY_REGEX) + String, + @NotNull @Valid + ClientSettingGlobalUpdateDto, + >, ) { newSettings.forEach { (key, setting) -> clientSettingsDtoDao.saveGlobal(key, setting.value, setting.allowUnauthorized) @@ -47,7 +59,12 @@ class ClientSettingsController( @ResponseStatus(HttpStatus.NO_CONTENT) fun saveUserSetting( @AuthenticationPrincipal principal: KomgaPrincipal, - @RequestBody newSettings: Map, + @RequestBody newSettings: Map< + @Pattern(regexp = KEY_REGEX) + String, + @NotNull @Valid + ClientSettingUserUpdateDto, + >, ) { newSettings.forEach { (key, setting) -> clientSettingsDtoDao.saveForUser(principal.user.id, key, setting.value) @@ -57,7 +74,10 @@ class ClientSettingsController( @DeleteMapping("global") @ResponseStatus(HttpStatus.NO_CONTENT) fun deleteGlobalSetting( - @RequestBody keysToDelete: Set, + @RequestBody keysToDelete: Set< + @Pattern(regexp = KEY_REGEX) + String, + >, ) { clientSettingsDtoDao.deleteGlobalByKeys(keysToDelete) } @@ -66,7 +86,10 @@ class ClientSettingsController( @ResponseStatus(HttpStatus.NO_CONTENT) fun deleteGlobalSetting( @AuthenticationPrincipal principal: KomgaPrincipal, - @RequestBody keysToDelete: Set, + @RequestBody keysToDelete: Set< + @Pattern(regexp = KEY_REGEX) + String, + >, ) { clientSettingsDtoDao.deleteByUserIdAndKeys(principal.user.id, keysToDelete) } diff --git a/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/dto/ClientSettingDto.kt b/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/dto/ClientSettingDto.kt index 896b5e075..b227fca0b 100644 --- a/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/dto/ClientSettingDto.kt +++ b/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/dto/ClientSettingDto.kt @@ -1,6 +1,7 @@ package org.gotson.komga.interfaces.api.rest.dto import com.fasterxml.jackson.annotation.JsonInclude +import jakarta.validation.constraints.NotBlank @JsonInclude(JsonInclude.Include.NON_NULL) data class ClientSettingDto( @@ -9,10 +10,12 @@ data class ClientSettingDto( ) data class ClientSettingGlobalUpdateDto( + @get:NotBlank val value: String, val allowUnauthorized: Boolean, ) data class ClientSettingUserUpdateDto( + @get:NotBlank val value: String, ) diff --git a/komga/src/test/kotlin/org/gotson/komga/interfaces/api/rest/ClientSettingsControllerTest.kt b/komga/src/test/kotlin/org/gotson/komga/interfaces/api/rest/ClientSettingsControllerTest.kt index cba606e53..bf1d78200 100644 --- a/komga/src/test/kotlin/org/gotson/komga/interfaces/api/rest/ClientSettingsControllerTest.kt +++ b/komga/src/test/kotlin/org/gotson/komga/interfaces/api/rest/ClientSettingsControllerTest.kt @@ -8,12 +8,15 @@ import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.BeforeAll import org.junit.jupiter.api.Nested import org.junit.jupiter.api.Test +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.ValueSource import org.springframework.beans.factory.annotation.Autowired import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc import org.springframework.boot.test.context.SpringBootTest import org.springframework.http.MediaType import org.springframework.security.test.context.support.WithAnonymousUser import org.springframework.test.web.servlet.MockMvc +import org.springframework.test.web.servlet.delete import org.springframework.test.web.servlet.get import org.springframework.test.web.servlet.patch @@ -155,6 +158,100 @@ class ClientSettingsControllerTest( jsonPath("$.setting.value") { value("value") } } } + + @ParameterizedTest + @ValueSource( + strings = [ + "UPPERCASE", + " ", + "", + "symbols!", + "two..dots", + ".start.with.dot", + "end.with.dot.", + ], + ) + @WithMockCustomUser(id = "user1") + fun `given non-admin user when updating user settings with invalid key then validation error is thrown`(key: String) { + //language=JSON + val jsonString = + """ + { + "$key": { + "value": "value" + } + } + """.trimIndent() + + mockMvc + .patch("/api/v1/client-settings/user") { + contentType = MediaType.APPLICATION_JSON + content = jsonString + }.andExpect { + status { isBadRequest() } + } + } + + @ParameterizedTest + @ValueSource( + strings = [ + "UPPERCASE", + " ", + "", + "symbols!", + "two..dots", + ".start.with.dot", + "end.with.dot.", + ], + ) + @WithMockCustomUser(roles = ["ADMIN"]) + fun `given non-admin user when deleting user settings with invalid key then validation error is thrown`(key: String) { + //language=JSON + val jsonString = + """ + ["$key"] + """.trimIndent() + + mockMvc + .delete("/api/v1/client-settings/user") { + contentType = MediaType.APPLICATION_JSON + content = jsonString + }.andExpect { + status { isBadRequest() } + } + } + + @ParameterizedTest + @ValueSource( + strings = [ + //language=JSON + "null", + //language=JSON + """{"value": null }""", + //language=JSON + """{ "value": " " }""", + //language=JSON + """{ "value": "" }""", + ], + ) + @WithMockCustomUser(id = "user1") + fun `given non-admin user when updating user settings with invalid value then validation error is thrown`(value: String) { + //language=JSON + val jsonString = + """ + { + "key": $value + } + """.trimIndent() + + mockMvc + .patch("/api/v1/client-settings/user") { + contentType = MediaType.APPLICATION_JSON + content = jsonString + }.andExpect { + status { isBadRequest() } + } + } } @Nested @@ -208,5 +305,114 @@ class ClientSettingsControllerTest( jsonPath("$.setting.allowUnauthorized") { value(false) } } } + + @ParameterizedTest + @ValueSource( + strings = [ + "UPPERCASE", + " ", + "", + "symbols!", + "two..dots", + ".start.with.dot", + "end.with.dot.", + ], + ) + @WithMockCustomUser(roles = ["ADMIN"]) + fun `given admin user when updating global settings with invalid key then validation error is thrown`(key: String) { + //language=JSON + val jsonString = + """ + { + "$key": { + "value": "value", + "allowUnauthorized": false + } + } + """.trimIndent() + + mockMvc + .patch("/api/v1/client-settings/global") { + contentType = MediaType.APPLICATION_JSON + content = jsonString + }.andExpect { + status { isBadRequest() } + } + } + + @ParameterizedTest + @ValueSource( + strings = [ + "UPPERCASE", + " ", + "", + "symbols!", + "two..dots", + ".start.with.dot", + "end.with.dot.", + ], + ) + @WithMockCustomUser(roles = ["ADMIN"]) + fun `given admin user when deleting global settings with invalid key then validation error is thrown`(key: String) { + //language=JSON + val jsonString = + """ + ["$key"] + """.trimIndent() + + mockMvc + .delete("/api/v1/client-settings/global") { + contentType = MediaType.APPLICATION_JSON + content = jsonString + }.andExpect { + status { isBadRequest() } + } + } + + @ParameterizedTest + @ValueSource( + strings = [ + //language=JSON + "null", + //language=JSON + """{ + "value": "1", + "allowUnauthorized": null + }""", + //language=JSON + """{ + "value": " ", + "allowUnauthorized": true + }""", + //language=JSON + """{ + "value": " ", + "allowUnauthorized": false + }""", + //language=JSON + """{ + "value": null, + "allowUnauthorized": false + }""", + ], + ) + @WithMockCustomUser(roles = ["ADMIN"]) + fun `given admin user when updating global settings with invalid value then validation error is thrown`(value: String) { + //language=JSON + val jsonString = + """ + { + "setting": $value + } + """.trimIndent() + + mockMvc + .patch("/api/v1/client-settings/global") { + contentType = MediaType.APPLICATION_JSON + content = jsonString + }.andExpect { + status { isBadRequest() } + } + } } }