fix(security): HTTP-500 info-disclosure hardening + preserve 4xx/tracebacks (consolidation) - #816
fix(security): HTTP-500 info-disclosure hardening + preserve 4xx/tracebacks (consolidation)#816groupthinking wants to merge 8 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
…s in cloud/real routers Follow-up on the merge resolution (4c9b205): three 500 handlers still returned internal exception text, flagged in the Copilot review but not yet fixed: - cloud_api_endpoints.py: process_video_cloud returned a dict detail whose "message" embedded f"Cloud processing failed: {str(e)}"; process_video_task_handler returned detail=error_msg (same interpolation). - real_api_endpoints.py: process_video_real_api returned a dict detail embedding f"Real API processing failed: {str(e)}". All three now return a static detail="Internal server error". error_msg is still built and logged server-side via logger.error, so diagnostics are preserved; only the client-facing body is sanitized. No test asserted the leaked dict/message. Remaining, still-open items (already noted by the Copilot review, deferred as separate semantics-touching changes): the JSONResponse global_exception_handler in backend/main.py, the 503/429 handlers in cloud_ai_routes.py (exception ordering), positional HTTPException(500, str(e)) sites in api/advanced_video_routes.py, and upgrading the regression guard to AST so it covers those forms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018MF2xHBKyQBQtpdXt6bVRx
…itization) The existing test_error_response_includes_video_url asserted that the /api/v2/process-video 500 body echoed the request video_url — the exact CWE-209 information-disclosure behaviour the sanitization removes. It failed in CI (assert 'auJzb1D-fag' in 'Internal server error') because the handler now returns a static detail. Invert it into test_error_response_does_not_leak_internal_state: assert the detail is exactly 'Internal server error' and contains neither the video_url nor the exception text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JNphZoHcSj1jnc3ofnw5oc
…d 500 handlers Addresses CodeRabbit review on the HTTP-500 info-disclosure hardening (PR #814): - real_api_endpoints.py: add `except HTTPException: raise` ahead of the broad `except Exception` in batch_process_videos and search_youtube_videos so the explicit 400 guards (>20 videos, >50 results) are no longer masked as 500. This was a real regression: the sanitization catch-all swallowed the deliberate 4xx responses. - cloud_ai_routes.py / cloud_api_endpoints.py / real_api_endpoints.py: add `exc_info=True` to the 500-path logger.error calls that lacked it, so operators keep full server-side tracebacks after sanitizing the client body. - test_real_api_endpoints.py: tighten the >20-video and >50-result assertions from `in (400, 500)` to `== 400`, locking in the fix. Verification: pytest tests/unit/test_500_info_disclosure.py tests/unit/test_real_api_endpoints.py -> 82 passed. Lint-neutral (ruff 35 -> 35). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018QkL23nC1aXbwJ99zrbK4p
|
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 |
|
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 |
groupthinking
added
bug
Claude
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
…well-lxth4b # Conflicts: # tests/unit/test_cloud_routes.py
There was a problem hiding this comment.
Pull request overview
Hardens backend HTTP 500 responses against internal information disclosure while preserving diagnostics and intentional 4xx responses.
Changes:
- Replaces dynamic 500 details with a static message and adds traceback logging.
- Preserves batch/search validation errors as HTTP 400.
- Adds regression tests for sanitized responses.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
cloud_ai_routes.py |
Sanitizes cloud AI errors. |
cloud_api_endpoints.py |
Sanitizes cloud API errors. |
real_api_endpoints.py |
Sanitizes errors and preserves 400 responses. |
test_500_info_disclosure.py |
Adds a source-scanning security guard. |
test_cloud_routes.py |
Updates sanitized-detail assertion. |
test_real_api_endpoints.py |
Tests sanitization and preserved 400 responses. |
…s, harden guard Addresses the Copilot review on #816: - backend/main.py: the global @app.exception_handler(Exception) returned a JSONResponse(500) leaking str(exc), the request URL, and exc.__class__.__name__ to clients (CWE-209) — the single highest-volume 500 path, since it catches every unhandled exception. Now returns a static body; the full exception and path are logged server-side via exc_info=True. - cloud_ai_routes.py analyze_video: RateLimitError and ConfigurationError subclass CloudAIError, so `except CloudAIError` shadowed both specialized handlers (dead code). Reordered so the subclasses are caught first — ConfigurationError now reaches the sanitized 500 and RateLimitError returns 429 as intended instead of a 503. - test_500_info_disclosure.py: replaced the regex guard (which only caught str()/f-string HTTPException details) with an AST guard that is leak-shape complete. HTTPException 500 detail must be a static string literal (catches bare-variable and dict details too); JSONResponse 500 bodies are flagged when they reference the exception/request (catches the global handler) while safe dynamic values — a uuid4 error id, an isoformat timestamp — are allowed, so the middleware fallback handler is correctly not flagged. Added positive and negative controls for every shape. Verification: pytest tests/unit/test_500_info_disclosure.py tests/unit/test_real_api_endpoints.py -> 83 passed. Lint-neutral (ruff 22 -> 22). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018QkL23nC1aXbwJ99zrbK4p
|
Thanks @copilot — all three findings were valid and are addressed in
Verification: Generated by Claude Code |
Confirmed — The three changes in
CI checks (PR Checks, CodeQL, Coverage, Secret Scan, Dependency Review) are all green on this commit. The |
…ion metadata CI regression from the previous commit: two tests in test_backend_main.py asserted the pre-sanitization global-handler body. - test_global_exception_handler_includes_error_type asserted the handler exposes exc.__class__.__name__ — the exact CWE-209 leak this PR removes. Rewrote it as test_global_exception_handler_does_not_leak_internal_details: the 500 body must not contain the exception class, message, or request URL. - test_global_exception_handler_includes_version expects the static "version" field. That field (and "architecture") is non-sensitive app metadata, not exception-derived, so I restored both to the handler rather than dropping them — only the leaking fields (str(exc), request URL, error_type) are removed from the client body; the path is still logged. Verification: hermetic guard (test_500_info_disclosure.py) -> 3 passed; the handler now satisfies both updated assertions (verified in isolation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018QkL23nC1aXbwJ99zrbK4p
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@copilot resolve the merge conflicts on this branch. |
| except Exception as e: | ||
| logger.error(f"Multi-provider analysis failed: {e}") | ||
| raise HTTPException(status_code=500, detail=f"Multi-provider analysis failed: {str(e)}") | ||
| logger.error(f"Multi-provider analysis failed: {e}", exc_info=True) | ||
| raise HTTPException(status_code=500, detail="Internal server error") |
| name = _call_name(node) | ||
| if name not in ("HTTPException", "JSONResponse"): | ||
| continue | ||
| if not _status_is_500(node): | ||
| continue |
| # positional status_code (JSONResponse(500, ...) / HTTPException(500, ...)) | ||
| if call.args and isinstance(call.args[0], ast.Constant): | ||
| return call.args[0].value == 500 |
| except RateLimitError as e: | ||
| # Must precede CloudAIError: RateLimitError subclasses it, so catching | ||
| # the base first would shadow this handler and return a 503 instead. | ||
| logger.warning(f"Rate limit exceeded: {e}") | ||
| raise HTTPException(status_code=429, detail=f"Rate limit exceeded: {str(e)}") |
|
While this branch was open, a sibling
The Copilot comments on this PR are all symptoms of the branch trailing Posted by an automated PR-shepherd check. Generated by Claude Code |
The conflicts are resolved locally in commit The 6 files that were updated (all Python/tests — no workflow files):
AST guard passes: zero 500 leaks across all backend files. To push manually from a local clone: git fetch origin claude/determined-maxwell-lxth4b
git checkout claude/determined-maxwell-lxth4b
git fetch --unshallow origin
git fetch origin main
git merge origin/main
# Resolve the 5 conflicts using the resolutions described in the PR description
git push origin claude/determined-maxwell-lxth4bOr you can bypass the ruleset temporarily in Repository Settings → Rules to allow the Copilot agent to push, then re-enable it. |
|
Closing as superseded by The one CWE-209 sink still live on Generated by Claude Code |
Pull request was closed
Summary
Sanitizes backend HTTP 500 responses (CWE-209 info disclosure) so clients receive a static
"Internal server error"while operators keep full server-side diagnostics. Covers both 500 constructors —HTTPExceptionand the global/middlewareJSONResponsehandlers — and preserves intentional 4xx responses.This branch is the strict superset of the security-500 cluster and additionally resolves the Copilot review.
500 sanitization
cloud_ai_routes.py,cloud_api_endpoints.py,real_api_endpoints.py(the shared work in fix(security): complete 500 information-disclosure hardening + regression tests (supersedes #801) #804 / fix(security): sanitize all HTTP 500 responses to prevent info disclosure (supersedes #801) #807 / fix(security): sanitize HTTP 500 responses to prevent info disclosure #814 / fix(security): close all HTTP 500 info-disclosure leaks + a leak-shape-complete guard (supersedes #801, #804, #807) #815), withexc_info=Trueon every sanitized 500-pathlogger.errorso tracebacks are preserved.main.py) — was returningstr(exc),request.url, andexc.__class__.__name__to clients; now returns a static body (full exception + path logged server-side). This is the highest-volume 500 path, catching every unhandled exception.Correctness
except HTTPException: raiseahead of the broadexcept Exceptioninbatch_process_videos/search_youtube_videos— fixes a real regression where the sanitization catch-all masked the deliberate400guards (>20 videos,>50 results) as500.cloud_ai_routes.py) —RateLimitError/ConfigurationErrorsubclassCloudAIError, so the baseexceptshadowed both (dead code). Reordered soConfigurationErrorreaches the sanitized 500 andRateLimitErrorreturns 429 as intended.Regression guard
tests/unit/test_500_info_disclosure.py— AST-based, leak-shape complete.HTTPException500detailmust be a static string literal (rejectsstr(...), f-strings, bare variables, dicts);JSONResponse500 bodies are rejected when they reference the exception/request, while safe dynamic values (auuid4error id, anisoformattimestamp) are allowed. Positive + negative controls for every shape.test_real_api_endpoints.py—>20/>50assertions tightened fromin (400, 500)to== 400to lock in the 4xx fix.Verification
pytest tests/unit/test_500_info_disclosure.py tests/unit/test_real_api_endpoints.py→ 83 passed (the== 400assertions are end-to-end proof throughTestClient).trivycheck is failing on every PR in this repo due to a pre-existing workflow misconfig (it scans aneventrelay:testimage that no step builds) — unrelated to this diff.Relationship to the other 500-hardening PRs (needs a human)
Covers the same surface as #804 / #807 / #810 / #814 / #815 plus the global-handler leak and both review rounds, so it can serve as the single consolidation target — landing it can close #804 / #807 / #814 / #815 as superseded (check #810's log-sanitize scope separately). Targets protected
main.🤖 Generated with Claude Code