Skip to content

fix: report the real reason a batch install or uninstall is denied - #3136

Open
Dhevenddra wants to merge 1 commit into
Comfy-Org:mainfrom
Dhevenddra:fix-denial-diagnostics
Open

fix: report the real reason a batch install or uninstall is denied#3136
Dhevenddra wants to merge 1 commit into
Comfy-Org:mainfrom
Dhevenddra:fix-denial-diagnostics

Conversation

@Dhevenddra

Copy link
Copy Markdown
Contributor

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.

Fixes Comfy-Org#3128.

The batch install path replaced every 404 body with a message about the
"default channel" and the security level. Neither can resolve the denial
it fires on most often: the risky_level == 'high' arm routes through
is_dedicated_install_allowed(), which reads the allow_git_url_install
flag and the listen address and never consults security_level at all. It
also swallowed the informative 404s on the same route, such as "Following
node pack doesn't provide `nightly` version". Dropping the branch lets
the existing else arm surface the server's own body, which is what the
403 path already does.

The uninstall path carried the same substitution, but that route returns
403 and 200 only, so the message could only ever appear when the endpoint
itself was missing, where a note about channels is actively misleading.

SECURITY_MESSAGE_FLAG_GIT_URL and SECURITY_MESSAGE_FLAG_PIP told users to
set a flag without mentioning that is_dedicated_install_allowed() also
requires a loopback listen address, so anyone running --listen 0.0.0.0
was sent to re-check a setting they had already enabled. Both messages
now name the loopback requirement, and the existing denial-copy guard
asserts it.
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: caaac627-b659-493b-a115-c25e74d7c420

📥 Commits

Reviewing files that changed from the base of the PR and between d47c934 and 27dc3a5.

📒 Files selected for processing (4)
  • glob/manager_server.py
  • js/common.js
  • js/custom-nodes-manager.js
  • tests/test_install_flags_gates.py
💤 Files with no reviewable changes (2)
  • js/custom-nodes-manager.js
  • js/common.js

📝 Walkthrough

Walkthrough

Changes

Installation denial diagnostics

Layer / File(s) Summary
Loopback-aware security messages
glob/manager_server.py, tests/test_install_flags_gates.py
Git URL and pip denial messages now state the required configuration flag and loopback listener. Tests verify the loopback requirement.
Batch 404 error handling
js/custom-nodes-manager.js, js/common.js
Batch install and uninstall requests now use the server response text for HTTP 404 errors instead of the default-channel message.

Possibly related PRs

Suggested reviewers: ltdrdata

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes remove misleading 404 substitutions and update Git URL and pip messages to state the loopback requirement, matching issue #3128.
Out of Scope Changes check ✅ Passed All changes directly support the messaging fixes in issue #3128; no unrelated code or policy changes are present.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai
coderabbitai Bot requested a review from ltdrdata August 6, 2026 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Batch install/uninstall map every 404 to the "default channel" message, masking the real denial reason

1 participant