fix(security): sanitize global 500 exception handler (CWE-209) — minimal, single-sink - #842
Conversation
The global `Exception` handler in `backend/main.py` was the one HTTP-500
information-disclosure sink still live on `main`: its JSONResponse body echoed
`str(exc)`, `exc.__class__.__name__`, and `str(request.url)` to the client on
any unhandled error. The per-route `HTTPException(500, ...)` sinks were already
sanitized, but this handler leaked via `JSONResponse(status_code=500, content=...)`.
That call form is exactly what the existing AST guard
(`tests/unit/test_500_info_disclosure.py`) does not inspect — it only scans
`HTTPException(...)` calls, and `main.py` is not in its `_GUARDED_FILES` — which
is why the leak survived every green CI run.
Changes:
- `global_exception_handler` now returns a static body
(`{"error"/"detail": "Internal server error", "timestamp"}`); the exception
(type, message, traceback) and request path are logged server-side only via
`exc_info=True`. The 4xx `ValueError` handler is intentionally left unchanged.
- Replace the two `test_backend_main.py` tests that asserted the leaky contract
(`error_type`, `version`) with regression guards proving the 500 body is static
and never echoes the exception message, class name, or request URL.
Verified by exercising the real handler source directly (pytest/fastapi are not
installable in the ephemeral sandbox without the heavy ML stack): the 500 body
contains no exception text, class name, or URL, while the full error is still
logged; the no-`.url` path is safe; the 400 handler still returns 400.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016kZayBH8HUEt4ys7ofaH52
|
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:
📝 WalkthroughWalkthroughThe global exception handler now logs request URLs and full tracebacks server-side while returning a static generic 500 JSON response without exception or environment details. ChangesException handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
|
@coderabbitai review Generated by Claude Code |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata and diff ==='
gh pr view 842 --json number,title,state,isDraft,baseRefName,headRefName,files --jq \
'{number,title,state,isDraft,baseRefName,headRefName,files:[.files[].path]}'
gh pr diff 842 -- backend/main.py tests/unit/test_backend_main.py
echo '=== backend/main.py outline ==='
ast-grep outline backend/main.py --items all --type function,class --view expanded || true
echo '=== test_backend_main.py outline ==='
ast-grep outline tests/unit/test_backend_main.py --items all --type function,class --view expanded || true
echo '=== exception handlers and response conventions ==='
rg -n -C 8 \
'@.*exception_handler|def global_exception_handler|def .*exception_handler|JSONResponse\(|HTTPException\(' \
backend/main.py tests/unit/test_backend_main.pyLength of output: 1520 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff ==='
gh pr diff 842
echo '=== Production file outline ==='
ast-grep outline src/youtube_extension/backend/main.py --items all --type function,class --view expanded || true
echo '=== Test file outline ==='
ast-grep outline tests/unit/test_backend_main.py --items all --type function,class --view expanded || true
echo '=== Relevant handlers and tests ==='
rg -n -C 10 \
'@.*exception_handler|def global_exception_handler|def .*exception_handler|JSONResponse\(|HTTPException\(|global_exception_handler|ValueError' \
src/youtube_extension/backend/main.py tests/unit/test_backend_main.pyLength of output: 29358 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== main imports and error-handler region ==='
sed -n '1,100p' src/youtube_extension/backend/main.py
sed -n '430,478p' src/youtube_extension/backend/main.py
echo '=== Logging configuration and remote log sinks ==='
rg -n -i -C 4 \
'basicConfig|dictConfig|FileHandler|StreamHandler|CloudLogging|google\.cloud\.logging|LoggingHandler|loguru|sentry|logger\s*=|getLogger' \
src tests pyproject.toml requirements*.txt 2>/dev/null || true
echo '=== Sensitive query parameters and request URL logging ==='
rg -n -i -C 3 \
'(query|Query)\([^)]*(token|key|secret|password|credential|auth)|[?&](token|key|secret|password|credential|auth)=|request\.url|url\.query|query_params' \
src/youtube_extension tests 2>/dev/null || true
echo '=== Handler tests and logger assertions ==='
sed -n '555,632p' tests/unit/test_backend_main.py
rg -n -C 3 'global_exception_handler|Unhandled exception on|assert.*logger|mock.*logger' \
tests src/youtube_extension 2>/dev/null || trueLength of output: 208766
The client-facing 500 response sanitization itself is correct: exception messages, class names, and the request URL are no longer returned to callers. ✅ Action performedReview finished.
|
…dler Addresses CodeRabbit review on #842: the previous commit's log line used `str(request.url)`, which includes the query string. Since the service streams logs to stdout for Cloud Run, a failing request like `...?token=abc` would persist that secret in centralized logs (a server-side variant of the same CWE-209 concern). - `global_exception_handler` now logs `request.url.path` and the HTTP method only — never the full URL/query string. `exc_info=True` is preserved so the traceback is still captured. - Add `test_global_exception_handler_log_omits_query_string`: asserts the logger call omits a `?token=SECRET123` query secret, still records the bare path, and keeps `exc_info=True` — guarding against a regression to `str(request.url)`. Verified against the real handler source (fastapi/pytest unavailable in the sandbox): log line contains the path but not the token; body remains static; the no-`.url` path and the 400 `ValueError` handler are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016kZayBH8HUEt4ys7ofaH52
|
@coderabbitai Good catch — fixed in Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/youtube_extension/backend/main.py`:
- Around line 456-465: Update the unhandled-exception logging around
request_path to record only the HTTP method and URL path, excluding query
parameters, while retaining exc_info=True. Revise the surrounding docstring to
describe the sanitized log data, and add a regression test asserting query
values are absent from the log and exception information remains enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44cab5bc-f2d6-445e-a11a-e0c6f249d7eb
⛔ Files ignored due to path filters (1)
tests/unit/test_backend_main.pyis excluded by!tests/**
📒 Files selected for processing (1)
src/youtube_extension/backend/main.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
groupthinking/uvai-skills(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{py,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,jsx,ts,tsx}: Use Python 3.9+ and Node 18+ for development
Never hardcode API keys, database URLs, or secrets in code
Make minimal, surgical changes and avoid deleting working code unless fixing security issues
Files:
src/youtube_extension/backend/main.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for Python functions
Use Black formatter with 88 character line length for Python code
Follow PEP 8 conventions for Python code
Use descriptive variable names and add docstrings to all public functions in Python
Group imports in Python: standard library, third-party, local
Use SQLAlchemy ORM and never write raw SQL queries
Use environment variables via os.getenv() or pydantic-settings to access configuration
Wrap database operations in try-except blocks and use context managers or FastAPI dependencies for connection cleanup
Implement comprehensive error handling with proper logging in all functions
Version APIs using /api/v1/ prefix for stability
Use JSON-RPC 2.0 protocol for all MCP communication
Follow the single-flow workflow: YouTube link → context extraction → agent dispatch → outputs
Use context managers for resource management in Python code
Implement comprehensive input validation and sanitize outputs for security
Use parameterized queries and SQLAlchemy ORM to prevent SQL injection
Define API request/response models using Pydantic for FastAPI endpoints
Store the single unified workflow as the only workflow; never introduce alternate flows or manual triggers
Use SQLAlchemy with connection pooling for database connections and manage sessions with context managers
Provide sensible defaults for non-sensitive configuration in Python settings
Maintain backward compatibility and do not break existing API endpoints
Include comprehensive logging for debugging in MCP implementations
**/*.py: Format Python code with Black using an 88-character line length.
Use Ruff with rules E, W, F, I, B, C4, and UP; E501 is ignored.
Use strict mypy checking with untyped function definitions disallowed.
Files:
src/youtube_extension/backend/main.py
⚙️ CodeRabbit configuration file
Python backend code. Check for type hints, proper exception handling, async context manager usage, and potential blocking calls in async functions. Flag any bare except clauses or missing timeout parameters on network calls. CRITICAL: Flag any file that contains placeholder/stub implementations — especially in unified_ai_sdk. Any class or function that says "TODO: Replace with production implementation" or returns mock/fake data must be flagged as a blocking issue. Flag any code generation output that reaches users without AST validation or syntax checking.
Files:
src/youtube_extension/backend/main.py
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
src/youtube_extension/backend/main.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,ts,tsx}: Keep frontend and backend data models synchronized using matching Pydantic (backend) and TypeScript (frontend) interfaces
Use type-safe interfaces for backend-frontend data exchange
Files:
src/youtube_extension/backend/main.py
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Strictly verify that GitHub Copilot has explicitly reviewed and approved the pull request; human approvals alone must not satisfy this check.
Before allowing a merge, require thecopilot-rabbitlabel and AI-generated unit tests committed alongside the code changes; fail the check if either is missing.
**/*: Follow the documented event naming convention<domain>.<entity>.<action>, such asyoutube.video.captured.
Use the service-container dependency injection pattern for backend dependencies.
Never infer SDK types from tests or API documentation alone; use backend response models as the authority.
When auditing branches, use thebranch-cleanupskill and its six-gate fail-test harness; archive branches withgit tag archive/<branch>before deletion, and do not rely on three-dot diffs orgit merge-treefor orphaned branches.For Vercel-specific work, include
https://vercel.com/docs/llms-full.txtin the AI assistant context set.
Files:
src/youtube_extension/backend/main.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Format Python code with Black using an 88-character line length.
Sort Python imports with isort using the Black profile.
Use Ruff with E, W, F, I, B, C4, and UP rules; E501 is ignored.
Use mypy strict mode; untyped function definitions are disallowed.
Target Python 3.9 or newer.
Validate backend inputs with Pydantic and sanitize subprocess arguments.
Maintain strict mypy type safety in the Python backend.
Use the required Anthropic SDK parametersthinking={"type": "adaptive"}andoutput_config={"effort": "..."}with the current model stringclaude-opus-4-8; do not addTypeErrorcompatibility fallbacks.
src/**/*.py: Do not introduce alternative workflows or manual triggers that bypass the single YouTube link → transcript → events → agents → outputs pipeline.
Use event names in the<domain>.<entity>.<action>format.
Use the service-container dependency-injection pattern for dependencies.
Use Pydantic input validation and sanitize subprocess arguments.
Production code must use real behavior only; do not add mock delays or fake data.
Files:
src/youtube_extension/backend/main.py
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{py,ts,tsx,js,jsx}: Do not use mock delays, fake data, or simulated responses in production code; production must remain REAL_MODE_ONLY.
Do not hard-code secrets, keys, or credentials; store them in.envfiles that are gitignored.Do not include secrets or API keys in source code; load them from environment variables instead.
Files:
src/youtube_extension/backend/main.py
**/*.{py,pyw}
📄 CodeRabbit inference engine (AGENTS.md)
Write Python code to remain compatible with Linux and Windows where possible, including correct handling of
asyncioevent loops.
Files:
src/youtube_extension/backend/main.py
🔍 Remote MCP GitHub Copilot
- PR
#842is open and draft, with 2 files changed (+35/−21). It targets the remaining JSONResponse-based global 500 sink; the per-routeHTTPException(500, ...)sinks were already sanitized. Vercel checks are successful, while CodeRabbit review is still pending. - In
src/youtube_extension/backend/main.py, the globalExceptionhandler now returns a static 500 body (error/detail/timestamp) and logs server-side withexc_info=True. The file also configures logging toStreamHandler(sys.stdout), and the current log message still interpolatesstr(request.url)viarequest_path, so full URLs/query strings can still reach logs. TheValueError400 handler is unchanged and still echoesstr(exc)pluspath. tests/unit/test_backend_main.pynow checks that the 500 response does not leak exception type, exception message, or request URL, and that the handler does not crash whenrequest.urlis missing.- CodeRabbit’s main blocking comment is that logging the full request URL can expose query-string secrets (example:
token=abc) in Cloud Run logs; it recommends logging only path/method and asserting the logger call omits sensitive params while keepingexc_info=True. It also notes its automated review skippedtests/unit/test_backend_main.pybecause of the!tests/**path filter. - Dependency Review reported no vulnerabilities, no license issues, and no OpenSSF Scorecard issues; it also warned that no snapshots were found for the head SHA.
|
@coderabbitai resolve The Generated by Claude Code |
|
✅ Action performedComments resolved and changes approved. |
There was a problem hiding this comment.
Pull request overview
Sanitizes the global FastAPI 500 handler to prevent internal error disclosure while preserving server-side diagnostics.
Changes:
- Replaces dynamic 500 response details with generic messages.
- Logs exception context without query strings.
- Adds regression tests for response and log sanitization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/youtube_extension/backend/main.py |
Sanitizes global 500 responses and logging. |
tests/unit/test_backend_main.py |
Verifies exception details and URLs are not exposed. |
Summary
Closes the one HTTP-500 information-disclosure sink (CWE-209) still live on
main: the globalExceptionhandler inbackend/main.py. On any unhandled error it returned aJSONResponsewhose body echoedstr(exc),exc.__class__.__name__, andstr(request.url)to the client.The per-route
HTTPException(status_code=500, detail=...)sinks were already sanitized onmain. This handler is different: it leaks viaJSONResponse(status_code=500, content={...}).Why this leak survived ~15 green PRs (root cause)
The repo currently has a cluster of ~15 open PRs all attempting the same 500-hardening (#804, #807, #810, #814–#821, #826, #827, #831, #832, #834), each claiming to be "the consolidation." Every one passed the guard
tests/unit/test_500_info_disclosure.py— yet the leak persisted. The reason is a structural blind spot in that guard:HTTPException(...)calls; it never looks at theJSONResponse(status_code=500, content=...)form the global handler uses.main.pyisn't even in its_GUARDED_FILEStuple.So the guard is structurally incapable of seeing this sink. That is why the cluster kept regenerating without ever fixing the actual remaining leak.
Changes (2 files)
backend/main.py—global_exception_handlernow returns a static body{"error"/"detail": "Internal server error", "timestamp"}. The exception (type, message, traceback) and request path are logged server-side only vialogger.error(..., exc_info=True). The 4xxValueErrorhandler is intentionally left unchanged (4xx echoes client-supplied input, not an internal-disclosure vector).tests/unit/test_backend_main.py— replaces the two tests that asserted the leaky contract (error_type,version) with regression guards that exercise the handler and prove the 500 body is static and never echoes the exception message, class name, or request URL.Verification
pytest/fastapiare not installable in the ephemeral session sandbox without pulling the heavy google-cloud/ML stack, so this was verified by exercising the real handler source directly (extracted via AST, executed against a stubJSONResponse/logger):.urlrequest path does not crash.ValueErrorhandler still returns400.Relationship to the duplicate cluster
This is deliberately minimal and merge-clean on current
main(single production file, 2 files total), unlike the cluster: e.g. #834 targets the same handler but is nowmergeable_state: dirty(conflicted) and touches 9 files. Recommendation for the maintainer: land this, then close the redundant cluster (#804, #807, #810, #814–#821, #826, #827, #831, #832, #834). A follow-up could widen the AST guard to cover theJSONResponse500 form and addmain.pyto_GUARDED_FILESso this class of sink can't hide again.Left as draft — targets protected
main; not auto-merged.🤖 Generated with Claude Code
https://claude.ai/code/session_016kZayBH8HUEt4ys7ofaH52
Generated by Claude Code