Skip to content

fix: address all critical (P0) security findings#27

Merged
hybridindie merged 2 commits intomainfrom
fix/critical-security-findings
Mar 12, 2026
Merged

fix: address all critical (P0) security findings#27
hybridindie merged 2 commits intomainfrom
fix/critical-security-findings

Conversation

@hybridindie
Copy link
Owner

Summary

  • Client _request hardening: Replaced getattr(c, method) dynamic dispatch with c.request(normalized, path) using httpx's official API, guarded by _ALLOWED_HTTP_METHODS frozenset
  • URL path traversal prevention: Added _validate_prompt_id() (UUID regex) and _validate_path_segment() for all URL-interpolated parameters (prompt_id, node_class, task_id)
  • Input mutation fix: Removed node_data["inputs"] = {} in validate_workflow that was mutating the caller's dict and could mask injection patterns before the workflow inspector runs
  • SSE transport warning: Added field_validator on SSESettings.host that logs a warning when SSE is bound to non-localhost (no auth mechanism exists)
  • Docker non-root: Container now runs as dedicated app user instead of root
  • GitHub Actions pinning: All actions in ci.yml and docker.yml pinned to full commit SHAs

Test plan

  • All 351 tests pass
  • ruff check clean
  • ruff format --check clean
  • mypy clean
  • All pre-commit hooks pass
  • Verify CI passes on GitHub

🤖 Generated with Claude Code

…eview

- Replace getattr dynamic dispatch in client._request with httpx c.request()
  and _ALLOWED_HTTP_METHODS guard to prevent method injection
- Add _validate_prompt_id (UUID regex) and _validate_path_segment validation
  for all URL-interpolated parameters to block path traversal
- Remove input mutation in validate_workflow that could mask injection
  patterns before the workflow inspector runs
- Add SSE host field_validator that warns when binding to non-localhost
- Run Docker container as non-root user (groupadd/useradd + USER app)
- Pin all GitHub Actions to full commit SHAs in ci.yml and docker.yml
- Update all test fixtures to use valid UUID prompt_ids

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 12, 2026 22:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Security hardening PR that addresses P0 findings: hardens HTTP dispatch in the client, adds URL path traversal prevention via input validation, removes an input mutation bug in workflow validation, warns on non-localhost SSE binding, and improves Docker/CI security posture.

Changes:

  • Replaced dynamic getattr HTTP dispatch with c.request() guarded by an allowlist, and added UUID/path-segment validation for URL-interpolated parameters
  • Removed node_data["inputs"] = {} mutation in validate_workflow and added SSE host warning validator
  • Pinned GitHub Actions to commit SHAs and switched Docker container to non-root user

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/comfyui_mcp/client.py Added HTTP method allowlist, UUID validation, and path segment validation for URL parameters
src/comfyui_mcp/workflow/validation.py Removed input mutation that could mask injection patterns
src/comfyui_mcp/config.py Added field_validator warning for non-localhost SSE binding
Dockerfile Added non-root app user
.github/workflows/ci.yml Pinned actions to commit SHAs
.github/workflows/docker.yml Pinned actions to commit SHAs
tests/* Updated test prompt IDs to valid UUID format matching new validation

You can also share your feedback on Copilot code review. Take the survey.

Dockerfile Outdated
Comment on lines +12 to +13
RUN groupadd --system app && useradd --system --gid app app
USER app
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The groupadd/useradd commands should be run before COPY and RUN uv sync so that the user creation layer is cached independently of source changes. Also, since files were copied as root, the app user may not have read access to /app. Consider adding --chown=app:app to the COPY directives or running chown before switching to USER app.

Copilot uses AI. Check for mistakes.
Address PR review feedback: create the app user early for better layer
caching and use --chown=app:app on COPY directives so the non-root user
can read the application files.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hybridindie hybridindie merged commit a6d62b3 into main Mar 12, 2026
3 checks passed
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.

2 participants