fix(settings): save a section transactionally and stop null values corrupting settings.yml

app/common/src/main/java/stirling/software/common/util/YamlHelper.java

@@ -136,8 +136,17 @@ public class YamlHelper {
                 } else if ("true".equals(newValue) || "false".equals(newValue)) {
                     newValueNode =
                             new ScalarNode(Tag.BOOL, String.valueOf(newValue), ScalarStyle.PLAIN);
+                } else if (newValue instanceof Map<?, ?> map
+                        && valueNode instanceof MappingNode existingMapping) {
+                    // Merge into the existing block instead of replacing it: callers send
+                    // partial maps (the admin UI only submits changed fields), so replacing
+                    // would delete every sibling key and reset it to the template default.
+                    mergeIntoMappingNode(existingMapping, map);
+                    updatedTuples.add(tuple);
+                    updated = true;
+                    continue;
                 } else if (newValue instanceof Map<?, ?> map) {
-                    // Handle Map objects - convert to MappingNode
+                    // No existing block to merge into - build one from scratch
                     List<NodeTuple> mapTuples = new ArrayList<>();
                     for (Map.Entry<?, ?> entry : map.entrySet()) {
                         ScalarNode mapKeyNode =
@@ -167,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)
@@ -196,6 +212,31 @@ public class YamlHelper {
         return updated;
     }
 
+    /**
+     * 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.
+     *
+     * <p>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.
+     *
+     * <p>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()) {
+            String key = String.valueOf(entry.getKey());
+            if (updateValue(target, List.of(key), entry.getValue())) {
+                continue;
+            }
+            target.getValue()
+                    .add(
+                            new NodeTuple(
+                                    new ScalarNode(Tag.STR, key, ScalarStyle.PLAIN),
+                                    convertValueToNode(entry.getValue())));
+        }
+    }
+
     /**
      * Fetches a value based on an exact key path.
      *

app/common/src/test/java/stirling/software/common/util/YamlHelperMoreTest.java

@@ -70,6 +70,154 @@ class YamlHelperMoreTest {
             assertThat(h.getValueByExactKeyPath("meta", "data", "year")).isEqualTo("2024");
         }
 
+        @Test
+        @DisplayName("merges a partial Map into an existing block, keeping siblings and comments")
+        void partialMapMergesIntoExistingBlock() {
+            YamlHelper h =
+                    helper(
+                            "sharing:\n"
+                                    + "  enabled: true\n"
+                                    + "  linkEnabled: true # keep me\n"
+                                    + "  emailEnabled: false\n"
+                                    + "  linkExpirationDays: 3\n");
+            assertThat(h.updateValue(List.of("sharing"), Map.of("emailEnabled", true))).isTrue();
+
+            assertThat(h.getValueByExactKeyPath("sharing", "emailEnabled")).isEqualTo("true");
+            assertThat(h.getValueByExactKeyPath("sharing", "enabled")).isEqualTo("true");
+            assertThat(h.getValueByExactKeyPath("sharing", "linkEnabled")).isEqualTo("true");
+            assertThat(h.getValueByExactKeyPath("sharing", "linkExpirationDays")).isEqualTo("3");
+            assertThat(h.convertNodeToYaml(h.getUpdatedRootNode())).contains("# keep me");
+        }
+
+        @Test
+        @DisplayName("a Map merge adds keys the existing block does not have")
+        void partialMapAddsUnknownKeys() {
+            YamlHelper h = helper("sharing:\n  enabled: false\n");
+            Map<String, Object> values = new LinkedHashMap<>();
+            values.put("enabled", true);
+            values.put("emailEnabled", true);
+            assertThat(h.updateValue(List.of("sharing"), values)).isTrue();
+
+            assertThat(h.getValueByExactKeyPath("sharing", "enabled")).isEqualTo("true");
+            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<String, Object> 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() {

app/core/src/test/java/stirling/software/common/configuration/ConfigInitializerRestartTest.java

@@ -6,6 +6,7 @@ import static org.mockito.Mockito.mockStatic;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Map;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
@@ -57,6 +58,45 @@ class ConfigInitializerRestartTest {
         }
     }
 
+    /**
+     * The admin UI only submits the fields it changed, so a nested block such as storage.sharing
+     * arrives as a partial map. Persisting it must not drop the siblings, otherwise the next
+     * restart resets them to the template defaults - the bug behind "enabling email sharing turns
+     * Enable Sharing back off after a restart".
+     */
+    @Test
+    void partialSharingSave_keepsSiblingsAcrossRestart(@TempDir Path tmp) throws Exception {
+        Path settings = tmp.resolve("settings.yml");
+        Path custom = tmp.resolve("custom_settings.yml");
+
+        try (MockedStatic<InstallationPathConfig> paths =
+                mockStatic(InstallationPathConfig.class)) {
+            paths.when(InstallationPathConfig::getSettingsPath).thenReturn(settings.toString());
+            paths.when(InstallationPathConfig::getCustomSettingsPath).thenReturn(custom.toString());
+
+            ConfigInitializer init = new ConfigInitializer();
+            init.ensureConfigExists();
+
+            // Admin turns on storage and sharing, then restarts.
+            GeneralUtils.saveKeyToSettings("storage.enabled", true);
+            GeneralUtils.saveKeyToSettings("storage.sharing.enabled", true);
+            init.ensureConfigExists();
+
+            // Admin now flips only "Enable Email Sharing", then restarts again. Both the leaf key
+            // the controller writes today and the whole-block map an older client may send have to
+            // leave the siblings alone.
+            GeneralUtils.saveKeyToSettings("storage.sharing.emailEnabled", true);
+            GeneralUtils.saveKeyToSettings("storage.sharing", Map.of("emailEnabled", true));
+            init.ensureConfigExists();
+
+            assertEquals("true", read(settings, "storage", "enabled"));
+            assertEquals("true", read(settings, "storage", "sharing", "enabled"));
+            assertEquals("true", read(settings, "storage", "sharing", "emailEnabled"));
+            assertEquals("true", read(settings, "storage", "sharing", "linkEnabled"));
+            assertEquals("3", read(settings, "storage", "sharing", "linkExpirationDays"));
+        }
+    }
+
     @Test
     void legacyPascalCaseConfig_isMigratedAndPreservedOnRestart(@TempDir Path tmp)
             throws Exception {

app/proprietary/src/main/java/stirling/software/proprietary/security/controller/api/AdminSettingsController.java

@@ -364,12 +364,11 @@ public class AdminSettingsController {
                 }
             }
 
-            int updatedCount = 0;
-            for (Map.Entry<String, Object> entry : sectionData.entrySet()) {
-                String propertyKey = entry.getKey();
-                String fullKey = sectionName + "." + propertyKey;
-                Object value = entry.getValue();
+            Map<String, Object> flattened = new LinkedHashMap<>();
+            flattenSectionData(sectionName, sectionData, flattened);
 
+            // 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(
@@ -378,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<String, Object> 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(
@@ -1025,6 +1029,26 @@ public class AdminSettingsController {
         return result;
     }
 
+    /**
+     * Flattens a section payload to dotted leaf keys. The UI submits only the fields it changed, so
+     * a nested block arrives as a partial map - keeping it whole would make {@link #pendingChanges}
+     * forget the siblings a previous save of the same block set.
+     */
+    private void flattenSectionData(
+            String prefix, Map<String, Object> sectionData, Map<String, Object> flattened) {
+        for (Map.Entry<String, Object> entry : sectionData.entrySet()) {
+            String key = prefix + "." + entry.getKey();
+            Object value = entry.getValue();
+            if (value instanceof Map<?, ?> nested && !nested.isEmpty()) {
+                @SuppressWarnings("unchecked")
+                Map<String, Object> nestedMap = (Map<String, Object>) nested;
+                flattenSectionData(key, nestedMap, flattened);
+            } else {
+                flattened.put(key, value);
+            }
+        }
+    }
+
     /**
      * Set a value in a nested map using dot notation
      *

app/proprietary/src/test/java/stirling/software/proprietary/security/controller/api/AdminSettingsControllerTest.java

@@ -1,8 +1,10 @@
 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.Mockito.mockStatic;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 
 import java.io.IOException;
@@ -429,7 +431,36 @@ 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")));
+            }
+        }
+
+        @Test
+        @DisplayName("flattens nested blocks to leaf keys so partial saves keep siblings")
+        void flattensNestedBlocks() {
+            java.util.Map<String, Object> section = new java.util.HashMap<>();
+            section.put("sharing", new java.util.HashMap<>(Map.of("emailEnabled", true)));
+
+            try (MockedStatic<GeneralUtils> mocked = mockStatic(GeneralUtils.class)) {
+                ResponseEntity<Map<String, Object>> response =
+                        controller.updateSettingsSection("storage", section);
+
+                assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
+                mocked.verify(
+                        () ->
+                                GeneralUtils.updateSettingsTransactional(
+                                        Map.of("storage.sharing.emailEnabled", true)));
+                // The whole "storage.sharing" block is never written, so its siblings survive.
+                mocked.verify(
+                        () ->
+                                GeneralUtils.updateSettingsTransactional(
+                                        argThat(
+                                                (Map<String, Object> m) ->
+                                                        m.containsKey("storage.sharing"))),
+                        never());
             }
         }
 
@@ -445,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<String, Object> 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<String, Object> sharing = new java.util.HashMap<>();
+            sharing.put("emailEnabled", null);
+            java.util.Map<String, Object> section = new java.util.HashMap<>();
+            section.put("sharing", sharing);
+
+            try (MockedStatic<GeneralUtils> mocked = mockStatic(GeneralUtils.class)) {
+                ResponseEntity<Map<String, Object>> response =
+                        controller.updateSettingsSection("storage", section);
+
+                assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
+                mocked.verify(
+                        () ->
+                                GeneralUtils.updateSettingsTransactional(
+                                        argThat(
+                                                (Map<String, Object> m) ->
+                                                        m.containsKey(leafKey)
+                                                                && m.get(leafKey) == null)));
             }
         }
 
@@ -453,7 +514,7 @@ class AdminSettingsControllerTest {
         @DisplayName("returns 500 when persistence throws IOException")
         void persistenceIOException() {
             try (MockedStatic<GeneralUtils> mocked = mockStatic(GeneralUtils.class)) {
-                mocked.when(() -> GeneralUtils.saveKeyToSettings("ui.appName", "New"))
+                mocked.when(() -> GeneralUtils.updateSettingsTransactional(any()))
                         .thenThrow(new IOException("io"));
 
                 ResponseEntity<Map<String, Object>> response =