docs(triage): PR remediation run 2026-07-29 + refreshed CWE-209 (#831) canonical - #1077
docs(triage): PR remediation run 2026-07-29 + refreshed CWE-209 (#831) canonical#1077groupthinking wants to merge 15 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.
Records oldest-first triage of all 36 open PRs. Every open PR is a draft (scope-gated -> DEFERRED(draft)); 0 autonomously mergeable, 0 HALTED. #903 (previously the single human-gated PR) has been returned to draft. Matches the 2026-07-27 (#1044) and 2026-07-28 (#1059) conclusions: the backlog is human-gated by design and the remediation loop should idle until a draft flips to ready or a PR gains the automerge label. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CQDL89d1xNEm4hijHZMaSf
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
documentation
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 |
Agent Completion Truth Gate: BLOCKEDReasons: Machine-readable verdict{
"details": {
"invalid_fields": [
"issue.number",
"policy.agent_login",
"policy.run_id"
]
},
"reasons": [
"invalid_payload"
],
"verdict": "blocked"
} |
groupthinking
added
duplicate
ChatGPT Codex Connector
|
Contained as draft duplicate evidence.
Do not transfer or merge this mixed payload as a competing implementation. Keep #1077 draft while the existing canonical unit remains #912/#831. No branch was deleted and no work was discarded. |
groupthinking
added
tests
and removed
duplicate
Claude
|
Acknowledged — and verified directly against the repo, not just this thread. #1077 is a duplicate and I am keeping it draft. It will not be marked ready, merged, or transferred. What I confirmed by reading the canonical units:
On the red checks (none are code defects, none are autonomously fixable here):
Focused tests on the security payload pass locally (119 passed across Generated by Claude Code |
|
Noted that this PR was marked ready for review (actor: #1077 is still a verified duplicate — triage record → canonical #1076; CWE-209 payload → canonical #831 / #912 at head
I am not merging this to protected This appears to contradict the earlier instruction on this PR ("keep #1077 draft ... do not merge as a competing implementation"). Since I can't reconcile that with the un-draft autonomously, I'm holding for an explicit owner decision:
Holding here. Further identical governance re-fires on this head are already accounted for. Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Adds the July 29 PR-remediation audit and hardens API responses against CWE-209 information disclosure.
Changes:
- Records triage results for 36 draft PRs.
- Sanitizes exception details across cloud and real API responses.
- Expands regression tests and generated FastAPI error handling.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
docs/triage/pr-remediation-2026-07-29.md |
Records remediation outcomes. |
src/agents/specialized/code_generator.py |
Generates sanitized 500 responses. |
src/youtube_extension/backend/cloud_ai_routes.py |
Sanitizes rate-limit details. |
src/youtube_extension/backend/cloud_api_endpoints.py |
Sanitizes cloud response trees. |
src/youtube_extension/backend/real_api_endpoints.py |
Sanitizes real API responses. |
src/youtube_extension/backend/services/youtube/adapters/official_api.py |
Removes exception text from validation responses. |
tests/unit/test_500_info_disclosure.py |
Expands static disclosure guards. |
tests/unit/test_cloud_routes.py |
Tests cloud-response sanitization. |
tests/unit/test_code_generator_agent.py |
Tests generated endpoint safety. |
tests/unit/test_real_api_endpoints.py |
Tests recursive response sanitization. |
| def _uses_public_error_sanitizer(node: ast.AST) -> bool: | ||
| return isinstance(node, ast.Call) and _call_name(node) in { | ||
| "_client_safe_error", | ||
| "_sanitize_public_error", | ||
| "_sanitize_response_errors", | ||
| "_sanitize_error_list", | ||
| } |
There was a problem hiding this comment.
Verified — valid false negative. The Pass-3 allowlist in _uses_public_error_sanitizer accepts _sanitize_response_errors and _sanitize_error_list as sufficient for a scalar error= sink, but those are whole-tree sanitizers that return non-container inputs unchanged. So VideoAnalysisResponse(error=_sanitize_response_errors(result["error"])) satisfies the guard while still forwarding the raw diagnostic (Pydantic then coerces it to a string).
Fix: for scalar response-model error fields, require a scalar-replacing helper (_client_safe_error / _sanitize_public_error); keep the tree sanitizers allowlisted only for whole dict/list processor results, which are checked separately.
Generated by Claude Code
| def _status_symbol_table(tree: ast.Module) -> dict[str, int]: | ||
| """Resolve module constants that alias literal or standard status values.""" | ||
| symbols: dict[str, int] = {} | ||
| changed = True | ||
| while changed: | ||
| changed = False | ||
| for node in tree.body: | ||
| targets: list[ast.AST] = [] | ||
| value: ast.AST | None = None | ||
| if isinstance(node, ast.Assign): | ||
| targets, value = node.targets, node.value | ||
| elif isinstance(node, ast.AnnAssign) and node.value is not None: | ||
| targets, value = [node.target], node.value | ||
| if value is None: | ||
| continue | ||
| status = _status_code_value(value, symbols) |
There was a problem hiding this comment.
Verified against the code — this is a real false negative. _status_symbol_table only walks tree.body, so a status aliased inside a function (e.g. server_failure = status.HTTP_503_SERVICE_UNAVAILABLE followed by HTTPException(status_code=server_failure, detail=str(exc))) never enters symbols. _status_code_value then falls through to _named_status_code('server_failure') → None, and _status_is_server_error skips the call entirely.
Fix: resolve assignments in the enclosing lexical scope of each response call (walk FunctionDef/AsyncFunctionDef bodies, not just module scope) and add a local-alias control case so the guard stays honest.
Recording this as verified rather than force-landing a patch here: this PR is currently red on the agent-completion gate (a known misfire being fixed separately) and awaits a human protected-main publish decision, and its branch differs from this session's mandated working branch — so these two guard fixes should land on this branch (or fold into the #831 canonical work) via an authorized push, not a competing duplicate PR.
Generated by Claude Code
|
Automated shepherd note — PR marked ready-for-review. I diagnosed the red CI and am recording why it is not auto-fixed. Code checks pass: Every red check is a repo agentic-governance gate, in two distinct causes:
Merge is a protected- Terminal state: Generated by Claude Code |
|
Re: Copilot's two review comments on
Disposition: I am not pushing these fixes here. #1077 is a duplicate held for a close-vs-canonical decision, and the identical code is canonical in #831 / #912 at the same head Generated by Claude Code |
Both gaps were found by Copilot on #1077 and verified against the source; the fix is landed on this branch per the owner's direction (not a new PR). 1. Local status aliases: _status_symbol_table only walked module scope, so a 5xx code aliased inside a function body (server_failure = HTTP_503; then HTTPException(status_code=server_failure, ...)) resolved to None and the call was skipped. Resolve each response call against its module scope plus every enclosing FunctionDef/AsyncFunctionDef scope. Adds local-alias positive (5xx) and negative (4xx) control cases. 2. Scalar pass-through sanitizer: the keyword-sink allowlist accepted the whole-tree walkers (_sanitize_response_errors/_sanitize_error_list), which return non-container inputs unchanged, so error=_sanitize_response_errors( scalar) evaded the guard while forwarding the raw diagnostic. Scalar error/error_message sinks now require a value-replacing helper (_client_safe_error/_sanitize_public_error); errors (list) keeps _sanitize_error_list; the tree walkers remain valid only for whole processor results, checked separately. Adds matching controls. Refs #831 #912 (canonical CWE-209 work); Copilot review on #1077. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CQDL89d1xNEm4hijHZMaSf
|
Both verified guard false-negatives are now fixed and pushed to this branch (head 1. Local status aliases. 2. Scalar pass-through sanitizer. The keyword-sink allowlist is now field-aware and value-replacing: scalar Verification: Note on canonical sync: the identical gaps exist in #831/#912 (same code at Generated by Claude Code |
|
Quick note on the checks after the
The two CWE-209 guard fixes you authorized are in and green ( Generated by Claude Code |
Independent verification of PR #1077 (CWE-209) — no substantive findingsI picked up #912 as apparently-unclaimed work (canonical PR #831 is closed unmerged), independently re-derived the fix from the acceptance criteria, and only then found PR #1077. Rather than open a competing PR, I converted the work into the independent verification this issue is blocked on. My branch was deleted; no competing PR was opened. Verified against 1. Convergent-implementation evidenceI built my version from #912's text alone, without reading #1077. It converged on the same core fix, including the identical constructor-signature insight:
Two independent derivations landing on the same constructor-index fix is decent evidence it's the right one. #1077 is a strict superset of what I built — it has four capabilities I lacked and none of mine are missing from it. My work is redundant and I'm dropping it. 2. Test execution(Needed 3. Non-vacuity — the part that actually mattersA green guard proves nothing until you prove it can go red. I injected four regressions, one per claimed capability:
Every claimed capability is independently exercised. No vacuous assertions found. NC-2 and NC-4 are worth calling out: both are invisible to a scanner that only reads integer literals, and both are realistic regression shapes. The symbol-table + enclosing-scope resolution is doing real work. 4. Correctness of the
|
Canonical issue
Relates to the recurring PR-remediation runbook (prior records: #1044, #1059). The
security commits on this branch are the refreshed canonical CWE-209 work also tracked by
#831 — this branch rebases that work onto verified
main; it is not a competingre-implementation.
Outcome
Two things land on this branch:
docs/triage/pr-remediation-2026-07-29.md— the audit record for the 2026-07-29remediation run. Oldest-first scan of all 36 open PRs. Result: 0 autonomously
mergeable (the correct, safe outcome). 36
DEFERRED(draft), 0HALTED, 0MERGED.Every open PR is a draft; fix(auth): restore Google OAuth configuration in Vercel production #903 (the single human-gated PR on 07-28) has been returned
to draft, so nothing is HALTED today.
insertions across
cloud_api_endpoints.py,real_api_endpoints.py,cloud_ai_routes.py,official_api.py,code_generator.py, and their tests. Guardsagainst internal exception / error-tree leakage in non-500 and cloud response bodies.
Rebased onto verified
main(0 commits behind).Scope
docs/triage/pr-remediation-2026-07-29.md— full triage table + duplicate-clusterhousekeeping notes + loop decision
canonical work, rebased)
main(publish gate is human-by-default; no PR carriesautomerge)jules-*) branchesRisk
added tests asserting exact sanitized shapes
Verification
test_500_info_disclosure.py,test_cloud_routes.py,test_code_generator_agent.py— 119 passed locally(
test_real_api_endpoints.pyrequires the pinned pydantic-v1 / DB stack the localsandbox couldn't reconstruct; it is exercised by required CI on the head SHA)
Production evidence
Not applicable to the triage doc (documentation only). The CWE-209 guards change API
response construction but no production deploy is performed by this PR — production
remains on verified
main.Agent handoff
work rebased, not a new implementation
clears the protected-
mainpublish gate on the security change, and decides whether tofold fix(security): restore CWE-209 response protections #831 into this refreshed branch or keep them separate
Agent provenance
Agent-authored triage + refreshed security branch. Scope and terminal states are
authoritative in the committed
docs/triage/pr-remediation-2026-07-29.md.Generated by Claude Code