Fix SharedPreferences no-op guard silently discarding writes

ThomasFarstrike · ThomasFarstrike · commit 9be0035fc0ce · 2026-05-29T15:23:32.000+02:00
The no-op guard in Editor.commit() compares filtered_data against
preferences.data and skips the file write if they match. But any code
that got a mutable reference via get_dict()/get_list(), mutated it,
then created an Editor had already mutated preferences.data in-place
before the Editor's deep-copy &mdash; so the guard always saw equality and
never wrote to disk. This affected ALL preferences, not just WiFi.

Fixes:
- config.py: get_dict() and get_list() now return copies so that
  accidental mutation doesn't corrupt prefs.data before the Editor
  snapshot.
- wifi_service.py: use editor.put_dict_item() / remove_dict_item()
  to operate on the Editor's deep copy instead.
- testing/mocks.py: add put_dict_item() / remove_dict_item() to
  MockEditor for test compatibility.
- tests: add regression tests for the
  get_dict -&gt; mutate -&gt; edit -&gt; commit pattern.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,13 @@ Future release (next version)
 
 Put unreleased changes here!
 
+0.11.2
+======
+
+Frameworks:
+- Fix SharedPreferences no-op guard silently discarding writes (affects all settings, not just WiFi)
+
+
 0.11.1
 ======
 
diff --git a/internal_filesystem/lib/mpos/config.py b/internal_filesystem/lib/mpos/config.py
@@ -123,7 +123,7 @@ def get_bool(self, key, default=False):
     def get_list(self, key, default=None):
         """Retrieve a list for the given key, with a default if not found."""
         if key in self.data:
-            return self.data[key]
+            return list(self.data[key])  # return a copy &mdash; callers must not mutate prefs.data directly
         # Key not in stored data, check defaults
         # Method default takes precedence if provided
         if default is not None:
@@ -137,7 +137,7 @@ def get_list(self, key, default=None):
     def get_dict(self, key, default=None):
         """Retrieve a dictionary for the given key, with a default if not found."""
         if key in self.data:
-            return self.data[key]
+            return dict(self.data[key])  # return a copy &mdash; callers must not mutate prefs.data directly
         # Key not in stored data, check defaults
         # Method default takes precedence if provided
         if default is not None:
diff --git a/internal_filesystem/lib/mpos/net/wifi_service.py b/internal_filesystem/lib/mpos/net/wifi_service.py
@@ -730,21 +730,20 @@ def save_network(ssid, password, hidden=False):
         """
         # Load current saved networks
         prefs = mpos.config.SharedPreferences(WIFI_SERVICE_PREFS_KEY)
-        access_points = prefs.get_dict("access_points")
 
-        # Add or update the network
+        # Build the network config
         network_config = {"password": password}
         if hidden:
             network_config["hidden"] = True
-        access_points[ssid] = network_config
 
-        # Save back to config
+        # Modify the deep copy inside the editor, not prefs.data directly,
+        # otherwise the no-op guard in commit() sees no change and skips writing.
         editor = prefs.edit()
-        editor.put_dict("access_points", access_points)
+        editor.put_dict_item("access_points", ssid, network_config)
         editor.commit()
 
         # Update class-level cache
-        WifiService.access_points = access_points
+        WifiService.access_points = prefs.get_dict("access_points")
 
         print(f"WifiService: Saved network '{ssid}' (hidden={hidden})")
 
@@ -761,23 +760,20 @@ def forget_network(ssid):
         """
         # Load current saved networks
         prefs = mpos.config.SharedPreferences(WIFI_SERVICE_PREFS_KEY)
-        access_points = prefs.get_dict("access_points")
 
-        # Remove the network if it exists
-        if ssid in access_points:
-            del access_points[ssid]
+        # Check if the network exists without mutating prefs.data
+        if ssid not in prefs.get_dict("access_points"):
+            return False
 
-            # Save back to config
-            editor = prefs.edit()
-            editor.put_dict("access_points", access_points)
-            editor.commit()
+        # Modify the deep copy inside the editor, not prefs.data directly,
+        # otherwise the no-op guard in commit() sees no change and skips writing.
+        editor = prefs.edit()
+        editor.remove_dict_item("access_points", ssid)
+        editor.commit()
 
