[PR #3136] [CLOSED] fix: report the real reason a batch install or uninstall is denied #3045

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/ComfyUI-Manager/pull/3136
Author: @Dhevenddra
Created: 8/6/2026
Status: ❌ Closed

Base: main ← Head: fix-denial-diagnostics

📝 Commits (1)

27dc3a5 fix: report the real reason a batch install or uninstall is denied

📊 Changes

4 files changed (+6 additions, -6 deletions)

View changed files

📝 glob/manager_server.py (+2 -2)
📝 js/common.js (+0 -2)
📝 js/custom-nodes-manager.js (+0 -2)
📝 tests/test_install_flags_gates.py (+4 -0)

📄 Description

Fixes #3128.

Problem

1. The batch install path throws away the server's 404 body.

js/custom-nodes-manager.js replaced every 404 with a message about the "default channel" and the security level. The denial it fires on most often cannot be resolved by either. In install_custom_node, the risky_level == 'high' arm goes through is_dedicated_install_allowed(core.get_config()['allow_git_url_install'], args.listen), which is flag AND loopback and never reads security_level. Setting security_level = weak changes nothing, so the message is a false lead that users do follow.

The same branch also swallowed the informative 404s on that route, for example Following node pack doesn't provide `nightly` version, which the user never got to see.

2. The uninstall path carried the same substitution for a status it never returns.

uninstall_custom_node returns 403 or 200 only. The 404 arm in js/common.js could therefore only be reached when the endpoint itself was missing (an outdated or partly-installed Manager), where a note about channel configuration points in exactly the wrong direction.

3. SECURITY_MESSAGE_FLAG_GIT_URL omits half the gate.

The message asks for allow_git_url_install = true and adds that the setting is independent of security_level, but is_dedicated_install_allowed also requires a loopback listen address. Anyone on --listen 0.0.0.0 is told to enable a flag they already enabled, with no hint about the real bound. SECURITY_MESSAGE_FLAG_PIP has the identical gap.

Fix

Drop the 404 special case in both JS paths so the existing else arm surfaces the server's own body. This is what the 403 path already does, and the dedicated /customnode/install/git_url surface already names the responsible flag through js/common.js.
Name the loopback requirement in both flag messages.

The loopback bound itself is deliberate per #2991 and is unchanged here. This is diagnostics only: no gate, status code, or policy moves.

Evidence

python -m pytest tests -q on a clean 3.12 venv:

38 passed, 14 skipped, 22 subtests passed in 36.09s

ruff check . (the CI gate) passes.

The denial-copy guard in tests/test_install_flags_gates.py now asserts the loopback requirement. Reverting only glob/manager_server.py and re-running it fails as expected:

E   AssertionError: 'loopback' not found in "ERROR: This action requires
    'allow_git_url_install = true' in config.ini ([default] section).
    This setting is independent of security_level. ..."
    : constant must name the loopback requirement
FAILED tests/test_install_flags_gates.py::DenialConstantsTest::test_flag_constants_content

The JS side has no test harness in this repo, so those two changes are deletions that fall through to the existing else branch rather than new logic.

_{🔄 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/3136 **Author:** [@Dhevenddra](https://github.com/Dhevenddra) **Created:** 8/6/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix-denial-diagnostics` --- ### 📝 Commits (1) - [`27dc3a5`](https://github.com/Comfy-Org/ComfyUI-Manager/commit/27dc3a57e7c2f7838674ff2099e443278574af84) fix: report the real reason a batch install or uninstall is denied ### 📊 Changes **4 files changed** (+6 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `glob/manager_server.py` (+2 -2) 📝 `js/common.js` (+0 -2) 📝 `js/custom-nodes-manager.js` (+0 -2) 📝 `tests/test_install_flags_gates.py` (+4 -0) </details> ### 📄 Description Fixes #3128. ## Problem **1. The batch install path throws away the server's 404 body.** `js/custom-nodes-manager.js` replaced every 404 with a message about the `"default channel"` and the security level. The denial it fires on most often cannot be resolved by either. In `install_custom_node`, the `risky_level == 'high'` arm goes through `is_dedicated_install_allowed(core.get_config()['allow_git_url_install'], args.listen)`, which is `flag AND loopback` and never reads `security_level`. Setting `security_level = weak` changes nothing, so the message is a false lead that users do follow. The same branch also swallowed the informative 404s on that route, for example ``Following node pack doesn't provide `nightly` version``, which the user never got to see. **2. The uninstall path carried the same substitution for a status it never returns.** `uninstall_custom_node` returns 403 or 200 only. The 404 arm in `js/common.js` could therefore only be reached when the endpoint itself was missing (an outdated or partly-installed Manager), where a note about channel configuration points in exactly the wrong direction. **3. `SECURITY_MESSAGE_FLAG_GIT_URL` omits half the gate.** The message asks for `allow_git_url_install = true` and adds that the setting is independent of `security_level`, but `is_dedicated_install_allowed` also requires a loopback listen address. Anyone on `--listen 0.0.0.0` is told to enable a flag they already enabled, with no hint about the real bound. `SECURITY_MESSAGE_FLAG_PIP` has the identical gap. ## Fix - Drop the 404 special case in both JS paths so the existing `else` arm surfaces the server's own body. This is what the 403 path already does, and the dedicated `/customnode/install/git_url` surface already names the responsible flag through `js/common.js`. - Name the loopback requirement in both flag messages. The loopback bound itself is deliberate per #2991 and is unchanged here. This is diagnostics only: no gate, status code, or policy moves. ## Evidence `python -m pytest tests -q` on a clean 3.12 venv: ``` 38 passed, 14 skipped, 22 subtests passed in 36.09s ``` `ruff check .` (the CI gate) passes. The denial-copy guard in `tests/test_install_flags_gates.py` now asserts the loopback requirement. Reverting only `glob/manager_server.py` and re-running it fails as expected: ``` E AssertionError: 'loopback' not found in "ERROR: This action requires 'allow_git_url_install = true' in config.ini ([default] section). This setting is independent of security_level. ..." : constant must name the loopback requirement FAILED tests/test_install_flags_gates.py::DenialConstantsTest::test_flag_constants_content ``` The JS side has no test harness in this repo, so those two changes are deletions that fall through to the existing `else` branch rather than new logic. --- <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:45 +00:00