Mc/updates

[ayomaska18]

• refactor to match graphite build humcp
• new tools
• refactor to fastmcp v3

[cyrusagent]

PR Review: Mc/updates (HuMCP Refactor + FastMCP v3 Upgrade)

Reviewer: Cyrus (automated review for ENG-104)

This is a large PR (427 files, ~78K additions) that upgrades to FastMCP v3, adds ~121 new tool implementations across 20 categories, refactors the humcp core with auth/middleware/permissions/storage modules, and expands .env.example comprehensively.

---

Critical Issues (Must Fix Before Merge)

1. auth.py:72 — is_auth_enabled() has a string membership bug

return auth_enabled in ("true")

("true") is not a tuple — it's just the string "true". Python evaluates "t" in "true" as True, meaning AUTH_ENABLED=t, AUTH_ENABLED=ru, etc. would all incorrectly return True. Fix:

return auth_enabled == "true"

2. server.py:153 — Accesses FastMCP private internals (_local_provider._components)

fn_tool = mcp._local_provider._components[f"tool:{tool_name}@"]

This reaches into undocumented private attributes with a magic key format ("tool:{name}@"). Any FastMCP v3 patch can break this silently. At minimum, wrap in try/except with a clear error message and file a FastMCP issue for a public API.

3. src/tools/database/postgres.py:120-121 — SQL Injection

result = await conn.execute(text(query))

User-supplied SQL is executed verbatim with no restrictions. Any authenticated user can run DROP TABLE, read pg_shadow, etc. Consider: read-only mode by default, query type allowlisting, or a dedicated read-only database role.

4. src/tools/storage/object_tools.py:206-210 — Path traversal on upload

upload_from_path does NOT call validate_local_path(), unlike download_to_path (line 373). A user could upload arbitrary filesystem files (e.g., /etc/shadow) to storage.

5. src/tools/builder/tools.py — No auth checks on builder tools

None of tool_builder_create, tool_builder_delete, tool_builder_enable, or execute functions call require_auth() or check_permission(). Any caller can create and execute arbitrary sandboxed code.

---

Security Concerns

6. routes.py:321 — XSS via token injection in login callback HTML

The access_token is interpolated directly into an inline <script> via f-string. If the token contains ' or </script>, it enables XSS. Escape for JS string context, or remove the localStorage line since the cookie is already set with httponly=True.

7. auth.py:57-61 — Debug logging forced in production

logging.getLogger("fastmcp.server.auth").setLevel(logging.DEBUG)

Runs at module import time unconditionally, leaking auth details (tokens, client secrets) into production logs. Gate behind a DEBUG_AUTH env var.

8. middleware.py:15-16 — Env vars read at import time (stale values)

If env vars are set after import (tests, late-binding containers), the middleware uses stale empty values, silently disabling API key auth.

9. routes.py:207 — _pkce_store is an unbounded in-memory dict

Each /login call adds an entry. If callbacks never fire, entries accumulate forever (memory leak / DoS). Add TTL-based expiry or size limits.

10. routes.py:168-169 — Hardcoded localhost:8080 for OAuth URLs

Breaks any non-local deployment. Should use MCP_SERVER_URL env var.

11. src/tools/builder/sandbox.py:105-107 — Sandbox guards are no-ops

_write_ is lambda obj: obj and _getitem_ is lambda obj, key: obj[key], disabling RestrictedPython's write protection. Also getattr (the real builtin) is exposed in safe builtins (line 77).

---

Code Quality & Consistency Issues

12. Inconsistent credential resolution

slack.py:47 and postgres.py:83 read API keys via os.getenv() directly, bypassing resolve_credential() used by all other tools.

13. Sync SDK calls in async functions block the event loop

• slack.py:89 — client.chat_postMessage() (sync WebClient)
• exa.py:139 — exa_client.search() (sync SDK)
• Brave search tools — sync methods in async functions

