[PR #3157] [MERGED] fix(#3128): state the real install gate in denial messages + stop write_config() clobbering hand-edited config.ini #3060

New issue

Closed

opened 2026-10-28 21:14:52 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-28 21:14:52 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/ComfyUI-Manager/pull/3157
Author: @ltdrdata
Created: 8/13/2026
Status: ✅ Merged
Merged: 8/13/2026
Merged by: @ltdrdata

Base: main ← Head: fix/issue-3128

📝 Commits (2)

ffb2f03 fix(security-messages): state the actual gate in install-denial messages (#3128)
aaed26a fix(config): merge write_config() onto disk instead of rebuilding from the startup snapshot (#3128)

📊 Changes

4 files changed (+640 additions, -36 deletions)

View changed files

📝 glob/manager_core.py (+158 -26)
📝 glob/manager_server.py (+9 -8)
📝 js/common.js (+2 -2)
➕ tests/test_write_config_persistence.py (+471 -0)

📄 Description

Fixes the diagnostic half of #3128, plus a config-persistence data-loss bug in the same area.

1. Install-denial messages state the actual gate

The allow_git_url_install / allow_pip_install policy (introduced in #2991) is
flag AND loopback: the install is allowed only when the flag is true and
the server listens on a loopback address (is_dedicated_install_allowed =
bool(flag) and is_loopback(listen)). The code and the README both documented
this, but the denial message shipped in #2991 mentioned only the flag:

ERROR: This action requires 'allow_git_url_install = true' in config.ini
([default] section). This setting is independent of security_level.

A user on --listen 0.0.0.0 who set the flag was still denied, with no hint why.
This is issue #3128 (C2). The messages now state both conditions, print the live
--listen value at the emission site, and give the correct restart guidance
(the values are read once at startup — stop the server, edit, then start):

SECURITY_MESSAGE_FLAG_GIT_URL / _FLAG_PIP — both conditions + live listen value.
SECURITY_MESSAGE_MIDDLE_OR_BELOW — dropped the non-existent 'middle' value;
now names only real levels (any value other than strong).
Blocked-risk denials get a dedicated SECURITY_MESSAGE_BLOCKED_RISK instead of
reusing the security_level message (no security_level value can lift a block).
js/common.js dedicated-403 messages mirror the loopback clause.

No behavior change — only the messages. The predicate is untouched.

2. write_config() no longer reverts hand-edited config.ini

write_config() rebuilt config.ini's [default] section entirely from the
startup snapshot into a fresh parser opened 'w'. Any Manager settings change
therefore silently:

reverted values hand-edited while ComfyUI was running (the same flags §1 tells
users to edit),
deleted two read-but-never-written keys (http_channel_enabled,
default_cache_as_channel_url),
erased every non-[default] section.

get_config() now tracks which keys callers actually change; write_config()
re-reads config.ini and overlays only those keys plus the live preview_method.
Hand-edited values, unknown keys, and foreign sections survive; CRLF sanitization
still applies; an unparsable config.ini falls back to a full rewrite instead of
failing every settings endpoint.

Scope

Out of scope (deliberately): the client-side 404-body masking on the batch
install/uninstall path (#3128 C1) — the client discards the server body and
substitutes a "default channel" lead. Not addressed here.

Verification

tests/test_write_config_persistence.py — pins the three loss classes and the
settings round-trip (5 passed).
Live-server e2e on the fixed tree: real ComfyUI + this branch, denial message
carries the live listen value across loopback / arbitrary-loopback / 0.0.0.0
binds; real 403 dialog shows the new copy; hand-edit + foreign key + custom
section survive a settings change.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/ComfyUI-Manager/pull/3157 **Author:** [@ltdrdata](https://github.com/ltdrdata) **Created:** 8/13/2026 **Status:** ✅ Merged **Merged:** 8/13/2026 **Merged by:** [@ltdrdata](https://github.com/ltdrdata) **Base:** `main` ← **Head:** `fix/issue-3128` --- ### 📝 Commits (2) - [`ffb2f03`](https://github.com/Comfy-Org/ComfyUI-Manager/commit/ffb2f03e92f5f3979b97c10845b6b5405903f73c) fix(security-messages): state the actual gate in install-denial messages (#3128) - [`aaed26a`](https://github.com/Comfy-Org/ComfyUI-Manager/commit/aaed26a5a4df4558486f1a829cd609ff31ce3701) fix(config): merge write_config() onto disk instead of rebuilding from the startup snapshot (#3128) ### 📊 Changes **4 files changed** (+640 additions, -36 deletions) <details> <summary>View changed files</summary> 📝 `glob/manager_core.py` (+158 -26) 📝 `glob/manager_server.py` (+9 -8) 📝 `js/common.js` (+2 -2) ➕ `tests/test_write_config_persistence.py` (+471 -0) </details> ### 📄 Description Fixes the diagnostic half of #3128, plus a config-persistence data-loss bug in the same area. ## 1. Install-denial messages state the actual gate The `allow_git_url_install` / `allow_pip_install` policy (introduced in #2991) is `flag AND loopback`: the install is allowed only when the flag is `true` **and** the server listens on a loopback address (`is_dedicated_install_allowed` = `bool(flag) and is_loopback(listen)`). The code and the README both documented this, but the denial message shipped in #2991 mentioned only the flag: ERROR: This action requires 'allow_git_url_install = true' in config.ini ([default] section). This setting is independent of security_level. A user on `--listen 0.0.0.0` who set the flag was still denied, with no hint why. This is issue #3128 (C2). The messages now state both conditions, print the live `--listen` value at the emission site, and give the correct restart guidance (the values are read once at startup — stop the server, edit, then start): - `SECURITY_MESSAGE_FLAG_GIT_URL` / `_FLAG_PIP` — both conditions + live listen value. - `SECURITY_MESSAGE_MIDDLE_OR_BELOW` — dropped the non-existent `'middle'` value; now names only real levels (any value other than `strong`). - Blocked-risk denials get a dedicated `SECURITY_MESSAGE_BLOCKED_RISK` instead of reusing the security_level message (no security_level value can lift a block). - `js/common.js` dedicated-403 messages mirror the loopback clause. No behavior change — only the messages. The predicate is untouched. ## 2. write_config() no longer reverts hand-edited config.ini `write_config()` rebuilt config.ini's `[default]` section entirely from the startup snapshot into a fresh parser opened `'w'`. Any Manager settings change therefore silently: - reverted values hand-edited while ComfyUI was running (the same flags §1 tells users to edit), - deleted two read-but-never-written keys (`http_channel_enabled`, `default_cache_as_channel_url`), - erased every non-`[default]` section. `get_config()` now tracks which keys callers actually change; `write_config()` re-reads config.ini and overlays only those keys plus the live `preview_method`. Hand-edited values, unknown keys, and foreign sections survive; CRLF sanitization still applies; an unparsable config.ini falls back to a full rewrite instead of failing every settings endpoint. ## Scope - Out of scope (deliberately): the client-side 404-body masking on the batch install/uninstall path (#3128 C1) — the client discards the server body and substitutes a "default channel" lead. Not addressed here. ## Verification - `tests/test_write_config_persistence.py` — pins the three loss classes and the settings round-trip (5 passed). - Live-server e2e on the fixed tree: real ComfyUI + this branch, denial message carries the live listen value across loopback / arbitrary-loopback / 0.0.0.0 binds; real 403 dialog shows the new copy; hand-edit + foreign key + custom section survive a settings change. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-28 21:14:52 +00:00