From 5f36574f73babf6aee1bb31f11ee23c176151c75 Mon Sep 17 00:00:00 2001 From: Anthony Stirling <77850077+Frooodle@users.noreply.github.com> Date: Sun, 30 Aug 2026 10:29:15 +0100 Subject: [PATCH] fix(settings): save a section transactionally and stop null values corrupting settings.yml --- .../software/common/util/YamlHelper.java | 13 ++ .../common/util/YamlHelperMoreTest.java | 116 ++++++++++++++++++ .../api/AdminSettingsController.java | 24 ++-- .../api/AdminSettingsControllerTest.java | 51 +++++++- 4 files changed, 187 insertions(+), 17 deletions(-) diff --git a/app/common/src/main/java/stirling/software/common/util/YamlHelper.java b/app/common/src/main/java/stirling/software/common/util/YamlHelper.java index f6b5dc196c..53d5d87850 100644 --- a/app/common/src/main/java/stirling/software/common/util/YamlHelper.java +++ b/app/common/src/main/java/stirling/software/common/util/YamlHelper.java @@ -176,6 +176,13 @@ public class YamlHelper { new ScalarNode(tag, String.valueOf(obj), ScalarStyle.PLAIN)); } newValueNode = new SequenceNode(Tag.SEQ, sequenceNodes, FlowStyle.FLOW); + } else if (newValue == null) { + // A null must not inherit the old tag: !!int 'null' and !!map 'null' + // make settings.yml unloadable and the app will not boot. + newValueNode = new ScalarNode(Tag.NULL, "null", ScalarStyle.PLAIN); + } else if (tag == Tag.INT || tag == Tag.FLOAT) { + // Numeric values were handled above, so this one is not numeric. + newValueNode = convertValueToNode(newValue); } else if (tag == Tag.NULL) { if ("true".equals(newValue) || "false".equals(newValue) @@ -209,6 +216,12 @@ public class YamlHelper { * Applies each entry of {@code values} onto {@code target} in place, keeping any key of {@code * target} the map does not mention (along with its comments). Keys absent from {@code target} * are appended. + * + *

The merge is additive-only: an entry can add or overwrite a key but never remove one, and + * a null entry writes a null value rather than deleting the key. + * + *

An appended key the settings template does not contain is dropped on the next restart, + * because ConfigInitializer merges the user file into the template. */ private void mergeIntoMappingNode(MappingNode target, Map values) { for (Map.Entry entry : values.entrySet()) { diff --git a/app/common/src/test/java/stirling/software/common/util/YamlHelperMoreTest.java b/app/common/src/test/java/stirling/software/common/util/YamlHelperMoreTest.java index 79de5118b8..ea529b92f5 100644 --- a/app/common/src/test/java/stirling/software/common/util/YamlHelperMoreTest.java +++ b/app/common/src/test/java/stirling/software/common/util/YamlHelperMoreTest.java @@ -102,6 +102,122 @@ class YamlHelperMoreTest { assertThat(h.getValueByExactKeyPath("sharing", "emailEnabled")).isEqualTo("true"); } + @Test + @DisplayName("a deep partial merge keeps every unrelated nested key, on disk too") + void deepMergeKeepsUnrelatedNestedKeys() { + YamlHelper h = + helper( + "oauth2:\n" + + " enabled: false\n" + + " client:\n" + + " google:\n" + + " clientId: OLD\n" + + " clientSecret: SEC\n" + + " github:\n" + + " clientId: GH\n"); + assertThat( + h.updateValue( + List.of("oauth2"), + Map.of("client", Map.of("google", Map.of("clientId", "NEW"))))) + .isTrue(); + + // Re-parse the emitted YAML: the siblings must survive the round-trip to disk, + // not just the in-memory node tree. + YamlHelper reloaded = helper(h.convertNodeToYaml(h.getUpdatedRootNode())); + assertThat(reloaded.getValueByExactKeyPath("oauth2", "client", "google", "clientId")) + .isEqualTo("NEW"); + assertThat( + reloaded.getValueByExactKeyPath( + "oauth2", "client", "google", "clientSecret")) + .isEqualTo("SEC"); + assertThat(reloaded.getValueByExactKeyPath("oauth2", "client", "github", "clientId")) + .isEqualTo("GH"); + assertThat(reloaded.getValueByExactKeyPath("oauth2", "enabled")).isEqualTo("false"); + } + + @Test + @DisplayName("a list inside a merge replaces the whole sequence instead of appending") + void listInsideMergeReplacesTheWholeSequence() { + YamlHelper h = + helper( + "mcp:\n" + + " enabled: true\n" + + " allowedOperations:\n" + + " - a\n" + + " - b\n"); + assertThat(h.updateValue(List.of("mcp"), Map.of("allowedOperations", List.of("z")))) + .isTrue(); + + // A security allowlist must still be shortenable, so the sequence is replaced. + List operations = (List) h.getValueByExactKeyPath("mcp", "allowedOperations"); + assertThat(operations).hasSize(1); + assertThat(operations.getFirst()).isEqualTo("z"); + assertThat(h.getValueByExactKeyPath("mcp", "enabled")).isEqualTo("true"); + } + + @Test + @DisplayName("a null entry in a merge writes null and does not delete the key") + void nullEntryDoesNotDeleteTheKey() { + YamlHelper h = + helper( + "sharing:\n" + + " enabled: true\n" + + " linkEnabled: true\n" + + " emailEnabled: false\n" + + " linkExpirationDays: 3\n"); + Map values = new LinkedHashMap<>(); + values.put("emailEnabled", null); + assertThat(h.updateValue(List.of("sharing"), values)).isTrue(); + + assertThat(h.getValueByExactKeyPath("sharing", "emailEnabled")).isEqualTo("null"); + assertThat(h.getValueByExactKeyPath("sharing", "enabled")).isEqualTo("true"); + assertThat(h.getValueByExactKeyPath("sharing", "linkEnabled")).isEqualTo("true"); + assertThat(h.getValueByExactKeyPath("sharing", "linkExpirationDays")).isEqualTo("3"); + } + + @Test + @DisplayName("merging an empty map leaves every key in the block unchanged") + void emptyMapLeavesTheBlockUnchanged() { + YamlHelper h = + helper( + "sharing:\n" + + " enabled: true\n" + + " linkEnabled: true\n" + + " emailEnabled: false\n" + + " linkExpirationDays: 3\n"); + h.updateValue(List.of("sharing"), Map.of()); + + assertThat(h.getValueByExactKeyPath("sharing", "enabled")).isEqualTo("true"); + assertThat(h.getValueByExactKeyPath("sharing", "linkEnabled")).isEqualTo("true"); + assertThat(h.getValueByExactKeyPath("sharing", "emailEnabled")).isEqualTo("false"); + assertThat(h.getValueByExactKeyPath("sharing", "linkExpirationDays")).isEqualTo("3"); + } + + @Test + @DisplayName("a non-numeric value does not inherit the existing INT tag") + void nonNumericValueDoesNotInheritIntTag() { + YamlHelper h = helper("sharing:\n linkExpirationDays: 3\n"); + assertThat(h.updateValue(List.of("sharing", "linkExpirationDays"), "notanumber")) + .isTrue(); + + // !!int 'notanumber' would make settings.yml unloadable and brick the boot. + String dumped = h.convertNodeToYaml(h.getUpdatedRootNode()); + assertThat(dumped).doesNotContain("!!int"); + assertThat(helper(dumped).getValueByExactKeyPath("sharing", "linkExpirationDays")) + .isEqualTo("notanumber"); + } + + @Test + @DisplayName("a null value does not inherit the existing INT tag") + void nullValueDoesNotInheritIntTag() { + YamlHelper h = helper("sharing:\n linkExpirationDays: 3\n"); + assertThat(h.updateValue(List.of("sharing", "linkExpirationDays"), null)).isTrue(); + + String dumped = h.convertNodeToYaml(h.getUpdatedRootNode()); + assertThat(dumped).doesNotContain("!!int"); + assertThat(h.getValueByExactKeyPath("sharing", "linkExpirationDays")).isEqualTo("null"); + } + @Test @DisplayName("replaces a scalar with a List value (SequenceNode)") void listValue() { diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/AdminSettingsController.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/AdminSettingsController.java index 77568841c1..9e4fb6316d 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/AdminSettingsController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/AdminSettingsController.java @@ -367,11 +367,8 @@ public class AdminSettingsController { Map flattened = new LinkedHashMap<>(); flattenSectionData(sectionName, sectionData, flattened); - int updatedCount = 0; - for (Map.Entry entry : flattened.entrySet()) { - String fullKey = entry.getKey(); - Object value = entry.getValue(); - + // Validate every key before writing, so an invalid one cannot half-update the file. + for (String fullKey : flattened.keySet()) { if (!isValidSettingKey(fullKey)) { return ResponseEntity.badRequest() .body( @@ -380,19 +377,24 @@ public class AdminSettingsController { "Invalid setting key format: " + HtmlUtils.htmlEscape(fullKey))); } + } + // Load once, apply all, save once instead of one full rewrite per leaf key. + GeneralUtils.updateSettingsTransactional(flattened); + + for (Map.Entry entry : flattened.entrySet()) { + String fullKey = entry.getKey(); + Object value = entry.getValue(); log.info( "Admin updating section setting: {} = {}", fullKey, logSafeValue(fullKey, value)); - GeneralUtils.saveKeyToSettings(fullKey, value); - - // Track this as a pending change - pendingChanges.put(fullKey, value); - - updatedCount++; + // pendingChanges is a ConcurrentHashMap and rejects null values. + pendingChanges.put(fullKey, value != null ? value : ""); } + int updatedCount = flattened.size(); + String escapedSectionName = HtmlUtils.htmlEscape(sectionName); return ResponseEntity.ok( Map.of( diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/AdminSettingsControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/AdminSettingsControllerTest.java index 8e879e315c..b5155cafbc 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/AdminSettingsControllerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/AdminSettingsControllerTest.java @@ -3,7 +3,6 @@ package stirling.software.proprietary.security.controller.api; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -432,7 +431,10 @@ class AdminSettingsControllerTest { assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(response.getBody().get("message").toString()).contains("Successfully"); - mocked.verify(() -> GeneralUtils.saveKeyToSettings("ui.appName", "New")); + mocked.verify( + () -> + GeneralUtils.updateSettingsTransactional( + Map.of("ui.appName", "New"))); } } @@ -448,9 +450,16 @@ class AdminSettingsControllerTest { assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); mocked.verify( - () -> GeneralUtils.saveKeyToSettings("storage.sharing.emailEnabled", true)); + () -> + GeneralUtils.updateSettingsTransactional( + Map.of("storage.sharing.emailEnabled", true))); + // The whole "storage.sharing" block is never written, so its siblings survive. mocked.verify( - () -> GeneralUtils.saveKeyToSettings(eq("storage.sharing"), any()), + () -> + GeneralUtils.updateSettingsTransactional( + argThat( + (Map m) -> + m.containsKey("storage.sharing"))), never()); } } @@ -467,7 +476,37 @@ class AdminSettingsControllerTest { assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); // enabled flag auto-added and persisted - mocked.verify(() -> GeneralUtils.saveKeyToSettings("premium.enabled", true)); + mocked.verify( + () -> + GeneralUtils.updateSettingsTransactional( + argThat( + (Map m) -> + Boolean.TRUE.equals( + m.get("premium.enabled"))))); + } + } + + @Test + @DisplayName("a null leaf value is persisted and does not blow up pendingChanges") + void nullLeafValueStillReturns200() { + String leafKey = "storage.sharing.emailEnabled"; + java.util.Map sharing = new java.util.HashMap<>(); + sharing.put("emailEnabled", null); + java.util.Map section = new java.util.HashMap<>(); + section.put("sharing", sharing); + + try (MockedStatic mocked = mockStatic(GeneralUtils.class)) { + ResponseEntity> response = + controller.updateSettingsSection("storage", section); + + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); + mocked.verify( + () -> + GeneralUtils.updateSettingsTransactional( + argThat( + (Map m) -> + m.containsKey(leafKey) + && m.get(leafKey) == null))); } } @@ -475,7 +514,7 @@ class AdminSettingsControllerTest { @DisplayName("returns 500 when persistence throws IOException") void persistenceIOException() { try (MockedStatic mocked = mockStatic(GeneralUtils.class)) { - mocked.when(() -> GeneralUtils.saveKeyToSettings("ui.appName", "New")) + mocked.when(() -> GeneralUtils.updateSettingsTransactional(any())) .thenThrow(new IOException("io")); ResponseEntity> response =