refactor(api): add validation on client-settings api

komga/build.gradle.kts

@@ -145,6 +145,7 @@ tasks {
       freeCompilerArgs =
         listOf(
           "-Xjsr305=strict",
+          "-Xemit-jvm-type-annotations",
           "-opt-in=kotlin.time.ExperimentalTime",
         )
     }

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<String, ClientSettingGlobalUpdateDto>,
+    @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<String, ClientSettingUserUpdateDto>,
+    @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<String>,
+    @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<String>,
+    @RequestBody keysToDelete: Set<
+      @Pattern(regexp = KEY_REGEX)
+      String,
+      >,
   ) {
     clientSettingsDtoDao.deleteByUserIdAndKeys(principal.user.id, keysToDelete)
   }

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

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() }
+        }
+    }
   }
 }