diff --git a/src/youtube_extension/backend/main.py b/src/youtube_extension/backend/main.py index 086fa73d8..a6af822cd 100644 --- a/src/youtube_extension/backend/main.py +++ b/src/youtube_extension/backend/main.py @@ -451,22 +451,33 @@ 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. + + Returns a static body only. The exception type, message, traceback, and + request *path* are recorded server-side via ``logger.error(..., exc_info=True)`` + and never returned to the client, closing a CWE-209 information-disclosure + leak (the previous body echoed ``str(exc)``, ``exc.__class__.__name__``, and + the request URL). + + The log line uses only ``request.url.path`` and the method — never the full + URL/query string, which can carry secrets (e.g. ``?token=...``) that must not + be persisted to centralized (Cloud Run) logs. + """ + url = getattr(request, "url", None) + request_path = getattr(url, "path", None) or "unknown" + method = getattr(request, "method", "unknown") + logger.error( + f"Unhandled exception on {method} {request_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(), + }, + ) # Application lifecycle events diff --git a/tests/unit/test_backend_main.py b/tests/unit/test_backend_main.py index a73dcf792..2a35296c5 100644 --- a/tests/unit/test_backend_main.py +++ b/tests/unit/test_backend_main.py @@ -590,23 +590,64 @@ 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_exception_type(self): + """CWE-209: the 500 body must not disclose the exception class name.""" 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" + assert "error_type" not in body + assert "RuntimeError" not in response.body.decode() - async def test_global_exception_handler_includes_version(self): + async def test_global_exception_handler_body_is_static(self): + """CWE-209: the 500 body must not echo the exception message or request URL.""" import json as _json mock_req = MagicMock() - mock_req.url = "http://test/api" - response = await main_module.global_exception_handler(mock_req, Exception("e")) + mock_req.url = "http://test/secret-internal-path?token=abc" + response = await main_module.global_exception_handler( + mock_req, Exception("db password is hunter2") + ) body = _json.loads(response.body) - assert body["version"] == "2.0.0" + assert body["error"] == "Internal server error" + assert body["detail"] == "Internal server error" + raw = response.body.decode() + assert "hunter2" not in raw + assert "secret-internal-path" not in raw + + async def test_global_exception_handler_log_omits_query_string(self): + """CWE-209: the server-side log line must record only the route path and + method, never the full URL/query string (which can carry secrets such as + ``?token=...`` that would then persist in centralized/Cloud Run logs). + Guards against a regression to ``str(request.url)`` while keeping + ``exc_info=True`` so the traceback is still captured.""" + from unittest.mock import patch + + class _URL: + path = "/api/process" + + def __str__(self): # what str(request.url) would leak + return "http://test/api/process?token=SECRET123" + + class _Req: + url = _URL() + method = "POST" + + with patch.object(main_module, "logger") as mock_logger: + response = await main_module.global_exception_handler( + _Req(), Exception("boom") + ) + + assert response.status_code == 500 + mock_logger.error.assert_called_once() + args, kwargs = mock_logger.error.call_args + logged = " ".join(str(a) for a in args) + assert "SECRET123" not in logged + assert "token=" not in logged + assert "/api/process" in logged # the bare path is safe to log + assert kwargs.get("exc_info") is True async def test_global_exception_handler_request_without_url(self): """Handler should not crash if request has no .url attribute."""