docs(security): file idea.md findings from codebase security review#503
Conversation
Captures five findings from a read-only security review of the backend attack surface. Each is filed as a planned-feature idea.md for triage: - bug_cluster_url_ssrf_hostname_bypass (P2): cluster base_url validator only inspects literal IPs; DNS hostnames (incl. metadata.google.internal and any internal name) bypass the SSRF guard entirely. Link-local IPs are already covered by py3.11.4+/3.13 ipaddress semantics. - bug_request_id_header_unvalidated_log_injection (P2): client X-Request-ID adopted verbatim (no length/charset cap) → log injection + amplification. - chore_agent_confirmation_tool_name_word_boundary (P2): mutating-tool confirmation uses substring match + one affirmative blanket-authorizes multiple tools named in a turn. - chore_test_router_conditional_mount (P2): dev-only _test router mounted unconditionally; safety rests on per-route guards with no invariant test. - chore_cors_credentials_origin_hardening (Backlog/GA): allow_credentials with operator-configurable origins — latent until the auth surface lands. Several agent-reported items were verified as NOT vulnerabilities (git PR subprocess injection, webhook HMAC, SQL injection, TLS verify, secret logging) and intentionally omitted. Signed-off-by: Claude <noreply@anthropic.com>
Regenerate the planned-features dashboards to include the five new security-review idea folders (pre-commit hook did not fire in the remote sandbox). Signed-off-by: Claude <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds several design documents (ideas) outlining planned security-hardening features and bug fixes for MVP2 and GA. These include addressing a cluster URL SSRF hostname bypass, validating client-supplied request IDs to prevent log injection, tightening the agent confirmation tool matching, conditionally mounting the test router, and hardening CORS credential origins. The review comments provide important corrections to the proposed implementation details: pointing out that the suggested regex tokenization would break on tool names with underscores, warning that calling get_settings() at module load time violates design constraints, and advising against performing synchronous DNS resolution inside Pydantic validators to avoid blocking the FastAPI event loop.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| ### Bind the confirmation to a specific proposed tool | ||
|
|
||
| - Switch the tool-name test from substring to whole-word/boundary matching (mirror the `re.findall(r"[a-z]+", ...)` token approach already in `confirmation.py`). |
There was a problem hiding this comment.
Using the re.findall(r"[a-z]+", ...) tokenization approach from confirmation.py to match tool names will cause issues. In confirmation.py, re.findall(r"[a-z]+", ...) extracts only alphabetic characters, which means any tool name containing underscores (such as create_study, cancel_study, or open_pr) will be split into separate words (e.g., ['create', 'study']). Consequently, checking if the full tool name is in the set of words will always fail. Consider using a pattern that preserves underscores, such as re.findall(r"[a-z_]+", ...), or using regex word boundaries \b directly on the tool name.
|
|
||
| ### Make "dev-only" structural, not per-route | ||
|
|
||
| - Register the router conditionally: only `app.include_router(test_router.router, ...)` when `get_settings().environment == "development"`. This is belt-and-suspenders with the per-endpoint guard (keep both). |
There was a problem hiding this comment.
Calling get_settings() at module load time to conditionally register the router will violate the design constraint documented in backend/app/main.py (lines 18-21), which states that settings must not be read at module load to prevent unit tests from crashing when imported without the runtime stack. Consider attaching _require_development_env as a router-level dependency instead, or checking the environment variable directly via os.environ.get("ENVIRONMENT") or os.getenv to avoid initializing the full settings object at import time.
|
|
||
| ### Close the hostname SSRF path | ||
|
|
||
| - Resolve the hostname at validation time (and ideally re-check at connect time) and apply the same `is_private / is_loopback / is_link_local / is_reserved / is_multicast / is_unspecified` rejection to **every** resolved address, not just literal-IP hosts. Reject if any resolved A/AAAA record lands in a blocked range while `RELYLOOP_ALLOW_PRIVATE_CLUSTERS` is false. |
There was a problem hiding this comment.
Performing DNS resolution at validation time inside a Pydantic validator can introduce significant performance issues. Pydantic field validators are synchronous by default, so performing blocking network I/O (like DNS resolution) inside them will block the FastAPI event loop. It is highly recommended to perform DNS resolution and validation asynchronously at the service or controller layer (e.g., during the connection test or cluster registration handler) rather than during schema validation.
Adjudicated 3 Gemini Medium comments (all Accept) — each corrected the proposed fix approach in an idea.md: - SSRF: move DNS resolve-and-check to the async service/controller layer, not the sync Pydantic validator (would block the event loop). - confirmation: \b-anchored regex on the tool name; [a-z]+ would split create_study and never match. - test_router: prefer a router-level dependency; if a module-load check is used, read os.environ directly rather than building full settings. Signed-off-by: Claude <noreply@anthropic.com>
Gemini review adjudication (3 findings)All three Gemini Medium comments reviewed the proposed fix approaches in the idea.md files and were technically correct — all accepted and folded into the ideas in
These are refinements to future implementation guidance captured in planning docs — no code changed in this PR. Each idea still routes through cross-model review: Gemini Code Assist (live cross-family gate; GPT-5.5 unreachable in the remote sandbox) Generated by Claude Code |
2 participants
Summary
Read-only security review of the backend attack surface (git/PR/webhook, DB/query layer, engine adapters + LLM, API/middleware/agent). Each genuine finding is filed as a planned-feature
idea.mdfor triage — docs-only, no code/API/migration change (Alembic head stays0023).Findings filed (5 idea files)
base_urlSSRF — validator only inspects literal IPs; DNS hostnames (incl.metadata.google.internal) bypass it; DNS-rebinding defeats even a resolved check02_mvp2/bug_cluster_url_ssrf_hostname_bypassX-Request-IDadopted verbatim — no length/charset cap → log injection + amplification02_mvp2/bug_request_id_header_unvalidated_log_injection02_mvp2/chore_agent_confirmation_tool_name_word_boundary_testrouter mounted unconditionally — per-route guards with no invariant test (structural fragility)02_mvp2/chore_test_router_conditional_mountallow_credentials=Truewith operator-configurable origins — latent until auth/cookies land04_ga/chore_cors_credentials_origin_hardeningVerified NOT vulnerabilities (corrected from agent over-claims, no files filed)
169.254.169.254asis_private=True, so the AWS metadata IP is already blocked at the default config. The SSRF idea was rewritten around the genuine hostname-bypass gap._testroute is default-deny gated (!= "development"→ 404). Downgraded to a defense-in-depth idea.GIT_CONFIG_*, never argv, noshell=True), webhook HMAC (hmac.compare_digest, verified before parse), SQL/FTS layer (parameterized +plainto_tsquery+ sort allowlists + strict cursor decode), TLS (verify=Truedefault), and secret redaction were all confirmed sound.Test plan
Docs-only — no tests affected. Each idea.md follows the planned-feature template and routes to
/idea-preflight→/pipelinefor execution.https://claude.ai/code/session_01WmcRoU8EEzy7dZKB1Jkq34
Generated by Claude Code