From 31804b9a08ace16bed801c68e75b963eb228c81c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 01:52:49 +0000 Subject: [PATCH 1/7] fix(security): sanitize all HTTP 500 responses to prevent info disclosure (supersedes #801) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multiple FastAPI handlers returned internal exception text to clients via `HTTPException(status_code=500, detail=str(e))` (or f-strings embedding `{e}`), leaking stack-adjacent messages, backend API errors, and database/Looker errors. This is an information-disclosure vector (CWE-209). #801 sanitized ~24 handlers but left three live endpoints leaking: `generate_video_pack` and `generate_blueprint` (v1/router.py) and the mounted `reporting_routes.py` dashboard endpoint — the exact gaps Copilot flagged on that PR. A tree-wide scan surfaced 13 further leaks in cloud_ai_routes.py, cloud_api_endpoints.py, and real_api_endpoints.py that #801 never touched. Changes: - Replace the dynamic `detail` in every 500 response across the backend with a static "Internal server error"; the full exception is now logged server-side (`logger.error(..., exc_info=True)`) so diagnostics are preserved. - Add reporting_routes.py a module logger (previously none). - Add tests/unit/test_500_info_disclosure.py: a hermetic source-scan guard that fails if any backend 500 response uses a dynamic `detail`, closing the test-coverage gap (existing exception-path tests asserted only status code, so they passed while the body leaked). Includes a self-check that the scanner detects a synthetic leak and ignores 4xx responses. 4xx responses (which echo client-supplied validation input) are intentionally left unchanged. Verified: all touched files compile; ruff findings are identical to the base branch (lint-neutral); guard test passes. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_018MF2xHBKyQBQtpdXt6bVRx --- .../backend/api/reporting_routes.py | 7 +- .../backend/api/v1/router.py | 44 ++++----- .../backend/cloud_ai_routes.py | 10 +- .../backend/cloud_api_endpoints.py | 9 +- src/youtube_extension/backend/main.py | 8 +- .../backend/real_api_endpoints.py | 15 ++- tests/unit/test_500_info_disclosure.py | 94 +++++++++++++++++++ 7 files changed, 147 insertions(+), 40 deletions(-) create mode 100644 tests/unit/test_500_info_disclosure.py diff --git a/src/youtube_extension/backend/api/reporting_routes.py b/src/youtube_extension/backend/api/reporting_routes.py index 041b1be06..dba800a07 100644 --- a/src/youtube_extension/backend/api/reporting_routes.py +++ b/src/youtube_extension/backend/api/reporting_routes.py @@ -1,9 +1,13 @@ +import logging + from fastapi import APIRouter, Depends, HTTPException, Query from pydantic import BaseModel from typing import Optional from src.integration.looker_embedded import LookerEmbeddedService +logger = logging.getLogger(__name__) + router = APIRouter(prefix="/api/v1/reporting", tags=["Reporting & Dashboards"]) class DashboardEmbedRequest(BaseModel): @@ -36,7 +40,8 @@ async def generate_dashboard_url( ) return DashboardEmbedResponse(embed_url=url) except Exception as e: - raise HTTPException(status_code=500, detail=f"Failed to generate dashboard embed URL: {str(e)}") + logger.error(f"Unhandled error in generate_dashboard_url: {e}", exc_info=True) + raise HTTPException(status_code=500, detail="Internal server error") @router.get("/dashboards") async def list_available_dashboards(tenant_id: str = Query(...)): diff --git a/src/youtube_extension/backend/api/v1/router.py b/src/youtube_extension/backend/api/v1/router.py index 7d33ffb63..275eadc63 100644 --- a/src/youtube_extension/backend/api/v1/router.py +++ b/src/youtube_extension/backend/api/v1/router.py @@ -249,7 +249,7 @@ async def health_check_v1( return HealthResponse(**health_status) except Exception as e: logger.error(f"Health check failed: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.get( @@ -278,7 +278,7 @@ async def detailed_health_check_v1( } except Exception as e: logger.error(f"Detailed health check failed: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # Capabilities / Model availability @@ -657,7 +657,7 @@ async def chat_v1( except Exception as e: logger.error(f"Error in chat endpoint: {e}", exc_info=True) - raise HTTPException(status_code=500, detail=str(e)) from e + raise HTTPException(status_code=500, detail="Internal server error") from e # Video Processing Endpoints @@ -716,7 +716,7 @@ async def process_video_v1( {"url": request.video_url, "error": str(e)}, request.video_url, ) - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.post( @@ -762,7 +762,7 @@ async def process_video_markdown_v1( except Exception as e: logger.error(f"Error in markdown processing: {e}") health_service.increment_metric("error_total") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.post( @@ -799,7 +799,7 @@ async def video_to_software_v1( except Exception as e: logger.error(f"Video-to-software processing failed: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # Cache Management Endpoints @@ -823,7 +823,7 @@ async def get_cache_stats_v1(cache_service: CacheService = Depends(get_cache_ser return CacheStats(**stats) except Exception as e: logger.error(f"Error getting cache stats: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.get( @@ -852,7 +852,7 @@ async def get_cached_video_v1( raise except Exception as e: logger.error(f"Error retrieving cached video: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.delete( @@ -875,7 +875,7 @@ async def clear_video_cache_v1( } except Exception as e: logger.error(f"Error clearing video cache: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.delete( @@ -895,7 +895,7 @@ async def clear_all_cache_v1(cache_service: CacheService = Depends(get_cache_ser } except Exception as e: logger.error(f"Error clearing all cache: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # Data Endpoints @@ -933,7 +933,7 @@ async def list_videos_v1( except Exception as e: logger.error(f"Error listing videos: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.get( @@ -957,7 +957,7 @@ async def get_video_detail_v1( raise except Exception as e: logger.error(f"Error getting video detail: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.get( @@ -973,7 +973,7 @@ async def get_learning_log_v1(data_service: DataService = Depends(get_data_servi return learning_log except Exception as e: logger.error(f"Error getting learning log: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.post( @@ -1024,7 +1024,7 @@ async def get_actions_by_video_v1(video_id: str): return actions except Exception as e: logger.error(f"Error retrieving actions for {video_id}: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.put( @@ -1072,7 +1072,7 @@ async def update_action_v1(action_id: str, payload: dict[str, Any]): return {"success": bool(success)} except Exception as e: logger.error(f"Error updating action {action_id}: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # Feedback Endpoints @@ -1119,7 +1119,7 @@ async def submit_feedback_v1( except Exception as e: logger.error(f"Error saving feedback: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # Metrics Endpoints @@ -1139,7 +1139,7 @@ async def get_metrics_v1( return JSONResponse(content="\n".join(metrics_lines), media_type="text/plain") except Exception as e: logger.error(f"Metrics endpoint failed: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # Frontend performance ingestion endpoints @@ -1159,7 +1159,7 @@ async def ingest_performance_alert_v1(payload: dict[str, Any]): return {"status": "ok", "recorded": metric_name} except Exception as e: logger.error(f"Failed to ingest performance alert: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/performance/report", summary="Ingest frontend performance report") @@ -1177,7 +1177,7 @@ async def ingest_performance_report_v1(report: dict[str, Any]): return {"status": "ok", "metrics_recorded": len(metrics)} except Exception as e: logger.error(f"Failed to ingest performance report: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # ============================================================ @@ -1710,7 +1710,7 @@ async def get_or_create_videopack(request: VideoPackRequest): return ApiResponse.success(pack.model_dump()) except Exception as e: logger.error(f"Failed to create VideoPack: {e}") - raise HTTPException(status_code=500, detail=f"VideoPack generation failed: {e}") + raise HTTPException(status_code=500, detail="Internal server error") @router.post( @@ -1759,7 +1759,7 @@ async def generate_blueprint(request: BlueprintRequest): return ApiResponse.success(blueprint) except Exception as e: logger.error(f"Failed to generate blueprint: {e}") - raise HTTPException(status_code=500, detail=f"Blueprint generation failed: {e}") + raise HTTPException(status_code=500, detail="Internal server error") @router.post( @@ -1785,7 +1785,7 @@ async def generate_project_code(request: GenerateCodeRequest): return ApiResponse.success(result) except Exception as e: logger.error(f"Code generation failed: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.get( diff --git a/src/youtube_extension/backend/cloud_ai_routes.py b/src/youtube_extension/backend/cloud_ai_routes.py index a1242a418..0172e1617 100644 --- a/src/youtube_extension/backend/cloud_ai_routes.py +++ b/src/youtube_extension/backend/cloud_ai_routes.py @@ -229,7 +229,7 @@ async def get_provider_status(): except Exception as e: logger.error(f"Failed to get provider status: {e}") - raise HTTPException(status_code=500, detail=f"Failed to get provider status: {str(e)}") + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/analyze/video", response_model=VideoAnalysisResponse) @@ -263,12 +263,12 @@ async def analyze_video(request: VideoAnalysisRequest): raise HTTPException(status_code=429, detail=f"Rate limit exceeded: {str(e)}") except ConfigurationError as e: logger.error(f"Configuration error: {e}") - raise HTTPException(status_code=500, detail=f"Configuration error: {str(e)}") + raise HTTPException(status_code=500, detail="Internal server error") except HTTPException: raise except Exception as e: logger.error(f"Unexpected error during video analysis: {e}") - raise HTTPException(status_code=500, detail=f"Analysis failed: {str(e)}") + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/analyze/batch") @@ -301,7 +301,7 @@ async def analyze_batch_videos(request: BatchAnalysisRequest, background_tasks: raise except Exception as e: logger.error(f"Failed to start batch analysis: {e}") - raise HTTPException(status_code=500, detail=f"Batch analysis failed: {str(e)}") + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/analyze/multi-provider", response_model=list[VideoAnalysisResponse]) @@ -325,7 +325,7 @@ async def analyze_video_multi_provider(request: VideoAnalysisRequest): except Exception as e: logger.error(f"Multi-provider analysis failed: {e}") - raise HTTPException(status_code=500, detail=f"Multi-provider analysis failed: {str(e)}") + raise HTTPException(status_code=500, detail="Internal server error") @router.get("/analysis-types") diff --git a/src/youtube_extension/backend/cloud_api_endpoints.py b/src/youtube_extension/backend/cloud_api_endpoints.py index e38655f99..165bc76c9 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -260,9 +260,10 @@ async def batch_process_videos_cloud(request: BatchCloudProcessingRequest): except HTTPException: raise except Exception as e: + logger.error(f"Unhandled error in batch_process_videos_cloud: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Batch processing failed: {str(e)}" + detail="Internal server error" ) @router.get("/api/v3/videos/{video_id}/status", response_model=VideoStatusResponse) @@ -293,9 +294,10 @@ async def get_video_status(video_id: str): except HTTPException: raise except Exception as e: + logger.error(f"Unhandled error in get_video_status: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Error retrieving status: {str(e)}" + detail="Internal server error" ) @router.get("/api/v3/videos/{video_id}/result") @@ -330,9 +332,10 @@ async def get_video_result(video_id: str): except HTTPException: raise except Exception as e: + logger.error(f"Unhandled error in get_video_result: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Error retrieving result: {str(e)}" + detail="Internal server error" ) @router.get("/api/v3/queue/stats") diff --git a/src/youtube_extension/backend/main.py b/src/youtube_extension/backend/main.py index 0e52a9341..e0ffa5c3e 100644 --- a/src/youtube_extension/backend/main.py +++ b/src/youtube_extension/backend/main.py @@ -206,7 +206,7 @@ async def legacy_chat(request: dict): except Exception as e: logger.error(f"Legacy chat endpoint error: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @app.post("/api/process-video-markdown") @@ -240,7 +240,7 @@ async def legacy_process_video_markdown(request: dict): raise except Exception as e: logger.error(f"Legacy markdown processing error: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @app.post("/api/process-video") @@ -297,7 +297,7 @@ async def legacy_process_video(request: dict): except Exception as e: logger.error(f"Legacy video processing error: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # Other legacy endpoints with redirects @@ -368,7 +368,7 @@ async def system_info(): except Exception as e: logger.error(f"System info error: {e}") - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # Enhanced OpenAPI schema generation diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 03fd9e4e6..1b0433cfc 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -150,9 +150,10 @@ async def validate_video_url(request: VideoValidationRequest): } except Exception as e: + logger.error(f"Unhandled error in validate_video_url: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Validation failed: {str(e)}" + detail="Internal server error" ) @app.post("/api/v2/batch-process") @@ -177,9 +178,10 @@ async def batch_process_videos(request: BatchProcessingRequest): return result except Exception as e: + logger.error(f"Unhandled error in batch_process_videos: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Batch processing failed: {str(e)}" + detail="Internal server error" ) @app.get("/api/v2/videos/list") @@ -252,9 +254,10 @@ async def get_video_analysis(video_id: str): except HTTPException: raise except Exception as e: + logger.error(f"Unhandled error in get_video_analysis: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Error retrieving video analysis: {str(e)}" + detail="Internal server error" ) @app.get("/api/v2/cost-dashboard") @@ -384,9 +387,10 @@ async def clear_processing_cache(): } except Exception as e: + logger.error(f"Unhandled error in clear_processing_cache: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Failed to clear cache: {str(e)}" + detail="Internal server error" ) @app.post("/api/v2/search-videos") @@ -432,9 +436,10 @@ async def search_youtube_videos( } except Exception as e: + logger.error(f"Unhandled error in search_youtube_videos: {e}", exc_info=True) raise HTTPException( status_code=500, - detail=f"Search failed: {str(e)}" + detail="Internal server error" ) logger.info("🚀 Real API endpoints setup complete") diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py new file mode 100644 index 000000000..d561c422e --- /dev/null +++ b/tests/unit/test_500_info_disclosure.py @@ -0,0 +1,94 @@ +"""Regression guard against information disclosure in HTTP 500 responses. + +Context: several FastAPI handlers historically raised +``HTTPException(status_code=500, detail=str(e))`` (or an f-string embedding the +exception), leaking internal exception text — stack-adjacent messages, backend +API errors, database errors — to clients. See PR #801, which sanitized most but +not all handlers. + +This test encodes the invariant directly on the source: a 500 response must use +a *static* ``detail`` string, never one derived from the caught exception. It is +hermetic (pure source scan, no app import / no pydantic) so it runs anywhere and +catches new leaks in any backend route, not just the ones fixed today. + +It deliberately does not constrain 4xx responses: those echo client-supplied +validation errors, which are not internal-disclosure vectors. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +_BACKEND = Path(__file__).resolve().parents[2] / "src" / "youtube_extension" / "backend" + +# Match a single `raise HTTPException(...)` call, capturing its argument list, +# tolerant of the call spanning multiple lines. +_HTTP_EXC = re.compile(r"HTTPException\((?P.*?)\)", re.DOTALL) + +# A `detail=` argument whose value is derived from a variable/exception: +# detail=str(e) detail=str(exc) +# detail=f"... {e} ..." detail=f"... {str(e)} ..." +_DYNAMIC_DETAIL = re.compile( + r"""detail\s*=\s*(?: + str\( # detail=str(...) + | f["'][^"']*\{ # detail=f"...{...}..." + )""", + re.VERBOSE, +) + + +def _backend_python_files() -> list[Path]: + return sorted(_BACKEND.rglob("*.py")) + + +def _iter_500_dynamic_detail(text: str): + """Yield (line_no, snippet) for each 500 HTTPException with a dynamic detail.""" + for m in _HTTP_EXC.finditer(text): + args = m.group("args") + if "status_code=500" not in args.replace(" ", "").replace( + "status_code =", "status_code=" + ): + # normalize minor spacing; only care about 500 responses + if "status_code=500" not in re.sub(r"\s+", "", args): + continue + if _DYNAMIC_DETAIL.search(args): + line_no = text.count("\n", 0, m.start()) + 1 + yield line_no, " ".join(args.split())[:120] + + +def test_no_dynamic_detail_in_500_responses() -> None: + offenders: list[str] = [] + for path in _backend_python_files(): + text = path.read_text(encoding="utf-8") + for line_no, snippet in _iter_500_dynamic_detail(text): + rel = path.relative_to(_BACKEND.parents[2]) + offenders.append(f"{rel}:{line_no}: HTTPException({snippet})") + + assert not offenders, ( + "HTTP 500 responses must use a static `detail` string (e.g. " + '"Internal server error") and never leak the caught exception. ' + "Log the full error server-side instead. Offending sites:\n " + + "\n ".join(offenders) + ) + + +def test_guard_detects_a_synthetic_leak() -> None: + """Sanity check: the scanner actually flags a dynamic 500 detail.""" + leaky = "raise HTTPException(status_code=500, detail=str(e))" + safe = 'raise HTTPException(status_code=500, detail="Internal server error")' + client_err = "raise HTTPException(status_code=400, detail=str(exc))" + + assert list(_iter_500_dynamic_detail(leaky)), "scanner missed a real 500 leak" + assert not list( + _iter_500_dynamic_detail(safe) + ), "scanner false-positived a static detail" + assert not list( + _iter_500_dynamic_detail(client_err) + ), "scanner must ignore 4xx responses" + + +if __name__ == "__main__": # pragma: no cover + raise SystemExit(pytest.main([__file__, "-v"])) From 448fe6822a3c037a7fb06ba1e692a08d6121a491 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 02:18:41 +0000 Subject: [PATCH 2/7] fix(security): close dict/variable 500-detail leaks and harden the guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The source-scan guard added for the 500 info-disclosure work only matched inline detail=str(e) / detail=f"...{e}...". It silently passed while three handlers still leaked internal state through detail shapes it did not model: - cloud_api_endpoints.py process_video_cloud -> detail={... "message": error_msg ...} - cloud_api_endpoints.py process_video_task -> detail=error_msg (bare variable) - real_api_endpoints.py process_video_real_api -> detail={... "message": error_msg ...} Fix all three to a static "Internal server error" (the exception is logged server-side via logger.error(..., exc_info=True)), and rewrite the guard to flag any 500 detail= that is not an inline static string literal — a leading '{' (dict) or identifier char (f-string, str(), or a bare variable) now fails the scan. Self-checks extended to cover the dict and variable shapes. test_error_response_includes_video_url asserted the old leaking body; it is replaced by test_error_response_is_sanitized, which asserts the 500 body is exactly "Internal server error" and contains neither the exception nor the caller-supplied video_url. Strict superset of the #807 approach; the router.py/reporting_routes.py sinks #804 targeted are already clean on main and covered by the tree-wide guard. --- .../backend/cloud_api_endpoints.py | 19 +++------ .../backend/real_api_endpoints.py | 14 ++----- tests/unit/test_500_info_disclosure.py | 41 ++++++++++++------- tests/unit/test_real_api_endpoints.py | 11 +++-- 4 files changed, 43 insertions(+), 42 deletions(-) diff --git a/src/youtube_extension/backend/cloud_api_endpoints.py b/src/youtube_extension/backend/cloud_api_endpoints.py index 165bc76c9..613aa6f2f 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -142,18 +142,10 @@ async def process_video_cloud( ) except Exception as e: - error_msg = f"Cloud processing failed: {str(e)}" - logger.error(error_msg) + logger.error(f"Cloud processing failed: {e}", exc_info=True) - raise HTTPException( - status_code=500, - detail={ - "error": "cloud_processing_failed", - "message": error_msg, - "video_url": request.video_url, - "timestamp": datetime.now(timezone.utc).isoformat() - } - ) + # detail must be a static string — the exception is logged above only. + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/api/v3/process-video-task") async def process_video_task_handler( @@ -216,7 +208,7 @@ async def process_video_task_handler( except Exception as e: error_msg = f"Task processing failed: {str(e)}" - logger.error(error_msg) + logger.error(error_msg, exc_info=True) # Update state with error try: @@ -229,7 +221,8 @@ async def process_video_task_handler( except Exception as state_error: logger.error(f"Failed to update error state: {state_error}") - raise HTTPException(status_code=500, detail=error_msg) + # detail must be a static string — error_msg (with the exception) is logged above only. + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/api/v3/batch-process") async def batch_process_videos_cloud(request: BatchCloudProcessingRequest): diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 1b0433cfc..b886bf192 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -118,18 +118,10 @@ async def process_video_real_api(request: VideoProcessingRequest, background_tas return response except Exception as e: - error_msg = f"Real API processing failed: {str(e)}" - logger.error(error_msg) + logger.error(f"Real API processing failed: {e}", exc_info=True) - raise HTTPException( - status_code=500, - detail={ - "error": "video_processing_failed", - "message": error_msg, - "video_url": request.video_url, - "timestamp": datetime.now(timezone.utc).isoformat() - } - ) + # detail must be a static string — the exception is logged above only. + raise HTTPException(status_code=500, detail="Internal server error") @app.post("/api/v2/validate-video") async def validate_video_url(request: VideoValidationRequest): diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index d561c422e..d67f2bddc 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -28,16 +28,17 @@ # tolerant of the call spanning multiple lines. _HTTP_EXC = re.compile(r"HTTPException\((?P.*?)\)", re.DOTALL) -# A `detail=` argument whose value is derived from a variable/exception: -# detail=str(e) detail=str(exc) -# detail=f"... {e} ..." detail=f"... {str(e)} ..." -_DYNAMIC_DETAIL = re.compile( - r"""detail\s*=\s*(?: - str\( # detail=str(...) - | f["'][^"']*\{ # detail=f"...{...}..." - )""", - re.VERBOSE, -) +# A 500 `detail=` is safe only when it is an inline *static string literal* +# (``detail="Internal server error"``). Anything else can carry internal state to +# the client and is flagged: +# detail=str(e) detail=f"... {e} ..." (inline dynamic string) +# detail=error_msg (a variable — may hold f"...{e}...") +# detail={...} (a dict whose values embed the exception) +# The value after ``detail=`` is dynamic unless its first non-space character +# opens a plain string literal (``"`` or ``'``). A leading ``{`` (dict) or any +# identifier char — ``f`` of an f-string, ``s`` of ``str(``, or a bare variable +# name — means it is not a static literal. +_DYNAMIC_DETAIL = re.compile(r"""detail\s*=\s*(?:\{|[A-Za-z_])""") def _backend_python_files() -> list[Path]: @@ -76,15 +77,25 @@ def test_no_dynamic_detail_in_500_responses() -> None: def test_guard_detects_a_synthetic_leak() -> None: - """Sanity check: the scanner actually flags a dynamic 500 detail.""" - leaky = "raise HTTPException(status_code=500, detail=str(e))" + """Sanity check: the scanner flags every dynamic-detail shape, not just str(e).""" + # Each of these leaks internal state and must be flagged. + leaks = [ + "raise HTTPException(status_code=500, detail=str(e))", + 'raise HTTPException(status_code=500, detail=f"failed: {e}")', + "raise HTTPException(status_code=500, detail=error_msg)", # bare variable + 'raise HTTPException(status_code=500, detail={"message": error_msg})', # dict + ] + for leak in leaks: + assert list(_iter_500_dynamic_detail(leak)), f"scanner missed a real 500 leak: {leak}" + + # A static string literal is the only safe form. safe = 'raise HTTPException(status_code=500, detail="Internal server error")' - client_err = "raise HTTPException(status_code=400, detail=str(exc))" - - assert list(_iter_500_dynamic_detail(leaky)), "scanner missed a real 500 leak" assert not list( _iter_500_dynamic_detail(safe) ), "scanner false-positived a static detail" + + # 4xx responses echo client-supplied input and are intentionally out of scope. + client_err = "raise HTTPException(status_code=400, detail=str(exc))" assert not list( _iter_500_dynamic_detail(client_err) ), "scanner must ignore 4xx responses" diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index 4eb7109c6..b9f14406a 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -344,14 +344,19 @@ def test_returns_500_when_processor_raises(self, client, mock_processor): ) assert response.status_code == 500 - def test_error_response_includes_video_url(self, client, mock_processor): + def test_error_response_is_sanitized(self, client, mock_processor): + # A 500 must not leak internal state (CWE-209): the response body must be a + # static message, never the caught exception or the caller-supplied video_url. mock_processor.process_video = AsyncMock(side_effect=RuntimeError("crash")) response = client.post( "/api/v2/process-video", json={"video_url": "https://youtube.com/watch?v=auJzb1D-fag"}, ) - detail = response.json()["detail"] - assert "auJzb1D-fag" in str(detail) + assert response.status_code == 500 + detail = str(response.json()["detail"]) + assert detail == "Internal server error" + assert "crash" not in detail + assert "auJzb1D-fag" not in detail def test_missing_video_url_returns_422(self, client): response = client.post("/api/v2/process-video", json={}) From 93d4847c0df09bcdd8b0ffb3520a3d8d780645f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 02:30:03 +0000 Subject: [PATCH 3/7] =?UTF-8?q?fix(security):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20global=20handler=20leak,=20persisted-error=20leak,?= =?UTF-8?q?=20400=E2=86=92500=20swallow?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round of fixes from the CodeRabbit full review and the Vercel VADE bot on #815: - main.py global_exception_handler leaked str(exc) and the exception class name (error_type) in its 500 JSONResponse body (VADE finding). Return a static body; the full exception is already logged with exc_info=True. - cloud_api_endpoints.process_video_task_handler persisted error_msg (containing str(e)) as the Firestore 'error_message', which get_video_status / get_video_result return verbatim to clients — exfiltrating the exception despite the sanitized 500. Persist a generic message; keep the detail in logs only. - real_api_endpoints batch_process_videos and search_youtube_videos caught their own HTTPException(400) validation errors in the broad 'except Exception' and turned them into 500s. Re-raise HTTPException first to preserve the 400. - cloud_ai_routes: add exc_info=True to the five 500-handler logger.error calls so the traceback is retained server-side. - Guard: exception handlers are a second 500 sink the HTTPException scan didn't model. Add test_no_disclosure_in_500_exception_handlers — it flags any @app.exception_handler that returns 500 while placing str(exc)/str(e) or __class__.__name__ in the response body (docstrings/comments/log lines excluded). --- .../backend/cloud_ai_routes.py | 10 +- .../backend/cloud_api_endpoints.py | 4 +- src/youtube_extension/backend/main.py | 13 ++- .../backend/real_api_endpoints.py | 6 + tests/unit/test_500_info_disclosure.py | 105 ++++++++++++++++++ 5 files changed, 127 insertions(+), 11 deletions(-) diff --git a/src/youtube_extension/backend/cloud_ai_routes.py b/src/youtube_extension/backend/cloud_ai_routes.py index 0172e1617..a470cabe6 100644 --- a/src/youtube_extension/backend/cloud_ai_routes.py +++ b/src/youtube_extension/backend/cloud_ai_routes.py @@ -228,7 +228,7 @@ async def get_provider_status(): ) except Exception as e: - logger.error(f"Failed to get provider status: {e}") + logger.error(f"Failed to get provider status: {e}", exc_info=True) raise HTTPException(status_code=500, detail="Internal server error") @@ -262,12 +262,12 @@ async def analyze_video(request: VideoAnalysisRequest): logger.warning(f"Rate limit exceeded: {e}") raise HTTPException(status_code=429, detail=f"Rate limit exceeded: {str(e)}") except ConfigurationError as e: - logger.error(f"Configuration error: {e}") + logger.error(f"Configuration error: {e}", exc_info=True) raise HTTPException(status_code=500, detail="Internal server error") except HTTPException: raise except Exception as e: - logger.error(f"Unexpected error during video analysis: {e}") + logger.error(f"Unexpected error during video analysis: {e}", exc_info=True) raise HTTPException(status_code=500, detail="Internal server error") @@ -300,7 +300,7 @@ async def analyze_batch_videos(request: BatchAnalysisRequest, background_tasks: except HTTPException: raise except Exception as e: - logger.error(f"Failed to start batch analysis: {e}") + logger.error(f"Failed to start batch analysis: {e}", exc_info=True) raise HTTPException(status_code=500, detail="Internal server error") @@ -324,7 +324,7 @@ async def analyze_video_multi_provider(request: VideoAnalysisRequest): return formatted_results except Exception as e: - logger.error(f"Multi-provider analysis failed: {e}") + logger.error(f"Multi-provider analysis failed: {e}", exc_info=True) raise HTTPException(status_code=500, detail="Internal server error") diff --git a/src/youtube_extension/backend/cloud_api_endpoints.py b/src/youtube_extension/backend/cloud_api_endpoints.py index 613aa6f2f..fc7dedee0 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -213,10 +213,12 @@ async def process_video_task_handler( # Update state with error try: firestore_service = await get_firestore_service() + # error_message is returned to clients by the status/result endpoints, + # so it must stay generic — the full error_msg is in the logs above only. await firestore_service.update_state( payload.video_id, status='failed', - error_message=error_msg + error_message="Internal server error" ) except Exception as state_error: logger.error(f"Failed to update error state: {state_error}") diff --git a/src/youtube_extension/backend/main.py b/src/youtube_extension/backend/main.py index ff1234740..c7dabe921 100644 --- a/src/youtube_extension/backend/main.py +++ b/src/youtube_extension/backend/main.py @@ -444,21 +444,24 @@ async def value_error_handler(request, exc): @app.exception_handler(Exception) async def global_exception_handler(request, exc): - """Global exception handler with enhanced error details""" + """Global handler for unhandled exceptions. + + The full exception — type, message, and traceback — is logged server-side + only. The client receives a static body: neither the exception message + (``str(exc)``) nor its class name may be disclosed, as both leak internal + state to the caller (CWE-209 information disclosure). + """ logger.error(f"Unhandled exception: {exc}", exc_info=True) error_detail = { "error": "Internal server error", - "detail": str(exc), + "detail": "Internal server error", "timestamp": datetime.now().isoformat(), "path": str(request.url) if hasattr(request, "url") else "unknown", "version": "2.0.0", "architecture": "service-oriented", } - if hasattr(exc, "__class__"): - error_detail["error_type"] = exc.__class__.__name__ - return JSONResponse(status_code=500, content=error_detail) diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index b886bf192..65c8b9fe6 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -169,6 +169,9 @@ async def batch_process_videos(request: BatchProcessingRequest): return result + except HTTPException: + # Preserve intentional client errors (e.g. the 400 above). + raise except Exception as e: logger.error(f"Unhandled error in batch_process_videos: {e}", exc_info=True) raise HTTPException( @@ -427,6 +430,9 @@ async def search_youtube_videos( "timestamp": datetime.now(timezone.utc).isoformat() } + except HTTPException: + # Preserve intentional client errors (e.g. the 400 above). + raise except Exception as e: logger.error(f"Unhandled error in search_youtube_videos: {e}", exc_info=True) raise HTTPException( diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index d67f2bddc..c90f15407 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -60,6 +60,62 @@ def _iter_500_dynamic_detail(text: str): yield line_no, " ".join(args.split())[:120] +# FastAPI exception handlers are a second 500 sink the HTTPException scan above +# does not model: they build a response body directly (dict / JSONResponse) rather +# than raising. A handler must not place the exception message (`str(exc)`) or its +# class name (`exc.__class__.__name__`) into that body — both leak internal state. +# Logging the exception server-side is fine; those tokens appear only in response +# construction, never in a `logger.`/`log`/`raise`/comment line, so we exclude +# those lines to avoid false positives. +_HANDLER_DECORATOR = re.compile(r"^\s*@\w+\.exception_handler\(", re.MULTILINE) +_HANDLER_DISCLOSURE = re.compile(r"str\(\s*(?:exc|e)\s*\)|__class__\.__name__") +# Triple-quoted docstrings, so prose that *mentions* str(exc) is not mistaken for code. +_TRIPLE_STR = re.compile(r'"""(?:.|\n)*?"""|\'\'\'(?:.|\n)*?\'\'\'') + + +def _strip_docstrings(body: str) -> str: + """Blank triple-quoted blocks, preserving line count so line numbers stay aligned.""" + return _TRIPLE_STR.sub(lambda m: "\n" * m.group(0).count("\n"), body) + + +def _exception_handler_bodies(text: str): + """Yield (start_line, body_text) for each @.exception_handler function.""" + lines = text.splitlines(keepends=True) + for m in _HANDLER_DECORATOR.finditer(text): + start_line = text.count("\n", 0, m.start()) + # Find the `def`/`async def` line that the decorator applies to. + i = start_line + while i < len(lines) and not lines[i].lstrip().startswith(("def ", "async def ")): + i += 1 + if i >= len(lines): + continue + def_indent = len(lines[i]) - len(lines[i].lstrip()) + j = i + 1 + body: list[str] = [] + while j < len(lines): + line = lines[j] + stripped = line.strip() + if stripped and (len(line) - len(line.lstrip())) <= def_indent: + break # dedented back to <= the def's level: end of function + body.append(line) + j += 1 + yield i + 1, "".join(body) + + +def _iter_handler_disclosures(text: str): + """Yield (line_no, snippet) for exception-handler bodies that leak exc into the response.""" + for start_line, body in _exception_handler_bodies(text): + if "status_code=500" not in re.sub(r"\s+", "", body): + continue + for offset, line in enumerate(_strip_docstrings(body).splitlines()): + code = line.split("#", 1)[0] # ignore inline comments + bare = line.strip() + if bare.startswith(("logger", "log", "self.logger", "raise")): + continue + if _HANDLER_DISCLOSURE.search(code): + yield start_line + offset, bare[:120] + + def test_no_dynamic_detail_in_500_responses() -> None: offenders: list[str] = [] for path in _backend_python_files(): @@ -76,6 +132,55 @@ def test_no_dynamic_detail_in_500_responses() -> None: ) +def test_no_disclosure_in_500_exception_handlers() -> None: + offenders: list[str] = [] + for path in _backend_python_files(): + text = path.read_text(encoding="utf-8") + for line_no, snippet in _iter_handler_disclosures(text): + rel = path.relative_to(_BACKEND.parents[2]) + offenders.append(f"{rel}:{line_no}: {snippet}") + + assert not offenders, ( + "A FastAPI exception handler that returns HTTP 500 must not place the " + "exception message (`str(exc)`) or its class name (`__class__.__name__`) " + "into the response body — log it server-side instead. Offending sites:\n " + + "\n ".join(offenders) + ) + + +def test_guard_detects_a_synthetic_handler_leak() -> None: + """The exception-handler scanner flags exc message/class-name disclosure in a 500 body.""" + leaky = ( + "@app.exception_handler(Exception)\n" + "async def h(request, exc):\n" + ' logger.error(f"boom: {exc}", exc_info=True)\n' + ' body = {"detail": str(exc), "error_type": exc.__class__.__name__}\n' + " return JSONResponse(status_code=500, content=body)\n" + ) + assert list(_iter_handler_disclosures(leaky)), "handler scanner missed a real leak" + + safe = ( + "@app.exception_handler(Exception)\n" + "async def h(request, exc):\n" + ' logger.error(f"boom: {exc}", exc_info=True)\n' + ' body = {"detail": "Internal server error"}\n' + " return JSONResponse(status_code=500, content=body)\n" + ) + assert not list( + _iter_handler_disclosures(safe) + ), "handler scanner false-positived a sanitized 500 handler" + + # A 4xx handler may echo the exception (client-supplied validation input). + client_err = ( + "@app.exception_handler(ValueError)\n" + "async def h(request, exc):\n" + ' return JSONResponse(status_code=400, content={"detail": str(exc)})\n' + ) + assert not list( + _iter_handler_disclosures(client_err) + ), "handler scanner must ignore 4xx handlers" + + def test_guard_detects_a_synthetic_leak() -> None: """Sanity check: the scanner flags every dynamic-detail shape, not just str(e).""" # Each of these leaks internal state and must be flagged. From d78c9c3915424e0a2ab51625ed6f5624b79969ee Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 02:39:04 +0000 Subject: [PATCH 4/7] fix(security): close positional + Ray-Serve 500 leaks, fix stale tests, complete guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round (CodeRabbit incremental + Copilot + fixing the red test job): - advanced_video_routes.py: 8 handlers used the *positional* form `raise HTTPException(500, str(e))`, leaking the exception to clients. The guard missed them because it only matched keyword `status_code=500`/`detail=`. Sanitize all 8 (they already log with exc_info=True). - uvai/ml/serve.py: the Ray Serve checkpoint endpoint returned `JSONResponse({"error": str(exc)}, status_code=500)` — a client-facing leak. Return a static body and log the exception. - cloud_api_endpoints.py: log the Firestore state-update failure with exc_info=True. - test_backend_main.py: the global-handler test still asserted body["error_type"]; that key is now gone, so the assertion KeyError'd and failed the CI test job. Replace with assertions that the 500 body is sanitized (no exc message/class). - test_real_api_endpoints.py: the batch(>20) and search(>50) tests allowed 400 OR 500; now that the handlers preserve the intentional 400, assert == 400 exactly. Guard (test_500_info_disclosure.py) rewritten to be leak-shape-complete via balanced-paren extraction. It now covers all three 500 sinks: 1. HTTPException — keyword AND positional detail (`HTTPException(500, str(e))`). 2. @app.exception_handler bodies (str(exc)/__class__.__name__). 3. raw JSONResponse(..., status_code=500) embedding the exception. Scope extended to src/uvai/ml. Self-checks cover every shape and confirm 4xx is ignored. --- src/uvai/ml/serve.py | 5 +- .../backend/api/advanced_video_routes.py | 16 +- .../backend/cloud_api_endpoints.py | 2 +- tests/unit/test_500_info_disclosure.py | 262 ++++++++++++++---- tests/unit/test_backend_main.py | 9 +- tests/unit/test_real_api_endpoints.py | 19 +- 6 files changed, 233 insertions(+), 80 deletions(-) diff --git a/src/uvai/ml/serve.py b/src/uvai/ml/serve.py index 707db71b8..d547951d8 100644 --- a/src/uvai/ml/serve.py +++ b/src/uvai/ml/serve.py @@ -399,8 +399,11 @@ async def __call__( "ranker_samples": ranker_state.get("training_samples", 0), }) except Exception as exc: + # Log the full error server-side; return a static body so the + # exception text does not leak to the client (CWE-209). + logger.error(f"Checkpoint save failed: {exc}", exc_info=True) return JSONResponse( - {"error": str(exc)}, + {"error": "Internal server error"}, status_code=500, ) else: diff --git a/src/youtube_extension/backend/api/advanced_video_routes.py b/src/youtube_extension/backend/api/advanced_video_routes.py index 1d66b0bee..69f153e08 100644 --- a/src/youtube_extension/backend/api/advanced_video_routes.py +++ b/src/youtube_extension/backend/api/advanced_video_routes.py @@ -143,7 +143,7 @@ async def analyze_segment(request: TemporalSegmentRequest): } except Exception as e: logger.error(f"Segment analysis failed: {e}", exc_info=True) - raise HTTPException(500, str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/temporal/events") @@ -211,7 +211,7 @@ async def extract_temporal_events(request: TemporalEventsRequest): } except Exception as e: logger.error(f"Event extraction failed: {e}", exc_info=True) - raise HTTPException(500, str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/temporal/question") @@ -244,7 +244,7 @@ async def answer_temporal_question(request: TemporalQuestionRequest): } except Exception as e: logger.error(f"Temporal question failed: {e}", exc_info=True) - raise HTTPException(500, str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/temporal/timeline") @@ -285,7 +285,7 @@ async def create_timeline(request: TimelineRequest): raise except Exception as e: logger.error(f"Timeline creation failed: {e}", exc_info=True) - raise HTTPException(500, str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/temporal/compare-segments") @@ -319,7 +319,7 @@ async def compare_segments(request: SegmentComparisonRequest): } except Exception as e: logger.error(f"Segment comparison failed: {e}", exc_info=True) - raise HTTPException(500, str(e)) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/temporal/tutorial-steps") @@ -347,7 +347,7 @@ async def extract_tutorial_steps(request: TutorialStepsRequest): } except Exception as e: logger.error(f"Tutorial extraction failed: {e}", exc_info=True) - raise HTTPException(500, str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # ============ Structured Output Endpoint ============ @@ -435,7 +435,7 @@ async def analyze_with_schema(request: StructuredAnalysisRequest): } except Exception as e: logger.error(f"Structured analysis failed: {e}", exc_info=True) - raise HTTPException(500, str(e)) + raise HTTPException(status_code=500, detail="Internal server error") # ============ CloudEvents Publishing Endpoint ============ @@ -488,4 +488,4 @@ async def publish_video_event( } except Exception as e: logger.error(f"Event publishing failed: {e}", exc_info=True) - raise HTTPException(500, str(e)) + raise HTTPException(status_code=500, detail="Internal server error") diff --git a/src/youtube_extension/backend/cloud_api_endpoints.py b/src/youtube_extension/backend/cloud_api_endpoints.py index fc7dedee0..13bd33e8b 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -221,7 +221,7 @@ async def process_video_task_handler( error_message="Internal server error" ) except Exception as state_error: - logger.error(f"Failed to update error state: {state_error}") + logger.error(f"Failed to update error state: {state_error}", exc_info=True) # detail must be a static string — error_msg (with the exception) is logged above only. raise HTTPException(status_code=500, detail="Internal server error") diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index c90f15407..554ef9403 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -1,6 +1,6 @@ """Regression guard against information disclosure in HTTP 500 responses. -Context: several FastAPI handlers historically raised +Context: several handlers historically raised ``HTTPException(status_code=500, detail=str(e))`` (or an f-string embedding the exception), leaking internal exception text — stack-adjacent messages, backend API errors, database errors — to clients. See PR #801, which sanitized most but @@ -9,7 +9,13 @@ This test encodes the invariant directly on the source: a 500 response must use a *static* ``detail`` string, never one derived from the caught exception. It is hermetic (pure source scan, no app import / no pydantic) so it runs anywhere and -catches new leaks in any backend route, not just the ones fixed today. +catches new leaks in any backend route. + +It models the three distinct 500 sinks in this codebase: + 1. ``HTTPException`` — both keyword (``status_code=500, detail=...``) and + positional (``HTTPException(500, str(e))``) forms. + 2. FastAPI ``@app.exception_handler`` functions that build a 500 body directly. + 3. A raw ``JSONResponse(..., status_code=500)`` (e.g. a Ray Serve deployment). It deliberately does not constrain 4xx responses: those echo client-supplied validation errors, which are not internal-disclosure vectors. @@ -22,51 +28,124 @@ import pytest -_BACKEND = Path(__file__).resolve().parents[2] / "src" / "youtube_extension" / "backend" - -# Match a single `raise HTTPException(...)` call, capturing its argument list, -# tolerant of the call spanning multiple lines. -_HTTP_EXC = re.compile(r"HTTPException\((?P.*?)\)", re.DOTALL) - -# A 500 `detail=` is safe only when it is an inline *static string literal* -# (``detail="Internal server error"``). Anything else can carry internal state to -# the client and is flagged: -# detail=str(e) detail=f"... {e} ..." (inline dynamic string) -# detail=error_msg (a variable — may hold f"...{e}...") -# detail={...} (a dict whose values embed the exception) -# The value after ``detail=`` is dynamic unless its first non-space character -# opens a plain string literal (``"`` or ``'``). A leading ``{`` (dict) or any -# identifier char — ``f`` of an f-string, ``s`` of ``str(``, or a bare variable -# name — means it is not a static literal. -_DYNAMIC_DETAIL = re.compile(r"""detail\s*=\s*(?:\{|[A-Za-z_])""") +_REPO_ROOT = Path(__file__).resolve().parents[2] +_BACKEND = _REPO_ROOT / "src" / "youtube_extension" / "backend" +# Additional client-facing 500 surfaces outside the FastAPI backend package. +_EXTRA_ROOTS = [_REPO_ROOT / "src" / "uvai" / "ml"] def _backend_python_files() -> list[Path]: - return sorted(_BACKEND.rglob("*.py")) + files = list(_BACKEND.rglob("*.py")) + for root in _EXTRA_ROOTS: + if root.exists(): + files.extend(root.rglob("*.py")) + return sorted(set(files)) + + +# --- shared parsing helpers ------------------------------------------------ + +def _balanced_call_args(text: str, open_paren_idx: int) -> str: + """Return the argument text of a call, balancing nested parens/brackets/braces.""" + depth = 0 + quote: str | None = None + for i in range(open_paren_idx, len(text)): + c = text[i] + if quote: + if c == quote: + quote = None + continue + if c in "\"'": + quote = c + elif c in "([{": + depth += 1 + elif c in ")]}": + depth -= 1 + if depth == 0: + return text[open_paren_idx + 1 : i] + return text[open_paren_idx + 1 :] + + +def _split_top_level_commas(s: str) -> list[str]: + """Split on commas that are not nested inside parens/brackets/braces/strings.""" + parts: list[str] = [] + depth = 0 + quote: str | None = None + buf: list[str] = [] + for c in s: + if quote: + buf.append(c) + if c == quote: + quote = None + elif c in "\"'": + quote = c + buf.append(c) + elif c in "([{": + depth += 1 + buf.append(c) + elif c in ")]}": + depth -= 1 + buf.append(c) + elif c == "," and depth == 0: + parts.append("".join(buf)) + buf = [] + else: + buf.append(c) + parts.append("".join(buf)) + return parts + + +def _detail_is_dynamic(value: str) -> bool: + """A detail value is safe only if it is an inline static string literal. + + Anything else — ``str(e)``, an f-string, a bare variable, or a ``{...}`` dict — + can carry internal state to the client, so it is dynamic. + """ + v = value.strip() + if not v: + return False + return not v.startswith(('"', "'")) + + +# --- sink 1: HTTPException (keyword and positional) ------------------------ + +_HTTP_EXC_OPEN = re.compile(r"HTTPException\(") def _iter_500_dynamic_detail(text: str): """Yield (line_no, snippet) for each 500 HTTPException with a dynamic detail.""" - for m in _HTTP_EXC.finditer(text): - args = m.group("args") - if "status_code=500" not in args.replace(" ", "").replace( - "status_code =", "status_code=" - ): - # normalize minor spacing; only care about 500 responses - if "status_code=500" not in re.sub(r"\s+", "", args): - continue - if _DYNAMIC_DETAIL.search(args): + for m in _HTTP_EXC_OPEN.finditer(text): + args = _balanced_call_args(text, m.end() - 1) + parts = _split_top_level_commas(args) + nospace = re.sub(r"\s+", "", args) + first = parts[0].strip() if parts else "" + is_500 = "status_code=500" in nospace or first == "500" + if not is_500: + continue + + detail_val: str | None = None + for p in parts: + ps = p.strip() + if ps.startswith("detail="): + detail_val = ps[len("detail=") :] + break + if detail_val is None and len(parts) >= 2: + # positional detail is the 2nd argument, unless it is a keyword arg. + second = parts[1] + if "=" not in second.split("(", 1)[0]: + detail_val = second + + if detail_val is not None and _detail_is_dynamic(detail_val): line_no = text.count("\n", 0, m.start()) + 1 yield line_no, " ".join(args.split())[:120] -# FastAPI exception handlers are a second 500 sink the HTTPException scan above -# does not model: they build a response body directly (dict / JSONResponse) rather +# --- sink 2: FastAPI exception handlers ------------------------------------ +# +# Exception handlers build a response body directly (dict / JSONResponse) rather # than raising. A handler must not place the exception message (`str(exc)`) or its -# class name (`exc.__class__.__name__`) into that body — both leak internal state. -# Logging the exception server-side is fine; those tokens appear only in response -# construction, never in a `logger.`/`log`/`raise`/comment line, so we exclude -# those lines to avoid false positives. +# class name (`exc.__class__.__name__`) into that body. Logging the exception +# server-side is fine; those tokens never appear on a `logger.`/`log`/`raise`/ +# comment line, so we exclude those lines to avoid false positives. _HANDLER_DECORATOR = re.compile(r"^\s*@\w+\.exception_handler\(", re.MULTILINE) _HANDLER_DISCLOSURE = re.compile(r"str\(\s*(?:exc|e)\s*\)|__class__\.__name__") # Triple-quoted docstrings, so prose that *mentions* str(exc) is not mistaken for code. @@ -83,7 +162,6 @@ def _exception_handler_bodies(text: str): lines = text.splitlines(keepends=True) for m in _HANDLER_DECORATOR.finditer(text): start_line = text.count("\n", 0, m.start()) - # Find the `def`/`async def` line that the decorator applies to. i = start_line while i < len(lines) and not lines[i].lstrip().startswith(("def ", "async def ")): i += 1 @@ -116,19 +194,46 @@ def _iter_handler_disclosures(text: str): yield start_line + offset, bare[:120] +# --- sink 3: raw JSONResponse 500s ----------------------------------------- +# +# A ``JSONResponse(..., status_code=500)`` body must not embed the exception +# (``str(exc)`` / ``str(e)`` / ``str(error)``) or an f-string interpolating one. +_JSON_RESPONSE = re.compile(r"JSONResponse\(") +# Exception-like variable names, so an f-string interpolating a UUID / error-id +# (e.g. f"FALLBACK_{uuid.uuid4()...}") is not mistaken for an exception leak. +_EXC_TOKENS = r"(?:e|ex|exc|err|error|error_msg|error_message|exception)" +_JSON_DISCLOSURE = re.compile( + r"str\(\s*(?:exc|e|error)\s*\)" # str(exc) / str(e) / str(error) + r"|f[\"'][^\"']*\{[^{}]*\b" + _EXC_TOKENS + r"\b[^{}]*\}" # f"...{exc-ref}..." +) + + +def _iter_json_500_disclosures(text: str): + """Yield (line_no, snippet) for JSONResponse 500s whose body embeds the exception.""" + for m in _JSON_RESPONSE.finditer(text): + args = _balanced_call_args(text, m.end() - 1) + if "status_code=500" not in re.sub(r"\s+", "", args): + continue + if _JSON_DISCLOSURE.search(args): + line_no = text.count("\n", 0, m.start()) + 1 + yield line_no, " ".join(args.split())[:120] + + +# --- the guards ------------------------------------------------------------ + def test_no_dynamic_detail_in_500_responses() -> None: offenders: list[str] = [] for path in _backend_python_files(): text = path.read_text(encoding="utf-8") for line_no, snippet in _iter_500_dynamic_detail(text): - rel = path.relative_to(_BACKEND.parents[2]) + rel = path.relative_to(_REPO_ROOT) offenders.append(f"{rel}:{line_no}: HTTPException({snippet})") assert not offenders, ( "HTTP 500 responses must use a static `detail` string (e.g. " - '"Internal server error") and never leak the caught exception. ' - "Log the full error server-side instead. Offending sites:\n " - + "\n ".join(offenders) + '"Internal server error") and never leak the caught exception — in either ' + "the keyword or positional form. Log the full error server-side instead. " + "Offending sites:\n " + "\n ".join(offenders) ) @@ -137,7 +242,7 @@ def test_no_disclosure_in_500_exception_handlers() -> None: for path in _backend_python_files(): text = path.read_text(encoding="utf-8") for line_no, snippet in _iter_handler_disclosures(text): - rel = path.relative_to(_BACKEND.parents[2]) + rel = path.relative_to(_REPO_ROOT) offenders.append(f"{rel}:{line_no}: {snippet}") assert not offenders, ( @@ -148,6 +253,51 @@ def test_no_disclosure_in_500_exception_handlers() -> None: ) +def test_no_disclosure_in_json_500_responses() -> None: + offenders: list[str] = [] + for path in _backend_python_files(): + text = path.read_text(encoding="utf-8") + for line_no, snippet in _iter_json_500_disclosures(text): + rel = path.relative_to(_REPO_ROOT) + offenders.append(f"{rel}:{line_no}: JSONResponse({snippet})") + + assert not offenders, ( + "A raw JSONResponse with status_code=500 must not embed the caught " + "exception in its body — return a static message and log the error " + "server-side instead. Offending sites:\n " + "\n ".join(offenders) + ) + + +def test_guard_detects_a_synthetic_leak() -> None: + """The HTTPException scanner flags every dynamic-detail shape, keyword and positional.""" + leaks = [ + "raise HTTPException(status_code=500, detail=str(e))", + 'raise HTTPException(status_code=500, detail=f"failed: {e}")', + "raise HTTPException(status_code=500, detail=error_msg)", # bare variable + 'raise HTTPException(status_code=500, detail={"message": error_msg})', # dict + "raise HTTPException(500, str(e))", # positional detail + 'raise HTTPException(500, f"boom: {e}")', # positional f-string + ] + for leak in leaks: + assert list(_iter_500_dynamic_detail(leak)), f"scanner missed a real 500 leak: {leak}" + + # Static string literals are the only safe form — keyword or positional. + safe = [ + 'raise HTTPException(status_code=500, detail="Internal server error")', + 'raise HTTPException(500, "Internal server error")', + ] + for s in safe: + assert not list(_iter_500_dynamic_detail(s)), f"scanner false-positived: {s}" + + # 4xx responses echo client-supplied input and are intentionally out of scope. + client_errs = [ + "raise HTTPException(status_code=400, detail=str(exc))", + "raise HTTPException(400, str(exc))", + ] + for c in client_errs: + assert not list(_iter_500_dynamic_detail(c)), f"scanner must ignore 4xx: {c}" + + def test_guard_detects_a_synthetic_handler_leak() -> None: """The exception-handler scanner flags exc message/class-name disclosure in a 500 body.""" leaky = ( @@ -170,7 +320,6 @@ def test_guard_detects_a_synthetic_handler_leak() -> None: _iter_handler_disclosures(safe) ), "handler scanner false-positived a sanitized 500 handler" - # A 4xx handler may echo the exception (client-supplied validation input). client_err = ( "@app.exception_handler(ValueError)\n" "async def h(request, exc):\n" @@ -181,29 +330,24 @@ def test_guard_detects_a_synthetic_handler_leak() -> None: ), "handler scanner must ignore 4xx handlers" -def test_guard_detects_a_synthetic_leak() -> None: - """Sanity check: the scanner flags every dynamic-detail shape, not just str(e).""" - # Each of these leaks internal state and must be flagged. +def test_guard_detects_a_synthetic_json_500_leak() -> None: + """The JSONResponse scanner flags exception disclosure across nested braces.""" leaks = [ - "raise HTTPException(status_code=500, detail=str(e))", - 'raise HTTPException(status_code=500, detail=f"failed: {e}")', - "raise HTTPException(status_code=500, detail=error_msg)", # bare variable - 'raise HTTPException(status_code=500, detail={"message": error_msg})', # dict + 'return JSONResponse({"error": str(exc)}, status_code=500)', + 'JSONResponse(content={"m": f"failed: {e}"}, status_code=500)', + 'JSONResponse({"error": str(error)}, status_code=500)', ] for leak in leaks: - assert list(_iter_500_dynamic_detail(leak)), f"scanner missed a real 500 leak: {leak}" + assert list(_iter_json_500_disclosures(leak)), f"json scanner missed: {leak}" - # A static string literal is the only safe form. - safe = 'raise HTTPException(status_code=500, detail="Internal server error")' - assert not list( - _iter_500_dynamic_detail(safe) - ), "scanner false-positived a static detail" + safe = 'JSONResponse({"error": "Internal server error"}, status_code=500)' + assert not list(_iter_json_500_disclosures(safe)), "json scanner false-positived" - # 4xx responses echo client-supplied input and are intentionally out of scope. - client_err = "raise HTTPException(status_code=400, detail=str(exc))" + # 4xx JSONResponses may echo client input. + client_err = 'JSONResponse({"detail": str(exc)}, status_code=400)' assert not list( - _iter_500_dynamic_detail(client_err) - ), "scanner must ignore 4xx responses" + _iter_json_500_disclosures(client_err) + ), "json scanner must ignore 4xx" if __name__ == "__main__": # pragma: no cover diff --git a/tests/unit/test_backend_main.py b/tests/unit/test_backend_main.py index a73dcf792..878d7041c 100644 --- a/tests/unit/test_backend_main.py +++ b/tests/unit/test_backend_main.py @@ -590,14 +590,19 @@ async def test_global_exception_handler_returns_500(self): response = await main_module.global_exception_handler(mock_req, RuntimeError("crash")) assert response.status_code == 500 - async def test_global_exception_handler_includes_error_type(self): + async def test_global_exception_handler_is_sanitized(self): + """The 500 body must not leak the exception message or class name (CWE-209).""" import json as _json mock_req = MagicMock() mock_req.url = "http://test/api" response = await main_module.global_exception_handler(mock_req, RuntimeError("crash")) body = _json.loads(response.body) - assert body["error_type"] == "RuntimeError" + raw = response.body.decode() + assert "error_type" not in body + assert body["detail"] == "Internal server error" + assert "crash" not in raw + assert "RuntimeError" not in raw async def test_global_exception_handler_includes_version(self): import json as _json diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index b9f14406a..235c23837 100644 --- a/tests/unit/test_real_api_endpoints.py +++ b/tests/unit/test_real_api_endpoints.py @@ -441,16 +441,16 @@ def test_valid_batch_returns_200(self, client): ) assert response.status_code == 200 - def test_batch_with_more_than_20_videos_returns_error(self, client): - """Source code raises HTTPException(400) inside a try block that - re-wraps it as 500. Test matches actual behaviour.""" + def test_batch_with_more_than_20_videos_returns_400(self, client): + """The >20 batch limit is a client error: the handler re-raises the + intentional HTTPException(400) instead of masking it as a 500.""" urls = [f"https://youtube.com/watch?v=vid{i:05d}" for i in range(21)] response = client.post( "/api/v2/batch-process", json={"video_urls": urls, "max_concurrent": 3}, ) - # The HTTPException(400) is caught by the outer except -> HTTP 500 - assert response.status_code in (400, 500) + assert response.status_code == 400 + assert "Maximum 20 videos" in str(response.json()["detail"]) def test_batch_response_contains_results(self, client): response = client.post( @@ -801,11 +801,12 @@ def test_result_video_url_format(self, client): result = response.json()["results"][0] assert "youtube.com/watch?v=" in result["video_url"] - def test_max_results_above_50_returns_error(self, client): - """Source code raises HTTPException(400) inside a try block that - catches Exception -> results in HTTP 500.""" + def test_max_results_above_50_returns_400(self, client): + """The >50 results limit is a client error: the handler re-raises the + intentional HTTPException(400) instead of masking it as a 500.""" response = client.post("/api/v2/search-videos?query=python&max_results=51") - assert response.status_code in (400, 500) + assert response.status_code == 400 + assert "Maximum 50 results" in str(response.json()["detail"]) def test_default_order_is_relevance(self, client, mock_youtube): client.post("/api/v2/search-videos?query=test") From ec32df9d038644ff61ff7374d700621bc30ce1f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 02:42:00 +0000 Subject: [PATCH 5/7] test(security): make the 500 guard reject string-concatenation leaks via AST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit noted the detail predicate treated any value *starting* with a quote as safe, so `detail="Request failed: " + str(exc)` (a BinOp that begins with a literal but concatenates the exception) would pass. Parse the detail expression and accept only a single `ast.Constant` string; every other node — Call, BinOp, JoinedStr, Name, Dict — is dynamic. Self-check gains keyword and positional concatenation cases. No production leak existed (the code is already sanitized); this closes the guard's last false-negative. --- tests/unit/test_500_info_disclosure.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index 554ef9403..6913ae046 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -23,6 +23,7 @@ from __future__ import annotations +import ast import re from pathlib import Path @@ -95,15 +96,22 @@ def _split_top_level_commas(s: str) -> list[str]: def _detail_is_dynamic(value: str) -> bool: - """A detail value is safe only if it is an inline static string literal. + """A detail value is safe only if it is a single static string literal. - Anything else — ``str(e)``, an f-string, a bare variable, or a ``{...}`` dict — - can carry internal state to the client, so it is dynamic. + Parse the expression and accept *only* an ``ast.Constant`` string. Everything + else is dynamic: ``str(e)`` (Call), an f-string (JoinedStr), a bare variable + (Name), a ``{...}`` dict (Dict), and — the subtle one — string concatenation + such as ``"Request failed: " + str(exc)`` (BinOp), which *starts* with a + literal but still leaks the exception. """ - v = value.strip() + v = value.strip().rstrip(",").strip() if not v: return False - return not v.startswith(('"', "'")) + try: + node = ast.parse(v, mode="eval").body + except SyntaxError: + return True # unparseable → treat as suspicious rather than safe + return not (isinstance(node, ast.Constant) and isinstance(node.value, str)) # --- sink 1: HTTPException (keyword and positional) ------------------------ @@ -277,6 +285,8 @@ def test_guard_detects_a_synthetic_leak() -> None: 'raise HTTPException(status_code=500, detail={"message": error_msg})', # dict "raise HTTPException(500, str(e))", # positional detail 'raise HTTPException(500, f"boom: {e}")', # positional f-string + 'raise HTTPException(status_code=500, detail="Request failed: " + str(exc))', # concat + 'raise HTTPException(500, "boom: " + str(e))', # positional concat ] for leak in leaks: assert list(_iter_500_dynamic_detail(leak)), f"scanner missed a real 500 leak: {leak}" From 3f6f496b648afe200f8e217a2315de992a46bf00 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 02:53:18 +0000 Subject: [PATCH 6/7] test(security): assert sanitized 500 body in ml_serve checkpoint failure The Ray-Serve checkpoint POST path now returns a static {"error": "Internal server error"} on failure instead of leaking the exception text (CWE-209). Update the stale test that still asserted the leaked "Save failed" message, and add an assertion that the internal message does not appear anywhere in the response body. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01C4zt7nGZZZxdd1gGTeY2s8 --- tests/unit/test_ml_serve.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_ml_serve.py b/tests/unit/test_ml_serve.py index 017b0a1bb..41bbc4902 100644 --- a/tests/unit/test_ml_serve.py +++ b/tests/unit/test_ml_serve.py @@ -176,7 +176,10 @@ async def test_checkpoint_post_failure(mock_save, router): response = await router(request) assert response.status_code == 500 body = json.loads(response.body) - assert body["error"] == "Save failed" + # The internal exception text ("Save failed") must NOT leak to the client + # (CWE-209); the 500 body is a static, sanitized message. + assert body["error"] == "Internal server error" + assert "Save failed" not in json.dumps(body) @pytest.mark.asyncio @patch("uvai.ml.serve.load_checkpoint", return_value={"scorer_state": {}, "ranker_state": {}}) From 2d027d85cc9fc139b0103ad49f3df0a23754fcf4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 04:08:45 +0000 Subject: [PATCH 7/7] test(security): rewrite 500-leak guard as AST analysis (addresses review) Addresses four review findings on the regression guard, replacing the regex/textual scan with a real ast walk: - Scope: scan the whole deployed package src/youtube_extension (production entry point youtube_extension.main:app lives outside backend/) plus src/uvai/ml, not just backend/. - Status: recognize 500 as the literal or the FastAPI constant status.HTTP_500_INTERNAL_SERVER_ERROR, in keyword or positional form, whitespace-insensitively (status_code = 500). - Exception variable: derive it from the enclosing scope (except ... as NAME and @app.exception_handler function parameter) so an arbitrary name (error, problem) is tracked, not just e/exc. - Indirection: intra-function taint propagation catches leaks reached through a local (msg = str(exc); {"error": msg}); non-exception dynamic values (correlation ids) are still allowed in 500 bodies. Adds synthetic cases for each. Full-tree scan confirms no live leaks. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01C4zt7nGZZZxdd1gGTeY2s8 --- tests/unit/test_500_info_disclosure.py | 545 ++++++++++++++----------- 1 file changed, 310 insertions(+), 235 deletions(-) diff --git a/tests/unit/test_500_info_disclosure.py b/tests/unit/test_500_info_disclosure.py index 6913ae046..f825eeaaa 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -7,235 +7,262 @@ not all handlers. This test encodes the invariant directly on the source: a 500 response must use -a *static* ``detail`` string, never one derived from the caught exception. It is -hermetic (pure source scan, no app import / no pydantic) so it runs anywhere and -catches new leaks in any backend route. +a *static* message, never one derived from the caught exception. It is hermetic +(pure source scan via ``ast``, no app import / no pydantic) so it runs anywhere +and catches new leaks in any route. + +It uses a real **AST** analysis rather than a textual/regex scan, so idiomatic +variations cannot silently disable it: + + * status is recognized as the literal ``500`` **or** the FastAPI constant + ``status.HTTP_500_INTERNAL_SERVER_ERROR``, in keyword or positional form, + regardless of whitespace (``status_code = 500``); + * the caught-exception variable is derived from the enclosing scope — an + ``except ... as `` target or an ``@app.exception_handler`` function's + exception parameter — so an arbitrary name (``error``, ``problem``) is + tracked, not just ``e``/``exc``; + * leaks reached through a local variable (``msg = str(exc); {"error": msg}``) + are caught by intra-function taint propagation, not only direct ``str(exc)`` + text in the call. It models the three distinct 500 sinks in this codebase: - 1. ``HTTPException`` — both keyword (``status_code=500, detail=...``) and - positional (``HTTPException(500, str(e))``) forms. + 1. ``HTTPException`` — keyword (``status_code=500, detail=...``) and positional + (``HTTPException(500, str(e))``) forms. A 500 ``detail`` must be a static + string literal. 2. FastAPI ``@app.exception_handler`` functions that build a 500 body directly. 3. A raw ``JSONResponse(..., status_code=500)`` (e.g. a Ray Serve deployment). +For (2) and (3) the body may legitimately contain non-exception dynamic values +(a correlation id, a UUID), so those sinks flag only values that are *derived +from the caught exception*, via taint tracking — not every dynamic value. + It deliberately does not constrain 4xx responses: those echo client-supplied validation errors, which are not internal-disclosure vectors. + +Scope: the whole deployed package ``src/youtube_extension`` (the production +entry point is ``youtube_extension.main:app`` per the Dockerfile, which lives +outside ``backend/``) plus the ``src/uvai/ml`` serving surface. """ from __future__ import annotations import ast -import re from pathlib import Path import pytest _REPO_ROOT = Path(__file__).resolve().parents[2] -_BACKEND = _REPO_ROOT / "src" / "youtube_extension" / "backend" -# Additional client-facing 500 surfaces outside the FastAPI backend package. -_EXTRA_ROOTS = [_REPO_ROOT / "src" / "uvai" / "ml"] - - -def _backend_python_files() -> list[Path]: - files = list(_BACKEND.rglob("*.py")) - for root in _EXTRA_ROOTS: +# Scan the whole deployed package, not just backend/: the production entry point +# youtube_extension.main:app lives at src/youtube_extension/main.py, outside +# backend/. Also scan the uvai ML serving surface (raw JSONResponse 500s). +_ROOTS = [ + _REPO_ROOT / "src" / "youtube_extension", + _REPO_ROOT / "src" / "uvai" / "ml", +] + +# Fallback names treated as references to a caught exception when no structural +# binding is visible (e.g. a helper that takes the exception as a plain param: +# ``def to_500(exc): return JSONResponse({"error": str(exc)}, status_code=500)``). +# Names provably bound to a constant string in scope are excluded, so a benign +# ``error_message = "Internal server error"`` is not mistaken for a leak. +_EXC_TOKENS = {"e", "ex", "exc", "err", "error", "error_msg", "error_message", "exception"} + + +def _python_files() -> list[Path]: + files: list[Path] = [] + for root in _ROOTS: if root.exists(): files.extend(root.rglob("*.py")) return sorted(set(files)) -# --- shared parsing helpers ------------------------------------------------ - -def _balanced_call_args(text: str, open_paren_idx: int) -> str: - """Return the argument text of a call, balancing nested parens/brackets/braces.""" - depth = 0 - quote: str | None = None - for i in range(open_paren_idx, len(text)): - c = text[i] - if quote: - if c == quote: - quote = None - continue - if c in "\"'": - quote = c - elif c in "([{": - depth += 1 - elif c in ")]}": - depth -= 1 - if depth == 0: - return text[open_paren_idx + 1 : i] - return text[open_paren_idx + 1 :] - - -def _split_top_level_commas(s: str) -> list[str]: - """Split on commas that are not nested inside parens/brackets/braces/strings.""" - parts: list[str] = [] - depth = 0 - quote: str | None = None - buf: list[str] = [] - for c in s: - if quote: - buf.append(c) - if c == quote: - quote = None - elif c in "\"'": - quote = c - buf.append(c) - elif c in "([{": - depth += 1 - buf.append(c) - elif c in ")]}": - depth -= 1 - buf.append(c) - elif c == "," and depth == 0: - parts.append("".join(buf)) - buf = [] - else: - buf.append(c) - parts.append("".join(buf)) - return parts - - -def _detail_is_dynamic(value: str) -> bool: - """A detail value is safe only if it is a single static string literal. - - Parse the expression and accept *only* an ``ast.Constant`` string. Everything - else is dynamic: ``str(e)`` (Call), an f-string (JoinedStr), a bare variable - (Name), a ``{...}`` dict (Dict), and — the subtle one — string concatenation - such as ``"Request failed: " + str(exc)`` (BinOp), which *starts* with a - literal but still leaks the exception. - """ - v = value.strip().rstrip(",").strip() - if not v: +# --- AST helpers ----------------------------------------------------------- + +def _call_name(call: ast.Call) -> str: + func = call.func + if isinstance(func, ast.Name): + return func.id + if isinstance(func, ast.Attribute): + return func.attr + return "" + + +def _keyword(call: ast.Call, name: str) -> ast.expr | None: + for kw in call.keywords: + if kw.arg == name: + return kw.value + return None + + +def _is_500(expr: ast.expr | None) -> bool: + """True if an expression denotes HTTP status 500 (literal or FastAPI constant).""" + if isinstance(expr, ast.Constant) and expr.value == 500: + return True + # status.HTTP_500_INTERNAL_SERVER_ERROR / http.HTTPStatus.INTERNAL_SERVER_ERROR-style + if isinstance(expr, ast.Attribute) and ( + expr.attr.startswith("HTTP_500") or expr.attr == "INTERNAL_SERVER_ERROR" + ): + return True + return False + + +def _is_static_str(expr: ast.expr | None) -> bool: + return isinstance(expr, ast.Constant) and isinstance(expr.value, str) + + +def _seg(text: str, node: ast.AST) -> str: + seg = ast.get_source_segment(text, node) + if not seg: + return "<...>" + return " ".join(seg.split())[:120] + + +# --- HTTPException status/detail ------------------------------------------- + +def _http_exc_is_500(call: ast.Call) -> bool: + sc = _keyword(call, "status_code") + if sc is not None: + return _is_500(sc) + # positional: HTTPException(status_code, detail, ...) + return bool(call.args) and _is_500(call.args[0]) + + +def _http_exc_detail(call: ast.Call) -> ast.expr | None: + detail = _keyword(call, "detail") + if detail is not None: + return detail + if len(call.args) >= 2: # positional detail is the 2nd argument + return call.args[1] + return None + + +# --- JSONResponse status/content ------------------------------------------- + +def _json_is_500(call: ast.Call) -> bool: + return _is_500(_keyword(call, "status_code")) + + +def _json_content(call: ast.Call) -> ast.expr | None: + content = _keyword(call, "content") + if content is not None: + return content + return call.args[0] if call.args else None + + +# --- taint: which names carry the caught exception ------------------------- + +def _is_handler(func: ast.AST) -> bool: + for dec in getattr(func, "decorator_list", []): + target = dec.func if isinstance(dec, ast.Call) else dec + if isinstance(target, ast.Attribute) and target.attr == "exception_handler": + return True + if isinstance(target, ast.Name) and target.id == "exception_handler": + return True + return False + + +def _constant_str_names(func: ast.AST) -> set[str]: + """Names provably assigned a constant string somewhere in this function.""" + names: set[str] = set() + for node in ast.walk(func): + if isinstance(node, ast.Assign) and _is_static_str(node.value): + for tgt in node.targets: + if isinstance(tgt, ast.Name): + names.add(tgt.id) + elif ( + isinstance(node, ast.AnnAssign) + and node.value is not None + and _is_static_str(node.value) + and isinstance(node.target, ast.Name) + ): + names.add(node.target.id) + return names + + +def _function_taint(func: ast.AST) -> set[str]: + """Set of local names that carry (are derived from) the caught exception.""" + const_names = _constant_str_names(func) + taint: set[str] = set(_EXC_TOKENS) - const_names + + # structural seeds: exception-handler param and `except ... as name` + if _is_handler(func): + params = getattr(getattr(func, "args", None), "args", []) + if len(params) >= 2: # FastAPI passes (request, exc) + taint.add(params[1].arg) + for node in ast.walk(func): + if isinstance(node, ast.ExceptHandler) and node.name: + taint.add(node.name) + + # intra-function propagation: x = -> x tainted + changed = True + while changed: + changed = False + for node in ast.walk(func): + if isinstance(node, ast.Assign) and _expr_uses(node.value, taint): + for tgt in node.targets: + if ( + isinstance(tgt, ast.Name) + and tgt.id not in taint + and tgt.id not in const_names + ): + taint.add(tgt.id) + changed = True + return taint + + +def _expr_uses(expr: ast.expr | None, taint: set[str]) -> bool: + """True if the expression references any tainted (exception-derived) name.""" + if expr is None: return False + for node in ast.walk(expr): + if isinstance(node, ast.Name) and node.id in taint: + return True + return False + + +# --- unified scan ---------------------------------------------------------- + +def _scan(text: str): + """Return (http_leaks, json_leaks, handler_leaks) as lists of (line, snippet).""" + http_leaks: list[tuple[int, str]] = [] + json_leaks: list[tuple[int, str]] = [] + handler_leaks: list[tuple[int, str]] = [] try: - node = ast.parse(v, mode="eval").body + tree = ast.parse(text) except SyntaxError: - return True # unparseable → treat as suspicious rather than safe - return not (isinstance(node, ast.Constant) and isinstance(node.value, str)) - - -# --- sink 1: HTTPException (keyword and positional) ------------------------ - -_HTTP_EXC_OPEN = re.compile(r"HTTPException\(") - - -def _iter_500_dynamic_detail(text: str): - """Yield (line_no, snippet) for each 500 HTTPException with a dynamic detail.""" - for m in _HTTP_EXC_OPEN.finditer(text): - args = _balanced_call_args(text, m.end() - 1) - parts = _split_top_level_commas(args) - nospace = re.sub(r"\s+", "", args) - first = parts[0].strip() if parts else "" - is_500 = "status_code=500" in nospace or first == "500" - if not is_500: - continue - - detail_val: str | None = None - for p in parts: - ps = p.strip() - if ps.startswith("detail="): - detail_val = ps[len("detail=") :] - break - if detail_val is None and len(parts) >= 2: - # positional detail is the 2nd argument, unless it is a keyword arg. - second = parts[1] - if "=" not in second.split("(", 1)[0]: - detail_val = second - - if detail_val is not None and _detail_is_dynamic(detail_val): - line_no = text.count("\n", 0, m.start()) + 1 - yield line_no, " ".join(args.split())[:120] - - -# --- sink 2: FastAPI exception handlers ------------------------------------ -# -# Exception handlers build a response body directly (dict / JSONResponse) rather -# than raising. A handler must not place the exception message (`str(exc)`) or its -# class name (`exc.__class__.__name__`) into that body. Logging the exception -# server-side is fine; those tokens never appear on a `logger.`/`log`/`raise`/ -# comment line, so we exclude those lines to avoid false positives. -_HANDLER_DECORATOR = re.compile(r"^\s*@\w+\.exception_handler\(", re.MULTILINE) -_HANDLER_DISCLOSURE = re.compile(r"str\(\s*(?:exc|e)\s*\)|__class__\.__name__") -# Triple-quoted docstrings, so prose that *mentions* str(exc) is not mistaken for code. -_TRIPLE_STR = re.compile(r'"""(?:.|\n)*?"""|\'\'\'(?:.|\n)*?\'\'\'') - - -def _strip_docstrings(body: str) -> str: - """Blank triple-quoted blocks, preserving line count so line numbers stay aligned.""" - return _TRIPLE_STR.sub(lambda m: "\n" * m.group(0).count("\n"), body) - - -def _exception_handler_bodies(text: str): - """Yield (start_line, body_text) for each @.exception_handler function.""" - lines = text.splitlines(keepends=True) - for m in _HANDLER_DECORATOR.finditer(text): - start_line = text.count("\n", 0, m.start()) - i = start_line - while i < len(lines) and not lines[i].lstrip().startswith(("def ", "async def ")): - i += 1 - if i >= len(lines): - continue - def_indent = len(lines[i]) - len(lines[i].lstrip()) - j = i + 1 - body: list[str] = [] - while j < len(lines): - line = lines[j] - stripped = line.strip() - if stripped and (len(line) - len(line.lstrip())) <= def_indent: - break # dedented back to <= the def's level: end of function - body.append(line) - j += 1 - yield i + 1, "".join(body) - - -def _iter_handler_disclosures(text: str): - """Yield (line_no, snippet) for exception-handler bodies that leak exc into the response.""" - for start_line, body in _exception_handler_bodies(text): - if "status_code=500" not in re.sub(r"\s+", "", body): - continue - for offset, line in enumerate(_strip_docstrings(body).splitlines()): - code = line.split("#", 1)[0] # ignore inline comments - bare = line.strip() - if bare.startswith(("logger", "log", "self.logger", "raise")): + return http_leaks, json_leaks, handler_leaks + + module_taint = set(_EXC_TOKENS) - _constant_str_names(tree) + + def visit(node: ast.AST, taint: set[str], in_handler: bool) -> None: + for child in ast.iter_child_nodes(node): + if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)): + visit(child, taint | _function_taint(child), in_handler or _is_handler(child)) continue - if _HANDLER_DISCLOSURE.search(code): - yield start_line + offset, bare[:120] - - -# --- sink 3: raw JSONResponse 500s ----------------------------------------- -# -# A ``JSONResponse(..., status_code=500)`` body must not embed the exception -# (``str(exc)`` / ``str(e)`` / ``str(error)``) or an f-string interpolating one. -_JSON_RESPONSE = re.compile(r"JSONResponse\(") -# Exception-like variable names, so an f-string interpolating a UUID / error-id -# (e.g. f"FALLBACK_{uuid.uuid4()...}") is not mistaken for an exception leak. -_EXC_TOKENS = r"(?:e|ex|exc|err|error|error_msg|error_message|exception)" -_JSON_DISCLOSURE = re.compile( - r"str\(\s*(?:exc|e|error)\s*\)" # str(exc) / str(e) / str(error) - r"|f[\"'][^\"']*\{[^{}]*\b" + _EXC_TOKENS + r"\b[^{}]*\}" # f"...{exc-ref}..." -) - - -def _iter_json_500_disclosures(text: str): - """Yield (line_no, snippet) for JSONResponse 500s whose body embeds the exception.""" - for m in _JSON_RESPONSE.finditer(text): - args = _balanced_call_args(text, m.end() - 1) - if "status_code=500" not in re.sub(r"\s+", "", args): - continue - if _JSON_DISCLOSURE.search(args): - line_no = text.count("\n", 0, m.start()) + 1 - yield line_no, " ".join(args.split())[:120] + if isinstance(child, ast.Call): + name = _call_name(child) + if name == "HTTPException" and _http_exc_is_500(child): + detail = _http_exc_detail(child) + if detail is not None and not _is_static_str(detail): + http_leaks.append((child.lineno, _seg(text, child))) + elif name == "JSONResponse" and _json_is_500(child): + if _expr_uses(_json_content(child), taint): + target = handler_leaks if in_handler else json_leaks + target.append((child.lineno, _seg(text, child))) + visit(child, taint, in_handler) + + visit(tree, module_taint, False) + return http_leaks, json_leaks, handler_leaks # --- the guards ------------------------------------------------------------ def test_no_dynamic_detail_in_500_responses() -> None: offenders: list[str] = [] - for path in _backend_python_files(): - text = path.read_text(encoding="utf-8") - for line_no, snippet in _iter_500_dynamic_detail(text): - rel = path.relative_to(_REPO_ROOT) - offenders.append(f"{rel}:{line_no}: HTTPException({snippet})") + for path in _python_files(): + http_leaks, _, _ = _scan(path.read_text(encoding="utf-8")) + rel = path.relative_to(_REPO_ROOT) + offenders.extend(f"{rel}:{ln}: HTTPException({snip})" for ln, snip in http_leaks) assert not offenders, ( "HTTP 500 responses must use a static `detail` string (e.g. " @@ -247,37 +274,38 @@ def test_no_dynamic_detail_in_500_responses() -> None: def test_no_disclosure_in_500_exception_handlers() -> None: offenders: list[str] = [] - for path in _backend_python_files(): - text = path.read_text(encoding="utf-8") - for line_no, snippet in _iter_handler_disclosures(text): - rel = path.relative_to(_REPO_ROOT) - offenders.append(f"{rel}:{line_no}: {snippet}") + for path in _python_files(): + _, _, handler_leaks = _scan(path.read_text(encoding="utf-8")) + rel = path.relative_to(_REPO_ROOT) + offenders.extend(f"{rel}:{ln}: {snip}" for ln, snip in handler_leaks) assert not offenders, ( "A FastAPI exception handler that returns HTTP 500 must not place the " - "exception message (`str(exc)`) or its class name (`__class__.__name__`) " - "into the response body — log it server-side instead. Offending sites:\n " - + "\n ".join(offenders) + "caught exception (its message or class name, directly or via a local " + "variable) into the response body — log it server-side instead. " + "Offending sites:\n " + "\n ".join(offenders) ) def test_no_disclosure_in_json_500_responses() -> None: offenders: list[str] = [] - for path in _backend_python_files(): - text = path.read_text(encoding="utf-8") - for line_no, snippet in _iter_json_500_disclosures(text): - rel = path.relative_to(_REPO_ROOT) - offenders.append(f"{rel}:{line_no}: JSONResponse({snippet})") + for path in _python_files(): + _, json_leaks, _ = _scan(path.read_text(encoding="utf-8")) + rel = path.relative_to(_REPO_ROOT) + offenders.extend(f"{rel}:{ln}: JSONResponse({snip})" for ln, snip in json_leaks) assert not offenders, ( "A raw JSONResponse with status_code=500 must not embed the caught " - "exception in its body — return a static message and log the error " - "server-side instead. Offending sites:\n " + "\n ".join(offenders) + "exception in its body (directly or through a local variable) — return a " + "static message and log the error server-side instead. Offending sites:\n " + + "\n ".join(offenders) ) +# --- self-tests: the guard actually bites ---------------------------------- + def test_guard_detects_a_synthetic_leak() -> None: - """The HTTPException scanner flags every dynamic-detail shape, keyword and positional.""" + """The HTTPException scanner flags every dynamic-detail shape and idiom.""" leaks = [ "raise HTTPException(status_code=500, detail=str(e))", 'raise HTTPException(status_code=500, detail=f"failed: {e}")', @@ -287,17 +315,23 @@ def test_guard_detects_a_synthetic_leak() -> None: 'raise HTTPException(500, f"boom: {e}")', # positional f-string 'raise HTTPException(status_code=500, detail="Request failed: " + str(exc))', # concat 'raise HTTPException(500, "boom: " + str(e))', # positional concat + # AST-only wins over the old regex scan: + "raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(exc))", + "raise HTTPException(status_code = 500, detail = str(exc))", # whitespace ] for leak in leaks: - assert list(_iter_500_dynamic_detail(leak)), f"scanner missed a real 500 leak: {leak}" + http, _, _ = _scan(leak) + assert http, f"scanner missed a real 500 leak: {leak}" # Static string literals are the only safe form — keyword or positional. safe = [ 'raise HTTPException(status_code=500, detail="Internal server error")', 'raise HTTPException(500, "Internal server error")', + "raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=\"Internal server error\")", ] for s in safe: - assert not list(_iter_500_dynamic_detail(s)), f"scanner false-positived: {s}" + http, _, _ = _scan(s) + assert not http, f"scanner false-positived: {s}" # 4xx responses echo client-supplied input and are intentionally out of scope. client_errs = [ @@ -305,19 +339,23 @@ def test_guard_detects_a_synthetic_leak() -> None: "raise HTTPException(400, str(exc))", ] for c in client_errs: - assert not list(_iter_500_dynamic_detail(c)), f"scanner must ignore 4xx: {c}" + http, _, _ = _scan(c) + assert not http, f"scanner must ignore 4xx: {c}" def test_guard_detects_a_synthetic_handler_leak() -> None: - """The exception-handler scanner flags exc message/class-name disclosure in a 500 body.""" + """The handler scanner derives the exception param name from the signature.""" + # Exception parameter is named `problem` (not e/exc): structural derivation, + # not a hardcoded name list, must catch it. leaky = ( "@app.exception_handler(Exception)\n" - "async def h(request, exc):\n" - ' logger.error(f"boom: {exc}", exc_info=True)\n' - ' body = {"detail": str(exc), "error_type": exc.__class__.__name__}\n' + "async def h(request, problem):\n" + ' logger.error(f"boom: {problem}", exc_info=True)\n' + ' body = {"detail": str(problem), "error_type": problem.__class__.__name__}\n' " return JSONResponse(status_code=500, content=body)\n" ) - assert list(_iter_handler_disclosures(leaky)), "handler scanner missed a real leak" + _, _, handler = _scan(leaky) + assert handler, "handler scanner missed a real leak reached via the handler param" safe = ( "@app.exception_handler(Exception)\n" @@ -326,38 +364,75 @@ def test_guard_detects_a_synthetic_handler_leak() -> None: ' body = {"detail": "Internal server error"}\n' " return JSONResponse(status_code=500, content=body)\n" ) - assert not list( - _iter_handler_disclosures(safe) - ), "handler scanner false-positived a sanitized 500 handler" + _, _, handler = _scan(safe) + assert not handler, "handler scanner false-positived a sanitized 500 handler" client_err = ( "@app.exception_handler(ValueError)\n" "async def h(request, exc):\n" ' return JSONResponse(status_code=400, content={"detail": str(exc)})\n' ) - assert not list( - _iter_handler_disclosures(client_err) - ), "handler scanner must ignore 4xx handlers" + _, _, handler = _scan(client_err) + assert not handler, "handler scanner must ignore 4xx handlers" def test_guard_detects_a_synthetic_json_500_leak() -> None: - """The JSONResponse scanner flags exception disclosure across nested braces.""" + """The JSONResponse scanner flags direct and variable-indirection leaks.""" leaks = [ 'return JSONResponse({"error": str(exc)}, status_code=500)', 'JSONResponse(content={"m": f"failed: {e}"}, status_code=500)', 'JSONResponse({"error": str(error)}, status_code=500)', ] for leak in leaks: - assert list(_iter_json_500_disclosures(leak)), f"json scanner missed: {leak}" + # wrap in a function with a bound exception so this reflects real code + src = ( + "def f():\n" + " try:\n" + " pass\n" + " except Exception as e:\n" + f" {leak}\n" + ) + _, json_leaks, _ = _scan(src) + assert json_leaks, f"json scanner missed: {leak}" + + # Variable indirection: msg is not literally str(exc) at the call site. + indirection = ( + "def f():\n" + " try:\n" + " pass\n" + " except Exception as boom:\n" + " error_detail = str(boom)\n" + ' return JSONResponse({"error": error_detail}, status_code=500)\n' + ) + _, json_leaks, _ = _scan(indirection) + assert json_leaks, "json scanner missed a variable-indirection leak (msg = str(exc))" + + # A non-exception dynamic value (correlation id) in a 500 body is allowed. + allowed = ( + "def f():\n" + " correlation_id = new_id()\n" + ' return JSONResponse({"error": "Internal server error", "id": correlation_id}, status_code=500)\n' + ) + _, json_leaks, _ = _scan(allowed) + assert not json_leaks, "json scanner false-positived a non-exception dynamic value" - safe = 'JSONResponse({"error": "Internal server error"}, status_code=500)' - assert not list(_iter_json_500_disclosures(safe)), "json scanner false-positived" + safe = ( + "def f():\n" + ' return JSONResponse({"error": "Internal server error"}, status_code=500)\n' + ) + _, json_leaks, _ = _scan(safe) + assert not json_leaks, "json scanner false-positived a static 500 body" # 4xx JSONResponses may echo client input. - client_err = 'JSONResponse({"detail": str(exc)}, status_code=400)' - assert not list( - _iter_json_500_disclosures(client_err) - ), "json scanner must ignore 4xx" + client_err = ( + "def f():\n" + " try:\n" + " pass\n" + " except Exception as exc:\n" + ' return JSONResponse({"detail": str(exc)}, status_code=400)\n' + ) + _, json_leaks, _ = _scan(client_err) + assert not json_leaks, "json scanner must ignore 4xx" if __name__ == "__main__": # pragma: no cover