diff --git a/src/youtube_extension/backend/cloud_ai_routes.py b/src/youtube_extension/backend/cloud_ai_routes.py index a1242a418..c55d90042 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,25 @@ 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: + # RateLimitError subclasses CloudAIError, so it MUST be caught before the + # base class or it would be swallowed and mis-reported as a 503. logger.warning(f"Rate limit exceeded: {e}") - raise HTTPException(status_code=429, detail=f"Rate limit exceeded: {str(e)}") + raise HTTPException(status_code=429, detail="Rate limit exceeded") except ConfigurationError as e: - logger.error(f"Configuration error: {e}") - raise HTTPException(status_code=500, detail=f"Configuration error: {str(e)}") + # ConfigurationError subclasses CloudAIError, so it MUST be caught before + # the base class. Its message can embed internal configuration detail, so + # the client body stays generic (CWE-209); the full error is logged 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}", exc_info=True) + raise HTTPException(status_code=503, detail="AI service temporarily unavailable") 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 +305,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 +329,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..16378abdd 100644 --- a/src/youtube_extension/backend/cloud_api_endpoints.py +++ b/src/youtube_extension/backend/cloud_api_endpoints.py @@ -35,6 +35,18 @@ router = APIRouter() +def _client_safe_error(error_message: Optional[str]) -> Optional[str]: + """Return a client-safe error string. + + Processing state persisted by the video processor can embed raw exception + text (e.g. ``Error processing video ...: ``). That text must never + be echoed to clients through the status/result/process responses (CWE-209). + The full message stays in server-side state and logs; the client only learns + that an error occurred. + """ + return "Internal server error" if error_message else None + + # Pydantic models for API requests/responses class CloudVideoProcessingRequest(BaseModel): @@ -138,22 +150,13 @@ async def process_video_cloud( ai_analysis=result.ai_analysis, processing_time=result.processing_time, from_cache=result.from_cache, - error=result.error_message, + error=_client_safe_error(result.error_message), ) except Exception as e: - error_msg = f"Cloud processing failed: {str(e)}" - logger.error(error_msg) - - raise HTTPException( - status_code=500, - detail={ - "error": "cloud_processing_failed", - "message": error_msg, - "video_url": request.video_url, - "timestamp": datetime.now(timezone.utc).isoformat() - } - ) + # Exception text is logged server-side only; never returned to the client. + logger.error(f"Cloud processing failed: {e}", exc_info=True) + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/api/v3/process-video-task") async def process_video_task_handler( @@ -215,21 +218,23 @@ async def process_video_task_handler( } except Exception as e: - error_msg = f"Task processing failed: {str(e)}" - logger.error(error_msg) + # 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 error + # Update state with a user-safe message try: firestore_service = await get_firestore_service() 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) + 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 +265,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) @@ -287,15 +293,16 @@ async def get_video_status(video_id: str): created_at=state.created_at, updated_at=state.updated_at, processing_time=state.processing_time, - error_message=state.error_message, + error_message=_client_safe_error(state.error_message), ) except HTTPException: 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") @@ -324,15 +331,16 @@ async def get_video_result(video_id: str): "processing_time": state.processing_time, "created_at": state.created_at, "updated_at": state.updated_at, - "error_message": state.error_message, + "error_message": _client_safe_error(state.error_message), } except HTTPException: 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/real_api_endpoints.py b/src/youtube_extension/backend/real_api_endpoints.py index 03fd9e4e6..ab7dd1e86 100644 --- a/src/youtube_extension/backend/real_api_endpoints.py +++ b/src/youtube_extension/backend/real_api_endpoints.py @@ -117,19 +117,12 @@ async def process_video_real_api(request: VideoProcessingRequest, background_tas return response + except HTTPException: + raise except Exception as e: - error_msg = f"Real API processing failed: {str(e)}" - logger.error(error_msg) - - raise HTTPException( - status_code=500, - detail={ - "error": "video_processing_failed", - "message": error_msg, - "video_url": request.video_url, - "timestamp": datetime.now(timezone.utc).isoformat() - } - ) + # Exception text is logged server-side only; never returned to the client. + logger.error(f"Real API processing failed: {e}", exc_info=True) + 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,13 @@ async def batch_process_videos(request: BatchProcessingRequest): return result + except HTTPException: + 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,13 @@ async def search_youtube_videos( "timestamp": datetime.now(timezone.utc).isoformat() } + except HTTPException: + 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..69f7f6c82 --- /dev/null +++ b/tests/unit/test_500_info_disclosure.py @@ -0,0 +1,153 @@ +"""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. + +It deliberately does not constrain 4xx responses: those echo client-supplied +validation errors, which are not internal-disclosure vectors. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +_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" + ) + + +def _is_500_status(node: ast.expr) -> bool: + # Literal 500 + if isinstance(node, ast.Constant) and node.value == 500: + return True + # Symbolic constant: status.HTTP_500_INTERNAL_SERVER_ERROR or a direct import + if isinstance(node, ast.Attribute) and node.attr == "HTTP_500_INTERNAL_SERVER_ERROR": + return True + if isinstance(node, ast.Name) and node.id == "HTTP_500_INTERNAL_SERVER_ERROR": + return True + return False + + +def _is_500(call: ast.Call) -> bool: + # Positional form: HTTPException(, ...) + if call.args and _is_500_status(call.args[0]): + return True + # Keyword form: HTTPException(status_code=, ...) + for kw in call.keywords: + if kw.arg == "status_code" and _is_500_status(kw.value): + return True + 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 _iter_500_dynamic_detail(text: str): + """Yield (line_no, snippet) for each 500 ``HTTPException`` whose ``detail`` is + anything other than a static string literal.""" + tree = ast.parse(text) + for node in ast.walk(tree): + if not isinstance(node, ast.Call) or not _is_httpexception(node): + continue + if not _is_500(node): + 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): + continue + yield node.lineno, ast.unparse(node)[:160] + + +def test_no_dynamic_detail_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}") + + 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) + ) + + +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 status constant (status.HTTP_500_* and direct import forms) + "raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e))", + "raise HTTPException(HTTP_500_INTERNAL_SERVER_ERROR, str(e))", + ] + for src in leaky: + assert list(_iter_500_dynamic_detail(src)), f"scanner missed a real leak: {src}" + + 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")', + "raise HTTPException(status_code=400, detail=str(exc))", # 4xx echoes client input + 'raise HTTPException(status_code=404, detail=f"No such video: {video_id}")', # 4xx echoes client input + ] + # NOTE: this guard covers 500 responses only. The 429/503 server-error + # branches in analyze_video are sanitized too, but their regression coverage + # lives in the endpoint tests (test_cloud_routes.py), which assert the exact + # static bodies and the absence of the exception text. + for src in safe: + assert not list( + _iter_500_dynamic_detail(src) + ), f"scanner false-positived: {src}" + + +if __name__ == "__main__": # pragma: no cover + raise SystemExit(pytest.main([__file__, "-v"])) diff --git a/tests/unit/test_cloud_routes.py b/tests/unit/test_cloud_routes.py index 4c3b29839..12dde6699 100644 --- a/tests/unit/test_cloud_routes.py +++ b/tests/unit/test_cloud_routes.py @@ -358,10 +358,13 @@ def test_analyze_video_cloud_ai_error(self): "analysis_types": ["label_detection"], }) assert response.status_code == 503 + # The 503 body must be static; the exception text must not leak (CWE-209). + assert response.json()["detail"] == "AI service temporarily unavailable" + assert "service down" not in response.text def test_analyze_video_rate_limit_error(self): - # RateLimitError is a subclass of CloudAIError, so it's caught by the - # CloudAIError except clause first and returns 503 (not 429). + # RateLimitError subclasses CloudAIError but is now caught before the base + # class, so it correctly returns 429 (not 503). mock_ai = AsyncMock() mock_ai.__aenter__ = AsyncMock(return_value=mock_ai) mock_ai.__aexit__ = AsyncMock(return_value=False) @@ -373,11 +376,15 @@ def test_analyze_video_rate_limit_error(self): "video_url": "https://www.youtube.com/watch?v=auJzb1D-fag", "analysis_types": ["label_detection"], }) - assert response.status_code in (429, 503) + assert response.status_code == 429 + # The 429 body must be static; the exception text must not leak (CWE-209). + assert response.json()["detail"] == "Rate limit exceeded" + assert "too many requests" not in response.text def test_analyze_video_configuration_error(self): - # ConfigurationError is a subclass of CloudAIError, so it's caught by - # the CloudAIError clause -> 503. + # ConfigurationError subclasses CloudAIError but is now caught before the + # base class, so it returns a sanitized 500 (not a 503 leaking the config + # message). The internal detail must never reach the client. mock_ai = AsyncMock() mock_ai.__aenter__ = AsyncMock(return_value=mock_ai) mock_ai.__aexit__ = AsyncMock(return_value=False) @@ -389,7 +396,9 @@ def test_analyze_video_configuration_error(self): "video_url": "https://www.youtube.com/watch?v=auJzb1D-fag", "analysis_types": ["label_detection"], }) - assert response.status_code in (500, 503) + assert response.status_code == 500 + assert response.json()["detail"] == "Internal server error" + assert "missing key" not in response.text def test_analyze_video_generic_error(self): mock_ai = AsyncMock() @@ -574,7 +583,10 @@ def test_process_video_sync_failed(self): assert response.status_code == 200 data = response.json() assert data["status"] == "failed" - assert data["error"] == "something went wrong" + # The failure is reported, but the raw persisted exception text must be + # sanitized at the response boundary (CWE-209), not echoed to the client. + assert data["error"] == "Internal server error" + assert "something went wrong" not in response.text def test_process_video_exception(self): mock_processor = AsyncMock() @@ -1316,7 +1328,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..c320bde37 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={}) @@ -436,16 +441,17 @@ 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): + """A >20-video batch must surface the explicit 400 validation error and + not be swallowed into a generic 500 by the broad exception handler + (the handler re-raises HTTPException before the catch-all).""" 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 response.json()["detail"] def test_batch_response_contains_results(self, client): response = client.post( @@ -796,11 +802,13 @@ 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): + """A >50-result search must surface the explicit 400 validation error and + not be swallowed into a generic 500 by the broad exception handler + (the handler re-raises HTTPException before the catch-all).""" 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 response.json()["detail"] def test_default_order_is_relevance(self, client, mock_youtube): client.post("/api/v2/search-videos?query=test")