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_ai_routes.py b/src/youtube_extension/backend/cloud_ai_routes.py index a1242a418..a470cabe6 100644 --- a/src/youtube_extension/backend/cloud_ai_routes.py +++ b/src/youtube_extension/backend/cloud_ai_routes.py @@ -228,8 +228,8 @@ 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)}") + logger.error(f"Failed to get provider status: {e}", exc_info=True) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/analyze/video", response_model=VideoAnalysisResponse) @@ -262,13 +262,13 @@ 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}") - raise HTTPException(status_code=500, detail=f"Configuration error: {str(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}") - raise HTTPException(status_code=500, detail=f"Analysis failed: {str(e)}") + logger.error(f"Unexpected error during video analysis: {e}", exc_info=True) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/analyze/batch") @@ -300,8 +300,8 @@ 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}") - raise HTTPException(status_code=500, detail=f"Batch analysis failed: {str(e)}") + logger.error(f"Failed to start batch analysis: {e}", exc_info=True) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/analyze/multi-provider", response_model=list[VideoAnalysisResponse]) @@ -324,8 +324,8 @@ async def analyze_video_multi_provider(request: VideoAnalysisRequest): return formatted_results 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)}") + logger.error(f"Multi-provider analysis failed: {e}", exc_info=True) + 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..13bd33e8b 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,20 +208,23 @@ 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: 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}") + logger.error(f"Failed to update error state: {state_error}", exc_info=True) - 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): @@ -260,9 +255,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 +289,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 +327,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 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 03fd9e4e6..65c8b9fe6 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): @@ -150,9 +142,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") @@ -176,10 +169,14 @@ 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( status_code=500, - detail=f"Batch processing failed: {str(e)}" + detail="Internal server error" ) @app.get("/api/v2/videos/list") @@ -252,9 +249,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 +382,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") @@ -431,10 +430,14 @@ 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( 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..f825eeaaa --- /dev/null +++ b/tests/unit/test_500_info_disclosure.py @@ -0,0 +1,439 @@ +"""Regression guard against information disclosure in HTTP 500 responses. + +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 +not all handlers. + +This test encodes the invariant directly on the source: a 500 response must use +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`` — 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 +from pathlib import Path + +import pytest + +_REPO_ROOT = Path(__file__).resolve().parents[2] +# 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)) + + +# --- 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: + tree = ast.parse(text) + except SyntaxError: + 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 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 _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. " + '"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) + ) + + +def test_no_disclosure_in_500_exception_handlers() -> None: + offenders: list[str] = [] + 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 " + "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 _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 (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 and idiom.""" + 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 + '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: + 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: + 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 = [ + "raise HTTPException(status_code=400, detail=str(exc))", + "raise HTTPException(400, str(exc))", + ] + for c in client_errs: + http, _, _ = _scan(c) + assert not http, f"scanner must ignore 4xx: {c}" + + +def test_guard_detects_a_synthetic_handler_leak() -> None: + """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, 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" + ) + _, _, handler = _scan(leaky) + assert handler, "handler scanner missed a real leak reached via the handler param" + + 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" + ) + _, _, 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' + ) + _, _, 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 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: + # 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 = ( + "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 = ( + "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 + raise SystemExit(pytest.main([__file__, "-v"])) 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_cloud_routes.py b/tests/unit/test_cloud_routes.py index 4c3b29839..4c415a3ed 100644 --- a/tests/unit/test_cloud_routes.py +++ b/tests/unit/test_cloud_routes.py @@ -1316,7 +1316,7 @@ def test_generate_dashboard_url_service_error(self): } ) assert response.status_code == 500 - assert "Internal server error" in response.json()["detail"] + assert response.json()["detail"] == "Internal server error" def test_generate_dashboard_url_missing_fields(self): """Missing required fields return 422.""" 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": {}}) diff --git a/tests/unit/test_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index 4eb7109c6..235c23837 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={}) @@ -436,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( @@ -796,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")