fix(security): consolidate HTTP 500 info-disclosure hardening + preserve client 4xx - #821
fix(security): consolidate HTTP 500 info-disclosure hardening + preserve client 4xx#821groupthinking wants to merge 7 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).
…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
|
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 |
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 |
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Hardens backend 500 responses against internal error disclosure while preserving intended client 4xx responses.
Changes:
- Replaces dynamic 500 details with static messages and server-side logging.
- Sanitizes persisted Firestore errors and the global exception handler.
- Adds regression guards and endpoint tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
cloud_ai_routes.py |
Sanitizes cloud AI errors and preserves 400 responses. |
cloud_api_endpoints.py |
Sanitizes API and persisted task errors. |
real_api_endpoints.py |
Sanitizes real API errors and preserves 400 responses. |
main.py |
Removes global exception detail disclosure. |
test_500_info_disclosure.py |
Adds source-scanning security guards. |
test_backend_main.py |
Verifies global-handler sanitization. |
test_cloud_routes.py |
Tests sanitized cloud route behavior. |
test_real_api_endpoints.py |
Tests sanitized video-processing errors. |
| _HTTP_EXC = re.compile(r"HTTPException\((?P<args>.*?)\)", re.DOTALL) | ||
|
|
||
| # A 500 `detail=` is safe only when it is an inline *static string literal* | ||
| # (``detail="Internal server error"``). Anything else can carry internal state to | ||
| # the client and is flagged: | ||
| # detail=str(e) detail=f"... {e} ..." (inline dynamic string) | ||
| # detail=error_msg (a variable — may hold f"...{e}...") | ||
| # detail={...} (a dict whose values embed the exception) | ||
| # The value after ``detail=`` is dynamic unless its first non-space character | ||
| # opens a plain string literal (``"`` or ``'``). A leading ``{`` (dict) or any | ||
| # identifier char — ``f`` of an f-string, ``s`` of ``str(``, or a bare variable | ||
| # name — means it is not a static literal. | ||
| _DYNAMIC_DETAIL = re.compile(r"""detail\s*=\s*(?:\{|[A-Za-z_])""") |
| await firestore_service.update_state( | ||
| payload.video_id, | ||
| status='failed', | ||
| error_message=error_msg | ||
| error_message="Internal server error" |
| except HTTPException: | ||
| # Preserve intentional client errors (e.g. the 400 above). | ||
| raise |
| except HTTPException: | ||
| # Preserve intentional client errors (e.g. the 400 above). | ||
| raise |
…well-bloicm # Conflicts: # tests/unit/test_cloud_routes.py
…st-error tests Addresses the Copilot review on #821: - Guard hole (advertised tree-wide invariant not enforced): the source scan only recognized keyword-form `status_code=500, detail=...`, so positional leaks `HTTPException(500, str(e))` and concatenations that start with a literal (`detail="x: " + str(e)`) slipped through. Rewrite the scan to parse the AST and inspect every HTTPException call — status and detail resolved from positional *and* keyword args; a 500 detail is safe only if it is a static string literal (or a `+` concat of only literals) or is omitted. Positive controls now cover the positional and concat shapes. - The upgraded guard surfaced 8 real positional CWE-209 leaks the keyword-only scan missed, all in api/advanced_video_routes.py (`HTTPException(500, str(e))` at lines 146/214/247/288/322/350/438/491). Each already logs with exc_info=True; replace the dynamic detail with a static "Internal server error". Tree-wide scan is now clean (0 sites). - Persisted-error sink was untested: test_process_video_task_exception asserted only the status. Assert update_state is awaited with error_message="Internal server error" and that the exception text is not persisted (status/result endpoints return error_message verbatim). - 4xx re-raises were not locked in: test_batch_with_more_than_20_videos and test_max_results_above_50 accepted 400-or-500 and documented the old masking bug. Tighten both to require exactly 400 and update the stale descriptions. All touched tests pass (166 + 58); ruff findings identical to base (lint-neutral).
|
Addressed all four Copilot findings in
All touched tests pass locally (166 + 58); ruff findings identical to base. Generated by Claude Code |
|
The most recent push to this branch ( # src/youtube_extension/backend/main.py — global_exception_handler (current tip)
error_detail = {
"error": "Internal server error",
"detail": str(exc), # ← line ~452: leaks internal exception text
...
}
if hasattr(exc, "__class__"):
error_detail["error_type"] = exc.__class__.__name__ # ← line ~460: leaks exception class
return JSONResponse(status_code=500, content=error_detail)So the PR is now in the worst state: it looks done (no conflicts, "closes 500 leaks") but merging it would leave the actual 500-body leak live on Required fix before merge (server-side logging via error_detail = {
"error": "Internal server error",
"detail": "Internal server error", # static — no str(exc)
"timestamp": datetime.now().isoformat(),
"path": str(request.url) if hasattr(request, "url") else "unknown",
"version": "2.0.0",
"architecture": "service-oriented",
}
# drop the error_type / __class__.__name__ line entirely
return JSONResponse(status_code=500, content=error_detail)(The I verified this exact resolution locally against current I did not push over Generated by Claude Code |
|
Closing as superseded by Generated by Claude Code |
Pull request was closed
Summary
Closes the remaining HTTP 500 information-disclosure vectors (CWE-209) across the FastAPI backend, where handlers returned internal exception text to clients via
HTTPException(status_code=500, detail=str(e))(or f-strings embedding{e}), plus two secondary sinks (a global exception handler and a persisted Firestore error field). Also fixes three handlers that swallowed clientHTTPException(400)validation errors into generic 500s.This branch is the most complete version of the 500-hardening effort and is intended as a single consolidation target for the scattered duplicate PRs on the same issue.
Changes
"Internal server error"; the full exception is logged server-side withlogger.error(..., exc_info=True)so diagnostics are preserved.cloud_ai_routes.py,cloud_api_endpoints.py,real_api_endpoints.py,main.pydetail=str(e), f-string, dict-shaped (detail={... "message": error_msg}), and bare-variable (detail=error_msg) leak shapes.main.pyglobal_exception_handlerno longer leaksstr(exc)or the exception class name (error_type) in its 500 body.cloud_api_endpoints.process_video_task_handlerno longer persistsstr(e)to the Firestoreerror_messagethatget_video_status/get_video_resultreturn verbatim to clients; it persists a generic message, detail stays in logs.analyze_video_multi_provider,batch_process_videos, andsearch_youtube_videosre-raiseHTTPExceptionbefore the broadexcept Exception, so an invalid input returns400, not a masked500.tests/unit/test_500_info_disclosure.py— a hermetic source-scan guard that fails if any backend 500 response uses a dynamicdetail, and a second check for exception-handler bodies that placestr(exc)/__class__.__name__in a 500 response. Includes self-checks that the scanner catches synthetic leaks and ignores 4xx.Relationship to other open PRs
mainalready merged an initial pass (a0fae34), but a cluster of open PRs still targets this same issue. This branch is a strict superset:router.py,reporting_routes.py, and the mounted dashboard endpoint leaking).router.py/reporting_routes.pysinks fix(security): complete 500 information-disclosure hardening + regression tests (supersedes #801) #804 targeted are already clean onmainand now covered tree-wide by the guard.Recommend the maintainer treat this as the consolidation PR and close the redundant ones (#804, #807, #810, #814, #815, #816, #818, #819) after review.
Verification
tests/unit/test_500_info_disclosure.py— 4 passed (guard + self-checks).Notes
Not auto-merged — targets protected
mainand needs human review + CI. Draft pending reviewer sign-off.🤖 Generated with Claude Code
Generated by Claude Code