Ensure persistent settings load valid multiselects
What the developer wrote
Authored by kdmukai
Ensure persistent settings load valid multiselects
What changed, and why it matters
This commit fixes a bug in how SeedSigner reloads saved settings. If a setting that is supposed to be a list of choices (a 'multiselect') was saved as empty/None, the app could behave incorrectly. The patch makes the app fall back to the default list instead. It is a defensive fix rather than a clear-cut remote exploit.
Review whether any other setting types (e.g., single-select, booleans, integers) can also be persisted as None and whether downstream consumers assume non-null values. Consider adding validation at save time, not just load time, and ensure settings.json integrity is protected (e.g., checksums or tamper detection) if it is treated as trusted input.
Security signals we found
Input validation / sanitization of persisted configuration
Null/None value handling for list-type settings
Defensive default fallback for malformed saved settings
Evidence from the diff
In src/seedsigner/models/settings.py, when loading persistent settings from disk, multiselect entries whose stored value is None are now replaced with the entry’s default_value. Previously, only a comma-separated string was converted to a list; None would propagate through set_value and could cause downstream code expecting a list to misbehave. A test is added that writes a settings.json with SETTING__SIG_TYPES set to null, reloads Settings, and asserts the default value is restored.
Changed components
src/seedsigner/models/settings.pytests/test_settings.pyInspect captured patch +50 / −1
diff --git a/src/seedsigner/models/settings.py b/src/seedsigner/models/settings.py
index d15b165..c2bf933 100644
--- a/src/seedsigner/models/settings.py
+++ b/src/seedsigner/models/settings.py
@@ -161,6 +161,9 @@ class Settings(Singleton):
if type(new_settings[entry.attr_name]) == str:
# Break comma-separated SettingsQR input into List
new_settings[entry.attr_name] = new_settings[entry.attr_name].split(",")
+ elif new_settings[entry.attr_name] is None:
+ # Multiselect cannot be None; load defaults to avoid issues
+ new_settings[entry.attr_name] = entry.default_value
for key, value in new_settings.items():
self.set_value(key, value)
diff --git a/tests/test_settings.py b/tests/test_settings.py
index f896dca..2d548d1 100644
--- a/tests/test_settings.py
+++ b/tests/test_settings.py
@@ -1,7 +1,8 @@
+import json
import pytest
from base import BaseTest
from seedsigner.models.settings import InvalidSettingsQRData, Settings
-from seedsigner.models.settings_definition import SettingsConstants, SettingsDefinition
+from seedsigner.models.settings_definition import SettingsConstants, SettingsDefinition, SettingsEntry
@@ -35,6 +36,51 @@ class TestSettings(BaseTest):
assert settings.get_value(settings_entry.attr_name) == settings_entry.default_value
+ def test_load_persistent_settings(self):
+ """ Settings should load previously saved persistent settings from disk, if any
+ exist. Empty multiselect settings should load defaults. """
+ # Initial Settings will start with defaults
+ settings = Settings.get_instance()
+
+ # Enable persistent settings and make another change
+ settings.set_value(SettingsConstants.SETTING__PERSISTENT_SETTINGS, SettingsConstants.OPTION__ENABLED)
+
+ assert settings.get_value(SettingsConstants.SETTING__QR_DENSITY) != SettingsConstants.DENSITY__HIGH
+ settings.set_value(SettingsConstants.SETTING__QR_DENSITY, SettingsConstants.DENSITY__HIGH)
+
+ # Hold on to the settings.json content
+ settings_json = None
+ with open(Settings.SETTINGS_FILENAME) as settings_file:
+ settings_json = json.loads(settings_file.read())
+
+ # Now wipe out the Settings singleton
+ BaseTest.reset_settings()
+
+ # This also deletes settings.json, so recreate it
+ with open(Settings.SETTINGS_FILENAME, "w") as settings_file:
+ settings_file.write(json.dumps(settings_json))
+
+ # Now instantiate the Settings singleton again; it should load from disk
+ settings = Settings.get_instance()
+ assert settings.get_value(SettingsConstants.SETTING__QR_DENSITY) == SettingsConstants.DENSITY__HIGH
+
+ # Wipe out the Settings singleton again
+ BaseTest.reset_settings()
+
+ # Alter the settings.json to have an empty multiselect setting
+ settings_entry = SettingsDefinition.get_settings_entry(SettingsConstants.SETTING__SIG_TYPES)
+ settings_json[settings_entry.attr_name] = None
+ with open(Settings.SETTINGS_FILENAME, "w") as settings_file:
+ settings_file.write(json.dumps(settings_json))
+
+ print(json.dumps(settings_json, indent=4))
+
+ # Re-instantiate and verify that the multiselect setting has loaded its defaults
+ settings = Settings.get_instance()
+ sig_types = settings.get_value(settings_entry.attr_name)
+ assert sig_types == settings_entry.default_value
+
+
def test_parse_settingsqr_data(self):
"""
SettingsQR parser should successfully parse a valid settingsqr input string and
Why this scored 31/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.