-            # Update class-level cache
-            WifiService.access_points = access_points
+        # Update class-level cache
+        WifiService.access_points = prefs.get_dict("access_points")
 
-            print(f"WifiService: Forgot network '{ssid}'")
-            return True
-        else:
-            print(f"WifiService: Network '{ssid}' not found in saved networks")
-            return False
+        print(f"WifiService: Forgot network '{ssid}'")
+        return True
 
diff --git a/internal_filesystem/lib/mpos/testing/mocks.py b/internal_filesystem/lib/mpos/testing/mocks.py
@@ -973,6 +973,19 @@ def __init__(self, prefs):
     def put_dict(self, key, value):
         self.pending[key] = value
 
+    def put_dict_item(self, dict_key, item_key, config):
+        if dict_key not in self.pending:
+            existing = self.prefs._get_value(dict_key, {})
+            self.pending[dict_key] = dict(existing)
+        if isinstance(config, dict):
+            self.pending[dict_key][item_key] = config
+
+    def remove_dict_item(self, dict_key, item_key):
+        if dict_key not in self.pending:
+            existing = self.prefs._get_value(dict_key, {})
+            self.pending[dict_key] = dict(existing)
+        self.pending[dict_key].pop(item_key, None)
+
     def put_list(self, key, value):
         self.pending[key] = value
 
diff --git a/tests/test_shared_preferences.py b/tests/test_shared_preferences.py
@@ -308,6 +308,58 @@ def test_get_dict_keys_nonexistent(self):
         prefs = SharedPreferences(self.test_app_name)
         self.assertEqual(prefs.get_dict_keys("nonexistent"), [])
 
+    def test_get_dict_mutate_then_edit_persists(self):
+        """
+        Test that mutating a dict returned by get_dict() before creating an
+        editor does NOT prevent commit() from writing to disk.
+
+        Regression guard: get_dict() returns a direct reference to prefs.data.
+        Mutating that reference modifies prefs.data in-place.  If edit()
+        then deep-copies the already-mutated data, commit()'s no-op guard
+        sees filtered_data == preferences.data and skips the write.
+        Editors must operate on their own deep copy.
+        """
+        prefs = SharedPreferences(self.test_app_name)
+
+        # 1. Save initial data
+        prefs.edit().put_dict("access_points", {"ExistingNet": {"password": "old"}}).commit()
+
+        # 2. Simulate the WifiService bug pattern: get_dict, mutate, edit, commit
+        prefs2 = SharedPreferences(self.test_app_name)
+        aps = prefs2.get_dict("access_points")          # direct reference
+        aps["NewNet"] = {"password": "secret"}           # mutates prefs2.data in-place
+        editor = prefs2.edit()                            # deep-copies already-mutated data
+        editor.put_dict("access_points", aps)
+        editor.commit()
+
+        # 3. Verify the change was persisted
+        prefs3 = SharedPreferences(self.test_app_name)
+        saved = prefs3.get_dict("access_points")
+        self.assertIn("NewNet", saved)
+
+    def test_get_dict_mutate_remove_then_edit_persists(self):
+        """
+        Same as test_get_dict_mutate_then_edit_persists but for deletion.
+        """
+        prefs = SharedPreferences(self.test_app_name)
+
+        # 1. Save initial data
+        prefs.edit().put_dict("access_points", {"NetA": {"password": "a"}, "NetB": {"password": "b"}}).commit()
+
+        # 2. Bug pattern: get_dict, delete from it, then edit + commit
+        prefs2 = SharedPreferences(self.test_app_name)
+        aps = prefs2.get_dict("access_points")
+        del aps["NetA"]
+        editor = prefs2.edit()
+        editor.put_dict("access_points", aps)
+        editor.commit()
+
+        # 3. Verify the change was persisted
+        prefs3 = SharedPreferences(self.test_app_name)
+        saved = prefs3.get_dict("access_points")
+        self.assertFalse("NetA" in saved)
+        self.assertTrue("NetB" in saved)
+
     # ============================================================
     # Editor Operations
     # ============================================================
