diff --git a/src/youtube_extension/backend/cloud_ai_routes.py b/src/youtube_extension/backend/cloud_ai_routes.py index a1242a418..cdfe3f3ad 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) @@ -255,20 +255,24 @@ async def analyze_video(request: VideoAnalysisRequest): # Format and return response return format_analysis_result(result) - except CloudAIError as e: - logger.error(f"Cloud AI analysis failed: {e}") - raise HTTPException(status_code=503, detail=f"AI analysis failed: {str(e)}") except RateLimitError as e: + # 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: - logger.error(f"Configuration error: {e}") - raise HTTPException(status_code=500, detail=f"Configuration error: {str(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)}") 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 +304,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 +328,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..e1da6f629 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -143,17 +143,10 @@ async def process_video_cloud( except Exception as e: error_msg = f"Cloud processing failed: {str(e)}" - logger.error(error_msg) + logger.error(error_msg, 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 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") async def process_video_task_handler( @@ -216,7 +209,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 +222,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 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/batch-process") async def batch_process_videos_cloud(request: BatchCloudProcessingRequest): @@ -260,9 +254,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 +288,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 +326,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..46c22faf9 100644 --- a/src/youtube_extension/backend/main.py +++ b/src/youtube_extension/backend/main.py @@ -444,22 +444,28 @@ 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""" - logger.error(f"Unhandled exception: {exc}", exc_info=True) - - error_detail = { - "error": "Internal server error", - "detail": str(exc), - "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__ + """Global exception handler. + + The full exception (type, message, traceback) and the request path are + logged server-side only. The client receives a static body so an unhandled + error cannot disclose internal details — exception text, exception type, or + the request URL (CWE-209). + """ + path = str(request.url) if hasattr(request, "url") else "unknown" + logger.error( + f"Unhandled exception on {path}: {exc}", exc_info=True + ) - return JSONResponse(status_code=500, content=error_detail) + return JSONResponse( + status_code=500, + content={ + "error": "Internal server error", + "detail": "Internal server error", + "timestamp": datetime.now().isoformat(), + "version": "2.0.0", + "architecture": "service-oriented", + }, + ) # Application lifecycle events diff --git a/src/youtube_extension/backend/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 03fd9e4e6..a810032ae 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -119,17 +119,10 @@ async def process_video_real_api(request: VideoProcessingRequest, background_tas except Exception as e: error_msg = f"Real API processing failed: {str(e)}" - logger.error(error_msg) + logger.error(error_msg, 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 is a static string; error_msg (with 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 +143,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 +170,14 @@ async def batch_process_videos(request: BatchProcessingRequest): return result + except HTTPException: + # Preserve explicit 4xx responses (e.g. the 400 batch-size guard 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 +250,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 +383,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 +431,14 @@ async def search_youtube_videos( "timestamp": datetime.now(timezone.utc).isoformat() } + except HTTPException: + # Preserve explicit 4xx responses (e.g. the 400 max-results guard 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..95ba547b7 --- /dev/null +++ b/tests/unit/test_500_info_disclosure.py @@ -0,0 +1,172 @@ +"""Regression guard against information disclosure in HTTP 500 responses. + +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 input, which is not an internal-disclosure vector. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +_BACKEND = Path(__file__).resolve().parents[2] / "src" / "youtube_extension" / "backend" + +# 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 _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 _status_is_500(call: ast.Call) -> bool: + for kw in call.keywords: + if kw.arg == "status_code" and isinstance(kw.value, ast.Constant): + return kw.value.value == 500 + # positional status_code (JSONResponse(500, ...) / HTTPException(500, ...)) + if call.args and isinstance(call.args[0], ast.Constant): + return call.args[0].value == 500 + return False + + +def _call_name(call: ast.Call) -> str | None: + fn = call.func + return getattr(fn, "id", None) or getattr(fn, "attr", None) + + +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): + continue + name = _call_name(node) + if name not in ("HTTPException", "JSONResponse"): + continue + if not _status_is_500(node): + continue + 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" + + +def _backend_python_files() -> list[Path]: + return sorted(_BACKEND.rglob("*.py")) + + +def test_no_information_disclosure_in_500_responses() -> None: + offenders: list[str] = [] + 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 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_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__})', + ] + for sample in leaky_samples: + assert list(_iter_500_leaks(sample)), f"scanner missed a real leak: {sample}" + + +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")', + # 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 sample in safe_samples: + assert not list(_iter_500_leaks(sample)), f"scanner false-positived: {sample}" + + +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..20cfc21db 100644 --- a/tests/unit/test_backend_main.py +++ b/tests/unit/test_backend_main.py @@ -590,14 +590,27 @@ 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_does_not_leak_internal_details(self): + """The 500 body must not disclose the exception (message or class) or the + request URL (CWE-209); those go to the server log only.""" import json as _json mock_req = MagicMock() - mock_req.url = "http://test/api" - response = await main_module.global_exception_handler(mock_req, RuntimeError("crash")) + mock_req.url = "http://test/api/secret-path" + response = await main_module.global_exception_handler( + mock_req, RuntimeError("crash: db password = hunter2") + ) body = _json.loads(response.body) - assert body["error_type"] == "RuntimeError" + # No exception class name, exception message, or request URL in the body. + assert "error_type" not in body + serialized = _json.dumps(body) + assert "RuntimeError" not in serialized + assert "crash" not in serialized + assert "hunter2" not in serialized + assert "secret-path" not in serialized + # Static, non-sensitive fields remain. + assert body["error"] == "Internal server error" + assert body["detail"] == "Internal server error" 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_real_api_endpoints.py b/tests/unit/test_real_api_endpoints.py index 4eb7109c6..fdadef769 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_does_not_leak_internal_state(self, client, mock_processor): 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"}, ) + assert response.status_code == 500 + # 500 responses must not disclose internal state (CWE-209): + # neither the request-supplied video_url nor the exception text. detail = response.json()["detail"] - assert "auJzb1D-fag" in str(detail) + assert detail == "Internal server error" + assert "auJzb1D-fag" not in str(detail) + assert "crash" not in str(detail) def test_missing_video_url_returns_422(self, client): response = client.post("/api/v2/process-video", json={}) @@ -437,15 +442,15 @@ 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.""" + """The >20-video guard raises HTTPException(400); an ``except HTTPException: + raise`` ahead of the broad handler preserves it instead of masking it as 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) + # The explicit 400 must survive the outer exception handler. + assert response.status_code == 400 def test_batch_response_contains_results(self, client): response = client.post( @@ -797,10 +802,10 @@ def test_result_video_url_format(self, client): 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.""" + """The >50-results guard raises HTTPException(400); an ``except HTTPException: + raise`` ahead of the broad handler preserves it instead of masking it as 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 def test_default_order_is_relevance(self, client, mock_youtube): client.post("/api/v2/search-videos?query=test")