Use asyncio.to_thread() or async SDK variants.

14. ToolResponse[T] defined in schemas.py but never used

routes.py:491 returns {"result": result} instead of wrapping in ToolResponse. Inconsistent with CLAUDE.md response pattern.

15. routes.py:489 — exclude_none=True silently drops intentional None params

16. permissions.py:39-55 — check_permission ignores all parameters

object_type, object_id, relation accepted but never used. Callers may assume permissions are enforced.

17. middleware.py:39 — PUBLIC_PATHS incomplete

Missing /login, /login/callback, /logout, /mcp, /apps. OAuth login flow breaks if SERVICE_API_KEY is set.

18. Missing input validation

• brave.py news/image search — no query emptiness check
• slack.py — no text/channel validation
• postgres.py — no query emptiness check

---

Test Coverage Gap (Critical)

New core modules — 0% test coverage:

Module	Lines	Risk
auth.py	286	HIGH — security, monkey-patching
middleware.py	84	HIGH — API key/JWT validation
permissions.py	56	MEDIUM
storage_path.py	202	MEDIUM
credentials.py	16	LOW

121 new tool files — 0 tests.

At minimum, tests for auth.py, middleware.py, and permissions.py should be required before merge.

---

What Looks Good

• .env.example is comprehensive and well-organized
• Tool category structure follows CLAUDE.md patterns
• SKILL.md files provide good documentation
• Dependency additions are reasonable

---

Summary

Severity	Count
Critical (must fix)	5
Security concern	6
Code quality	7
Test coverage	Critical gap

Recommendation: Address critical issues (SQL injection, auth bug, path traversal, missing builder auth) and add tests for security-critical modules before merging.

[cyrusagent]

PR Review: Mc/updates — Refactor HuMCP to match graphite-workflow-builder

Overall, the PR makes significant progress aligning humcp with the graphite-workflow-builder implementation. The tool-authoring pattern (decorator, typed responses, SKILL.md) is well-executed across ~400+ tools. However, there are several critical security issues, architectural gaps, and documentation omissions that need attention before merge.

---

CRITICAL Issues

1. SQL Injection in src/tools/database/postgres.py

The execute_query tool accepts raw SQL and executes it via text(query) with no parameterization, no allowlisting, and no read-only restrictions. After mutation queries, it also does string-interpolated SELECT * FROM {table_name}. A misbehaving LLM agent or attacker could DROP TABLE, exfiltrate data, or run pg_read_file().

Recommendation: Default to read-only mode (SELECT/WITH only), block DDL/destructive statements, use parameterized queries where possible.

2. SSRF Vulnerability in src/tools/api/http_client.py

The http_request tool allows HTTP requests to any URL with no internal IP blocking. Attackers could probe cloud metadata endpoints (169.254.169.254), internal services, or private network infrastructure.

Recommendation: Block private/internal IP ranges (127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16). Consider a configurable domain allowlist.

3. Sandbox Write Guards Disabled in src/tools/builder/sandbox.py

RestrictedPython's _write_ guard is set to lambda obj: obj, effectively disabling it. Combined with _getitem_ also being a pass-through, this weakens the sandbox significantly. The 60s timeout has no CPU/memory limits.

Recommendation: Restore proper _write_ guards or use process-based sandboxing with ulimit constraints.

---

HIGH Issues

4. credentials.py — Simplified to env-only, loses multi-tenant security

The graphite-workflow-builder reference has per-actor credential lookup from iam.actor_credentials with encrypted storage and dev-only env fallback. The PR version is a trivial os.getenv() wrapper. This is fine for standalone mode but should be clearly documented as a stub.

5. permissions.py — check_permission() ignores all arguments

The function accepts object_type, object_id, relation but just calls require_auth(). Storage tools calling check_permission("storage_bucket", bucket, "editor") get zero authorization checking. This creates a false sense of security — the code looks like it has RBAC, but it doesn't.

Recommendation: Add clear warning comments/logs, or raise 403 for insufficient permissions like the reference does.

6. Google OAuth retained but removed from reference

The PR keeps create_auth_provider(), OAuth routes, and related imports in server.py and auth.py. The graphite-workflow-builder reference has none of this — only JWT or no auth. This dual-auth adds complexity without clear justification.

---

MEDIUM Issues

7. Blocking sync calls in async context

• src/tools/search/serpapi.py: search.get_dict() blocks the event loop
• src/tools/messaging/slack.py: All slack_sdk.WebClient calls are synchronous

Fix: Use asyncio.to_thread() or async SDK variants.

8. Inconsistent credential resolution

Slack reads SLACK_TOKEN via os.getenv() directly while GitHub correctly uses resolve_credential("GITHUB_TOKEN"). All tools should use the abstraction consistently.

9. Missing schema classes vs reference

PR lacks ToolResponseBase, ToolSuccessResponse[T], ToolErrorResponse, and create_response_model() factory that exist in the reference schemas.py.

10. Fallback value mismatch

PR uses "unknown" as fallback app name; reference uses "general". This could cause issues if downstream code checks for specific fallback values.

11. register_routes() signature diverges from reference

PR signature includes auth_provider, title, version, apps_count params. Reference signature is just (app, tools_path, tools).

12. Default port mismatch

PR defaults MCP_SERVER_URL to port 8080; reference uses 8003 (matching graphite-workflow-builder toolset).

13. Root endpoint exposes auth_enabled

Minor information disclosure — tells attackers whether auth is configured.

---

LOW Issues

14. pyproject.toml — 40+ new dependencies

Massive footprint (web3, moviepy, boto3, docker, etc.). Consider optional extras: pip install humcp[search], humcp[cloud].

15. Claude tool returns raw dict instead of typed Pydantic response

Every other tool returns typed ToolResponse[T] — the Claude tool breaks the pattern.

16. Naive datetimes in src/tools/builder/storage.py

datetime.now() without timezone; should use datetime.now(timezone.utc).

17. No client-side rate limiting on external API calls

A looping agent could exhaust API quotas for GitHub, Slack, SerpAPI, Claude, etc.

---

CLAUDE.md Not Updated

The PR introduces substantial new architecture but does NOT update CLAUDE.md:

• New app concept in RegisteredTool, ToolMetadata, decorator, schemas — undocumented
• New files (middleware.py, permissions.py, credentials.py, storage_path.py, playground.py) — not listed
• New src/apps/ directory with HTML bundles — not documented
• registry.py removed (merged into decorator.py) — still listed in CLAUDE.md
• JWT auth mode, /apps, /playground endpoints — not documented

This violates the project's own procedure of keeping CLAUDE.md in sync with architecture.

---

Tests — Insufficient Coverage

Only minimal test updates (app="test_app" added to RegisteredTool). No tests for:

• APIKeyMiddleware
• get_current_user_id() / JWT auth path
• permissions.py
• credentials.py
• playground.py
• App discovery and registration
• Root/playground/apps endpoints

---

Positive Patterns

• Consistent @tool() decorator usage across all tools
• Well-structured typed Pydantic response schemas
• Good error handling with specific exception types and logging
• Storage client has solid validation (bucket allowlisting, path traversal prevention)
• GitHub tool correctly uses resolve_credential()
• Input validation on pagination (capping per_page, limit)
• No hardcoded credentials found anywhere
• APIKeyMiddleware uses secrets.compare_digest for constant-time comparison
• storage_path.py identical to reference — clean implementation

---

Summary

The tool-authoring side of this PR is solid. The core framework changes need work: fix the security issues (SQL injection, SSRF, sandbox), decide on the auth strategy (JWT-only vs dual OAuth), update CLAUDE.md, add tests for new infrastructure, and align schemas/fallbacks with the reference implementation.

changes already took place