fix(security): consolidate HTTP 500 info-disclosure hardening - #818
Closed
groupthinking wants to merge 5 commits into
Closed
fix(security): consolidate HTTP 500 info-disclosure hardening#818groupthinking wants to merge 5 commits into
groupthinking wants to merge 5 commits into
Conversation
…sure (supersedes #801) Multiple FastAPI handlers returned internal exception text to clients via `HTTPException(status_code=500, detail=str(e))` (or f-strings embedding `{e}`), leaking stack-adjacent messages, backend API errors, and database/Looker errors. This is an information-disclosure vector (CWE-209). #801 sanitized ~24 handlers but left three live endpoints leaking: `generate_video_pack` and `generate_blueprint` (v1/router.py) and the mounted `reporting_routes.py` dashboard endpoint — the exact gaps Copilot flagged on that PR. A tree-wide scan surfaced 13 further leaks in cloud_ai_routes.py, cloud_api_endpoints.py, and real_api_endpoints.py that #801 never touched. Changes: - Replace the dynamic `detail` in every 500 response across the backend with a static "Internal server error"; the full exception is now logged server-side (`logger.error(..., exc_info=True)`) so diagnostics are preserved. - Add reporting_routes.py a module logger (previously none). - Add tests/unit/test_500_info_disclosure.py: a hermetic source-scan guard that fails if any backend 500 response uses a dynamic `detail`, closing the test-coverage gap (existing exception-path tests asserted only status code, so they passed while the body leaked). Includes a self-check that the scanner detects a synthetic leak and ignores 4xx responses. 4xx responses (which echo client-supplied validation input) are intentionally left unchanged. Verified: all touched files compile; ruff findings are identical to the base branch (lint-neutral); guard test passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018MF2xHBKyQBQtpdXt6bVRx
The source-scan guard added for the 500 info-disclosure work only matched
inline detail=str(e) / detail=f"...{e}...". It silently passed while three
handlers still leaked internal state through detail shapes it did not model:
- cloud_api_endpoints.py process_video_cloud -> detail={... "message": error_msg ...}
- cloud_api_endpoints.py process_video_task -> detail=error_msg (bare variable)
- real_api_endpoints.py process_video_real_api -> detail={... "message": error_msg ...}
Fix all three to a static "Internal server error" (the exception is logged
server-side via logger.error(..., exc_info=True)), and rewrite the guard to
flag any 500 detail= that is not an inline static string literal — a leading
'{' (dict) or identifier char (f-string, str(), or a bare variable) now fails
the scan. Self-checks extended to cover the dict and variable shapes.
test_error_response_includes_video_url asserted the old leaking body; it is
replaced by test_error_response_is_sanitized, which asserts the 500 body is
exactly "Internal server error" and contains neither the exception nor the
caller-supplied video_url.
Strict superset of the #807 approach; the router.py/reporting_routes.py sinks
#804 targeted are already clean on main and covered by the tree-wide guard.
…leak, 400→500 swallow Round of fixes from the CodeRabbit full review and the Vercel VADE bot on #815: - main.py global_exception_handler leaked str(exc) and the exception class name (error_type) in its 500 JSONResponse body (VADE finding). Return a static body; the full exception is already logged with exc_info=True. - cloud_api_endpoints.process_video_task_handler persisted error_msg (containing str(e)) as the Firestore 'error_message', which get_video_status / get_video_result return verbatim to clients — exfiltrating the exception despite the sanitized 500. Persist a generic message; keep the detail in logs only. - real_api_endpoints batch_process_videos and search_youtube_videos caught their own HTTPException(400) validation errors in the broad 'except Exception' and turned them into 500s. Re-raise HTTPException first to preserve the 400. - cloud_ai_routes: add exc_info=True to the five 500-handler logger.error calls so the traceback is retained server-side. - Guard: exception handlers are a second 500 sink the HTTPException scan didn't model. Add test_no_disclosure_in_500_exception_handlers — it flags any @app.exception_handler that returns 500 while placing str(exc)/str(e) or __class__.__name__ in the response body (docstrings/comments/log lines excluded).
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
…ler test - analyze_video_multi_provider caught HTTPException(400) from parse_analysis_types in its broad 'except Exception' and re-raised it as a generic 500, masking a client error (flagged in review). Add 'except HTTPException: raise' to mirror analyze_video / analyze_batch_videos, plus a regression test asserting an invalid analysis type returns 400. - Update the stale global-exception-handler test: it asserted the 500 body leaks 'error_type' (the exception class name), which the info-disclosure hardening deliberately removes (CWE-209). Assert the secure contract instead: neither the exception type nor its message appears in the response. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014KW6wTcE4r8Uk11QpXBhVq
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This was referenced Jul 17, 2026
groupthinking
dismissed
coderabbitai[bot]’s stale review
July 17, 2026 04:18
The merge-base changed after approval.
groupthinking
pushed a commit
that referenced
this pull request
Jul 17, 2026
reporting_routes.generate_dashboard_url already returns a static
detail="Internal server error" (CWE-209 hardening), but
test_generate_dashboard_url_service_error still asserted the old
"Failed to generate" substring, leaving main CI red on the `test`
job (1 failed, 7163 passed).
Update the assertion to expect the sanitized body and to prove the
caught exception text ("Looker unavailable") does not leak. This is
the same test change already staged in PR #818; landing it directly
unblocks main.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014BmkwKENnbsy3v94m1sBid
Owner
Author
|
Closing as superseded by Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sanitizes all HTTP 500 responses across the backend so internal exception details, dict/variable contents, and persisted error records are never leaked to clients. This is the consolidated version of the 500 info-disclosure work and supersedes the earlier one-off attempts.
Changes:
backend/main.py— close global exception-handler detail leak; stop the 400→500 swallow that surfaced internal messages.backend/cloud_ai_routes.py,backend/cloud_api_endpoints.py,backend/real_api_endpoints.py— replace dynamicdetail=payloads with static, safe messages; log the real error server-side only.tests/unit/test_500_info_disclosure.py— new guard suite (4 tests) that scans handlers/responses for dynamic-detail leaks and includes synthetic-leak detectors to keep the guard honest.Verification
All four changed source files compile; no conflict markers introduced.
Relationship to other open PRs
A cluster of overlapping automated PRs targets this same concern: #801, #804, #807, #810, #814, #815, #816. This branch consolidates that work into one reviewable change and is intended to supersede them. Recommend closing the redundant PRs in favour of this one once reviewed (do not merge the cluster in parallel — they would conflict on the same handlers).
Opened as draft for human review — not auto-merged.
🤖 Generated with Claude Code
https://claude.ai/code/session_014KW6wTcE4r8Uk11QpXBhVq
Generated by Claude Code