From 93bceeb75bccdd9ebe6b7d65d5394fa94d0dfe69 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 08:16:57 +0000 Subject: [PATCH 1/2] fix(security): sanitize global 500 exception handler (CWE-209) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The global `Exception` handler in `backend/main.py` was the one HTTP-500 information-disclosure sink still live on `main`: its JSONResponse body echoed `str(exc)`, `exc.__class__.__name__`, and `str(request.url)` to the client on any unhandled error. The per-route `HTTPException(500, ...)` sinks were already sanitized, but this handler leaked via `JSONResponse(status_code=500, content=...)`. That call form is exactly what the existing AST guard (`tests/unit/test_500_info_disclosure.py`) does not inspect — it only scans `HTTPException(...)` calls, and `main.py` is not in its `_GUARDED_FILES` — which is why the leak survived every green CI run. Changes: - `global_exception_handler` now returns a static body (`{"error"/"detail": "Internal server error", "timestamp"}`); the exception (type, message, traceback) and request path are logged server-side only via `exc_info=True`. The 4xx `ValueError` handler is intentionally left unchanged. - Replace the two `test_backend_main.py` tests that asserted the leaky contract (`error_type`, `version`) with regression guards proving the 500 body is static and never echoes the exception message, class name, or request URL. Verified by exercising the real handler source directly (pytest/fastapi are not installable in the ephemeral sandbox without the heavy ML stack): the 500 body contains no exception text, class name, or URL, while the full error is still logged; the no-`.url` path is safe; the 400 handler still returns 400. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_016kZayBH8HUEt4ys7ofaH52 --- src/youtube_extension/backend/main.py | 35 +++++++++++++++------------ tests/unit/test_backend_main.py | 21 +++++++++++----- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/youtube_extension/backend/main.py b/src/youtube_extension/backend/main.py index 086fa73d8..c4ebe035d 100644 --- a/src/youtube_extension/backend/main.py +++ b/src/youtube_extension/backend/main.py @@ -451,22 +451,27 @@ 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 URL 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). + """ + request_path = str(request.url) if hasattr(request, "url") else "unknown" + logger.error( + f"Unhandled exception on {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..78b7054df 100644 --- a/tests/unit/test_backend_main.py +++ b/tests/unit/test_backend_main.py @@ -590,23 +590,32 @@ 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_request_without_url(self): """Handler should not crash if request has no .url attribute.""" From 6456ba1e9a42a3a567e101686b32061dbe18d2cd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 17 Jul 2026 08:22:05 +0000 Subject: [PATCH 2/2] fix(security): log only request path, not full URL, in global 500 handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit review on #842: the previous commit's log line used `str(request.url)`, which includes the query string. Since the service streams logs to stdout for Cloud Run, a failing request like `...?token=abc` would persist that secret in centralized logs (a server-side variant of the same CWE-209 concern). - `global_exception_handler` now logs `request.url.path` and the HTTP method only — never the full URL/query string. `exc_info=True` is preserved so the traceback is still captured. - Add `test_global_exception_handler_log_omits_query_string`: asserts the logger call omits a `?token=SECRET123` query secret, still records the bare path, and keeps `exc_info=True` — guarding against a regression to `str(request.url)`. Verified against the real handler source (fastapi/pytest unavailable in the sandbox): log line contains the path but not the token; body remains static; the no-`.url` path and the 400 `ValueError` handler are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_016kZayBH8HUEt4ys7ofaH52 --- src/youtube_extension/backend/main.py | 12 +++++++--- tests/unit/test_backend_main.py | 32 +++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/youtube_extension/backend/main.py b/src/youtube_extension/backend/main.py index c4ebe035d..a6af822cd 100644 --- a/src/youtube_extension/backend/main.py +++ b/src/youtube_extension/backend/main.py @@ -454,14 +454,20 @@ async def global_exception_handler(request, exc): """Global exception handler. Returns a static body only. The exception type, message, traceback, and - request URL are recorded server-side via ``logger.error(..., exc_info=True)`` + 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. """ - request_path = str(request.url) if hasattr(request, "url") else "unknown" + url = getattr(request, "url", None) + request_path = getattr(url, "path", None) or "unknown" + method = getattr(request, "method", "unknown") logger.error( - f"Unhandled exception on {request_path}: {exc}", exc_info=True + f"Unhandled exception on {method} {request_path}: {exc}", exc_info=True ) return JSONResponse( diff --git a/tests/unit/test_backend_main.py b/tests/unit/test_backend_main.py index 78b7054df..2a35296c5 100644 --- a/tests/unit/test_backend_main.py +++ b/tests/unit/test_backend_main.py @@ -617,6 +617,38 @@ async def test_global_exception_handler_body_is_static(self): 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.""" mock_req = MagicMock(spec=[]) # no attributes