From d14e7de009dbf2d52f998259b6558ec307dd3b72 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Jul 2026 08:11:27 +0000 Subject: [PATCH 01/13] fix(security): sanitize exception text in non-500 response bodies (CWE-209) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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": } 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) Claude-Session: https://claude.ai/code/session_01He4GxmJcWW8XdwxYQQh8Sa --- .../backend/cloud_api_endpoints.py | 27 +++-- .../backend/real_api_endpoints.py | 24 ++-- tests/unit/test_500_info_disclosure.py | 112 ++++++++++++++++++ 3 files changed, 139 insertions(+), 24 deletions(-) diff --git a/src/youtube_extension/backend/cloud_api_endpoints.py b/src/youtube_extension/backend/cloud_api_endpoints.py index cdf2844e2..c40ab64fd 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -345,11 +345,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 +372,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 +389,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 +404,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..3ea322575 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -265,10 +265,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 +289,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 +305,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() } @@ -354,11 +354,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/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index 5c0a40f4e..417ca0ef8 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -201,5 +201,117 @@ def test_guard_allows_sanitized_and_safe_dynamic_bodies() -> None: 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"} + + +def _name_referenced(node: ast.AST, name: str) -> bool: + """True if *node*'s subtree reads the identifier *name*.""" + return any( + isinstance(leaf, ast.Name) and leaf.id == name for leaf in ast.walk(node) + ) + + +def _iter_response_error_leaks(text: str): + """Yield (line_no, reason) for dict bodies that put the exception under "error". + + 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_dict_values(scope: ast.AST): + for node in ast.walk(scope): + if not isinstance(node, ast.Dict): + continue + for key, value in zip(node.keys, node.values): + if isinstance(key, ast.Constant) and key.value == "error": + yield node, value + + # Pass 1: values that reference the exception/request by convention. + for node, value in _error_dict_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``. + for handler in ast.walk(tree): + if not isinstance(handler, ast.ExceptHandler) or not handler.name: + continue + for node, value in _error_dict_values(handler): + line = getattr(value, "lineno", node.lineno) + if _name_referenced(value, handler.name) and line not in seen: + seen.add(line) + yield line, reason + + +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}"}', + # 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', + ): + 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()}', + # Static body inside a nonstandard-named handler is fine. + "try:\n pass\nexcept Exception as failure:\n" + ' return {"error": "Internal server error"}\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"])) From edeb0cf903dfec4b71a3835939a1571d7df10c1c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Jul 2026 08:17:24 +0000 Subject: [PATCH 02/13] test(security): track exception aliases in response-body guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01He4GxmJcWW8XdwxYQQh8Sa --- tests/unit/test_500_info_disclosure.py | 67 +++++++++++++++++++++++--- 1 file changed, 61 insertions(+), 6 deletions(-) diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index 417ca0ef8..b1b2450ad 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -218,13 +218,55 @@ def test_guard_allows_sanitized_and_safe_dynamic_bodies() -> None: _GUARDED_RESPONSE_FILES = {"cloud_api_endpoints.py", "real_api_endpoints.py"} -def _name_referenced(node: ast.AST, name: str) -> bool: - """True if *node*'s subtree reads the identifier *name*.""" +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 == name for leaf in ast.walk(node) + 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 dict bodies that put the exception under "error". @@ -255,14 +297,16 @@ def _error_dict_values(scope: ast.AST): 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``. + # 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_dict_values(handler): line = getattr(value, "lineno", node.lineno) - if _name_referenced(value, handler.name) and line not in seen: + if _refs_any_name(value, tainted) and line not in seen: seen.add(line) yield line, reason @@ -297,6 +341,13 @@ def test_response_body_guard_flags_and_allows() -> None: ' 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', ): assert list(_iter_response_error_leaks(leak)), f"scanner missed a leak: {leak}" @@ -307,6 +358,10 @@ def test_response_body_guard_flags_and_allows() -> None: # 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', ): assert not list( _iter_response_error_leaks(safe) From cbe08d423c46d0d6dbf59e07884ac91bb33108de Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Tue, 21 Jul 2026 03:25:38 -0500 Subject: [PATCH 03/13] fix(security): close remaining CWE-209 read-boundary gaps 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. --- .../backend/cloud_ai_routes.py | 2 +- .../backend/cloud_api_endpoints.py | 18 +++++--- tests/unit/test_500_info_disclosure.py | 13 ++++-- tests/unit/test_cloud_routes.py | 45 ++++++++++++++++--- 4 files changed, 62 insertions(+), 16 deletions(-) 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 c40ab64fd..db100b594 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -35,6 +35,14 @@ router = APIRouter() +def _client_safe_error(error_message: Optional[str]) -> 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 + # Pydantic models for API requests/responses class CloudVideoProcessingRequest(BaseModel): @@ -138,7 +146,7 @@ async def process_video_cloud( ai_analysis=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 +226,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 +288,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: @@ -318,7 +326,7 @@ async def get_video_result(video_id: str): "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: diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index b1b2450ad..8324e33dd 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -83,16 +83,18 @@ def _refs_exception_or_request(node: ast.AST) -> bool: return False -def _status_is_500(call: ast.Call, name: str) -> bool: +def _status_is_server_error(call: ast.Call, name: str) -> bool: for kw in call.keywords: if kw.arg == "status_code" and isinstance(kw.value, ast.Constant): - return kw.value.value == 500 + status = kw.value.value + return isinstance(status, int) 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 and isinstance(call.args[idx], ast.Constant): - return call.args[idx].value == 500 + status = call.args[idx].value + return isinstance(status, int) and 500 <= status <= 599 return False @@ -110,7 +112,7 @@ def _iter_500_leaks(text: str): name = _call_name(node) if name not in ("HTTPException", "JSONResponse"): continue - if not _status_is_500(node, name): + if not _status_is_server_error(node, name): continue # Check keyword arguments for kw in node.keywords: @@ -174,10 +176,13 @@ def test_guard_detects_every_known_leak_shape() -> None: 'raise HTTPException(500, str(e))', 'raise HTTPException(500, f"internal: {exc}")', 'raise HTTPException(500, error_msg)', + 'raise HTTPException(status_code=503, 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)', ] for sample in leaky_samples: assert list(_iter_500_leaks(sample)), f"scanner missed a real leak: {sample}" diff --git a/tests/unit/test_cloud_routes.py b/tests/unit/test_cloud_routes.py index 2bcf29ac8..1c4d6d3be 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,8 @@ 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_exception(self): mock_processor = AsyncMock() @@ -699,6 +702,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 +753,21 @@ 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_not_found(self): mock_processor = AsyncMock() mock_processor.get_processing_status = AsyncMock(return_value=None) From 78526eabaca0ed9186c8b95e055d2bb046a6e966 Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Tue, 21 Jul 2026 03:50:42 -0500 Subject: [PATCH 04/13] fix(security): close remaining CWE-209 pass-through bypasses --- .../backend/real_api_endpoints.py | 43 ++++++- tests/unit/test_500_info_disclosure.py | 114 ++++++++++++++++-- tests/unit/test_real_api_endpoints.py | 75 ++++++++++++ 3 files changed, 216 insertions(+), 16 deletions(-) diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 3ea322575..cca64c5f1 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -26,6 +26,37 @@ # 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_response_errors(value: Any) -> Any: + """Copy a response tree while replacing every non-null ``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. + """ + if isinstance(value, dict): + return { + key: ( + _sanitize_public_error(item) + if key == "error" + else _sanitize_response_errors(item) + ) + for key, item in value.items() + } + 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") @@ -110,7 +141,7 @@ async def process_video_real_api(request: VideoProcessingRequest, background_tas cost_breakdown=result.get('cost_breakdown'), processing_time=processing_time, cached=result.get('cached', False), - error=result.get('error') + error=_sanitize_public_error(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 +199,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 +239,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 +278,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 @@ -336,7 +369,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), diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index 8324e33dd..a37e32d77 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -106,6 +106,26 @@ 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) + + # 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 @@ -120,7 +140,7 @@ 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. @@ -131,7 +151,7 @@ def _iter_500_leaks(text: str): # 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_exception_or_request(node.args[0]): + if _refs_server_error_state(node, node.args[0]): yield node.lineno, "JSONResponse 500 body references the exception/request" @@ -183,6 +203,11 @@ def test_guard_detects_every_known_leak_shape() -> None: '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', ] for sample in leaky_samples: assert list(_iter_500_leaks(sample)), f"scanner missed a real leak: {sample}" @@ -273,7 +298,7 @@ def _tainted_names(handler: ast.ExceptHandler) -> set[str]: def _iter_response_error_leaks(text: str): - """Yield (line_no, reason) for dict bodies that put the exception under "error". + """Yield (line_no, reason) for responses that can expose an ``error`` value. A value discloses the caught exception two ways, both flagged: @@ -287,16 +312,26 @@ def _iter_response_error_leaks(text: str): seen: set[int] = set() reason = 'response body "error" field references the caught exception' - def _error_dict_values(scope: ast.AST): + def _error_values(scope: ast.AST): for node in ast.walk(scope): - if not isinstance(node, ast.Dict): - continue - for key, value in zip(node.keys, node.values): - if isinstance(key, ast.Constant) and key.value == "error": - yield node, value + if isinstance(node, ast.Dict): + for key, value in zip(node.keys, node.values): + if isinstance(key, ast.Constant) and key.value == "error": + yield node, value + elif isinstance(node, ast.Call): + for keyword in node.keywords: + if keyword.arg == "error": + yield node, keyword.value + + 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", + } # Pass 1: values that reference the exception/request by convention. - for node, value in _error_dict_values(tree): + 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) @@ -309,12 +344,60 @@ def _error_dict_values(scope: ast.AST): if not isinstance(handler, ast.ExceptHandler) or not handler.name: continue tainted = _tainted_names(handler) - for node, value in _error_dict_values(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 != "error": + 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_public_error_sanitizer(value) + ): + 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): + if not isinstance(node, ast.Assign): + continue + value = node.value.value if isinstance(node.value, ast.Await) else node.value + if not isinstance(value, ast.Call) or _call_name(value) not in { + "process_video", + "batch_process_videos", + }: + continue + for target in node.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" + def test_no_exception_in_response_error_fields() -> None: offenders: list[str] = [] @@ -353,6 +436,10 @@ def test_response_body_guard_flags_and_allows() -> None: "try:\n pass\nexcept Exception as failure:\n" ' detail = f"boom: {failure}"\n' ' return {"error": detail}\n', + 'response = VideoAnalysisResponse(error=result.get("error"))', + "async def endpoint():\n" + " result = await processor.batch_process_videos([])\n" + " return result\n", ): assert list(_iter_response_error_leaks(leak)), f"scanner missed a leak: {leak}" @@ -367,6 +454,11 @@ def test_response_body_guard_flags_and_allows() -> None: "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")))', + "async def endpoint():\n" + " result = await processor.batch_process_videos([])\n" + " return _sanitize_response_errors(result)\n", ): assert not list( _iter_response_error_leaks(safe) diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index c320bde37..78c352526 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -358,6 +358,24 @@ 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_missing_video_url_returns_422(self, client): response = client.post("/api/v2/process-video", json={}) assert response.status_code == 422 @@ -468,6 +486,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 +615,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") From 8e3260da843b2eccb6b5ba7461f549107317120d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Jul 2026 09:04:04 +0000 Subject: [PATCH 05/13] fix(security): sanitize plural errors list and nested ai_analysis (CWE-209) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01GwvL8n17iZJqaj83ARzWrQ --- .../backend/real_api_endpoints.py | 43 ++++++++++---- tests/unit/test_real_api_endpoints.py | 58 +++++++++++++++++++ 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index cca64c5f1..cc5a64768 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -34,23 +34,46 @@ def _sanitize_public_error(value: Any) -> Optional[str]: 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) + if isinstance(value, str): + return _PUBLIC_PROCESSING_ERROR + return value + + def _sanitize_response_errors(value: Any) -> Any: """Copy a response tree while replacing every non-null ``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. + 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): - return { - key: ( - _sanitize_public_error(item) - if key == "error" - else _sanitize_response_errors(item) - ) - for key, item in value.items() - } + sanitized: dict[Any, Any] = {} + for key, item in value.items(): + if key == "error": + 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): @@ -137,7 +160,7 @@ async def process_video_real_api(request: VideoProcessingRequest, background_tas success=result.get('success', False), metadata=result.get('metadata'), transcript=result.get('transcript'), - ai_analysis=result.get('ai_analysis'), + ai_analysis=_sanitize_response_errors(result.get('ai_analysis')), cost_breakdown=result.get('cost_breakdown'), processing_time=processing_time, cached=result.get('cached', False), diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index 78c352526..49d64880c 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -376,6 +376,64 @@ def test_processor_failure_record_is_sanitized(self, client, mock_processor): 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_missing_video_url_returns_422(self, client): response = client.post("/api/v2/process-video", json={}) assert response.status_code == 422 From bb83fcd0e4dc447593b30cb63eff6d07e705c291 Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Tue, 21 Jul 2026 04:06:31 -0500 Subject: [PATCH 06/13] test(security): cover annotated processor pass-throughs --- tests/unit/test_500_info_disclosure.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index a37e32d77..c52527d2f 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -378,15 +378,23 @@ def _uses_public_error_sanitizer(node: ast.AST) -> bool: # that are allowed to carry diagnostic ``error`` records. processor_results: set[str] = set() for node in ast.walk(tree): - if not isinstance(node, ast.Assign): + 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 - value = node.value.value if isinstance(node.value, ast.Await) else node.value + 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 node.targets: + for target in targets: processor_results.update(_assigned_names(target)) for node in ast.walk(tree): if ( @@ -440,6 +448,10 @@ def test_response_body_guard_flags_and_allows() -> None: "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}" @@ -459,6 +471,10 @@ def test_response_body_guard_flags_and_allows() -> None: "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) From b4597bdedc5ef3b7192f12127efb8ca97b90a312 Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Tue, 21 Jul 2026 04:18:14 -0500 Subject: [PATCH 07/13] fix(security): close remaining response leak guards --- .../backend/real_api_endpoints.py | 21 ++--- tests/unit/test_500_info_disclosure.py | 88 ++++++++++++++++--- tests/unit/test_real_api_endpoints.py | 42 +++++++++ 3 files changed, 130 insertions(+), 21 deletions(-) diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index cc5a64768..6e707a364 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -55,7 +55,7 @@ def _sanitize_error_list(value: Any) -> Any: def _sanitize_response_errors(value: Any) -> Any: - """Copy a response tree while replacing every non-null ``error`` value. + """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 @@ -67,7 +67,7 @@ def _sanitize_response_errors(value: Any) -> Any: if isinstance(value, dict): sanitized: dict[Any, Any] = {} for key, item in value.items(): - if key == "error": + if key in {"error", "error_message"}: sanitized[key] = _sanitize_public_error(item) elif key == "errors": sanitized[key] = _sanitize_error_list(item) @@ -149,22 +149,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=_sanitize_response_errors(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=_sanitize_public_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}") diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index c52527d2f..b2df460e6 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -36,6 +36,7 @@ from __future__ import annotations import ast +from http import HTTPStatus from pathlib import Path import pytest @@ -83,18 +84,70 @@ def _refs_exception_or_request(node: ast.AST) -> bool: return False -def _status_is_server_error(call: ast.Call, name: str) -> 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(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) + 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): - status = kw.value.value - return isinstance(status, int) and 500 <= status <= 599 + 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 and isinstance(call.args[idx], ast.Constant): - status = call.args[idx].value - return isinstance(status, int) and 500 <= status <= 599 + if len(call.args) > idx: + status = _status_code_value(call.args[idx], symbols) + return status is not None and 500 <= status <= 599 return False @@ -106,6 +159,7 @@ 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) + status_symbols = _status_symbol_table(tree) # Bind every response constructor to the exception aliases visible from # its enclosing handler. The fixed conventional-name set remains useful @@ -132,7 +186,7 @@ def _refs_server_error_state(call: ast.Call, value: ast.AST) -> bool: name = _call_name(node) if name not in ("HTTPException", "JSONResponse"): continue - if not _status_is_server_error(node, name): + if not _status_is_server_error(node, name, status_symbols): continue # Check keyword arguments for kw in node.keywords: @@ -197,6 +251,10 @@ def test_guard_detects_every_known_leak_shape() -> None: 'raise HTTPException(500, f"internal: {exc}")', '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]). @@ -221,6 +279,7 @@ def test_guard_allows_sanitized_and_safe_dynamic_bodies() -> None: '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=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={' '"id": f"FALLBACK_{uuid.uuid4().hex}", ' @@ -246,6 +305,7 @@ def test_guard_allows_sanitized_and_safe_dynamic_bodies() -> None: # 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"} +_RESPONSE_ERROR_FIELDS = {"error", "error_message"} def _refs_any_name(node: ast.AST, names: set[str]) -> bool: @@ -316,11 +376,14 @@ 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 == "error": + 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 == "error": + if keyword.arg in _RESPONSE_ERROR_FIELDS: yield node, keyword.value def _uses_public_error_sanitizer(node: ast.AST) -> bool: @@ -358,7 +421,7 @@ def _uses_public_error_sanitizer(node: ast.AST) -> bool: if not isinstance(node, ast.Call): continue for keyword in node.keywords: - if keyword.arg != "error": + if keyword.arg not in _RESPONSE_ERROR_FIELDS: continue value = keyword.value line = getattr(value, "lineno", node.lineno) @@ -431,6 +494,7 @@ def test_response_body_guard_flags_and_allows() -> None: '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" @@ -445,6 +509,7 @@ def test_response_body_guard_flags_and_allows() -> None: ' detail = f"boom: {failure}"\n' ' return {"error": detail}\n', 'response = VideoAnalysisResponse(error=result.get("error"))', + 'response = VideoAnalysisResponse(error_message=state.error_message)', "async def endpoint():\n" " result = await processor.batch_process_videos([])\n" " return result\n", @@ -459,6 +524,7 @@ def test_response_body_guard_flags_and_allows() -> None: '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', diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index 49d64880c..52e351f72 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -434,6 +434,48 @@ def test_ai_analysis_errors_list_scalars_are_sanitized(self, client, mock_proces ] assert "leaked-internal-key" not in response.text + 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 From bed22ffd3b4b15ee0a73165ff3d3b9a1a9f89aa1 Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Tue, 21 Jul 2026 04:26:34 -0500 Subject: [PATCH 08/13] fix(security): sanitize complete cloud result trees --- .../backend/cloud_api_endpoints.py | 46 +++++++++-- tests/unit/test_cloud_routes.py | 78 +++++++++++++++++++ 2 files changed, 117 insertions(+), 7 deletions(-) diff --git a/src/youtube_extension/backend/cloud_api_endpoints.py b/src/youtube_extension/backend/cloud_api_endpoints.py index db100b594..7b23e0a04 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -35,7 +35,7 @@ router = APIRouter() -def _client_safe_error(error_message: Optional[str]) -> Optional[str]: +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 @@ -44,6 +44,38 @@ def _client_safe_error(error_message: Optional[str]) -> Optional[str]: 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) + if isinstance(value, str): + return "Internal server error" + return value + + +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): video_url: str = Field(..., description="YouTube video URL or ID") @@ -141,9 +173,9 @@ 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=_client_safe_error(result.error_message), @@ -320,9 +352,9 @@ 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, diff --git a/tests/unit/test_cloud_routes.py b/tests/unit/test_cloud_routes.py index 1c4d6d3be..d69449322 100644 --- a/tests/unit/test_cloud_routes.py +++ b/tests/unit/test_cloud_routes.py @@ -579,6 +579,53 @@ def test_process_video_sync_failed(self): 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() mock_processor._extract_video_id = MagicMock(side_effect=Exception("boom")) @@ -768,6 +815,37 @@ def test_get_video_result_sanitizes_legacy_persisted_error(self): 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) From 0b564b21adcf05632d18eed1026bf2abb486d563 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 23:29:57 +0000 Subject: [PATCH 09/13] fix(security): sanitize non-string scalar leaves under errors (CWE-209) 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) Claude-Session: https://claude.ai/code/session_015A4gdsfGkyZdRYQwm4o99e --- .../backend/cloud_api_endpoints.py | 10 +++-- .../backend/real_api_endpoints.py | 10 +++-- tests/unit/test_real_api_endpoints.py | 37 +++++++++++++++++++ 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/src/youtube_extension/backend/cloud_api_endpoints.py b/src/youtube_extension/backend/cloud_api_endpoints.py index 7b23e0a04..1b9431c5e 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -52,9 +52,13 @@ def _sanitize_error_list(value: Any) -> Any: return tuple(_sanitize_error_list(item) for item in value) if isinstance(value, dict): return _sanitize_response_errors(value) - if isinstance(value, str): - return "Internal server error" - return 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: diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 6e707a364..5f86fa013 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -49,9 +49,13 @@ def _sanitize_error_list(value: Any) -> Any: return tuple(_sanitize_error_list(item) for item in value) if isinstance(value, dict): return _sanitize_response_errors(value) - if isinstance(value, str): - return _PUBLIC_PROCESSING_ERROR - return 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: diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index 52e351f72..be0ba8202 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -434,6 +434,43 @@ def test_ai_analysis_errors_list_scalars_are_sanitized(self, client, mock_proces ] 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( From 7f8697a1265aa47d6c76489bc770bbbe14be3efb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 23:34:48 +0000 Subject: [PATCH 10/13] fix(security): sanitize validate-video message and guard returned-exception 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 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) Claude-Session: https://claude.ai/code/session_01He4GxmJcWW8XdwxYQQh8Sa --- .../services/youtube/adapters/official_api.py | 12 +- tests/unit/test_500_info_disclosure.py | 108 +++++++++++++++++- 2 files changed, 115 insertions(+), 5 deletions(-) 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..c020bd34a 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 video 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 b2df460e6..a9e8883b1 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -305,7 +305,11 @@ def test_guard_allows_sanitized_and_safe_dynamic_bodies() -> None: # 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"} -_RESPONSE_ERROR_FIELDS = {"error", "error_message"} +# ``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: @@ -391,6 +395,7 @@ def _uses_public_error_sanitizer(node: ast.AST) -> bool: "_client_safe_error", "_sanitize_public_error", "_sanitize_response_errors", + "_sanitize_error_list", } # Pass 1: values that reference the exception/request by convention. @@ -470,6 +475,99 @@ def _uses_public_error_sanitizer(node: ast.AST) -> bool: 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(): @@ -510,6 +608,11 @@ def test_response_body_guard_flags_and_allows() -> None: ' return {"error": detail}\n', 'response = VideoAnalysisResponse(error=result.get("error"))', 'response = VideoAnalysisResponse(error_message=state.error_message)', + # 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", @@ -534,6 +637,9 @@ def test_response_body_guard_flags_and_allows() -> None: ' return {"error": message}\n', 'response = VideoAnalysisResponse(error=_sanitize_public_error(' 'result.get("error")))', + # 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", From eef5da0475af2b18101d8cb65b81b69322181818 Mon Sep 17 00:00:00 2001 From: Hayden <154503486+groupthinking@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:34:15 -0500 Subject: [PATCH 11/13] test: align sanitized invalid URL response --- .../backend/services/youtube/adapters/official_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c020bd34a..cc8dfa4f1 100644 --- a/src/youtube_extension/backend/services/youtube/adapters/official_api.py +++ b/src/youtube_extension/backend/services/youtube/adapters/official_api.py @@ -582,7 +582,7 @@ async def validate_video_url(self, url: str) -> tuple[bool, str, str]: # 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 video URL format" + return False, "", "Invalid URL format" except Exception: logger.error("Video URL validation failed", exc_info=True) return False, "", "Video validation failed" From 2943c15174c609cf76344e0034a34c0545f10829 Mon Sep 17 00:00:00 2001 From: groupthinking <154503486+groupthinking@users.noreply.github.com> Date: Wed, 29 Jul 2026 13:39:30 +0000 Subject: [PATCH 12/13] docs(triage): PR remediation run 2026-07-29 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) Claude-Session: https://claude.ai/code/session_01CQDL89d1xNEm4hijHZMaSf --- docs/triage/pr-remediation-2026-07-29.md | 111 +++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 docs/triage/pr-remediation-2026-07-29.md 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. From ea4333797ee5b2ea90ad3ee3c763388265beac75 Mon Sep 17 00:00:00 2001 From: groupthinking <154503486+groupthinking@users.noreply.github.com> Date: Sat, 1 Aug 2026 19:59:03 +0000 Subject: [PATCH 13/13] fix(security): close two CWE-209 guard false-negatives 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) Claude-Session: https://claude.ai/code/session_01CQDL89d1xNEm4hijHZMaSf --- tests/unit/test_500_info_disclosure.py | 135 ++++++++++++++++++------- 1 file changed, 97 insertions(+), 38 deletions(-) diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index a9e8883b1..0c95ccec8 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -106,13 +106,22 @@ def _status_code_value(node: ast.AST, symbols: dict[str, int]) -> int | None: return None -def _status_symbol_table(tree: ast.Module) -> dict[str, int]: - """Resolve module constants that alias literal or standard status values.""" - symbols: dict[str, int] = {} +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 tree.body: + for node in body: targets: list[ast.AST] = [] value: ast.AST | None = None if isinstance(node, ast.Assign): @@ -125,18 +134,13 @@ def _status_symbol_table(tree: ast.Module) -> dict[str, int]: if status is None: continue for target in targets: - if ( - isinstance(target, ast.Name) - and symbols.get(target.id) != status - ): + 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: +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": status = _status_code_value(kw.value, symbols) @@ -159,7 +163,35 @@ 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) - status_symbols = _status_symbol_table(tree) + 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 @@ -186,7 +218,7 @@ def _refs_server_error_state(call: ast.Call, value: ast.AST) -> bool: name = _call_name(node) if name not in ("HTTPException", "JSONResponse"): continue - if not _status_is_server_error(node, name, status_symbols): + if not _status_is_server_error(node, name, _symbols_for(node)): continue # Check keyword arguments for kw in node.keywords: @@ -239,22 +271,22 @@ 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(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(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]). @@ -266,6 +298,11 @@ def test_guard_detects_every_known_leak_shape() -> None: "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}" @@ -278,13 +315,18 @@ 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=status.HTTP_400_BAD_REQUEST, 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}" @@ -390,13 +432,24 @@ def _error_values(scope: ast.AST): if keyword.arg in _RESPONSE_ERROR_FIELDS: yield node, keyword.value - 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", - } + # 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): @@ -433,10 +486,8 @@ def _uses_public_error_sanitizer(node: ast.AST) -> bool: 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_public_error_sanitizer(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' @@ -607,7 +658,11 @@ def test_response_body_guard_flags_and_allows() -> None: ' detail = f"boom: {failure}"\n' ' return {"error": detail}\n', 'response = VideoAnalysisResponse(error=result.get("error"))', - 'response = VideoAnalysisResponse(error_message=state.error_message)', + "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)]}', @@ -635,8 +690,12 @@ def test_response_body_guard_flags_and_allows() -> None: "try:\n pass\nexcept Exception as failure:\n" ' message = "Internal server error"\n' ' return {"error": message}\n', - 'response = VideoAnalysisResponse(error=_sanitize_public_error(' + "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"))}',