diff --git a/docs/triage/pr-remediation-2026-07-29.md b/docs/triage/pr-remediation-2026-07-29.md new file mode 100644 index 000000000..c8a2e68cb --- /dev/null +++ b/docs/triage/pr-remediation-2026-07-29.md @@ -0,0 +1,111 @@ +# PR Remediation Run — 2026-07-29 + +Runbook: PR Remediation & Publish Runbook (RASOR). GitHub surface: `github-mcp` +(PR read + comment + merge). Scan performed against `groupthinking/eventrelay`, +oldest-first. Auto-merge policy: `label:automerge` (no open PR carries it) → every +merge to protected `main` remains a human gate by design. Prior records: #1044 +(2026-07-27), #1059 (2026-07-28). + +## Definition-of-done outcome + +- **Open PRs scanned:** 36 +- **Autonomously mergeable this run:** 0 — the correct, safe outcome, not a failure. + All 36 open PRs are drafts (scope-gated → `DEFERRED(draft)`). Merging any of them + requires the author to first mark the PR ready and a human to clear the publish gate + on protected `main`. +- **Terminal-state tally:** 36 `DEFERRED(draft)`, 0 `HALTED`, 0 `MERGED`. +- **Delta vs 2026-07-28:** +3 net open PRs (33 → 36). New since last run: #1059 + (the 07-28 triage record itself), #1064 (`docs(runbook)` Google OAuth 403 diagnosis), + #1075 (`fix(pipeline)` preserve captured transcript on analysis timeout). No PR was + merged or closed between runs. +- **#903 status change:** on 2026-07-28 #903 was the single non-draft, human-gated PR + (`HALTED(awaiting_human_production_approval)`). It has since been **restored to draft** + (`draft: true`, head `jules-15243187445261469621-ffdb089e@57ff988`, + `mergeable_state: unstable`). It therefore scope-gates to `DEFERRED(draft)` this run — + no PR remains in a HALTED (human-decision-pending) state today. + +## Why zero autonomous merges (unchanged, structural) + +1. **Every open PR is a draft.** The runbook scope gate routes drafts to + `DEFERRED` — the author must request review before any remediation loop can advance + them. This is a repo-wide posture, not a per-PR defect. +2. **Protected `main` + human publish gate.** No open PR carries the `automerge` label, + so `auto_merge_policy: label:automerge` yields no eligible merge. Auto-merging to a + protected branch is explicitly out of scope for the runbook. +3. **No outward-facing CodeRabbit fan-out performed.** Posting `@coderabbitai full review` + across 36 draft PRs (15 of them dependabot bumps, several duplicate bot proposals) + would consume the paid review allowance for no mergeable outcome. Deferred until a PR + flips to ready. + +## Deferred — drafts (scope gate: draft → `DEFERRED(draft)`) + +| PR | Age (d) | Author | Title | Terminal state | +|----|---------|--------|-------|----------------| +| 734 | 17 | groupthinking | fix(security): pin cloud callbacks against DNS rebinding | DEFERRED(draft) | +| 810 | 12 | groupthinking | fix(security): sanitize user-controlled values in API logs (CWE-117) | DEFERRED(draft) | +| 831 | 12 | groupthinking | fix(security): restore CWE-209 response protections | DEFERRED(draft) | +| 869 | 11 | groupthinking | fix: harden API-cost webhook outbox retries (MYX-79) | DEFERRED(draft) | +| 903 | 9 | jules[bot] | fix(auth): restore Google OAuth configuration in Vercel production | DEFERRED(draft) | +| 906 | 9 | groupthinking | fix(ci): remediate PR #877 rollout and verification gaps | DEFERRED(draft) | +| 961 | 6 | jules[bot] | [DRAFT EVIDENCE] duplicate dashboard accessibility proposal | DEFERRED(draft) | +| 987 | 4 | Copilot | [DRAFT EVIDENCE] unbound CI and module-shadowing proposal | DEFERRED(draft) | +| 995 | 4 | groupthinking | perf(mcp): reuse pooled aiohttp session in orchestrator | DEFERRED(draft) | +| 996 | 4 | groupthinking | fix(mcp): actually reuse pooled aiohttp session | DEFERRED(draft) | +| 997 | 4 | jules[bot] | Bolt: optimize layout boundary in AgentFlowVisualizer | DEFERRED(draft) | +| 999 | 4 | dependabot | build(deps): bump gh-aw-actions/setup 0.82.14→0.83 | DEFERRED(draft) | +| 1000 | 4 | dependabot | build(deps): bump actions/checkout 4.2.2→7.0.1 | DEFERRED(draft) | +| 1001 | 4 | dependabot | build(deps): bump actions/setup-python 6→7 | DEFERRED(draft) | +| 1002 | 4 | dependabot | build(deps-dev): bump locust 2.45.0→2.46.0 | DEFERRED(draft) | +| 1003 | 4 | dependabot | build(deps): bump actions/github-script 8→9 | DEFERRED(draft) | +| 1004 | 4 | dependabot | build(deps): bump @opentelemetry/exporter-trace-otlp-http | DEFERRED(draft) | +| 1005 | 4 | dependabot | build(deps): bump @opentelemetry/core 2.9.0→2.10.0 | DEFERRED(draft) | +| 1006 | 4 | dependabot | build(deps): bump @opentelemetry/instrumentation | DEFERRED(draft) | +| 1007 | 4 | dependabot | build(deps): bump @opentelemetry/resources 2.9.0→2.10.0 | DEFERRED(draft) | +| 1008 | 4 | dependabot | build(deps): bump @opentelemetry/sdk-trace-base | DEFERRED(draft) | +| 1020 | 3 | jules[bot] | perf: optimize call stack operations and string allocations | DEFERRED(draft) | +| 1022 | 3 | jules[bot] | perf(web): optimize bounding box in AgentFlowVisualizer | DEFERRED(draft) | +| 1038 | 2 | jules[bot] | feat: implement MCPOrchestrator._execute_on_server E2E | DEFERRED(draft) | +| 1040 | 2 | groupthinking | fix(mcp): green up MCPOrchestrator._execute_on_server E2E | DEFERRED(draft) | +| 1043 | 2 | jules[bot] | perf(web): optimize viewBox boundary computation | DEFERRED(draft) | +| 1044 | 2 | groupthinking | docs(triage): PR remediation run 2026-07-27 | DEFERRED(draft) | +| 1045 | 2 | jules[bot] | Palette: add keyboard focus-visible styling to dashboard | DEFERRED(draft) | +| 1047 | 2 | jules[bot] | ci: suppress failure issues on no-op runs | DEFERRED(draft) | +| 1049 | 2 | groupthinking | fix(a11y): complete dashboard focus contrast + coverage | DEFERRED(draft) | +| 1050 | 2 | jules[bot] | configure agentic workflows no-op comment suppression | DEFERRED(draft) | +| 1052 | 2 | jules[bot] | fix: allow awmg-mcpg gateway in workflow firewalls | DEFERRED(draft) | +| 1055 | 1 | dependabot | build(deps): bump npm-minor-patch group | DEFERRED(draft) | +| 1059 | 1 | groupthinking | docs(triage): PR remediation run 2026-07-28 | DEFERRED(draft) | +| 1064 | 1 | groupthinking | docs(runbook): diagnose Google OAuth 403 org_internal | DEFERRED(draft) | +| 1075 | 0 | groupthinking | fix(pipeline): preserve captured transcript on analysis timeout | DEFERRED(draft) | + +## Duplicate clusters worth a human close (housekeeping, not autonomous) + +These are labeled `duplicate` by the repo's own triage automation and represent +redundant bot proposals accumulating in the backlog. A human closing the superseded +members would shrink the draft queue without any code risk: + +- **AgentFlowVisualizer boundary/viewBox perf:** #997, #1022, #1043 (and related #1020) + — repeated bot passes over the same layout-computation hot path. +- **OpenTelemetry JS bumps:** #1004–#1008 all tagged `duplicate` (overlap with the + #1055 `npm-minor-patch` group bump). +- **aiohttp pooled-session reuse:** #995 vs #996 (same MCP orchestrator change). + +## Loop decision + +**More autonomous work available: no.** Every open PR is a draft; none carries +`automerge`; the one previously human-gated PR (#903) has been returned to draft. The +remediation loop cannot advance any PR to `MERGED` without a human first marking a PR +ready for review and clearing the protected-`main` publish gate. This matches the +2026-07-27 (#1044) and 2026-07-28 (#1059) conclusions — the backlog is human-gated by +design, so the loop should idle until a draft flips to ready, a PR gains `automerge`, or +the owner acts on the #903 / #900 production-OAuth decision. + +## This run's branch (`claude/determined-maxwell-82cm3w`) + +Beyond this triage record, the run's branch carries the **refreshed canonical CWE-209 +response-sanitization work for #831** (13 commits, ~1000 insertions across +`cloud_api_endpoints.py`, `real_api_endpoints.py`, `cloud_ai_routes.py`, +`official_api.py`, `code_generator.py`, and their tests), rebased onto verified `main` +(0 commits behind). It is surfaced here as a **draft** PR for human review — it is a +security change, so it stays behind the same human publish gate as every other PR and is +not auto-merged. diff --git a/src/agents/specialized/code_generator.py b/src/agents/specialized/code_generator.py index 345f51cb6..4ffd19d53 100644 --- a/src/agents/specialized/code_generator.py +++ b/src/agents/specialized/code_generator.py @@ -22,6 +22,10 @@ def _load_templates(self) -> dict[str, str]: return { "fastapi_endpoint": textwrap.dedent( """ + import logging + + logger = logging.getLogger(__name__) + @app.post("/api/v1/{endpoint_name}") async def {function_name}({parameters}): \"\"\" @@ -42,8 +46,12 @@ async def {function_name}({parameters}): }} except ValidationError as e: raise HTTPException(status_code=400, detail=str(e)) - except Exception as e: - raise HTTPException(status_code=500, detail=str(e)) + except Exception: + logger.exception("Generated endpoint failed") + raise HTTPException( + status_code=500, + detail="Internal server error", + ) """ ), "rest_api": textwrap.dedent( diff --git a/src/youtube_extension/backend/cloud_ai_routes.py b/src/youtube_extension/backend/cloud_ai_routes.py index 49804aba1..a53361ea9 100644 --- a/src/youtube_extension/backend/cloud_ai_routes.py +++ b/src/youtube_extension/backend/cloud_ai_routes.py @@ -259,7 +259,7 @@ async def analyze_video(request: VideoAnalysisRequest): # Must precede CloudAIError: RateLimitError subclasses it, so catching # the base first would shadow this handler and return a 503 instead. logger.warning(f"Rate limit exceeded: {e}") - raise HTTPException(status_code=429, detail=f"Rate limit exceeded: {str(e)}") + raise HTTPException(status_code=429, detail="Rate limit exceeded") except ConfigurationError as e: # Must precede CloudAIError (same subclassing reason) so configuration # failures reach this sanitized 500 rather than the dynamic 503 below. diff --git a/src/youtube_extension/backend/cloud_api_endpoints.py b/src/youtube_extension/backend/cloud_api_endpoints.py index cdf2844e2..1b9431c5e 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -35,6 +35,50 @@ router = APIRouter() +def _client_safe_error(error_message: Any) -> Optional[str]: + """Return a stable client-safe value for persisted processor failures. + + Older Firestore records may predate the write-side sanitizer, so every read + boundary must treat stored error text as untrusted rather than echoing it. + """ + return "Internal server error" if error_message else None + + +def _sanitize_error_list(value: Any) -> Any: + """Replace scalar diagnostic entries while preserving structured shapes.""" + if isinstance(value, list): + return [_sanitize_error_list(item) for item in value] + if isinstance(value, tuple): + return tuple(_sanitize_error_list(item) for item in value) + if isinstance(value, dict): + return _sanitize_response_errors(value) + # Every remaining leaf is a scalar diagnostic. Only ``None`` (absence of an + # error) is preserved; any other non-null leaf — including non-string types + # such as bytes, ints, or bools that FastAPI can still serialize — is + # replaced so it cannot bypass the scalar sanitization invariant. + if value is None: + return None + return "Internal server error" + + +def _sanitize_response_errors(value: Any) -> Any: + """Copy a client response tree and remove persisted diagnostic messages.""" + if isinstance(value, dict): + sanitized: Dict[Any, Any] = {} + for key, item in value.items(): + if key in {"error", "error_message"}: + sanitized[key] = _client_safe_error(item) + elif key == "errors": + sanitized[key] = _sanitize_error_list(item) + else: + sanitized[key] = _sanitize_response_errors(item) + return sanitized + if isinstance(value, list): + return [_sanitize_response_errors(item) for item in value] + if isinstance(value, tuple): + return tuple(_sanitize_response_errors(item) for item in value) + return value + # Pydantic models for API requests/responses class CloudVideoProcessingRequest(BaseModel): @@ -133,12 +177,12 @@ async def process_video_cloud( video_url=result.video_url, success=result.success, status='completed' if result.success else 'failed', - metadata=result.metadata, - transcript=result.transcript, - ai_analysis=result.ai_analysis, + metadata=_sanitize_response_errors(result.metadata), + transcript=_sanitize_response_errors(result.transcript), + ai_analysis=_sanitize_response_errors(result.ai_analysis), processing_time=result.processing_time, from_cache=result.from_cache, - error=result.error_message, + error=_client_safe_error(result.error_message), ) except Exception as e: @@ -218,8 +262,8 @@ async def process_video_task_handler( status='failed', error_message="Task processing failed" ) - except Exception as state_error: - logger.error(f"Failed to update error state: {state_error}") + except Exception: + logger.error("Failed to update error state", exc_info=True) raise HTTPException(status_code=500, detail="Internal server error") @@ -280,7 +324,7 @@ async def get_video_status(video_id: str): created_at=state.created_at, updated_at=state.updated_at, processing_time=state.processing_time, - error_message=state.error_message, + error_message=_client_safe_error(state.error_message), ) except HTTPException: @@ -312,13 +356,13 @@ async def get_video_result(video_id: str): "video_url": state.video_url, "status": state.status, "current_stage": state.current_stage, - "metadata": state.metadata, - "transcript": state.transcript, - "ai_analysis": state.ai_analysis, + "metadata": _sanitize_response_errors(state.metadata), + "transcript": _sanitize_response_errors(state.transcript), + "ai_analysis": _sanitize_response_errors(state.ai_analysis), "processing_time": state.processing_time, "created_at": state.created_at, "updated_at": state.updated_at, - "error_message": state.error_message, + "error_message": _client_safe_error(state.error_message), } except HTTPException: @@ -345,11 +389,11 @@ async def get_queue_stats(): "timestamp": datetime.now(timezone.utc).isoformat(), } - except Exception as e: - logger.error(f"Error getting queue stats: {e}") + except Exception: + logger.error("Error getting queue stats", exc_info=True) return { "success": False, - "error": str(e), + "error": "Internal server error", "timestamp": datetime.now(timezone.utc).isoformat(), } @@ -372,10 +416,11 @@ async def get_cloud_status(): "status": "operational", "enabled": True, } - except Exception as e: + except Exception: + logger.error("firestore status check failed", exc_info=True) status["services"]["firestore"] = { "status": "error", - "error": str(e), + "error": "Service unavailable", } status["overall_status"] = "degraded" @@ -388,10 +433,11 @@ async def get_cloud_status(): "enabled": True, "queue_stats": stats, } - except Exception as e: + except Exception: + logger.error("cloud_tasks status check failed", exc_info=True) status["services"]["cloud_tasks"] = { "status": "error", - "error": str(e), + "error": "Service unavailable", } status["overall_status"] = "degraded" @@ -402,20 +448,21 @@ async def get_cloud_status(): "status": "operational", "enabled": True, } - except Exception as e: + except Exception: + logger.error("vertex_ai status check failed", exc_info=True) status["services"]["vertex_ai"] = { "status": "error", - "error": str(e), + "error": "Service unavailable", } status["overall_status"] = "degraded" return status - except Exception as e: - logger.error(f"Error getting cloud status: {e}") + except Exception: + logger.error("Error getting cloud status", exc_info=True) return { "overall_status": "error", - "error": str(e), + "error": "Internal server error", "timestamp": datetime.now(timezone.utc).isoformat(), } diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 2fd932310..5f86fa013 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -26,6 +26,64 @@ # Configure logging logger = logging.getLogger(__name__) +_PUBLIC_PROCESSING_ERROR = "Video processing failed" + + +def _sanitize_public_error(value: Any) -> Optional[str]: + """Return a client-safe processing error without exposing provider details.""" + return None if value is None else _PUBLIC_PROCESSING_ERROR + + +def _sanitize_error_list(value: Any) -> Any: + """Sanitize a plural ``errors`` collection. + + The AI processor records per-step failures as scalar strings that embed + exception text (e.g. ``"content_analysis: "`` in + ``real_ai_processor.analyze_video_content``). Replace those scalar strings + with the public message, while preserving — and recursing into — structured + error records so batch result shapes stay intact. + """ + if isinstance(value, list): + return [_sanitize_error_list(item) for item in value] + if isinstance(value, tuple): + return tuple(_sanitize_error_list(item) for item in value) + if isinstance(value, dict): + return _sanitize_response_errors(value) + # Every remaining leaf is a scalar diagnostic. Only ``None`` (absence of an + # error) is preserved; any other non-null leaf — including non-string types + # such as bytes, ints, or bools that FastAPI can still serialize — is + # replaced so it cannot bypass the scalar sanitization invariant. + if value is None: + return None + return _PUBLIC_PROCESSING_ERROR + + +def _sanitize_response_errors(value: Any) -> Any: + """Copy a response tree while replacing every diagnostic error value. + + Processor results can contain exception text at the top level and inside + batch, step, or provider records. Sanitizing the complete response tree at + the HTTP boundary also protects cached legacy records without mutating the + processor's internal diagnostic value. The plural ``errors`` collection is + handled by :func:`_sanitize_error_list` because it carries scalar exception + strings that a plain recursion would pass through unchanged. + """ + if isinstance(value, dict): + sanitized: dict[Any, Any] = {} + for key, item in value.items(): + if key in {"error", "error_message"}: + sanitized[key] = _sanitize_public_error(item) + elif key == "errors": + sanitized[key] = _sanitize_error_list(item) + else: + sanitized[key] = _sanitize_response_errors(item) + return sanitized + if isinstance(value, list): + return [_sanitize_response_errors(item) for item in value] + if isinstance(value, tuple): + return tuple(_sanitize_response_errors(item) for item in value) + return value + # Pydantic models for API requests/responses class VideoProcessingRequest(BaseModel): video_url: str = Field(..., description="YouTube video URL or ID") @@ -95,22 +153,23 @@ async def process_video_real_api(request: VideoProcessingRequest, background_tas video_url=request.video_url, force_refresh=request.force_refresh ) + safe_result = _sanitize_response_errors(result) # Track metrics processing_time = (datetime.now(timezone.utc) - start_time).total_seconds() # Format response response = VideoAnalysisResponse( - video_id=result.get('video_id', ''), + video_id=safe_result.get('video_id', ''), video_url=request.video_url, - success=result.get('success', False), - metadata=result.get('metadata'), - transcript=result.get('transcript'), - ai_analysis=result.get('ai_analysis'), - cost_breakdown=result.get('cost_breakdown'), + success=safe_result.get('success', False), + metadata=safe_result.get('metadata'), + transcript=safe_result.get('transcript'), + ai_analysis=safe_result.get('ai_analysis'), + cost_breakdown=safe_result.get('cost_breakdown'), processing_time=processing_time, - cached=result.get('cached', False), - error=result.get('error') + cached=safe_result.get('cached', False), + error=_sanitize_public_error(safe_result.get('error')) ) logger.info(f"✅ Real API processing completed: {result.get('video_id')} - ${result.get('cost_breakdown', {}).get('total_cost', 0):.4f}") @@ -168,7 +227,7 @@ async def batch_process_videos(request: BatchProcessingRequest): max_concurrent=request.max_concurrent ) - return result + return _sanitize_response_errors(result) except HTTPException: # Preserve explicit 4xx responses (e.g. the 400 batch-size guard above). @@ -208,7 +267,9 @@ async def get_processed_videos_list(): "has_transcript": video_data.get('transcript', {}).get('has_transcript', False), "ai_analysis_success": video_data.get('ai_analysis', {}).get('success', False), "total_cost": video_data.get('cost_breakdown', {}).get('total_cost', 0.0), - "analysis": video_data.get('ai_analysis', {}), + "analysis": _sanitize_response_errors( + video_data.get('ai_analysis', {}) + ), "createdAt": video_data.get('timestamp'), "updatedAt": video_data.get('timestamp') }) @@ -245,7 +306,7 @@ async def get_video_analysis(video_id: str): with open(cache_path, encoding='utf-8') as f: video_data = json.load(f) - return video_data + return _sanitize_response_errors(video_data) except HTTPException: raise @@ -265,10 +326,10 @@ async def get_cost_dashboard(): dashboard = await cost_monitor.get_cost_dashboard() return dashboard - except Exception as e: - logger.error(f"Error getting cost dashboard: {e}") + except Exception: + logger.error("Error getting cost dashboard", exc_info=True) return { - "error": str(e), + "error": "Internal server error", "timestamp": datetime.now(timezone.utc).isoformat() } @@ -289,10 +350,10 @@ async def get_usage_analytics(days: int = 7): except HTTPException: raise - except Exception as e: - logger.error(f"Error getting usage analytics: {e}") + except Exception: + logger.error("Error getting usage analytics", exc_info=True) return { - "error": str(e), + "error": "Internal server error", "timestamp": datetime.now(timezone.utc).isoformat() } @@ -305,10 +366,10 @@ async def get_optimization_recommendations(): recommendations = await cost_monitor.optimize_api_usage() return recommendations - except Exception as e: - logger.error(f"Error getting optimization recommendations: {e}") + except Exception: + logger.error("Error getting optimization recommendations", exc_info=True) return { - "error": str(e), + "error": "Internal server error", "recommendations": [], "timestamp": datetime.now(timezone.utc).isoformat() } @@ -336,7 +397,7 @@ async def get_service_status(): return { "overall_status": "operational" if processor_status.get('service_status') == 'operational' else "degraded", "timestamp": datetime.now(timezone.utc).isoformat(), - "processor": processor_status, + "processor": _sanitize_response_errors(processor_status), "cost_monitoring": { "status": "operational", "today_cost": cost_dashboard.get('today_summary', {}).get('total_cost', 0.0), @@ -354,11 +415,11 @@ async def get_service_status(): "version": "2.0.0-real-api-integration" } - except Exception as e: - logger.error(f"Error getting service status: {e}") + except Exception: + logger.error("Error getting service status", exc_info=True) return { "overall_status": "error", - "error": str(e), + "error": "Internal server error", "timestamp": datetime.now(timezone.utc).isoformat() } diff --git a/src/youtube_extension/backend/services/youtube/adapters/official_api.py b/src/youtube_extension/backend/services/youtube/adapters/official_api.py index 80d2dd2f3..cc8dfa4f1 100644 --- a/src/youtube_extension/backend/services/youtube/adapters/official_api.py +++ b/src/youtube_extension/backend/services/youtube/adapters/official_api.py @@ -578,10 +578,14 @@ async def validate_video_url(self, url: str) -> tuple[bool, str, str]: return True, video_id, "Video is public and accessible" - except ValueError as e: - return False, "", f"Invalid URL format: {e}" - except Exception as e: - return False, "", f"Video validation failed: {e}" + except ValueError: + # This message is returned to clients under ``message`` by + # /api/v2/validate-video; never embed the exception text (CWE-209). + logger.warning("Video URL validation failed: invalid URL format", exc_info=True) + return False, "", "Invalid URL format" + except Exception: + logger.error("Video URL validation failed", exc_info=True) + return False, "", "Video validation failed" async def close(self): """Close the HTTP client""" diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index 98ce7f49c..0c95ccec8 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -36,11 +36,16 @@ from __future__ import annotations import ast +from http import HTTPStatus from pathlib import Path import pytest -_BACKEND = Path(__file__).resolve().parents[2] / "src" / "youtube_extension" / "backend" +_REPO_ROOT = Path(__file__).resolve().parents[2] +_BACKEND = _REPO_ROOT / "src" / "youtube_extension" / "backend" +# The Ray Serve ML surface returns raw ``JSONResponse(...)`` bodies and lives +# outside ``backend/``; it must be scanned too or 500 leaks there go unguarded. +_ML_SERVE = _REPO_ROOT / "src" / "uvai" / "ml" # Identifiers that, when referenced inside a 500 body, indicate a leak of the # caught exception or the inbound request. @@ -79,13 +84,74 @@ def _refs_exception_or_request(node: ast.AST) -> bool: return False -def _status_is_500(call: ast.Call) -> bool: +def _named_status_code(name: str) -> int | None: + """Resolve standard HTTP status symbols without importing application code.""" + if name.startswith("HTTP_"): + code = name.removeprefix("HTTP_").split("_", 1)[0] + if len(code) == 3 and code.isdigit(): + return int(code) + member = HTTPStatus.__members__.get(name) + return int(member) if member is not None else None + + +def _status_code_value(node: ast.AST, symbols: dict[str, int]) -> int | None: + if isinstance(node, ast.Constant) and isinstance(node.value, int): + return node.value + if isinstance(node, ast.Name): + return symbols.get(node.id, _named_status_code(node.id)) + if isinstance(node, ast.Attribute): + if node.attr == "value": + return _status_code_value(node.value, symbols) + return _named_status_code(node.attr) + return None + + +def _status_symbol_table( + body: list[ast.stmt], base: dict[str, int] | None = None +) -> dict[str, int]: + """Resolve constants in one lexical scope that alias status values. + + ``body`` is a scope's direct statement list — a module body or a + ``FunctionDef``/``AsyncFunctionDef`` body. ``base`` seeds the table with + symbols visible from enclosing scopes so a local alias of an outer constant + still resolves; only the direct statements of ``body`` are scanned, so a + status aliased inside a *nested* function is not leaked into this scope. + """ + symbols: dict[str, int] = dict(base) if base else {} + changed = True + while changed: + changed = False + for node in 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) + if status is None: + continue + for target in targets: + if isinstance(target, ast.Name) and symbols.get(target.id) != status: + symbols[target.id] = status + changed = True + return symbols + + +def _status_is_server_error(call: ast.Call, name: str, symbols: dict[str, int]) -> bool: for kw in call.keywords: - if kw.arg == "status_code" and isinstance(kw.value, ast.Constant): - return kw.value.value == 500 - # positional status_code (JSONResponse(500, ...) / HTTPException(500, ...)) - if call.args and isinstance(call.args[0], ast.Constant): - return call.args[0].value == 500 + if kw.arg == "status_code": + status = _status_code_value(kw.value, symbols) + return status is not None and 500 <= status <= 599 + # The positional slot of ``status_code`` differs by constructor: + # HTTPException(status_code, detail, ...) -> args[0] + # JSONResponse(content, status_code, ...) -> args[1] + idx = 1 if name == "JSONResponse" else 0 + if len(call.args) > idx: + status = _status_code_value(call.args[idx], symbols) + return status is not None and 500 <= status <= 599 return False @@ -97,13 +163,62 @@ def _call_name(call: ast.Call) -> str | None: def _iter_500_leaks(text: str): """Yield (line_no, reason) for each 500 response that can leak internals.""" tree = ast.parse(text) + module_symbols = _status_symbol_table(tree.body) + + # A status code aliased inside a function body is only visible in that + # lexical scope, so resolving against module scope alone is a false negative + # (``server_failure = status.HTTP_503_...`` then + # ``HTTPException(status_code=server_failure, ...)``). Resolve each response + # call against its module scope plus every enclosing function scope. + parents: dict[int, ast.AST] = {} + for parent in ast.walk(tree): + for child in ast.iter_child_nodes(parent): + parents[id(child)] = parent + + scope_cache: dict[int, dict[str, int]] = {} + + def _symbols_for(call: ast.AST) -> dict[str, int]: + enclosing: list[ast.AST] = [] + cur = parents.get(id(call)) + while cur is not None: + if isinstance(cur, (ast.FunctionDef, ast.AsyncFunctionDef)): + enclosing.append(cur) + cur = parents.get(id(cur)) + symbols = module_symbols + for func in reversed(enclosing): # outermost enclosing scope first + cached = scope_cache.get(id(func)) + if cached is None: + cached = _status_symbol_table(func.body, base=symbols) + scope_cache[id(func)] = cached + symbols = cached + return symbols + + # Bind every response constructor to the exception aliases visible from + # its enclosing handler. The fixed conventional-name set remains useful + # outside a handler, but it must not be the tree-wide security boundary: + # ``except Exception as failure`` and aliases derived from ``failure`` are + # equally sensitive. + handler_taint_by_call: dict[int, set[str]] = {} + for handler in ast.walk(tree): + if not isinstance(handler, ast.ExceptHandler) or not handler.name: + continue + tainted = _tainted_names(handler) + for child in ast.walk(handler): + if isinstance(child, ast.Call): + handler_taint_by_call.setdefault(id(child), set()).update(tainted) + + def _refs_server_error_state(call: ast.Call, value: ast.AST) -> bool: + return _refs_exception_or_request(value) or _refs_any_name( + value, handler_taint_by_call.get(id(call), set()) + ) + for node in ast.walk(tree): if not isinstance(node, ast.Call): continue name = _call_name(node) if name not in ("HTTPException", "JSONResponse"): continue - if not _status_is_500(node): + if not _status_is_server_error(node, name, _symbols_for(node)): continue # Check keyword arguments for kw in node.keywords: @@ -111,29 +226,39 @@ def _iter_500_leaks(text: str): 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): + if _refs_server_error_state(node, kw.value): yield node.lineno, "JSONResponse 500 body references the exception/request" # Check positional detail argument: HTTPException(status_code, detail) # args[0] is status_code (already checked by _status_is_500); args[1] is detail. if name == "HTTPException" and len(node.args) >= 2: if not _is_static_string(node.args[1]): yield node.lineno, "HTTPException 500 detail is not a static string" + # Positional JSONResponse body: JSONResponse(, status_code=500) and + # the fully positional JSONResponse(, 500). The content is always + # args[0] for JSONResponse, regardless of how status_code is passed. + if name == "JSONResponse" and node.args: + if _refs_server_error_state(node, node.args[0]): + yield node.lineno, "JSONResponse 500 body references the exception/request" -def _backend_python_files() -> list[Path]: - return sorted(_BACKEND.rglob("*.py")) +def _guarded_python_files() -> list[Path]: + files: list[Path] = [] + for root in (_BACKEND, _ML_SERVE): + if root.exists(): + files.extend(root.rglob("*.py")) + return sorted(files) def test_no_information_disclosure_in_500_responses() -> None: offenders: list[str] = [] - for path in _backend_python_files(): + for path in _guarded_python_files(): text = path.read_text(encoding="utf-8") try: leaks = list(_iter_500_leaks(text)) except SyntaxError as exc: # pragma: no cover - source is valid Python raise AssertionError(f"could not parse {path}: {exc}") from exc for line_no, reason in leaks: - rel = path.relative_to(_BACKEND.parents[2]) + rel = path.relative_to(_REPO_ROOT) offenders.append(f"{rel}:{line_no}: {reason}") assert not offenders, ( @@ -146,17 +271,38 @@ def test_no_information_disclosure_in_500_responses() -> None: def test_guard_detects_every_known_leak_shape() -> None: """Positive controls: the scanner flags each historical leak shape.""" leaky_samples = [ - 'raise HTTPException(status_code=500, detail=str(e))', + "raise HTTPException(status_code=500, detail=str(e))", 'raise HTTPException(status_code=500, detail=f"boom: {e}")', - 'raise HTTPException(status_code=500, detail=error_msg)', + "raise HTTPException(status_code=500, detail=error_msg)", 'raise HTTPException(status_code=500, detail={"message": error_msg})', 'return JSONResponse(status_code=500, content={"detail": str(exc)})', 'return JSONResponse(status_code=500, content={"path": str(request.url)})', 'return JSONResponse(status_code=500, content={"t": exc.__class__.__name__})', # Positional-argument form: HTTPException(status_code, detail) - 'raise HTTPException(500, str(e))', + "raise HTTPException(500, str(e))", 'raise HTTPException(500, f"internal: {exc}")', - 'raise HTTPException(500, error_msg)', + "raise HTTPException(500, error_msg)", + "raise HTTPException(status_code=503, detail=str(e))", + "raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail=str(e))", + "raise HTTPException(status_code=HTTPStatus.INTERNAL_SERVER_ERROR, detail=str(e))", + "SERVER_FAILURE = status.HTTP_502_BAD_GATEWAY\n" + "raise HTTPException(status_code=SERVER_FAILURE, detail=str(e))", + 'raise HTTPException(599, f"internal: {exc}")', + # JSONResponse with a positional body (the real ml_serve leak shape) — + # status via keyword and fully positional (body=args[0], status=args[1]). + 'return JSONResponse({"error": str(exc)}, status_code=500)', + 'return JSONResponse({"error": str(exc)}, 500)', + 'return JSONResponse({"error": str(exc)}, 503)', + # A nonstandard handler name and intermediate alias must remain tainted + # in the tree-wide 5xx scanner. + "try:\n pass\nexcept Exception as failure:\n" + " message = str(failure)\n" + ' return JSONResponse({"error": message}, status_code=503)\n', + # A 5xx status aliased inside a *function* body (not module scope) must + # still be resolved, or the guard has a lexical-scope false negative. + "def handler(exc):\n" + " server_failure = status.HTTP_503_SERVICE_UNAVAILABLE\n" + " raise HTTPException(status_code=server_failure, detail=str(exc))\n", ] for sample in leaky_samples: assert list(_iter_500_leaks(sample)), f"scanner missed a real leak: {sample}" @@ -169,16 +315,402 @@ def test_guard_allows_sanitized_and_safe_dynamic_bodies() -> None: # Positional form with a static string is safe. 'raise HTTPException(500, "Internal server error")', # 4xx echoing client input is out of scope. - 'raise HTTPException(status_code=400, detail=str(exc))', + "raise HTTPException(status_code=400, detail=str(exc))", + "raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(exc))", # A random error id + timestamp is not an internal-disclosure vector. - 'return JSONResponse(status_code=500, content={' + "return JSONResponse(status_code=500, content={" '"id": f"FALLBACK_{uuid.uuid4().hex}", ' '"message": "An unexpected error occurred.", ' '"timestamp": datetime.now().isoformat()})', + # A 4xx status aliased inside a function body must resolve to 4xx and be + # left out of scope — local-scope resolution must not over-flag either. + "def handler(exc):\n" + " client_error = status.HTTP_400_BAD_REQUEST\n" + " raise HTTPException(status_code=client_error, detail=str(exc))\n", ] for sample in safe_samples: assert not list(_iter_500_leaks(sample)), f"scanner false-positived: {sample}" +# --------------------------------------------------------------------------- +# Response-body disclosure — a third 500-class sink the scan above does not model. +# --------------------------------------------------------------------------- +# Some handlers do not *raise* but *return* a dict body — often a 200 +# "degraded"/"failed" payload from a broad ``except Exception`` — that places the +# caught exception under an ``"error"`` key (e.g. ``return {"error": str(e)}``). +# That body reaches the client and leaks internal state exactly like a 500 detail +# would. The ``"error"`` key is targeted specifically: 4xx handlers echo +# client-supplied input under ``"detail"``, which is not a disclosure vector. +# +# The same shape exists more widely across the backend (services/, api/v1/ +# router.py, websocket_service.py); sweeping those is tracked separately, so this +# guard is scoped to the request handlers hardened here. Add a file to +# ``_GUARDED_RESPONSE_FILES`` once its response bodies have been sanitized. +_GUARDED_RESPONSE_FILES = {"cloud_api_endpoints.py", "real_api_endpoints.py"} +# ``errors`` (plural) is scanned too: an AI/batch processor records per-step +# failures as a list of scalar strings under this key, so ``{"errors": +# [str(e)]}`` leaks the caught exception exactly like a scalar ``error`` field +# and a future regression could reintroduce CWE-209 while the guard stayed green. +_RESPONSE_ERROR_FIELDS = {"error", "error_message", "errors"} + + +def _refs_any_name(node: ast.AST, names: set[str]) -> bool: + """True if *node*'s subtree reads any identifier in *names*.""" + return any( + isinstance(leaf, ast.Name) and leaf.id in names for leaf in ast.walk(node) + ) + + +def _assigned_names(target: ast.AST) -> list[str]: + """Names bound by an assignment target (handles tuple/list unpacking).""" + if isinstance(target, ast.Name): + return [target.id] + if isinstance(target, (ast.Tuple, ast.List)): + out: list[str] = [] + for elt in target.elts: + out.extend(_assigned_names(elt)) + return out + return [] + + +def _tainted_names(handler: ast.ExceptHandler) -> set[str]: + """Names that carry the caught exception's text within *handler*. + + Seeds with the handler-bound name and propagates to any variable assigned + from an expression that references an already-tainted name — so an + intermediate alias (``message = str(failure); {"error": message}``) does not + launder the leak past the guard. Iterates to a fixpoint; taint is monotonic. + """ + tainted = {handler.name} if handler.name else set() + if not tainted: + return tainted + changed = True + while changed: + changed = False + for node in ast.walk(handler): + 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, ast.AugAssign)) and node.value: + targets, value = [node.target], node.value + if value is not None and _refs_any_name(value, tainted): + for tgt in targets: + for name in _assigned_names(tgt): + if name not in tainted: + tainted.add(name) + changed = True + return tainted + + +def _iter_response_error_leaks(text: str): + """Yield (line_no, reason) for responses that can expose an ``error`` value. + + A value discloses the caught exception two ways, both flagged: + + * it references the request or a conventionally-named exception variable, or + calls ``str``/``repr`` on one (via ``_refs_exception_or_request``); or + * it references the identifier bound by the *enclosing* + ``except ... as `` handler — whatever that name is (``e``, ``exc``, + ``failure`` …), so an ordinary rename cannot bypass the guard. + """ + tree = ast.parse(text) + seen: set[int] = set() + reason = 'response body "error" field references the caught exception' + + def _error_values(scope: ast.AST): + for node in ast.walk(scope): + if isinstance(node, ast.Dict): + for key, value in zip(node.keys, node.values): + if ( + isinstance(key, ast.Constant) + and key.value in _RESPONSE_ERROR_FIELDS + ): + yield node, value + elif isinstance(node, ast.Call): + for keyword in node.keywords: + if keyword.arg in _RESPONSE_ERROR_FIELDS: + yield node, keyword.value + + # A scalar ``error``/``error_message`` sink must be scrubbed by a + # value-replacing helper. The whole-tree walker ``_sanitize_response_errors`` + # returns non-container inputs unchanged, so wrapping a bare scalar in it + # (``error=_sanitize_response_errors(result["error"])``) satisfies a naive + # allowlist while still forwarding the raw diagnostic. The plural ``errors`` + # list is scrubbed element-wise by ``_sanitize_error_list``. The tree walkers + # stay valid for whole processor results, which the pass below checks + # separately. + _scalar_error_sanitizers = {"_client_safe_error", "_sanitize_public_error"} + _error_list_sanitizers = {"_sanitize_error_list"} + + def _uses_sufficient_sanitizer(node: ast.AST, field: str) -> bool: + if not isinstance(node, ast.Call): + return False + allowed = ( + _error_list_sanitizers if field == "errors" else _scalar_error_sanitizers + ) + return _call_name(node) in allowed + + # Pass 1: values that reference the exception/request by convention. + for node, value in _error_values(tree): + line = getattr(value, "lineno", node.lineno) + if _refs_exception_or_request(value) and line not in seen: + seen.add(line) + yield line, reason + + # Pass 2: values that reference the *enclosing* except handler's bound name + # (including nonstandard names such as ``except Exception as failure``) or any + # intermediate alias assigned from it (``message = str(failure)``). + for handler in ast.walk(tree): + if not isinstance(handler, ast.ExceptHandler) or not handler.name: + continue + tainted = _tainted_names(handler) + for node, value in _error_values(handler): + line = getattr(value, "lineno", node.lineno) + if _refs_any_name(value, tainted) and line not in seen: + seen.add(line) + yield line, reason + + # Response-model keyword arguments are client sinks even when their value + # is not syntactically tied to the surrounding exception. A processor can + # return exception text through ``result.get("error")``; require an + # explicit boundary sanitizer for every dynamic ``error=...`` value. + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + for keyword in node.keywords: + if keyword.arg not in _RESPONSE_ERROR_FIELDS: + continue + value = keyword.value + line = getattr(value, "lineno", node.lineno) + if ( + line not in seen + and not _is_static_string(value) + and not (isinstance(value, ast.Constant) and value.value is None) + and not _uses_sufficient_sanitizer(value, keyword.arg) + ): + seen.add(line) + yield line, 'response model "error" value is not sanitized' + + # Flag the direct pass-through shape used by batch endpoints. This narrow + # data-flow rule follows values returned by the two processor entry points + # that are allowed to carry diagnostic ``error`` records. + processor_results: set[str] = set() + for node in ast.walk(tree): + targets: list[ast.AST] + raw_value: ast.AST | None + if isinstance(node, ast.Assign): + targets, raw_value = node.targets, node.value + elif isinstance(node, ast.AnnAssign): + targets, raw_value = [node.target], node.value + else: + continue + if raw_value is None: + continue + value = raw_value.value if isinstance(raw_value, ast.Await) else raw_value + if not isinstance(value, ast.Call) or _call_name(value) not in { + "process_video", + "batch_process_videos", + }: + continue + for target in targets: + processor_results.update(_assigned_names(target)) + for node in ast.walk(tree): + if ( + isinstance(node, ast.Return) + and isinstance(node.value, ast.Name) + and node.value.id in processor_results + and node.lineno not in seen + ): + seen.add(node.lineno) + yield node.lineno, "processor result is returned without error sanitization" + + +# --------------------------------------------------------------------------- +# Returned-exception disclosure — a service/adapter ``return``s exception text. +# --------------------------------------------------------------------------- +# The scans above model handlers that *raise* a 500 or *return* an ``error``-keyed +# body. A third live shape slips past both: a service method catches an exception +# and ``return``s its text as an ordinary value (string/tuple), which a caller +# then forwards to the client. The real leak was +# ``official_api.validate_video_url`` returning ``f"Video validation failed: +# {e}"`` as its ``message`` element, echoed verbatim by /api/v2/validate-video +# under ``"message"`` with HTTP 200 — a live CWE-209 path the endpoint's own 500 +# handler never sees because the adapter swallows the exception and hands it back +# as data. This scan is scoped to files whose returned values reach a client and +# flags any ``return`` inside an ``except ... as `` handler that carries the +# caught exception (or an alias assigned from it). ``raise`` and ``logger`` calls +# are not returns and are unaffected. +_RETURNED_EXCEPTION_FILES = { + "official_api.py", + "real_api_endpoints.py", + "cloud_api_endpoints.py", +} + + +def _iter_returned_exception_leaks(text: str): + """Yield (line_no, reason) for ``return``s that carry the caught exception.""" + tree = ast.parse(text) + reason = "return value inside except handler carries the caught exception" + for handler in ast.walk(tree): + if not isinstance(handler, ast.ExceptHandler) or not handler.name: + continue + tainted = _tainted_names(handler) + for node in ast.walk(handler): + if ( + isinstance(node, ast.Return) + and node.value is not None + and _refs_any_name(node.value, tainted) + ): + yield node.lineno, reason + + +def test_no_returned_exception_in_guarded_files() -> None: + offenders: list[str] = [] + for path in _guarded_python_files(): + if path.name not in _RETURNED_EXCEPTION_FILES: + continue + text = path.read_text(encoding="utf-8") + for line_no, reason in _iter_returned_exception_leaks(text): + rel = path.relative_to(_REPO_ROOT) + offenders.append(f"{rel}:{line_no}: {reason}") + + assert not offenders, ( + "A service/adapter must not return the caught exception as a value that " + "reaches a client (CWE-209). Return a static message and log the " + "exception server-side. Offending sites:\n " + "\n ".join(offenders) + ) + + +def test_returned_exception_guard_flags_and_allows() -> None: + """Controls for the returned-exception scanner.""" + for leak in ( + # The real validate_video_url leak shape. + "try:\n pass\nexcept Exception as e:\n" + ' return False, "", f"Video validation failed: {e}"\n', + # A nonstandard handler name is still tracked. + "try:\n pass\nexcept ValueError as boom:\n" + ' return False, "", f"Invalid URL: {boom}"\n', + # An intermediate alias must not launder the taint. + "try:\n pass\nexcept Exception as failure:\n" + " msg = str(failure)\n" + ' return False, "", msg\n', + "try:\n pass\nexcept Exception as exc:\n return str(exc)\n", + ): + assert list( + _iter_returned_exception_leaks(leak) + ), f"scanner missed a returned-exception leak: {leak}" + + for safe in ( + # Static message inside a handler is fine (no binding referenced). + "try:\n pass\nexcept Exception as e:\n" + ' return False, "", "Video validation failed"\n', + # Logging the exception and raising a static 500 is fine — not a return. + "try:\n pass\nexcept Exception as e:\n" + " logger.error('failed', exc_info=True)\n" + ' raise HTTPException(500, "Internal server error")\n', + # A static alias is not tainted. + "try:\n pass\nexcept Exception as failure:\n" + ' msg = "Video validation failed"\n' + ' return False, "", msg\n', + ): + assert not list( + _iter_returned_exception_leaks(safe) + ), f"scanner false-positived: {safe}" + + +def test_no_exception_in_response_error_fields() -> None: + offenders: list[str] = [] + for path in _guarded_python_files(): + if path.name not in _GUARDED_RESPONSE_FILES: + continue + text = path.read_text(encoding="utf-8") + for line_no, reason in _iter_response_error_leaks(text): + rel = path.relative_to(_REPO_ROOT) + offenders.append(f"{rel}:{line_no}: {reason}") + + assert not offenders, ( + 'A response body must not place the caught exception under an "error" key ' + "(CWE-209), including on non-500 'degraded'/'failed' payloads. Return a " + "static status string and log the exception server-side. Offending " + "sites:\n " + "\n ".join(offenders) + ) + + +def test_response_body_guard_flags_and_allows() -> None: + """Controls for the response-body scanner.""" + for leak in ( + 'x = {"error": str(e)}', + 'x = {"status": "error", "error": str(exc)}', + 'x = {"error": f"failed: {e}"}', + 'x = {"error_message": str(e)}', + # A nonstandard exception name must not bypass the guard: the identifier + # is derived from the enclosing `except ... as ` handler. + "try:\n pass\nexcept Exception as failure:\n" + ' return {"error": str(failure)}\n', + "try:\n pass\nexcept Exception as boom:\n return {" + '"status": "error", "error": boom}\n', + # An intermediate alias must not launder the taint past the guard. + "try:\n pass\nexcept Exception as failure:\n" + " message = str(failure)\n" + ' return {"error": message}\n', + "try:\n pass\nexcept Exception as failure:\n" + ' detail = f"boom: {failure}"\n' + ' return {"error": detail}\n', + 'response = VideoAnalysisResponse(error=result.get("error"))', + "response = VideoAnalysisResponse(error_message=state.error_message)", + # A whole-tree walker is not a scalar sanitizer: it returns non-container + # inputs unchanged, so a scalar ``error`` wrapped in it still leaks. + "response = VideoAnalysisResponse(" + 'error=_sanitize_response_errors(result["error"]))', + # Plural ``errors`` sink: a scalar exception string in the list leaks + # just like a scalar ``error`` field. + 'x = {"errors": [str(e)]}', + "try:\n pass\nexcept Exception as failure:\n" + ' return {"errors": [str(failure)]}\n', + "async def endpoint():\n" + " result = await processor.batch_process_videos([])\n" + " return result\n", + "async def endpoint():\n" + " result: dict[str, object] = await " + "processor.batch_process_videos([])\n" + " return result\n", + ): + assert list(_iter_response_error_leaks(leak)), f"scanner missed a leak: {leak}" + + for safe in ( + 'x = {"error": "Internal server error"}', + 'x = {"status": "error", "error": "Service unavailable"}', + 'x = {"error": "failed", "timestamp": datetime.now().isoformat()}', + 'x = {"error_message": "Internal server error"}', + # Static body inside a nonstandard-named handler is fine. + "try:\n pass\nexcept Exception as failure:\n" + ' return {"error": "Internal server error"}\n', + # A static alias (not derived from the exception) is not tainted. + "try:\n pass\nexcept Exception as failure:\n" + ' message = "Internal server error"\n' + ' return {"error": message}\n', + "response = VideoAnalysisResponse(error=_sanitize_public_error(" + 'result.get("error")))', + # The element-wise list sanitizer is the correct scrubber for the plural + # ``errors`` keyword sink and remains sufficient there. + "response = BatchResponse(errors=_sanitize_error_list(" + 'result.get("errors")))', + # A static or sanitized plural ``errors`` collection is fine. + 'x = {"errors": ["Internal server error"]}', + 'x = {"errors": _sanitize_error_list(result.get("errors"))}', + "async def endpoint():\n" + " result = await processor.batch_process_videos([])\n" + " return _sanitize_response_errors(result)\n", + "async def endpoint():\n" + " result: dict[str, object] = await " + "processor.batch_process_videos([])\n" + " return _sanitize_response_errors(result)\n", + ): + assert not list( + _iter_response_error_leaks(safe) + ), f"scanner false-positived: {safe}" + + if __name__ == "__main__": # pragma: no cover raise SystemExit(pytest.main([__file__, "-v"])) diff --git a/tests/unit/test_cloud_routes.py b/tests/unit/test_cloud_routes.py index 2bcf29ac8..d69449322 100644 --- a/tests/unit/test_cloud_routes.py +++ b/tests/unit/test_cloud_routes.py @@ -360,8 +360,6 @@ def test_analyze_video_cloud_ai_error(self): assert response.status_code == 503 def test_analyze_video_rate_limit_error(self): - # RateLimitError is a subclass of CloudAIError, so it's caught by the - # CloudAIError except clause first and returns 503 (not 429). mock_ai = AsyncMock() mock_ai.__aenter__ = AsyncMock(return_value=mock_ai) mock_ai.__aexit__ = AsyncMock(return_value=False) @@ -373,7 +371,9 @@ def test_analyze_video_rate_limit_error(self): "video_url": "https://www.youtube.com/watch?v=auJzb1D-fag", "analysis_types": ["label_detection"], }) - assert response.status_code in (429, 503) + assert response.status_code == 429 + assert response.json()["detail"] == "Rate limit exceeded" + assert "too many requests" not in response.json()["detail"] def test_analyze_video_configuration_error(self): # ConfigurationError is a subclass of CloudAIError, so it's caught by @@ -389,7 +389,9 @@ def test_analyze_video_configuration_error(self): "video_url": "https://www.youtube.com/watch?v=auJzb1D-fag", "analysis_types": ["label_detection"], }) - assert response.status_code in (500, 503) + assert response.status_code == 500 + assert response.json()["detail"] == "Internal server error" + assert "missing key" not in response.json()["detail"] def test_analyze_video_generic_error(self): mock_ai = AsyncMock() @@ -558,7 +560,7 @@ def test_process_video_sync(self): def test_process_video_sync_failed(self): state = self._make_state(status="failed") state.success = False - state.error_message = "something went wrong" + state.error_message = "db_password=legacy-secret" mock_processor = AsyncMock() mock_processor._extract_video_id = MagicMock(return_value="auJzb1D-fag") @@ -574,7 +576,55 @@ def test_process_video_sync_failed(self): assert response.status_code == 200 data = response.json() assert data["status"] == "failed" - assert data["error"] == "something went wrong" + assert data["error"] == "Internal server error" + assert "legacy-secret" not in data["error"] + + def test_process_video_sync_sanitizes_complete_result_tree(self): + state = self._make_state(status="failed") + state.success = False + state.metadata = { + "error": "metadata-secret", + "nested": {"error_message": "nested-secret"}, + } + state.transcript = {"errors": ["transcript-secret"]} + state.ai_analysis = {"provider": {"error": "provider-secret"}} + + mock_processor = AsyncMock() + mock_processor._extract_video_id = MagicMock(return_value="auJzb1D-fag") + mock_processor.process_video_sync = AsyncMock(return_value=state) + + with patch( + "youtube_extension.backend.cloud_api_endpoints.get_cloud_video_processor", + return_value=mock_processor, + ): + client = self._build_app() + response = client.post( + "/api/v3/process-video", + json={ + "video_url": "https://www.youtube.com/watch?v=auJzb1D-fag", + "async_processing": False, + }, + ) + + assert response.status_code == 200 + body = response.json() + assert body["metadata"]["error"] == "Internal server error" + assert body["metadata"]["nested"]["error_message"] == ( + "Internal server error" + ) + assert body["transcript"]["errors"] == ["Internal server error"] + assert body["ai_analysis"]["provider"]["error"] == ( + "Internal server error" + ) + assert not any( + secret in response.text + for secret in ( + "metadata-secret", + "nested-secret", + "transcript-secret", + "provider-secret", + ) + ) def test_process_video_exception(self): mock_processor = AsyncMock() @@ -699,6 +749,21 @@ def test_get_video_status_found(self): assert data["video_id"] == "auJzb1D-fag" assert data["status"] == "completed" + def test_get_video_status_sanitizes_legacy_persisted_error(self): + state = self._make_state(status="failed") + state.error_message = "redis://user:password@internal-host" + mock_processor = AsyncMock() + mock_processor.get_processing_status = AsyncMock(return_value=state) + + with patch("youtube_extension.backend.cloud_api_endpoints.get_cloud_video_processor", + return_value=mock_processor): + client = self._build_app() + response = client.get("/api/v3/videos/auJzb1D-fag/status") + + assert response.status_code == 200 + assert response.json()["error_message"] == "Internal server error" + assert "internal-host" not in response.json()["error_message"] + def test_get_video_status_not_found(self): mock_processor = AsyncMock() mock_processor.get_processing_status = AsyncMock(return_value=None) @@ -735,6 +800,52 @@ def test_get_video_result_found(self): assert data["video_id"] == "auJzb1D-fag" assert data["status"] == "completed" + def test_get_video_result_sanitizes_legacy_persisted_error(self): + state = self._make_state(status="failed") + state.error_message = "Traceback: connection to 10.0.0.5 refused" + mock_processor = AsyncMock() + mock_processor.get_processing_status = AsyncMock(return_value=state) + + with patch("youtube_extension.backend.cloud_api_endpoints.get_cloud_video_processor", + return_value=mock_processor): + client = self._build_app() + response = client.get("/api/v3/videos/auJzb1D-fag/result") + + assert response.status_code == 200 + assert response.json()["error_message"] == "Internal server error" + assert "10.0.0.5" not in response.json()["error_message"] + + def test_get_video_result_sanitizes_complete_persisted_tree(self): + state = self._make_state(status="failed") + state.metadata = {"error": "metadata-secret"} + state.transcript = {"nested": {"error_message": "transcript-secret"}} + state.ai_analysis = {"errors": ["provider-secret"]} + mock_processor = AsyncMock() + mock_processor.get_processing_status = AsyncMock(return_value=state) + + with patch( + "youtube_extension.backend.cloud_api_endpoints.get_cloud_video_processor", + return_value=mock_processor, + ): + client = self._build_app() + response = client.get("/api/v3/videos/auJzb1D-fag/result") + + assert response.status_code == 200 + body = response.json() + assert body["metadata"]["error"] == "Internal server error" + assert body["transcript"]["nested"]["error_message"] == ( + "Internal server error" + ) + assert body["ai_analysis"]["errors"] == ["Internal server error"] + assert not any( + secret in response.text + for secret in ( + "metadata-secret", + "transcript-secret", + "provider-secret", + ) + ) + def test_get_video_result_not_found(self): mock_processor = AsyncMock() mock_processor.get_processing_status = AsyncMock(return_value=None) diff --git a/tests/unit/test_code_generator_agent.py b/tests/unit/test_code_generator_agent.py index 534fdc3c1..8fe8050ff 100644 --- a/tests/unit/test_code_generator_agent.py +++ b/tests/unit/test_code_generator_agent.py @@ -112,3 +112,20 @@ def test_execute_still_returns_success_flag(self) -> None: agent = CodeGeneratorAgent() result = asyncio.run(agent.execute({"intent": "api endpoint", "context": {}})) assert result["success"] is True + + def test_api_endpoint_hides_internal_exception_details(self) -> None: + agent = CodeGeneratorAgent() + result = asyncio.run(agent.execute({"intent": "api endpoint", "context": {}})) + code = result["generated_code"] + + assert 'detail="Internal server error"' in code + assert "status_code=500, detail=str(" not in code + assert "status_code=500,\n detail=str(" not in code + + def test_api_endpoint_logs_internal_exception_server_side(self) -> None: + agent = CodeGeneratorAgent() + result = asyncio.run(agent.execute({"intent": "api endpoint", "context": {}})) + code = result["generated_code"] + + assert "logger = logging.getLogger(__name__)" in code + assert 'logger.exception("Generated endpoint failed")' in code diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index c320bde37..be0ba8202 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -358,6 +358,161 @@ def test_error_response_does_not_leak_internal_state(self, client, mock_processo assert "auJzb1D-fag" not in str(detail) assert "crash" not in str(detail) + def test_processor_failure_record_is_sanitized(self, client, mock_processor): + mock_processor.process_video = AsyncMock( + return_value={ + "video_id": "auJzb1D-fag", + "success": False, + "cost_breakdown": {"total_cost": 0.0}, + "cached": False, + "error": "database password=super-secret", + } + ) + response = client.post( + "/api/v2/process-video", + json={"video_url": "https://youtube.com/watch?v=auJzb1D-fag"}, + ) + assert response.status_code == 200 + assert response.json()["error"] == "Video processing failed" + assert "super-secret" not in response.text + + def test_nested_ai_analysis_error_is_sanitized(self, client, mock_processor): + """The ``ai_analysis`` sub-tree is a client sink too: ``real_video_processor`` + stores ``ai_analysis['error'] = f'AI analysis failed: {e}'`` on failure, and + this 200 endpoint copies ``ai_analysis`` straight into the response. Its + nested ``error`` must be scrubbed like the top-level one.""" + mock_processor.process_video = AsyncMock( + return_value={ + "video_id": "auJzb1D-fag", + "success": True, + "cost_breakdown": {"total_cost": 0.0}, + "cached": False, + "error": None, + "ai_analysis": { + "success": False, + "error": "AI analysis failed: RuntimeError('provider-token-secret')", + }, + } + ) + response = client.post( + "/api/v2/process-video", + json={"video_url": "https://youtube.com/watch?v=auJzb1D-fag"}, + ) + assert response.status_code == 200 + assert response.json()["ai_analysis"]["error"] == "Video processing failed" + assert "provider-token-secret" not in response.text + + def test_ai_analysis_errors_list_scalars_are_sanitized(self, client, mock_processor): + """The plural ``errors`` collection carries scalar strings that embed + exception text (``real_ai_processor`` appends ``f'{step}: {str(result)}'``). + A plain recursion leaves those strings intact, so they must be replaced + while structured records stay recursible.""" + mock_processor.process_video = AsyncMock( + return_value={ + "video_id": "auJzb1D-fag", + "success": False, + "cost_breakdown": {"total_cost": 0.0}, + "cached": False, + "error": None, + "ai_analysis": { + "success": False, + "errors": [ + "content_analysis: KeyError('leaked-internal-key')", + "summary: Processing failed", + ], + }, + } + ) + response = client.post( + "/api/v2/process-video", + json={"video_url": "https://youtube.com/watch?v=auJzb1D-fag"}, + ) + assert response.status_code == 200 + assert response.json()["ai_analysis"]["errors"] == [ + "Video processing failed", + "Video processing failed", + ] + assert "leaked-internal-key" not in response.text + + def test_ai_analysis_errors_list_non_string_leaves_are_sanitized( + self, client, mock_processor + ): + """Non-string scalar leaves under ``errors`` are still diagnostics. + + FastAPI serializes non-string leaves (ints, bools, bytes), so a legacy + or provider value that is not a ``str`` must not bypass the scalar + sanitization invariant. Only ``None`` (absence of an error) is + preserved; every other non-null leaf is replaced.""" + mock_processor.process_video = AsyncMock( + return_value={ + "video_id": "auJzb1D-fag", + "success": False, + "cost_breakdown": {"total_cost": 0.0}, + "cached": False, + "error": None, + "ai_analysis": { + "success": False, + "errors": [ + 13, + True, + None, + ], + }, + } + ) + response = client.post( + "/api/v2/process-video", + json={"video_url": "https://youtube.com/watch?v=auJzb1D-fag"}, + ) + assert response.status_code == 200 + assert response.json()["ai_analysis"]["errors"] == [ + "Video processing failed", + "Video processing failed", + None, + ] + + def test_complete_processor_result_is_sanitized(self, client, mock_processor): + """Every response-model subtree is a client boundary, not just AI output.""" + mock_processor.process_video = AsyncMock( + return_value={ + "video_id": "auJzb1D-fag", + "success": False, + "metadata": { + "error": "metadata-secret", + "nested": {"error_message": "nested-secret"}, + }, + "transcript": {"errors": ["transcript-secret"]}, + "cost_breakdown": {"provider": {"error": "billing-secret"}}, + "cached": True, + "error": None, + } + ) + + response = client.post( + "/api/v2/process-video", + json={"video_url": "https://youtube.com/watch?v=auJzb1D-fag"}, + ) + + assert response.status_code == 200 + body = response.json() + assert body["metadata"]["error"] == "Video processing failed" + assert body["metadata"]["nested"]["error_message"] == ( + "Video processing failed" + ) + assert body["transcript"]["errors"] == ["Video processing failed"] + assert body["cost_breakdown"]["provider"]["error"] == ( + "Video processing failed" + ) + assert not any( + secret in response.text + for secret in ( + "metadata-secret", + "nested-secret", + "transcript-secret", + "billing-secret", + ) + ) + def test_missing_video_url_returns_422(self, client): response = client.post("/api/v2/process-video", json={}) assert response.status_code == 422 @@ -468,6 +623,43 @@ def test_batch_processor_error_returns_500(self, client, mock_processor): ) assert response.status_code == 500 + def test_batch_failure_records_are_sanitized_recursively( + self, client, mock_processor + ): + mock_processor.batch_process_videos = AsyncMock( + return_value={ + "results": [ + { + "success": True, + "ai_analysis": {"error": "provider-token-secret"}, + } + ], + "errors": [ + { + "success": False, + "error": "socket refused at 10.0.0.4", + "processing_steps": [ + {"error": "postgres://user:password@db"} + ], + } + ], + } + ) + response = client.post( + "/api/v2/batch-process", + json={"video_urls": ["https://youtube.com/watch?v=auJzb1D-fag"]}, + ) + assert response.status_code == 200 + data = response.json() + assert data["results"][0]["ai_analysis"]["error"] == "Video processing failed" + assert data["errors"][0]["error"] == "Video processing failed" + assert ( + data["errors"][0]["processing_steps"][0]["error"] + == "Video processing failed" + ) + assert "password" not in response.text + assert "10.0.0.4" not in response.text + def test_batch_max_concurrent_bounds_enforced_by_model(self, client): """max_concurrent=0 fails Pydantic validation (ge=1).""" response = client.post( @@ -560,6 +752,26 @@ def test_existing_video_returns_correct_data(self, client, mock_processor, tmp_c data = response.json() assert data["video_id"] == "auJzb1D-fag" + def test_legacy_cached_error_fields_are_sanitized( + self, client, mock_processor, tmp_cache + ): + tmp_cache.mkdir(parents=True, exist_ok=True) + cache_file = _write_cache_file( + tmp_cache, + "auJzb1D-fag", + { + "error": "legacy stack /srv/app.py:42", + "ai_analysis": {"success": False, "error": "api-key-secret"}, + }, + ) + mock_processor._get_cache_path.return_value = cache_file + response = client.get("/api/v2/videos/auJzb1D-fag") + assert response.status_code == 200 + assert response.json()["error"] == "Video processing failed" + assert response.json()["ai_analysis"]["error"] == "Video processing failed" + assert "app.py" not in response.text + assert "api-key-secret" not in response.text + def test_missing_video_returns_404(self, client, mock_processor, tmp_cache): mock_processor._get_cache_path.return_value = tmp_cache / "nonexistent_processed.json" response = client.get("/api/v2/videos/nonexistent")