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 b7fbc96df..49804aba1 100644 --- a/src/youtube_extension/backend/cloud_ai_routes.py +++ b/src/youtube_extension/backend/cloud_ai_routes.py @@ -256,17 +256,18 @@ async def analyze_video(request: VideoAnalysisRequest): return format_analysis_result(result) except RateLimitError as e: - # RateLimitError/ConfigurationError subclass CloudAIError, so they must be - # caught BEFORE the base handler or they are dead code (returning the - # dynamic 503 and leaking config detail — CWE-209). + # Must precede CloudAIError: RateLimitError subclasses it, so catching + # the base first would shadow this handler and return a 503 instead. logger.warning(f"Rate limit exceeded: {e}") raise HTTPException(status_code=429, detail=f"Rate limit exceeded: {str(e)}") except ConfigurationError as e: + # Must precede CloudAIError (same subclassing reason) so configuration + # failures reach this sanitized 500 rather than the dynamic 503 below. logger.error(f"Configuration error: {e}", exc_info=True) raise HTTPException(status_code=500, detail="Internal server error") except CloudAIError as e: - logger.error(f"Cloud AI analysis failed: {e}") - raise HTTPException(status_code=503, detail=f"AI analysis failed: {str(e)}") + logger.error(f"Cloud AI analysis failed: {e}", exc_info=True) + raise HTTPException(status_code=503, detail="AI service temporarily unavailable. Please retry in a few moments.") except HTTPException: raise except Exception as e: @@ -327,8 +328,6 @@ async def analyze_video_multi_provider(request: VideoAnalysisRequest): return formatted_results except HTTPException: - # parse_analysis_types() raises HTTPException(400, ...); preserve the 4xx - # contract instead of collapsing it into a 500 (matches the other handlers). raise except Exception as e: logger.error(f"Multi-provider analysis failed: {e}", exc_info=True) diff --git a/src/youtube_extension/backend/cloud_api_endpoints.py b/src/youtube_extension/backend/cloud_api_endpoints.py index af3088dd2..cdf2844e2 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -142,8 +142,10 @@ async def process_video_cloud( ) except Exception as e: - # Exception text is logged server-side only; never returned to the client. - logger.error(f"Cloud processing failed: {e}", exc_info=True) + error_msg = f"Cloud processing failed: {str(e)}" + logger.error(error_msg, exc_info=True) + + # detail is 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/process-video-task") @@ -206,21 +208,18 @@ async def process_video_task_handler( } except Exception as e: - # Exception text is logged server-side only. It must NOT be persisted to - # task state: get_video_status / get_video_result echo error_message back - # to clients, so a raw str(e) there would re-expose internal detail (CWE-209). logger.error(f"Task processing failed: {e}", exc_info=True) - # Update state with a user-safe message + # Update state with a static error message; raw exception is logged above only try: firestore_service = await get_firestore_service() await firestore_service.update_state( payload.video_id, status='failed', - error_message="Internal server error" + error_message="Task processing failed" ) except Exception as state_error: - logger.error(f"Failed to update error state: {state_error}", exc_info=True) + logger.error(f"Failed to update error state: {state_error}") 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 0984976e6..98ce7f49c 100644 --- a/tests/unit/test_500_info_disclosure.py +++ b/tests/unit/test_500_info_disclosure.py @@ -1,27 +1,36 @@ """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 / dict -embedding the exception), leaking internal exception text — stack-adjacent -messages, backend API errors, database errors — to clients (CWE-209). See PR -#801, which sanitized most but not all handlers. - -This test encodes the invariant directly on the source of the routers this PR -hardens: a 500 response must use a *static* string ``detail``, never one derived -from the caught exception or any runtime value. The scan is AST-based, so it -catches every call form — positional ``HTTPException(500, str(e))`` and keyword -``HTTPException(status_code=500, detail=...)`` alike, and flags ``str(...)``, -f-strings, dicts, and bare variables such as ``detail=error_msg``. It is hermetic -(no app import / no pydantic) so it runs anywhere. - -Scope note: ``_GUARDED_FILES`` is deliberately limited to the routers sanitized -here. Other backend modules still carry legacy dynamic 500 details (e.g. -``api/advanced_video_routes.py``); hardening those is tracked separately. Add a -router to ``_GUARDED_FILES`` once it has been sanitized to bring it under guard — -that is the intended way to widen coverage. +Context: several FastAPI handlers historically returned internal exception text +to clients via 500 responses (CWE-209) — through ``HTTPException`` details and +through ``JSONResponse`` bodies raised by global/middleware exception handlers. +Leaked shapes seen in this codebase: + +* ``raise HTTPException(status_code=500, detail=str(e))`` +* ``raise HTTPException(status_code=500, detail=f"... {e} ...")`` +* ``raise HTTPException(status_code=500, detail=error_msg)`` (variable) +* ``raise HTTPException(status_code=500, detail={"message": error_msg})`` (dict) +* ``JSONResponse(status_code=500, content={"detail": str(exc), + "path": str(request.url), "error_type": exc.__class__.__name__})`` + +This test encodes the invariant directly on the source (a pure AST scan — no app +import, no pydantic) so it runs anywhere and catches new leaks in *any* backend +route, global handler, or middleware, not only the ones fixed today. It covers +both 500 response constructors: ``HTTPException`` and ``JSONResponse``. + +Rules: + +* ``HTTPException(status_code=500, ...)``: ``detail`` must be an inline static + string literal. Anything else — ``str(...)``, an f-string, a bare variable, a + dict — is rejected (a 500 detail never legitimately needs to be computed). +* ``JSONResponse(status_code=500, ...)``: the ``content`` (or ``detail``) must + not reference the caught exception or the request. A ``str(exc)`` / ``repr``, + an f-string embedding the exception/request, a bare ``exc``/``e``/``error`` + name, or an attribute such as ``request.url`` / ``exc.__class__`` is rejected. + Safe dynamic values — a ``uuid4()`` error id, a ``datetime.now().isoformat()`` + timestamp — are intentionally allowed. It deliberately does not constrain 4xx responses: those echo client-supplied -validation errors, which are not internal-disclosure vectors. +validation input, which is not an internal-disclosure vector. """ from __future__ import annotations @@ -33,119 +42,142 @@ _BACKEND = Path(__file__).resolve().parents[2] / "src" / "youtube_extension" / "backend" -# Routers sanitized by this PR — the invariant is enforced exactly here. -_GUARDED_FILES = ( - "cloud_ai_routes.py", - "cloud_api_endpoints.py", - "real_api_endpoints.py", -) - - -def _is_httpexception(call: ast.Call) -> bool: - func = call.func - return (isinstance(func, ast.Name) and func.id == "HTTPException") or ( - isinstance(func, ast.Attribute) and func.attr == "HTTPException" - ) +# Identifiers that, when referenced inside a 500 body, indicate a leak of the +# caught exception or the inbound request. +_EXC_NAMES = {"e", "exc", "err", "error", "ex", "exception"} +_REQUEST_NAMES = {"request", "req"} +# Attributes that disclose internals when reached from an exception/request. +_LEAKY_ATTRS = {"url", "__class__", "args", "__cause__", "__context__"} + + +def _is_static_string(node: ast.AST) -> bool: + """True iff *node* is an inline string literal (optionally concatenated).""" + if isinstance(node, ast.Constant): + return isinstance(node.value, str) + # "a" "b" implicit concat / "a" + "b" + if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add): + return _is_static_string(node.left) and _is_static_string(node.right) + return False -def _is_500_status(node: ast.expr) -> bool: - """True if ``node`` denotes HTTP 500 as a literal (``500``) OR a symbolic - constant such as ``status.HTTP_500_INTERNAL_SERVER_ERROR`` or - ``HTTPStatus.INTERNAL_SERVER_ERROR`` — otherwise the guard could be bypassed - by the standard FastAPI symbolic form while still leaking a 500 detail.""" - if isinstance(node, ast.Constant) and node.value == 500: - return True - if isinstance(node, ast.Attribute) and node.attr in ( - "HTTP_500_INTERNAL_SERVER_ERROR", - "INTERNAL_SERVER_ERROR", - ): - return True +def _refs_exception_or_request(node: ast.AST) -> bool: + """True if *node*'s subtree reads the caught exception or the request.""" + for leaf in ast.walk(node): + if isinstance(leaf, ast.Name) and leaf.id in (_EXC_NAMES | _REQUEST_NAMES): + return True + if isinstance(leaf, ast.Attribute): + if leaf.attr in _LEAKY_ATTRS: + return True + base = leaf.value + if isinstance(base, ast.Name) and base.id in (_EXC_NAMES | _REQUEST_NAMES): + return True + if isinstance(leaf, ast.Call): + fn = leaf.func + if isinstance(fn, ast.Name) and fn.id in {"str", "repr", "format"}: + if any(_refs_exception_or_request(a) for a in leaf.args): + return True return False -def _is_500(call: ast.Call) -> bool: - # Positional form: HTTPException(500, ...) - if call.args and _is_500_status(call.args[0]): - return True - # Keyword form: HTTPException(status_code=500, ...) +def _status_is_500(call: ast.Call) -> bool: for kw in call.keywords: - if kw.arg == "status_code" and _is_500_status(kw.value): - return True + if kw.arg == "status_code" and isinstance(kw.value, ast.Constant): + return kw.value.value == 500 + # positional status_code (JSONResponse(500, ...) / HTTPException(500, ...)) + if call.args and isinstance(call.args[0], ast.Constant): + return call.args[0].value == 500 return False -def _detail_node(call: ast.Call) -> ast.expr | None: - # Keyword form: detail=... - for kw in call.keywords: - if kw.arg == "detail": - return kw.value - # Positional form: HTTPException(, ) - if len(call.args) >= 2: - return call.args[1] - return None +def _call_name(call: ast.Call) -> str | None: + fn = call.func + return getattr(fn, "id", None) or getattr(fn, "attr", None) -def _iter_500_dynamic_detail(text: str): - """Yield (line_no, snippet) for each 500 ``HTTPException`` whose ``detail`` is - anything other than a static string literal.""" +def _iter_500_leaks(text: str): + """Yield (line_no, reason) for each 500 response that can leak internals.""" tree = ast.parse(text) for node in ast.walk(tree): - if not isinstance(node, ast.Call) or not _is_httpexception(node): + if not isinstance(node, ast.Call): continue - if not _is_500(node): + name = _call_name(node) + if name not in ("HTTPException", "JSONResponse"): continue - detail = _detail_node(node) - if detail is None: - continue # no detail argument at all — nothing to leak - # A static string literal is the only safe form. - if isinstance(detail, ast.Constant) and isinstance(detail.value, str): + if not _status_is_500(node): continue - yield node.lineno, ast.unparse(node)[:160] - - -def test_no_dynamic_detail_in_500_responses() -> None: + # Check keyword arguments + for kw in node.keywords: + if name == "HTTPException" and kw.arg == "detail": + if not _is_static_string(kw.value): + yield node.lineno, "HTTPException 500 detail is not a static string" + elif name == "JSONResponse" and kw.arg in ("content", "detail"): + if _refs_exception_or_request(kw.value): + yield node.lineno, "JSONResponse 500 body references the exception/request" + # Check positional detail argument: HTTPException(status_code, detail) + # args[0] is status_code (already checked by _status_is_500); args[1] is detail. + if name == "HTTPException" and len(node.args) >= 2: + if not _is_static_string(node.args[1]): + yield node.lineno, "HTTPException 500 detail is not a static string" + + +def _backend_python_files() -> list[Path]: + return sorted(_BACKEND.rglob("*.py")) + + +def test_no_information_disclosure_in_500_responses() -> None: offenders: list[str] = [] - for name in _GUARDED_FILES: - path = _BACKEND / name - assert path.exists(), f"guarded file no longer exists: {name}" - for line_no, snippet in _iter_500_dynamic_detail(path.read_text(encoding="utf-8")): - offenders.append(f"{name}:{line_no}: {snippet}") + for path in _backend_python_files(): + text = path.read_text(encoding="utf-8") + try: + leaks = list(_iter_500_leaks(text)) + except SyntaxError as exc: # pragma: no cover - source is valid Python + raise AssertionError(f"could not parse {path}: {exc}") from exc + for line_no, reason in leaks: + rel = path.relative_to(_BACKEND.parents[2]) + offenders.append(f"{rel}:{line_no}: {reason}") assert not offenders, ( - "HTTP 500 responses must use a static `detail` string (e.g. " - '"Internal server error") and never leak the caught exception or any ' - "runtime value. Log the full error server-side instead. Offending sites:\n " - + "\n ".join(offenders) + "HTTP 500 responses must not disclose internal details. Use a static " + '"Internal server error" body and log the full exception server-side ' + "instead. Offending sites:\n " + "\n ".join(offenders) ) -def test_guard_detects_synthetic_leaks() -> None: - """Sanity check: the scanner flags every dynamic 500 detail form and ignores - static details and 4xx responses.""" - leaky = [ - "raise HTTPException(status_code=500, detail=str(e))", - "raise HTTPException(500, str(e))", # positional status + detail - '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": str(e)})', # dict - # symbolic 500 must not be a blind spot: - "raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e))", +def test_guard_detects_every_known_leak_shape() -> None: + """Positive controls: the scanner flags each historical leak shape.""" + leaky_samples = [ + 'raise HTTPException(status_code=500, detail=str(e))', + 'raise HTTPException(status_code=500, detail=f"boom: {e}")', + 'raise HTTPException(status_code=500, detail=error_msg)', + 'raise HTTPException(status_code=500, detail={"message": error_msg})', + 'return JSONResponse(status_code=500, content={"detail": str(exc)})', + 'return JSONResponse(status_code=500, content={"path": str(request.url)})', + 'return JSONResponse(status_code=500, content={"t": exc.__class__.__name__})', + # Positional-argument form: HTTPException(status_code, detail) + 'raise HTTPException(500, str(e))', + 'raise HTTPException(500, f"internal: {exc}")', + 'raise HTTPException(500, error_msg)', ] - for src in leaky: - assert list(_iter_500_dynamic_detail(src)), f"scanner missed a real leak: {src}" + for sample in leaky_samples: + assert list(_iter_500_leaks(sample)), f"scanner missed a real leak: {sample}" + - safe = [ +def test_guard_allows_sanitized_and_safe_dynamic_bodies() -> None: + """Negative controls: static bodies and safe dynamic values are allowed.""" + safe_samples = [ 'raise HTTPException(status_code=500, detail="Internal server error")', + # Positional form with a static string is safe. 'raise HTTPException(500, "Internal server error")', - "raise HTTPException(status_code=400, detail=str(exc))", # 4xx echoes client input - "raise HTTPException(status_code=429, detail=f'Rate limit: {e}')", - # symbolic 500 with a static detail is still safe: - 'raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error")', + # 4xx echoing client input is out of scope. + 'raise HTTPException(status_code=400, detail=str(exc))', + # A random error id + timestamp is not an internal-disclosure vector. + 'return JSONResponse(status_code=500, content={' + '"id": f"FALLBACK_{uuid.uuid4().hex}", ' + '"message": "An unexpected error occurred.", ' + '"timestamp": datetime.now().isoformat()})', ] - for src in safe: - assert not list( - _iter_500_dynamic_detail(src) - ), f"scanner false-positived: {src}" + for sample in safe_samples: + assert not list(_iter_500_leaks(sample)), f"scanner false-positived: {sample}" if __name__ == "__main__": # pragma: no cover