diff --git a/glob/manager_core.py b/glob/manager_core.py index 63ce1e4bd..5acdc4c21 100644 --- a/glob/manager_core.py +++ b/glob/manager_core.py @@ -1679,34 +1679,146 @@ class ManagerFuncs: manager_funcs = ManagerFuncs() +#: The keys `write_config()` owns in `config.ini`'s `[default]` section. +#: Anything else in the file - other keys, other sections - is hand-maintained +#: content that this function must carry through untouched. +#: `preview_method` is the one key that is NOT snapshot-backed: it is always +#: written from the live `manager_funcs.get_current_preview_method()`. +WRITTEN_CONFIG_KEYS = ( + 'preview_method', + 'git_exe', + 'use_uv', + 'channel_url', + 'share_option', + 'bypass_ssl', + 'file_logging', + 'component_policy', + 'update_policy', + 'windows_selector_event_loop_policy', + 'model_download_by_agent', + 'downgrade_blacklist', + 'security_level', + 'always_lazy_install', + 'network_mode', + 'db_mode', + 'allow_git_url_install', + 'allow_pip_install', +) + + +class DirtyTrackingConfig(dict): + """Config mapping that records which keys were changed after it was built. + + `write_config()` must persist ONLY the setting a caller actually changed, + otherwise it re-writes the other keys from the process-lifetime startup + snapshot and silently reverts whatever the user hand-edited in the + meantime. Every settings caller mutates the cached config the same way - + a subscript assignment such as `get_config()['db_mode'] = mode` - so + recording `__setitem__` is what makes "only what changed" knowable. + `update()` is covered too, so a future caller written that way cannot + lose its change silently. + + The wrapper is applied in `get_config()` AFTER `read_config()` has + returned, and that ordering is load-bearing: + `manager_migration.force_security_level_if_needed()` mutates the config + through a parameter alias from inside BOTH `read_config()` branches. If + that mutation were recorded, `security_level` would be dirty before the + user does anything, and every write would push the forced value over the + user's on-disk one - the exact clobber this tracking exists to remove. + """ + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.dirty_keys = set() + + def __setitem__(self, key, value): + self.dirty_keys.add(key) + super().__setitem__(key, value) + + def update(self, *args, **kwargs): + incoming = dict(*args, **kwargs) + self.dirty_keys.update(incoming) + super().update(incoming) + + def write_config(): + """Persist changed Manager settings without clobbering the rest of the file. + + The write starts from what is CURRENTLY on disk and overlays only the keys + a caller actually changed (tracked by `DirtyTrackingConfig`) plus the live + `preview_method`. Consequences, in the order users notice them: + + - a value hand-edited while the server is up is no longer reverted by an + unrelated settings change; + - keys this function does not own (`http_channel_enabled`, + `default_cache_as_channel_url`, anything a user added) survive; + - sections other than `[default]` survive. + + Reads still come from the startup snapshot until the process restarts, so + the "stop -> edit -> start" advice in the install-denial messages stays + correct; what changes is that the edit is no longer destroyed in between. + """ config = configparser.ConfigParser(strict=False) + try: + loaded = config.read(manager_config_path) + except Exception as e: + # An unparsable config.ini must not make a settings change raise. Fall + # back to the pre-merge behaviour and rewrite the file from scratch. + print(f"[ComfyUI-Manager] Warning: '{manager_config_path}' could not be parsed ({e}); rewriting it from the current settings.") + config = configparser.ConfigParser(strict=False) + else: + # `read()` opens each filename inside its own `try/except OSError` and + # SKIPS the ones it could not open, so a file that exists but denies + # reads comes back as a normal return with an EMPTY parser - not as the + # exception handled above. Merging from that parser would find no + # `[default]`, fall into the bootstrap branch below, and rewrite the + # file from defaults: every foreign key and every other section gone, + # with the settings change still reporting success. + # + # A write that cannot see the current contents has nothing to merge + # onto, so refuse it and leave the file alone. The caller surfaces the + # error (the settings endpoint answers 500), which is what the same + # permission problem already does when writes are denied too. + if not loaded and os.path.exists(manager_config_path): + raise OSError(f"'{manager_config_path}' exists but could not be read; refusing to overwrite it with default settings. Fix the file's permissions, then retry.") - config['default'] = { - 'preview_method': manager_funcs.get_current_preview_method(), - 'git_exe': get_config()['git_exe'], - 'use_uv': get_config()['use_uv'], - 'channel_url': get_config()['channel_url'], - 'share_option': get_config()['share_option'], - 'bypass_ssl': get_config()['bypass_ssl'], - "file_logging": get_config()['file_logging'], - 'component_policy': get_config()['component_policy'], - 'update_policy': get_config()['update_policy'], - 'windows_selector_event_loop_policy': get_config()['windows_selector_event_loop_policy'], - 'model_download_by_agent': get_config()['model_download_by_agent'], - 'downgrade_blacklist': get_config()['downgrade_blacklist'], - 'security_level': get_config()['security_level'], - 'always_lazy_install': get_config()['always_lazy_install'], - 'network_mode': get_config()['network_mode'], - 'db_mode': get_config()['db_mode'], - 'allow_git_url_install': get_config()['allow_git_url_install'], - 'allow_pip_install': get_config()['allow_pip_install'], - } + cached = get_config() - # Sanitize all string values to prevent CRLF injection attacks - for key, value in config['default'].items(): - if isinstance(value, str): - config['default'][key] = value.replace('\r', '').replace('\n', '').replace('\x00', '') + if config.has_section('default'): + # Ordinary settings change: persist only what this caller changed. + # The `getattr` fallback covers a `cached_config` that is a plain dict + # (nothing in this tree does that - it is here so a caller that + # bypasses get_config() degrades to the old full rewrite rather than + # writing nothing at all). + changed_keys = getattr(cached, 'dirty_keys', set(WRITTEN_CONFIG_KEYS)) + # Intersecting with WRITTEN_CONFIG_KEYS means a key this function does + # not own is NOT persisted even when a caller dirties it - unchanged + # from before, since those keys were never written either. Two such + # keys are live in the config today (`http_channel_enabled`, + # `default_cache_as_channel_url`): an endpoint that wants to persist + # one must add it to WRITTEN_CONFIG_KEYS, or the write will silently + # do nothing. + keys = [key for key in WRITTEN_CONFIG_KEYS if key in changed_keys] + else: + # Bootstrap: no `[default]` on disk (fresh install - the caller at + # manager_server.py:2098 is guarded by `if not os.path.exists`), or a + # file that could not be parsed. Seed the full default set. + config['default'] = {} + keys = list(WRITTEN_CONFIG_KEYS) + + updates = {key: cached[key] for key in keys} + # `preview_method` is written on EVERY write, from the live value rather + # than from disk - it is the one setting the running process, not the + # file, is authoritative for. So it is also the one key a hand edit does + # not survive; that is unchanged behaviour, not a gap in the merge. + updates['preview_method'] = manager_funcs.get_current_preview_method() + + section = config['default'] + for key, value in updates.items(): + # Sanitize to prevent CRLF injection. Applied to the values written + # here; values already on disk are carried through verbatim, so the + # merge cannot corrupt hand-maintained content. + section[key] = str(value).replace('\r', '').replace('\n', '').replace('\x00', '') directory = os.path.dirname(manager_config_path) if not os.path.exists(directory): @@ -1715,6 +1827,22 @@ def write_config(): with open(manager_config_path, 'w') as configfile: config.write(configfile) + # The write succeeded, so these keys now MATCH the file and are no longer + # pending changes. Without this the set is add-only: a key the user once + # changed through the UI stays dirty for the life of the process, and the + # next unrelated write overlays its stale cached value back over whatever + # the user has hand-edited since - the same clobber this function exists to + # remove, just deferred by one settings change. + # + # Discard exactly the keys THIS write persisted, never the whole set: a + # caller may have dirtied a key that was not selected here - one this + # function does not own, and therefore one it never persists at all (the + # WRITTEN_CONFIG_KEYS note above). That marker is not this write's to + # drop; `clear()` would lose it. The hasattr guard mirrors the `getattr` + # above, for a cache that is a plain dict. + if hasattr(cached, 'dirty_keys'): + cached.dirty_keys.difference_update(keys) + def read_config(): try: @@ -1789,7 +1917,11 @@ def get_config(): global cached_config if cached_config is None: - cached_config = read_config() + # Wrapped HERE, after read_config() has returned: the migration hook it + # calls internally mutates the config through a parameter alias, and + # that mutation must NOT count as a user change (see + # DirtyTrackingConfig). + cached_config = DirtyTrackingConfig(read_config()) if cached_config['http_channel_enabled']: print("[ComfyUI-Manager] Warning: http channel enabled, make sure server in secure env") diff --git a/glob/manager_server.py b/glob/manager_server.py index 11a7a2f38..4b8826260 100644 --- a/glob/manager_server.py +++ b/glob/manager_server.py @@ -33,12 +33,13 @@ logging.info("[ComfyUI-Manager] network_mode: " + core.get_config()['network_mod comfy_ui_hash = "-" comfyui_tag = None -SECURITY_MESSAGE_MIDDLE_OR_BELOW = "ERROR: To use this action, a security_level of `middle or below` is required. Please contact the administrator.\nReference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" +SECURITY_MESSAGE_MIDDLE_OR_BELOW = "ERROR: To use this action, security_level must be any value other than 'strong' (allowed: normal, normal-, weak). security_level is read once at ComfyUI startup, so changing it needs a restart, done with the server down: STOP ComfyUI, edit config.ini, then start it again. Please contact the administrator.\nReference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" SECURITY_MESSAGE_NORMAL_MINUS = "ERROR: To use this feature, you must either set '--listen' to a local IP and set the security level to 'normal-' or lower, or set the security level to 'middle' or 'weak'. Please contact the administrator.\nReference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" SECURITY_MESSAGE_GENERAL = "ERROR: This installation is not allowed in this security_level. Please contact the administrator.\nReference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_NORMAL_MINUS_MODEL = "ERROR: Downloading models that are not in '.safetensors' format is only allowed for models registered in the 'default' channel at this security level. If you want to download this model, set the security level to 'normal-' or lower." -SECURITY_MESSAGE_FLAG_GIT_URL = "ERROR: This action requires 'allow_git_url_install = true' in config.ini ([default] section). This setting is independent of security_level. Reference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" -SECURITY_MESSAGE_FLAG_PIP = "ERROR: This action requires 'allow_pip_install = true' in config.ini ([default] section). This setting is independent of security_level. Reference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" +SECURITY_MESSAGE_NORMAL_MINUS_MODEL = "ERROR: Downloading models not in '.safetensors' format is only allowed for models registered in the 'default' channel at this security level. To download this model, set security_level to 'weak' - or to 'normal-' if ComfyUI is listening on a loopback address (--listen 127.0.0.1 or ::1). Both security_level and the listen address are read once at ComfyUI startup, so changing either needs a restart, done with the server down: stop ComfyUI, edit config.ini, then start it again." +SECURITY_MESSAGE_FLAG_GIT_URL = "ERROR: This action requires BOTH: (1) 'allow_git_url_install = true' in config.ini ([default] section), AND (2) ComfyUI launched with a loopback --listen (127.0.0.1 or ::1) - currently listening on {listen}. BOTH values are read once at ComfyUI startup, so changing either one needs a restart, done with the server down: STOP ComfyUI, change the setting, then start it again. Both are independent of security_level. Reference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" +SECURITY_MESSAGE_FLAG_PIP = "ERROR: This action requires BOTH: (1) 'allow_pip_install = true' in config.ini ([default] section), AND (2) ComfyUI launched with a loopback --listen (127.0.0.1 or ::1) - currently listening on {listen}. BOTH values are read once at ComfyUI startup, so changing either one needs a restart, done with the server down: STOP ComfyUI, change the setting, then start it again. Both are independent of security_level. Reference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" +SECURITY_MESSAGE_BLOCKED_RISK = "ERROR: This node pack is classified as blocked-risk and cannot be installed at any security_level. This is not a configuration problem - contact the administrator if you believe the classification is wrong.\nReference: https://github.com/ltdrdata/ComfyUI-Manager#security-policy" routes = PromptServer.instance.routes @@ -1494,12 +1495,12 @@ async def install_custom_node(request): # term is load-bearing here — the 'middle' entry gate above has # no network-position term. if not is_dedicated_install_allowed(core.get_config()['allow_git_url_install'], args.listen): - logging.error(SECURITY_MESSAGE_FLAG_GIT_URL) + logging.error(SECURITY_MESSAGE_FLAG_GIT_URL.format(listen=args.listen)) return web.Response(status=404, text="A security error has occurred. Please check the terminal logs") elif not is_allowed_security_level(risky_level): # 'block' arm stays an unconditional deny (is_allowed_security_level # returns False for 'block'); 'middle'/'low' arms unchanged. - logging.error(SECURITY_MESSAGE_GENERAL) + logging.error(SECURITY_MESSAGE_BLOCKED_RISK) return web.Response(status=404, text="A security error has occurred. Please check the terminal logs") install_item = json_data.get('ui_id'), node_spec_str, json_data['channel'], json_data['mode'], skip_post_install @@ -1556,7 +1557,7 @@ async def fix_custom_node(request): @routes.post("/customnode/install/git_url") async def install_custom_node_git_url(request): if not is_dedicated_install_allowed(core.get_config()['allow_git_url_install'], args.listen): - logging.error(SECURITY_MESSAGE_FLAG_GIT_URL) + logging.error(SECURITY_MESSAGE_FLAG_GIT_URL.format(listen=args.listen)) return security_403_response(flag_token='allow_git_url_install') # Read the body as JSON (not raw text): a cross-origin
@@ -1586,7 +1587,7 @@ async def install_custom_node_git_url(request): @routes.post("/customnode/install/pip") async def install_custom_node_pip(request): if not is_dedicated_install_allowed(core.get_config()['allow_pip_install'], args.listen): - logging.error(SECURITY_MESSAGE_FLAG_PIP) + logging.error(SECURITY_MESSAGE_FLAG_PIP.format(listen=args.listen)) return security_403_response(flag_token='allow_pip_install') # JSON body (not raw text) for the same preflight-forcing reason as diff --git a/js/common.js b/js/common.js index 52ea7e21f..30af9c129 100644 --- a/js/common.js +++ b/js/common.js @@ -235,7 +235,7 @@ export async function install_pip(packages) { }); if(res.status == 403) { - await handle403Response(res, "To use this feature, set 'allow_pip_install = true' in config.ini ([default] section), then restart ComfyUI (the config is read once at startup). This setting is independent of security_level."); + await handle403Response(res, "To use this feature you need BOTH: (1) 'allow_pip_install = true' in config.ini ([default] section), AND (2) ComfyUI launched with a loopback --listen (127.0.0.1 or ::1). Both values are read once at ComfyUI startup, so changing either one needs a restart, done with the server down: stop ComfyUI, change the setting, then start it again."); return; } @@ -271,7 +271,7 @@ export async function install_via_git_url(url, manager_dialog) { }); if(res.status == 403) { - await handle403Response(res, "To use this feature, set 'allow_git_url_install = true' in config.ini ([default] section), then restart ComfyUI (the config is read once at startup). This setting is independent of security_level."); + await handle403Response(res, "To use this feature you need BOTH: (1) 'allow_git_url_install = true' in config.ini ([default] section), AND (2) ComfyUI launched with a loopback --listen (127.0.0.1 or ::1). Both values are read once at ComfyUI startup, so changing either one needs a restart, done with the server down: stop ComfyUI, change the setting, then start it again."); return; } diff --git a/tests/test_write_config_persistence.py b/tests/test_write_config_persistence.py new file mode 100644 index 000000000..cd1241b6f --- /dev/null +++ b/tests/test_write_config_persistence.py @@ -0,0 +1,471 @@ +"""GOAL #25 RED — write_config() clobbers hand-maintained config.ini content. + +Reproduces the three clobber classes the frozen MM +(`scratch/gm3-team/g25-mm.md`) records, as born-RED pytest nodes, plus two +born-GREEN regression guards. Node ids are the RED contract: the GREEN step +must drive these SAME ids to PASS. + +| node | MM row | AC | born | +|----------------------------------------|--------|------|-------| +| test_t1_hand_edited_flag_survives | T1 | [D1] | RED | +| test_t3_unwritten_key_survives | T3 | [D3] | RED | +| test_t4_non_default_section_survives | T4 | [D3] | RED | +| test_t2_each_caller_key_persists | T2 | [D2] | GREEN | +| test_t5_bootstrap_writes_defaults | T5 | S4 | GREEN | + +GOAL #53 adds one more node to the same harness: + +| node | AC | born | +|----------------------------------------|------|-------| +| test_t7_silent_read_failure_is_loud | [D1] | RED | + + T7 — `config.read()` SILENTLY suppresses OSError. A config.ini that exists + but cannot be read leaves the parser empty, so the merge falls into the + BOOTSTRAP branch and rewrites the file from defaults, destroying every + foreign key and non-`[default]` section. The failure must be loud, and + the file must be left untouched. + +Why they fail today (MM State Model, measured at 30bd355e): + T1 — `write_config()` sources 17 of its 18 keys from `get_config()`, the + process-lifetime startup snapshot (`manager_core.py:1685-1704`), so a + hand edit made after startup is overwritten with the stale value. + T3 — `read_config()` returns 20 keys, `write_config()` persists 18; + `http_channel_enabled` and `default_cache_as_channel_url` are read but + never written, so every write deletes them. + T4 — `write_config()` builds a FRESH `ConfigParser`, assigns only + `config['default']`, and opens the file `'w'` (`:1715`), erasing every + other section. + +Harness: the subprocess-isolated child from `tests/test_install_flags_config.py` +— stub `folder_paths`, `glob/` on the CHILD's sys.path only, `manager_core` +imported (NEVER `manager_server`: module level starts a network thread at +`:2093`), `manager_config_path` pointed at a tmp file, `cached_config` reset. + +`force_security_level_if_needed` (MM HAZARD C-1) is NOT stubbed — it runs on +the real `read_config` path. The `folder_paths` stub supplies a system-user +directory, so `has_system_user_api()` is True and the function takes its +no-force branch and returns without mutating. The alias-mutation branch is +therefore present but not exercised here; the GREEN step must keep it in view +when it pins the dirty-tracking wrap point. +""" +import json +import subprocess +import sys +import textwrap +import unittest +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent + +#: The real mutation shape every settings caller uses (MM Action Map): a +#: subscript assignment on the cached dict, then `write_config()`. Verified +#: exhaustive by AST at 30bd355e — 6 sites, no other shape. +_CALLER_KEYS = [ + ("db_mode", "local"), # manager_server.py:1779 -> :244 + ("component_policy", "higher"), # manager_server.py:1793 -> :238 + ("update_policy", "nightly-comfyui"), # manager_server.py:1806 -> :241 + ("channel_url", "https://example.invalid/ch"), # manager_server.py:1833 -> :1832 + ("share_option", "none"), # share_3rdparty.py:60 -> :59 +] + +_CHILD_PREAMBLE = textwrap.dedent( + """ + import sys, types, tempfile, os, json, configparser + tmp = tempfile.mkdtemp(prefix="cm_wcfg_") + stub = types.ModuleType("folder_paths") + stub.get_user_directory = lambda: tmp + stub.get_system_user_directory = lambda *a, **k: os.path.join(tmp, "sysuser") + sys.modules["folder_paths"] = stub + sys.path.insert(0, {glob_path!r}) + import manager_core + CONFIG_PATH = os.path.join(tmp, "config.ini") + manager_core.manager_config_path = CONFIG_PATH + manager_core.cached_config = None + + def write_ini(text): + with open(CONFIG_PATH, "w") as f: + f.write(text) + + def read_raw(): + with open(CONFIG_PATH) as f: + return f.read() + + def parse_disk(): + cp = configparser.ConfigParser(strict=False) + cp.read(CONFIG_PATH) + return cp + + def boot(): + \"\"\"Simulate process start: read config.ini once into the cache.\"\"\" + manager_core.cached_config = None + return manager_core.get_config() + + def settings_change(key, value): + \"\"\"The real caller shape: mutate the cached dict, then write.\"\"\" + manager_core.get_config()[key] = value + manager_core.write_config() + """ +) + + +def _run_child(body): + """Run a scenario body in the isolated child; return its JSON payload.""" + script = _CHILD_PREAMBLE.format(glob_path=str(REPO_ROOT / "glob")) + textwrap.dedent(body) + proc = subprocess.run( + [sys.executable, "-c", script], + capture_output=True, + text=True, + timeout=180, + cwd=str(REPO_ROOT), + ) + if proc.returncode != 0: + raise AssertionError( + "write-config-harness child failed (rc=%d). stderr tail:\n%s" + % (proc.returncode, "\n".join(proc.stderr.strip().splitlines()[-8:])) + ) + lines = proc.stdout.strip().splitlines() + if not lines: + raise AssertionError( + "write-config-harness child exited 0 but produced no stdout. stderr tail:\n%s" + % "\n".join(proc.stderr.strip().splitlines()[-8:]) + ) + try: + return json.loads(lines[-1]) + except json.JSONDecodeError as e: + raise AssertionError( + "write-config-harness child emitted a non-JSON last line: %r\nfull stdout:\n%s" + % (lines[-1], proc.stdout) + ) from e + + +class WriteConfigPersistenceTest(unittest.TestCase): + """MM Test Scenario Map T1-T5 (`scratch/gm3-team/g25-mm.md`).""" + + def test_t1_hand_edited_flag_survives(self): + """T1 / [D1] — a hand edit made while running survives a settings change. + + BORN-RED. The user follows GOAL #8's denial copy, edits config.ini, + then touches any Manager setting; `write_config()` re-writes the flag + from the startup snapshot and the edit is gone. + """ + payload = _run_child( + """ + write_ini("[default]\\nsecurity_level = normal\\nallow_git_url_install = false\\n") + boot() # startup snapshot: flag False + # user hand-edits config.ini while the server is UP + write_ini("[default]\\nsecurity_level = normal\\nallow_git_url_install = true\\n") + settings_change("db_mode", "local") # ordinary Manager settings change + cp = parse_disk() + print(json.dumps({ + "flag_on_disk": cp["default"].get("allow_git_url_install", ""), + "raw": read_raw(), + })) + """ + ) + self.assertEqual( + "true", str(payload["flag_on_disk"]).lower(), + "T1 [D1]: the hand-edited allow_git_url_install=true was reverted to " + "%r by an unrelated settings change. write_config() sources 17 of 18 " + "keys from the startup snapshot (manager_core.py:1685-1704), so the " + "user is denied again after restarting despite following the denial " + "message exactly. On-disk file:\n%s" + % (payload["flag_on_disk"], payload["raw"]), + ) + + def test_t3_unwritten_key_survives(self): + """T3 / [D3] — a key read_config reads but write_config never writes. + + BORN-RED. `http_channel_enabled` gates the security warning at + `manager_core.py:1793`; every write deletes it from the file. + """ + payload = _run_child( + """ + write_ini( + "[default]\\nsecurity_level = normal\\n" + "http_channel_enabled = true\\ndefault_cache_as_channel_url = true\\n" + ) + boot() + settings_change("db_mode", "local") + cp = parse_disk() + print(json.dumps({ + "http_channel_enabled": cp["default"].get("http_channel_enabled", ""), + "default_cache_as_channel_url": cp["default"].get( + "default_cache_as_channel_url", ""), + "raw": read_raw(), + })) + """ + ) + # NOT subTest: pytest-subtests reports a SUBFAIL while leaving the + # NODE status PASSED, which would break the node-id RED contract this + # WI pins — the GREEN step keys on these ids. An aggregate assertion + # keeps the per-key detail in the message AND fails the node. + lost = [ + key for key in ("http_channel_enabled", "default_cache_as_channel_url") + if payload[key] == "" + ] + self.assertEqual( + [], lost, + "T3 [D3]: %r were present in config.ini and are GONE after an " + "unrelated settings change. read_config() returns 20 keys, " + "write_config() persists 18 — these keys are read but never " + "written, so every write deletes them. On-disk file:\n%s" + % (lost, payload["raw"]), + ) + + def test_t4_non_default_section_survives(self): + """T4 / [D3] — a non-`default` section survives a settings change. + + BORN-RED. `write_config()` builds a fresh ConfigParser, assigns only + `config['default']`, and opens `'w'` (`manager_core.py:1715`). + """ + payload = _run_child( + """ + write_ini( + "[default]\\nsecurity_level = normal\\n\\n" + "[my_section]\\nmy_key = keepme\\n" + ) + boot() + settings_change("db_mode", "local") + cp = parse_disk() + print(json.dumps({ + "sections": cp.sections(), + "my_key": cp["my_section"].get("my_key", "") + if cp.has_section("my_section") else "
", + "raw": read_raw(), + })) + """ + ) + self.assertIn( + "my_section", payload["sections"], + "T4 [D3]: the user's [my_section] was erased by an unrelated " + "settings change — write_config() writes only config['default'] " + "into a fresh parser with mode 'w'. Sections left on disk: %r\n%s" + % (payload["sections"], payload["raw"]), + ) + self.assertEqual("keepme", payload["my_key"]) + + def test_t2_each_caller_key_persists(self): + """T2 / [D2] — regression guard, expected born-GREEN. + + Each settings caller's OWN key must reach disk with its new value. This + is what a fix must not break; it is exercised at the manager_core layer + with the same mutation + `write_config()` the handlers make, because + importing `manager_server` starts a network thread (`:2093`). + """ + payload = _run_child( + """ + import json as _j + cases = %s + out = {} + for key, value in cases: + write_ini("[default]\\nsecurity_level = normal\\n") + boot() + settings_change(key, value) + out[key] = parse_disk()["default"].get(key, "") + print(_j.dumps(out)) + """ % json.dumps(_CALLER_KEYS) + ) + # NOT subTest — same reason as T3: a regression guard whose node + # reports PASSED while a subtest fails cannot signal the regression + # it exists to catch. + wrong = { + key: payload[key] for key, value in _CALLER_KEYS + if payload[key] != value + } + self.assertEqual( + {}, wrong, + "T2 [D2]: these caller keys did not persist their new value: %r — " + "a fix must not regress the write path it is narrowing." + % (wrong,), + ) + + def test_t5_bootstrap_writes_defaults(self): + """T5 / S4 — regression guard, expected born-GREEN. + + The 7th `write_config()` caller (`manager_server.py:2098`) is guarded by + `if not os.path.exists(...)` (`:2096`) and runs only when config.ini is + absent, so it cannot clobber. Its bootstrap write must still produce a + usable defaults file. + """ + payload = _run_child( + """ + if os.path.exists(CONFIG_PATH): + os.remove(CONFIG_PATH) + manager_core.get_config() # the bootstrap pair at :2097-2098 + manager_core.write_config() + cp = parse_disk() + print(json.dumps({ + "exists": os.path.exists(CONFIG_PATH), + "security_level": cp["default"].get("security_level", ""), + "allow_git_url_install": cp["default"].get( + "allow_git_url_install", ""), + "key_count": len(cp["default"]), + })) + """ + ) + self.assertTrue(payload["exists"], "T5: bootstrap did not create config.ini") + self.assertEqual("normal", payload["security_level"]) + self.assertEqual( + "False", payload["allow_git_url_install"], + "T5: the install flag must bootstrap secure-by-default", + ) + self.assertEqual( + 18, payload["key_count"], + "T5: bootstrap wrote %d keys; write_config persists 18 " + "(manager_core.py:1685-1704). A change to that count is a scope " + "signal, not a nit." % payload["key_count"], + ) + + def test_t6_persisted_key_is_not_reclobbered(self): + """T6 / [D1] — a key stays writable by hand AFTER the UI has set it. + + BORN-RED. `dirty_keys` is add-only: `write_config()` never removes the + keys it just persisted. So a key the user once changed through the UI + remains dirty for the life of the process, and the NEXT unrelated + settings write overlays its stale cached value back over whatever the + user has since edited on disk — the same clobber class the merge-on- + write fix removes, just deferred by one settings change. + + This is the residual case: T1 covers a key the UI never touched, which + is why T1 passes while this fails. + """ + payload = _run_child( + """ + write_ini("[default]\\nsecurity_level = normal\\ndb_mode = cache\\n") + boot() + # 1. the user changes db_mode through the UI -> persisted, and the + # key is now marked dirty + settings_change("db_mode", "local") + after_ui = parse_disk()["default"].get("db_mode", "") + + # 2. the user hand-edits THAT SAME key on disk, touching nothing else + raw = read_raw() + assert "db_mode = local" in raw, raw + with open(CONFIG_PATH, "w") as f: + f.write(raw.replace("db_mode = local", "db_mode = remote")) + + # 3. one UNRELATED settings change + settings_change("component_policy", "higher") + + cp = parse_disk() + print(json.dumps({ + "after_ui": after_ui, + "db_mode_final": cp["default"].get("db_mode", ""), + "component_policy": cp["default"].get("component_policy", ""), + "raw": read_raw(), + })) + """ + ) + # Guard against a vacuous pass: step 1 must really have persisted, and + # step 3's own key must really have landed. Without these, a + # write_config() that wrote nothing at all would satisfy the assertion + # below for the wrong reason. + self.assertEqual("local", payload["after_ui"], "T6 precondition: the UI change did not persist") + self.assertEqual( + "higher", payload["component_policy"], + "T6 precondition: the unrelated settings change did not persist", + ) + self.assertEqual( + "remote", payload["db_mode_final"], + "T6 [D1]: the hand-edited db_mode=remote was reverted to %r by an " + "unrelated component_policy change. `db_mode` stayed in dirty_keys " + "after write_config() persisted it, so the stale cached value was " + "overlaid onto the re-read file. Any key the user has ever changed " + "through the UI becomes permanently un-hand-editable for the life " + "of the process. On-disk file:\\n%s" + % (payload["db_mode_final"], payload["raw"]), + ) + + def test_t7_silent_read_failure_is_loud(self): + """T7 / GOAL #53 [D1] — an UNREADABLE config.ini must not be wiped. + + BORN-RED. `configparser.ConfigParser.read()` opens each filename inside + a `try/except OSError` and just SKIPS the ones it could not open, so a + file that EXISTS but denies reads leaves `read()` returning normally + with an EMPTY parser. `write_config()` then sees no `[default]`, takes + the BOOTSTRAP branch, and rewrites the file from the default set — + silently destroying the foreign key and the user's `[my_section]`, + while the settings endpoint answers 200. + + The scenario is gm3-solver's measured boundary (GOAL #52 D2): reads + denied but writes allowed. When BOTH are denied the write itself + raises, so the failure is already loud and the file already survives; + this node pins the asymmetric case that is not. + """ + payload = _run_child( + """ + import builtins + + write_ini( + "[default]\\nsecurity_level = normal\\ndb_mode = cache\\n" + "http_channel_enabled = true\\n\\n" + "[my_section]\\nmy_key = keepme\\n" + ) + boot() + before = read_raw() + + # Deny READS of config.ini while leaving WRITES allowed — the + # asymmetric permission shape gm3-solver reproduced live. Reads go + # through builtins.open, which is what configparser.read() calls + # and whose OSError it swallows. + _real_open = builtins.open + deny = {"on": False} + + def guarded_open(file, mode="r", *args, **kwargs): + is_read = "r" in mode and "w" not in mode and "a" not in mode + if deny["on"] and is_read and os.path.abspath(str(file)) == os.path.abspath(CONFIG_PATH): + raise PermissionError(13, "Permission denied", str(file)) + return _real_open(file, mode, *args, **kwargs) + + builtins.open = guarded_open + raised = None + try: + deny["on"] = True + settings_change("db_mode", "local") + except Exception as e: + raised = "%s: %s" % (type(e).__name__, e) + finally: + deny["on"] = False + builtins.open = _real_open + + after = read_raw() + cp = parse_disk() + print(json.dumps({ + "raised": raised, + "unchanged": before == after, + "before": before, + "after": after, + "sections": cp.sections(), + "http_channel_enabled": cp["default"].get( + "http_channel_enabled", "") + if cp.has_section("default") else "", + })) + """ + ) + self.assertIsNotNone( + payload["raised"], + "T7 [D1]: write_config() returned SUCCESSFULLY on a config.ini it " + "could not read. config.read() suppressed the PermissionError, so " + "the empty parser routed the write into the bootstrap branch and " + "the settings change reports success while destroying the file. " + "Sections left on disk: %r\nOn-disk file after the write:\n%s" + % (payload["sections"], payload["after"]), + ) + self.assertTrue( + payload["unchanged"], + "T7 [D1]: config.ini was MODIFIED by a settings change that could " + "not read it. A write that cannot see the current contents must " + "leave them alone.\n--- before ---\n%s\n--- after ---\n%s" + % (payload["before"], payload["after"]), + ) + # Spell out the two losses the byte-comparison above rolls up, so a + # failure names the user-visible damage and not just "bytes differ". + self.assertIn("my_section", payload["sections"], "T7 [D1]: [my_section] was erased") + self.assertEqual( + "true", str(payload["http_channel_enabled"]).lower(), + "T7 [D1]: http_channel_enabled was erased", + ) + + +if __name__ == "__main__": # pragma: no cover + unittest.main()