fix(security): complete + regression-test HTTP 500 info-disclosure hardening - #820
Closed
groupthinking wants to merge 9 commits into
Closed
fix(security): complete + regression-test HTTP 500 info-disclosure hardening#820groupthinking wants to merge 9 commits into
groupthinking wants to merge 9 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).
…s, complete guard
Second review round (CodeRabbit incremental + Copilot + fixing the red test job):
- advanced_video_routes.py: 8 handlers used the *positional* form
`raise HTTPException(500, str(e))`, leaking the exception to clients. The guard
missed them because it only matched keyword `status_code=500`/`detail=`. Sanitize
all 8 (they already log with exc_info=True).
- uvai/ml/serve.py: the Ray Serve checkpoint endpoint returned
`JSONResponse({"error": str(exc)}, status_code=500)` — a client-facing leak.
Return a static body and log the exception.
- cloud_api_endpoints.py: log the Firestore state-update failure with exc_info=True.
- test_backend_main.py: the global-handler test still asserted body["error_type"];
that key is now gone, so the assertion KeyError'd and failed the CI test job.
Replace with assertions that the 500 body is sanitized (no exc message/class).
- test_real_api_endpoints.py: the batch(>20) and search(>50) tests allowed 400 OR
500; now that the handlers preserve the intentional 400, assert == 400 exactly.
Guard (test_500_info_disclosure.py) rewritten to be leak-shape-complete via
balanced-paren extraction. It now covers all three 500 sinks:
1. HTTPException — keyword AND positional detail (`HTTPException(500, str(e))`).
2. @app.exception_handler bodies (str(exc)/__class__.__name__).
3. raw JSONResponse(..., status_code=500) embedding the exception.
Scope extended to src/uvai/ml. Self-checks cover every shape and confirm 4xx is
ignored.
…via AST CodeRabbit noted the detail predicate treated any value *starting* with a quote as safe, so `detail="Request failed: " + str(exc)` (a BinOp that begins with a literal but concatenates the exception) would pass. Parse the detail expression and accept only a single `ast.Constant` string; every other node — Call, BinOp, JoinedStr, Name, Dict — is dynamic. Self-check gains keyword and positional concatenation cases. No production leak existed (the code is already sanitized); this closes the guard's last false-negative.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🔍 PR Validation |
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 |
The Ray-Serve checkpoint POST path now returns a static
{"error": "Internal server error"} on failure instead of leaking the
exception text (CWE-209). Update the stale test that still asserted the
leaked "Save failed" message, and add an assertion that the internal
message does not appear anywhere in the response body.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4zt7nGZZZxdd1gGTeY2s8
🔍 PR Validation |
…well-e9hha1 # Conflicts: # tests/unit/test_cloud_routes.py
This was referenced Jul 17, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Sanitizes HTTP 500 responses to prevent internal error disclosure and adds regression coverage.
Changes:
- Replaces dynamic 500 details with static messages while retaining server-side logging.
- Preserves intentional 400 responses and sanitizes persisted errors.
- Adds endpoint assertions and a source-scanning regression suite.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/youtube_extension/backend/real_api_endpoints.py |
Sanitizes v2 errors and preserves 400s. |
src/youtube_extension/backend/main.py |
Sanitizes the global exception response. |
src/youtube_extension/backend/cloud_api_endpoints.py |
Sanitizes cloud errors and persisted state. |
src/youtube_extension/backend/cloud_ai_routes.py |
Sanitizes cloud AI failures. |
src/youtube_extension/backend/api/advanced_video_routes.py |
Sanitizes positional HTTP 500 details. |
src/uvai/ml/serve.py |
Sanitizes checkpoint failure responses. |
tests/unit/test_500_info_disclosure.py |
Adds source-scanning regression guards. |
tests/unit/test_real_api_endpoints.py |
Verifies sanitized responses and preserved 400s. |
tests/unit/test_ml_serve.py |
Verifies sanitized checkpoint errors. |
tests/unit/test_cloud_routes.py |
Updates sanitized response expectations. |
tests/unit/test_backend_main.py |
Verifies global-handler sanitization. |
| import pytest | ||
|
|
||
| _REPO_ROOT = Path(__file__).resolve().parents[2] | ||
| _BACKEND = _REPO_ROOT / "src" / "youtube_extension" / "backend" |
Comment on lines
+127
to
+130
| nospace = re.sub(r"\s+", "", args) | ||
| first = parts[0].strip() if parts else "" | ||
| is_500 = "status_code=500" in nospace or first == "500" | ||
| if not is_500: |
Comment on lines
+157
to
+160
| _HANDLER_DECORATOR = re.compile(r"^\s*@\w+\.exception_handler\(", re.MULTILINE) | ||
| _HANDLER_DISCLOSURE = re.compile(r"str\(\s*(?:exc|e)\s*\)|__class__\.__name__") | ||
| # Triple-quoted docstrings, so prose that *mentions* str(exc) is not mistaken for code. | ||
| _TRIPLE_STR = re.compile(r'"""(?:.|\n)*?"""|\'\'\'(?:.|\n)*?\'\'\'') |
Comment on lines
+213
to
+225
| _JSON_DISCLOSURE = re.compile( | ||
| r"str\(\s*(?:exc|e|error)\s*\)" # str(exc) / str(e) / str(error) | ||
| r"|f[\"'][^\"']*\{[^{}]*\b" + _EXC_TOKENS + r"\b[^{}]*\}" # f"...{exc-ref}..." | ||
| ) | ||
|
|
||
|
|
||
| def _iter_json_500_disclosures(text: str): | ||
| """Yield (line_no, snippet) for JSONResponse 500s whose body embeds the exception.""" | ||
| for m in _JSON_RESPONSE.finditer(text): | ||
| args = _balanced_call_args(text, m.end() - 1) | ||
| if "status_code=500" not in re.sub(r"\s+", "", args): | ||
| continue | ||
| if _JSON_DISCLOSURE.search(args): |
🔍 PR Validation |
…iew)
Addresses four review findings on the regression guard, replacing the
regex/textual scan with a real ast walk:
- Scope: scan the whole deployed package src/youtube_extension (production
entry point youtube_extension.main:app lives outside backend/) plus
src/uvai/ml, not just backend/.
- Status: recognize 500 as the literal or the FastAPI constant
status.HTTP_500_INTERNAL_SERVER_ERROR, in keyword or positional form,
whitespace-insensitively (status_code = 500).
- Exception variable: derive it from the enclosing scope (except ... as NAME
and @app.exception_handler function parameter) so an arbitrary name (error,
problem) is tracked, not just e/exc.
- Indirection: intra-function taint propagation catches leaks reached through
a local (msg = str(exc); {"error": msg}); non-exception dynamic values
(correlation ids) are still allowed in 500 bodies.
Adds synthetic cases for each. Full-tree scan confirms no live leaks.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C4zt7nGZZZxdd1gGTeY2s8
🔍 PR Validation |
Owner
Author
|
Closing as superseded by Generated by Claude Code |
auto-merge was automatically disabled
July 19, 2026 05:00
Pull request was closed
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
Completes the HTTP-500 information-disclosure hardening and — the differentiating value — adds a comprehensive AST-based regression test suite so the class of leak cannot silently return.
Builds directly on top of current
main(which already carriesa0fae34 fix(security): complete information-disclosure hardening for 500 responses). This branch ismain+ 6 commits and merges cleanly (verified viagit merge-tree).Changes
detail=/ JSON-body leaks inreal_api_endpoints.py,cloud_api_endpoints.py,cloud_ai_routes.py,advanced_video_routes.py,main.py, anduvai/ml/serve.py(positional-arg + Ray-Serve 500 paths, global handler leak, persisted-error leak, 400->500 swallow).tests/unit/test_500_info_disclosure.py(364 lines): an AST-based guard that walks the route modules and fails if any 500 response embeds a dynamic value (dict, variable, f-string, or string concatenation), plus synthetic-leak detection tests to prove the guard bites.test_backend_main.py,test_cloud_routes.py,test_real_api_endpoints.pyto match sanitized responses.Verification
(Broader suite collection in a bare sandbox trips a pre-existing, unrelated
NameError: name 'types' is not definedinsrc/agents/gemini_video_master_agent.py— not touched by this diff.)Context — duplicate PR cluster
Several parallel agent sessions independently produced near-identical fixes for this same issue (e.g. #804, #807, #810, #814, #815, #816, #818, #819). This branch is the current-with-
main, cleanly-mergeable, regression-tested one and is intended to consolidate/supersede them. Recommend picking one and closing the rest to stop the churn.Draft: merge to protected
mainawaits human sign-off.Generated by Claude Code