🛡️ Sentinel: [MEDIUM] Fix internal error message leakage in API routes - #1145
🛡️ Sentinel: [MEDIUM] Fix internal error message leakage in API routes#1145google-labs-jules[bot] wants to merge 2 commits into
Conversation
🚨 Severity: MEDIUM 💡 Vulnerability: Exposes raw backend error messages directly to the client 🎯 Impact: Could leak internal architecture details or timeout diagnostics 🔧 Fix: Log the detailed error internally and return a generic safe message ✅ Verification: Verified via linting and tests
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
documentation
🔍 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 |
Agent Completion Truth Gate: BLOCKEDReasons: Machine-readable verdict{
"details": {
"collection_errors": [
"incomplete_linked_issue_contract",
"missing_linked_issue",
"missing_closing_issue_reference",
"missing_agent_run_id",
"missing_agent_login"
],
"invalid_fields": [
"issue.number",
"policy.agent_login",
"policy.run_id"
]
},
"reasons": [
"invalid_payload"
],
"verdict": "blocked"
} |
Automated review — ready for your merge decisionDiff verdict: ✅ clean, correct security fix. Across the 7 API routes it removes
The red CI is not this code — both failing checks are environmental:
Not merging autonomously. Generated by Claude Code |
groupthinking
left a comment
groupthinking
left a comment
There was a problem hiding this comment.
Review (automated, non-approving)
Assessment: the change is correct and a genuine security improvement. It removes client-facing details: String(error) / error.message from 7 Next.js API route handlers (agents/dispatch, agents/status, extract-events, jobs/[jobId], training/status, training/trigger, video/search), replacing them with static generic messages while preserving server-side console.error logging — and it adds the missing log line in jobs/[jobId]/route.ts. Scope is minimal (12/-9) and the code-level checks are green (lint-frontend, lint-python, build, bandit, CodeQL, dependency-review, both Security Scans).
One non-blocking suggestion — apps/web/src/app/api/training/trigger/route.ts, inner catch (uploadError) branch: this hunk removed details: String(uploadError) but the branch has no console.error, so that error is now discarded entirely rather than logged internally. That contradicts this PR's own stated principle ("log the detailed error internally … return a generic message"). Consider adding:
} catch (uploadError) {
console.error('Upload/trigger failed:', uploadError);
return NextResponse.json({
error: 'Upload/trigger failed',
fallback: { /* ... */ },so internal diagnostics survive. Non-blocking — CI lint passes as-is.
What's actually blocking merge is not the code. The red checks are all governance/infra gates, none of which this diff can satisfy on its own:
agent-completion/truth-gate→invalid_payload(missingissue.number,policy.agent_login,policy.run_id)PR Governance,Canonical issue and evidence,Agent completion enforcement→ failuregitleaks (working tree)→ failure (the uv.lock-style false-positive class being addressed separately in #1142)- PR title fails conventional-commits validation (
🛡️ Sentinel: [MEDIUM] …)
Deliberately submitting this as a comment, not an approval: this run is an automated routine, not live maintainer sign-off, and merging targets protected main. The merge/approval decision and the governance-gate wiring remain with the maintainer.
Generated by Claude Code
🚨 Severity: MEDIUM 💡 Vulnerability: Exposes raw backend error messages directly to the client 🎯 Impact: Could leak internal architecture details or timeout diagnostics 🔧 Fix: Log the detailed error internally and return a generic safe message ✅ Verification: Verified via linting and tests
Remediation review —
|
Blocker-watch containment — exact head
|
There was a problem hiding this comment.
Pull request overview
Sanitizes Next.js API error responses to prevent internal exception details from reaching clients.
Changes:
- Replaces raw exception details with generic client-facing errors.
- Adds internal server-side error logging.
- Documents the security remediation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
apps/web/src/app/api/video/search/route.ts |
Sanitizes vector-search failures. |
apps/web/src/app/api/training/trigger/route.ts |
Sanitizes training failures. |
apps/web/src/app/api/training/status/route.ts |
Sanitizes status-read failures. |
apps/web/src/app/api/jobs/[jobId]/route.ts |
Sanitizes backend proxy failures. |
apps/web/src/app/api/extract-events/route.ts |
Sanitizes extraction failures. |
apps/web/src/app/api/agents/status/route.ts |
Sanitizes agent-status failures. |
apps/web/src/app/api/agents/dispatch/route.ts |
Sanitizes dispatch failures. |
.jules/sentinel.md |
Records the security learning. |
| @@ -232,7 +232,6 @@ export async function POST(request: Request) { | |||
| } catch (uploadError) { | |||
| return NextResponse.json({ | |||
| error: 'Upload/trigger failed', | |||
| **Vulnerability:** API routes were returning internal server exceptions and stack traces directly to the client via `HTTPException(..., detail=str(e))`. | ||
| **Learning:** Developers often unintentionally leak sensitive deployment context (e.g., paths, database errors) when relying on generic exception catching blocks. | ||
| **Prevention:** Hardcode static error strings for unexpected 500 exceptions (e.g., `detail="Internal server error"`) while ensuring the full exception trace is securely logged server-side. Every sanitized 500 handler in `router.py`, `main.py`, and mounted routers (e.g. `reporting_routes.py`) now logs via `logger.error(..., exc_info=True)` so the traceback is preserved for internal monitoring without ever reaching the client. Guard against regressions with tests that assert the response body equals the generic message AND excludes the raised exception string (status-code-only assertions are insufficient). | ||
| ## 2025-01-21 - Prevent internal error leakage in API routes |
| console.error('Agent dispatch error:', error); | ||
| return NextResponse.json( | ||
| { error: 'Failed to dispatch agents', details: String(error) }, | ||
| { error: 'Failed to dispatch agents' }, |
| console.error('Agent status error:', error); | ||
| return NextResponse.json( | ||
| { error: 'Failed to get agent status', details: String(error) }, | ||
| { error: 'Failed to get agent status' }, |
| return NextResponse.json({ | ||
| success: false, | ||
| error: message, | ||
| error: 'Event extraction failed', |
| console.error('Job proxy error:', error); | ||
| return NextResponse.json( | ||
| { error: error instanceof Error ? error.message : String(error) }, | ||
| { error: 'Failed to fetch job status' }, |
| console.error('Video Search Error:', error); | ||
| return NextResponse.json( | ||
| { error: error.message || 'Internal server error during vector search' }, | ||
| { error: 'Internal server error during vector search' }, |
| console.error('Training status error:', error); | ||
| return NextResponse.json( | ||
| { error: 'Failed to read training status', details: String(error) }, | ||
| { error: 'Failed to read training status' }, |
| console.error('Training trigger error:', error); | ||
| return NextResponse.json( | ||
| { error: 'Failed to process training request', details: String(error) }, | ||
| { error: 'Failed to process training request' }, |
|
Automated triage of the open review threads — consolidating the 9 findings so the actionable ones are clear. Core fix — correct and complete for its stated scope. All seven routes now drop
Regression tests (7 threads) — the one gap worth closing before merge. The PR adds no failure-path tests, yet
On the red checks: both are infra noise, not code — Not merging from here: Generated by Claude Code |
Closing — branch orphaned by the secret-purge force-pushNo common ancestor with current This is unlanded security work — internal error message leakage in API routes. Note that #1281 ("route API error responses through Tracked in #1378. Branch retained for archive-tagging. Generated by Claude Code |
🚨 Severity: MEDIUM
💡 Vulnerability: Several Next.js API routes (
/api/jobs/[jobId],/api/agents/dispatch,/api/agents/status,/api/extract-events,/api/training/trigger,/api/training/status,/api/video/search) were catching exceptions from external backendfetchcalls and exposing the rawerror.messageorString(error)directly to the client.🎯 Impact: This information disclosure can inadvertently leak internal architecture details (such as backend hostnames, IPs, proxy configuration, or connection timeout diagnostics), which attackers could use for reconnaissance.
🔧 Fix: Changed the error handlers in these routes to catch external request errors, log the detailed error internally via
console.error, and return a generic, non-revealing error message (e.g., "Failed to communicate with backend service", "Failed to fetch job status") to the client.✅ Verification: Verified via linting (
npm run lint), type-checking (npm run type-check), and testing (npm test) inapps/web/. Appended the learning to.jules/sentinel.mdas required.PR created automatically by Jules for task 1121709532317009963 started by @groupthinking