Skip to content

security: validate custom node pack names#9256

Open
Ersa-tech wants to merge 2 commits into
invoke-ai:mainfrom
Ersa-tech:security/validate-custom-node-pack-name
Open

security: validate custom node pack names#9256
Ersa-tech wants to merge 2 commits into
invoke-ai:mainfrom
Ersa-tech:security/validate-custom-node-pack-name

Conversation

@Ersa-tech
Copy link
Copy Markdown

Summary

Validates custom node pack names before using them in filesystem paths during install and uninstall operations.

Security impact

The custom node installer derived pack_name directly from the submitted git source and then used it for clone targets, module loading, tags, and cleanup paths. This change rejects empty names, dot segments, path separators, and unsafe characters before any filesystem operation is attempted.

This is especially important on Windows, where backslashes in a URL segment can be interpreted as path separators when building Path targets.

Changes

  • Add a shared pack-name validator.
  • Use it when deriving names from install sources.
  • Use it before uninstall path construction.
  • Add regression coverage for valid git URLs, empty names, dot segments, and Windows separator traversal.

Verification

  • python -m py_compile invokeai/app/api/routers/custom_nodes.py tests/app/routers/test_custom_nodes.py

I could not run the focused pytest module locally because the checkout is missing the blake3 dependency before test collection reaches this module.

Signed-off-by: Security Researcher <security@example.com>
@github-actions github-actions Bot added api python PRs that change python files python-tests PRs that change python tests labels May 31, 2026
@Pfannkuchensack Pfannkuchensack self-assigned this Jun 2, 2026
@Pfannkuchensack
Copy link
Copy Markdown
Collaborator

Findings

Medium — invokeai/app/api/routers/custom_nodes.py:288-300: the directly-dangerous sink (uninstall rmtree) gained a guard but no test covers it.
The traversal-to-shutil.rmtree(target_dir) path in uninstall_custom_node_pack is exactly what makes this a security PR: target_dir = custom_nodes_path / pack_name followed by shutil.rmtree(target_dir) at line 318. The new _validate_pack_name(pack_name) call at line 298 is the guard. But the added tests (TestPackNameValidation) only exercise _extract_pack_name_from_source (the install/derivation side). Nothing asserts that the uninstall route, or _validate_pack_name itself, rejects .., ., foo/bar, or ..\x before reaching rmtree. A future refactor could drop the line-298 guard and every test would still pass while the parent-directory-deletion vector silently reopens.

  • Evidence chain: uninstall accepts pack_name path param directly (not through _extract_pack_name_from_source) -> builds custom_nodes_path / pack_name -> shutil.rmtree. Guard is _validate_pack_name, verified to reject .././foo/bar/..\x/foo\n and accept good_pack. The guard is correct; the coverage is not.
  • To expose this issue, add a test that calls _validate_pack_name("..") (and "foo/bar", ".") and asserts ValueError, plus a route-level test that DELETE of an invalid pack_name returns success=False and never invokes shutil.rmtree/workflow_records.delete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api python PRs that change python files python-tests PRs that change python tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants