fix(security): re-land CWE-209 response protections from closed PR #831 - #1690
Draft
linear-code[bot] wants to merge 14 commits into
Draft
fix(security): re-land CWE-209 response protections from closed PR #831#1690linear-code[bot] wants to merge 14 commits into
linear-code[bot] wants to merge 14 commits into
Conversation
…E-209)
Refreshed onto current main (was 73 commits behind). main already sanitizes the
500 HTTPException/JSONResponse details, the 503 CloudAIError, and the cloud-AI
exception ordering, but still returns the caught exception under an "error" key
in several handlers that *return* (not raise) a dict body — a 200
"degraded"/"failed" payload that discloses internal state just like a 500 detail
would.
- cloud_api_endpoints.py: /api/v3/queue/stats and /api/v3/cloud-status (three
per-service checks + outer handler) now return a static status string and log
the exception server-side with exc_info=True.
- real_api_endpoints.py: cost-dashboard, usage-analytics, optimization, and
service-status handlers likewise return "Internal server error" / "Service
unavailable" and log with exc_info=True.
- tests/unit/test_500_info_disclosure.py: extend main's AST guard with a
response-body scanner that flags {"error": <exception>} bodies. It derives the
caught identifier from the enclosing ast.ExceptHandler.name (per Copilot), so a
renamed variable (e.g. `except Exception as failure`) cannot bypass it; scoped
to the two handlers hardened here.
Existing endpoint tests assert status/degraded/key-presence, not the exception
string, so behavior is preserved. Guard suite: 5 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01He4GxmJcWW8XdwxYQQh8Sa
Copilot review: the response-body scanner lost the exception taint after an
intermediate assignment, e.g. `except Exception as failure: message =
str(failure); return {"error": message}` — a common refactor of the sanitized
sites — produced no finding.
Propagate taint from the handler-bound name to any variable assigned from an
expression that references an already-tainted name (fixpoint, monotonic), so an
alias cannot launder the leak past the guard. Added positive controls for the
str()/f-string alias forms and a negative control for a static alias.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01He4GxmJcWW8XdwxYQQh8Sa
Sanitize legacy persisted error text at every cloud endpoint read boundary, make rate-limit details static, extend the AST invariant to all 5xx statuses, and add focused regressions.
…E-209) Closes two current-head Copilot findings on #831 (both CWE-209 information disclosure through pass-through response sinks the boundary sanitizer missed): - real_api_endpoints.py:_sanitize_response_errors only rewrote the singular "error" key, so the plural "errors" collection — which real_ai_processor .analyze_video_content() fills with scalar strings like f"{step}: {str(result)}" — passed exception text through unchanged in batch, cached, list, and status responses. Add _sanitize_error_list to replace scalar string entries with the public message while preserving/recursing structured batch error records (keeps test_batch_failure_records_are_sanitized_recursively green). - /api/v2/process-video returned ai_analysis=result.get('ai_analysis') raw while every other endpoint wraps its payload; real_video_processor sets ai_analysis['error'] = f"AI analysis failed: {e}" on failure. Wrap it in _sanitize_response_errors so the nested error/errors are scrubbed too. Adds focused regression tests for both shapes. No behavior change beyond replacing leaked exception text with "Video processing failed"; server-side diagnostics and logs are untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GwvL8n17iZJqaj83ARzWrQ
The recursive `errors` sanitizer in real_api_endpoints and cloud_api_endpoints replaced only `str` leaves and returned any other scalar unchanged. FastAPI can serialize non-string leaves (bytes, ints, bools), so a legacy/provider diagnostic value that is not a string could bypass the scalar sanitization invariant and reach clients. Replace every non-null leaf with the public message after handling list/tuple/dict containers; only None (absence of an error) is preserved. Adds a positive-control test covering int/bool/None leaves. Addresses the current-head automated review finding on PR #831. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015A4gdsfGkyZdRYQwm4o99e
…eption leaks (CWE-209)
Resolves two Copilot review findings against the current head:
* Live leak: official_api.validate_video_url returned
f"Invalid URL format: {e}" / f"Video validation failed: {e}" as its
message element, which /api/v2/validate-video echoes verbatim to clients
under "message" with HTTP 200. The adapter swallowed the exception and
handed it back as data, so the endpoint's own 500 handler never saw it.
Return static messages and log the exception server-side instead.
* Regression guard gaps in test_500_info_disclosure.py:
- Scan the plural "errors" key so {"errors": [str(e)]} is flagged like a
scalar "error" field; accept _sanitize_error_list as a boundary
sanitizer; add positive/negative controls.
- Add _iter_returned_exception_leaks: flag any return inside an
except ... as <name> handler that carries the caught exception (or an
alias) in official_api.py / real_api_endpoints.py / cloud_api_endpoints.py.
This models the returned-value disclosure path neither prior scan caught.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01He4GxmJcWW8XdwxYQQh8Sa
Preserve exactly the nine declared CWE-209 implementation/test files while synchronizing the existing canonical branch with main@995fa268. No force push.
… resolution) Merges canonical head 1a0ce65 (fix/security: restore CWE-209 response protections) onto current main. real_api_endpoints.py conflicts resolved to keep main's worker-thread cache helpers; sanitization wiring into _collect_processed_videos_sync and _read_video_analysis_sync is still pending (see PR description). Generated with [Linear](https://linear.app/myxstack/agent-session/7158f322-d8ac-4792-aeb3-12eb46b269ec) Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com>
Contributor
Author
Contributor
|
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 |
🔍 PR Validation |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| # the event loop in proportion to its size. | ||
| # The helper already parsed, sanitized, and re-encoded the entry in | ||
| # the worker thread. Returning a Response skips FastAPI's | ||
| # jsonable_encoder/json.dumps round-trip, which would otherwise |
Contributor
There was a problem hiding this comment.
GET /api/v2/videos/{video_id} serves cached error text verbatim because _read_video_analysis_sync never applies the CWE-209 sanitizer it is documented to apply.
groupthinking
added
high-priority
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.
Re-lands the nine-file CWE-209 diff from closed PR #831 (exact head
1a0ce654, merge base995fa268) via a true three-way merge onto currentmain, restoring the constructor-aware 500–599 guard, positionalJSONResponsebody inspection,src/uvai/mlscan coverage, static 500 bodies in generated endpoints, and cloud/real API response-tree sanitization.real_api_endpoints.pyare unfinished:_collect_processed_videos_syncmust wrap the"analysis"field with_sanitize_response_errors(...)(main's worker-thread helper was kept; PR fix(security): restore CWE-209 response protections #831's sanitization of that field is not yet wired in)._read_video_analysis_syncmust parse, sanitize (_sanitize_response_errors), and re-encode the cache entry in the worker thread; its docstring still describes the old raw-bytes contract, and the handler comment now claims sanitization that isn't implemented yet.Do not merge until the above are completed and
tests/unit/test_500_info_disclosure.py,test_cloud_routes.py,test_code_generator_agent.py, andtest_real_api_endpoints.pypass.