What changed, and why it matters
This commit hardens how SeedSigner loads multiselect settings from saved files or QR codes. Previously, an empty or malformed multiselect value (for example, an empty string, a lone comma, or a missing entry) could leave the setting empty, which might cause unexpected behavior when the app later uses that setting. The patch now strips empty pieces from comma-separated input and falls back to the default value whenever the result is empty. A new test checks four empty/missing cases.
Treat as a defensive hardening patch. Review whether any other setting types have similar empty/missing-value handling gaps, and consider validating SettingsQR input more strictly before it reaches Settings.update().
Security signals we found
Input sanitization for persisted/QR settings
Defensive fallback to defaults for malformed multiselect values
New unit tests covering empty/missing multiselect edge cases
Evidence from the diff
In src/seedsigner/models/settings.py, the Settings.update() method now filters out empty strings after splitting a comma-separated multiselect value, and it replaces any falsy result (empty list or None) with the setting’s default. The test file adds a dedicated test_load_empty_multiselect_settings() that verifies defaults are loaded for ‘’, ‘,’, None, and a deleted key. The change is defensive hardening rather than a clear vulnerability fix.
Changed components
src/seedsigner/models/settings.pytests/test_settings.pyInspect captured patch +37 / −16
diff --git a/src/seedsigner/models/settings.py b/src/seedsigner/models/settings.py
index c2bf933..14741e6 100644
--- a/src/seedsigner/models/settings.py
+++ b/src/seedsigner/models/settings.py
@@ -159,10 +159,12 @@ class Settings(Singleton):
# Clean the incoming data, if necessary
if entry.type == SettingsConstants.TYPE__MULTISELECT:
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
+ # Break comma-separated multiselect options into List; avoid empty
+ # values.
+ new_settings[entry.attr_name] = [value for value in new_settings[entry.attr_name].split(",") if value.strip()]
+
+ if not new_settings[entry.attr_name]:
+ # Multiselect cannot be empty; load defaults to avoid issues
new_settings[entry.attr_name] = entry.default_value
for key, value in new_settings.items():
diff --git a/tests/test_settings.py b/tests/test_settings.py
index 487bf58..044f843 100644
--- a/tests/test_settings.py
+++ b/tests/test_settings.py
@@ -38,7 +38,7 @@ class TestSettings(BaseTest):
def test_load_persistent_settings(self):
""" Settings should load previously saved persistent settings from disk, if any
- exist. Empty multiselect settings should load defaults. """
+ exist. """
# Initial Settings will start with defaults
settings = Settings.get_instance()
@@ -62,21 +62,40 @@ class TestSettings(BaseTest):
# Now instantiate the Settings singleton again; it should load from disk
settings = Settings.get_instance()
+
+ # Persistent setting change should have survived
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))
- # Re-instantiate and verify that the multiselect setting has loaded its defaults
+ def test_load_empty_multiselect_settings(self):
+ """ Empty multiselect settings should load defaults. """
+ # Initial Settings will start with defaults
settings = Settings.get_instance()
- sig_types = settings.get_value(settings_entry.attr_name)
- assert sig_types == settings_entry.default_value
+
+ # Enable persistent settings to write settings.json to disk
+ settings.set_value(SettingsConstants.SETTING__PERSISTENT_SETTINGS, SettingsConstants.OPTION__ENABLED)
+
+ # Hold on to the settings.json content
+ settings_dict = None
+ with open(Settings.SETTINGS_FILENAME) as settings_file:
+ settings_dict = json.loads(settings_file.read())
+
+ def _verify_defaults_loaded(attr_name: str):
+ # Verify that the multiselect setting has loaded its defaults
+ settings = Settings.get_instance()
+ cur_setting_value = settings.get_value(attr_name)
+ assert cur_setting_value == SettingsDefinition.get_settings_entry(attr_name).default_value
+
+ # Alter the settings to test against various empty values
+ for empty_value in ["", ",", None]:
+ settings_dict[SettingsConstants.SETTING__SIG_TYPES] = empty_value
+ settings.update(settings_dict)
+ _verify_defaults_loaded(SettingsConstants.SETTING__SIG_TYPES)
+
+ # One last test: remove the multiselect setting entirely
+ del settings_dict[SettingsConstants.SETTING__SIG_TYPES]
+ settings.update(settings_dict)
+ _verify_defaults_loaded(SettingsConstants.SETTING__SIG_TYPES)
def test_parse_settingsqr_data(self):
Why this scored 35/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.