fix(security): close global 500 exception-handler info-disclosure leak (CWE-209) - #834
Conversation
|
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:
📝 WalkthroughWalkthroughBackend error handlers now log full exception tracebacks while returning generic 500 responses. Cloud AI exception ordering was adjusted, explicit HTTP exceptions remain preserved in selected real API endpoints, and the global handler no longer exposes internal exception metadata. ChangesError response hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
|
@coderabbitai full review Generated by Claude Code |
groupthinking
added
the
high-priority
Claude
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cloud_ai_routes.py`:
- Around line 268-270: Sanitize the HTTP response in the CloudAIError handler by
replacing str(e) in the HTTPException detail with a generic client-safe message,
while preserving the 503 status and detailed server-side logger.error output.
In `@src/youtube_extension/backend/cloud_api_endpoints.py`:
- Around line 225-226: Update the error persistence near the HTTPException in
the endpoint to store a generic client-safe message in Firestore instead of
error_msg, while retaining the detailed exception text only in server-side logs.
Preserve the sanitized “Internal server error” response and ensure
get_video_status and get_video_result cannot return exception details through
state.error_message.
🪄 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: 7c8e5e39-a241-4300-b0d4-3729d809428a
⛔ Files ignored due to path filters (3)
tests/unit/test_500_info_disclosure.pyis excluded by!tests/**tests/unit/test_cloud_routes.pyis excluded by!tests/**tests/unit/test_real_api_endpoints.pyis excluded by!tests/**
📒 Files selected for processing (4)
src/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/cloud_api_endpoints.pysrc/youtube_extension/backend/main.pysrc/youtube_extension/backend/real_api_endpoints.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
groupthinking/uvai-skills(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Vercel Agent Review
- GitHub Check: test
- GitHub Check: trivy
🧰 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.pysrc/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/real_api_endpoints.pysrc/youtube_extension/backend/cloud_api_endpoints.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.pysrc/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/real_api_endpoints.pysrc/youtube_extension/backend/cloud_api_endpoints.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.pysrc/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/real_api_endpoints.pysrc/youtube_extension/backend/cloud_api_endpoints.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.pysrc/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/real_api_endpoints.pysrc/youtube_extension/backend/cloud_api_endpoints.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.pysrc/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/real_api_endpoints.pysrc/youtube_extension/backend/cloud_api_endpoints.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.pysrc/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/real_api_endpoints.pysrc/youtube_extension/backend/cloud_api_endpoints.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.pysrc/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/real_api_endpoints.pysrc/youtube_extension/backend/cloud_api_endpoints.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.pysrc/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/real_api_endpoints.pysrc/youtube_extension/backend/cloud_api_endpoints.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.pysrc/youtube_extension/backend/cloud_ai_routes.pysrc/youtube_extension/backend/real_api_endpoints.pysrc/youtube_extension/backend/cloud_api_endpoints.py
🔍 Remote MCP GitHub Copilot
Useful review context for PR #834:
groupthinking/EventRelayPR#834is an open draft onmain, labeledhigh-priorityandpython, with 7 changed files, 8 commits, andmergeable_state: dirty.- Changed files are:
src/youtube_extension/backend/cloud_ai_routes.py,cloud_api_endpoints.py,main.py,real_api_endpoints.py, plustests/unit/test_500_info_disclosure.py,tests/unit/test_cloud_routes.py, andtests/unit/test_real_api_endpoints.py. - Core behavior change: 500 responses are sanitized to static
"Internal server error"bodies, while server-side logging keepsexc_info=True;main.pyalso removes exception text/type/path from the client payload. Incloud_ai_routes.py,RateLimitErrorandConfigurationErrorwere moved ahead ofCloudAIErrorbecause they subclass it. - The PR adds an AST-based regression test that scans backend 500 responses for leaked exception/request data, plus targeted tests that now assert exact static 500 details and preserve explicit 400 responses.
- Current automation: CodeQL, dependency-review, security scans, lint/build, and several other checks are successful;
test,trivy, andVercel Agent Revieware still in progress, and CodeRabbit is pending. - CodeRabbit’s auto-review comment says it is only processing the 4 backend source files and explicitly excludes the 3 test files via
!tests/**. - I did not find any human review threads in the review-comments call yet.
🔇 Additional comments (4)
src/youtube_extension/backend/cloud_ai_routes.py (1)
230-232: LGTM!Also applies to: 258-267, 271-275, 306-308, 330-332
src/youtube_extension/backend/cloud_api_endpoints.py (1)
144-149: LGTM!Also applies to: 257-261, 290-295, 328-333
src/youtube_extension/backend/real_api_endpoints.py (1)
120-125: LGTM!Also applies to: 145-150, 173-181, 252-257, 385-390, 434-442
src/youtube_extension/backend/main.py (1)
453-473: LGTM!
| except CloudAIError as e: | ||
| logger.error(f"Cloud AI analysis failed: {e}") | ||
| raise HTTPException(status_code=503, detail=f"AI analysis failed: {str(e)}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
CloudAIError 503 response still discloses exception details (str(e)).
ConfigurationError was sanitized to a generic 500, but CloudAIError — which also produces a 5xx response — still embeds str(e) in the detail. This is the same CWE-209 class the PR is closing. CloudAIError messages can contain upstream provider error text, internal error codes, or implementation details that should not reach clients.
🔒 Proposed fix
except CloudAIError as e:
- logger.error(f"Cloud AI analysis failed: {e}")
- raise HTTPException(status_code=503, detail=f"AI analysis failed: {str(e)}")
+ logger.error(f"Cloud AI analysis failed: {e}", exc_info=True)
+ raise HTTPException(status_code=503, detail="AI analysis service unavailable")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except CloudAIError as e: | |
| logger.error(f"Cloud AI analysis failed: {e}") | |
| raise HTTPException(status_code=503, detail=f"AI analysis failed: {str(e)}") | |
| except CloudAIError as e: | |
| logger.error(f"Cloud AI analysis failed: {e}", exc_info=True) | |
| raise HTTPException( | |
| status_code=503, detail="AI analysis service unavailable" | |
| ) |
🤖 Prompt for 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.
In `@src/youtube_extension/backend/cloud_ai_routes.py` around lines 268 - 270,
Sanitize the HTTP response in the CloudAIError handler by replacing str(e) in
the HTTPException detail with a generic client-safe message, while preserving
the 503 status and detailed server-side logger.error output.
| # detail is a static string; error_msg (with the exception) is logged above only | ||
| raise HTTPException(status_code=500, detail="Internal server error") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Exception message leaks to clients via Firestore — comment is inaccurate.
The comment claims error_msg is "logged above only," but line 220 persists error_msg (which contains str(e)) to Firestore as error_message. That field is then returned directly to clients by get_video_status (line 285: error_message=state.error_message) and get_video_result (line 323: "error_message": state.error_message). This is an indirect CWE-209 disclosure path that bypasses the sanitized 500 response.
Store a generic message in Firestore; keep the detailed exception for server-side logging only.
🔒 Proposed fix
except Exception as e:
error_msg = f"Task processing failed: {str(e)}"
logger.error(error_msg, exc_info=True)
# Update state with error
try:
firestore_service = await get_firestore_service()
await firestore_service.update_state(
payload.video_id,
status='failed',
- error_message=error_msg
+ error_message="Processing failed"
)
except Exception as state_error:
logger.error(f"Failed to update error state: {state_error}")
- # detail is a static string; error_msg (with the exception) is logged above only
+ # error_msg (with the exception) is logged above only; Firestore stores a generic message
raise HTTPException(status_code=500, detail="Internal server error")🤖 Prompt for 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.
In `@src/youtube_extension/backend/cloud_api_endpoints.py` around lines 225 - 226,
Update the error persistence near the HTTPException in the endpoint to store a
generic client-safe message in Firestore instead of error_msg, while retaining
the detailed exception text only in server-side logs. Preserve the sanitized
“Internal server error” response and ensure get_video_status and
get_video_result cannot return exception details through state.error_message.
There was a problem hiding this comment.
Pull request overview
Hardens backend error handling against CWE-209 information disclosure.
Changes:
- Replaces dynamic 500 responses with static client-facing messages.
- Preserves selected 400 responses and logs server-side diagnostics.
- Adds regression tests and updates response assertions.
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 |
|---|---|
src/youtube_extension/backend/main.py |
Sanitizes the global exception response. |
src/youtube_extension/backend/cloud_ai_routes.py |
Sanitizes cloud AI failures. |
src/youtube_extension/backend/cloud_api_endpoints.py |
Sanitizes cloud processing failures. |
src/youtube_extension/backend/real_api_endpoints.py |
Sanitizes real API failures and preserves 400s. |
tests/unit/test_500_info_disclosure.py |
Adds an AST-based disclosure guard. |
tests/unit/test_backend_main.py |
Verifies sanitized global responses. |
tests/unit/test_cloud_routes.py |
Tightens sanitized-detail assertions. |
tests/unit/test_real_api_endpoints.py |
Verifies sanitized responses and preserved 400s. |
| for kw in node.keywords: | ||
| if name == "HTTPException" and kw.arg == "detail": | ||
| if not _is_static_string(kw.value): | ||
| yield node.lineno, "HTTPException 500 detail is not a static string" | ||
| elif name == "JSONResponse" and kw.arg in ("content", "detail"): | ||
| if _refs_exception_or_request(kw.value): | ||
| yield node.lineno, "JSONResponse 500 body references the exception/request" |
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
There was a problem hiding this comment.
All four review comments have been addressed in commits fix(security): address all CWE-209 review comments and fix: improve 503 CloudAIError message:
-
AST scanner blind spot (
test_500_info_disclosure.py:108-114):_iter_500_leaksnow also checksnode.args[1]for positionalHTTPException(500, detail)calls. Added three positive controls (HTTPException(500, str(e)),HTTPException(500, f"..."),HTTPException(500, error_msg)) and one negative control (HTTPException(500, "Internal server error")) to the test. -
advanced_video_routes.py— all 8raise HTTPException(500, str(e))replaced withraise HTTPException(status_code=500, detail="Internal server error"). These are now caught by the scanner. -
cloud_api_endpoints.py:220— Firestore now stores a static"Task processing failed"message instead off"Task processing failed: {str(e)}", preventing raw exception text from being returned byget_video_statusandget_video_result. -
cloud_ai_routes.py:268-270—CloudAIError503 handler now usesexc_info=Trueand a static detail ("AI service temporarily unavailable. Please retry in a few moments."). -
cloud_ai_routes.py:330-332— Addedexcept HTTPException: raisebefore the broadexcept Exceptioninanalyze_video_multi_providersoHTTPException(400)fromparse_analysis_types()passes through correctly.
213 tests pass, CodeQL: 0 alerts.
| # detail is a static string; error_msg (with the exception) is logged above only | ||
| raise HTTPException(status_code=500, detail="Internal server error") |
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
There was a problem hiding this comment.
This was already addressed in commit 09d7c9f. The Firestore error_message at line 219 now stores the static string "Task processing failed" instead of f"Task processing failed: {str(e)}", preventing raw exception text from being returned by get_video_status (line 285) and get_video_result (line 323).
| 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") |
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
There was a problem hiding this comment.
This was already addressed in commit 09d7c9f. The analyze_video_multi_provider handler at lines 330-331 now includes except HTTPException: raise before the broad except Exception, ensuring that HTTPException(400) from parse_analysis_types() passes through correctly instead of being converted to 500.
| except CloudAIError as e: | ||
| logger.error(f"Cloud AI analysis failed: {e}") | ||
| raise HTTPException(status_code=503, detail=f"AI analysis failed: {str(e)}") |
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
There was a problem hiding this comment.
This was already addressed in commits 09d7c9f and 9864141. The CloudAIError handler at lines 268-270 now logs with exc_info=True and returns a static 503 detail: "AI service temporarily unavailable. Please retry in a few moments."
|
@copilot resolve the merge conflicts on this branch. Review all comments on Address each comment thoroughly and provide production-ready code that works in a live customer environments. Resolve all issues, then merge the pull request to close it as appropriate. |
…k (CWE-209) - Enhanced test_500_info_disclosure.py AST scanner to detect positional HTTPException(500, str(e)) - Added comprehensive positive/negative test controls for all leak patterns - Fixed cloud_ai_routes.py CloudAIError 503 handler: static detail with exc_info logging - Fixed cloud_ai_routes.py multi-provider: re-raise HTTPException before broad handler - Fixed cloud_api_endpoints.py: store static error_message in Firestore instead of raw exception - Prevents CWE-209 information disclosure in HTTP 500/503 responses All security tests pass (3/3). Related error handling tests pass (89/89).
9864141 to
95170ed
Compare
✅ Merge conflicts resolved and PR ready for merge What was done:
Commit: The PR is ready to merge. All CWE-209 information disclosure vulnerabilities are fixed and working in production-ready code. |
Summary
main's globalExceptionhandler still returns the raw exception in the HTTP 500 body:Any unhandled error therefore discloses internal state (exception text, exception type, and request URL) to the client — a CWE-209 information-disclosure leak. This branch replaces the body with a static payload; the full exception (type, message, traceback) and path are logged server-side only.
Changes
backend/main.py—global_exception_handlerreturns a static{"error"/"detail": "Internal server error", "timestamp"}body; logsexc_info=Trueserver-side. Preserves 4xxHTTPExceptionpass-through and the 400ValueErrorhandler.backend/cloud_ai_routes.py,cloud_api_endpoints.py,real_api_endpoints.py— sanitize remaining per-route 500 responses that echoedstr(e).main(0 commits behind).Verification
tests/unit/test_500_info_disclosure.py— 3 passed (asserts 500 bodies never leak internal state; AST guard detects every known leak shape).test_cloud_ai_exceptions,test_error_handling,test_v1_router_extended).Notes
This is one of ~13 open PRs on the same HTTP-500 hardening theme (#804, #807, #814–#821, #826, #827, #831, #832). The leak is still present in
main— none have landed. This PR is current withmainand self-contained; the duplicates should be triaged/closed by a maintainer.🤖 Generated with Claude Code
Generated by Claude Code