Log and mask server-side errors across all routers - #2477
Conversation
f044f30 to
cc8ed9e
Compare
5249b01 to
5ee307c
Compare
5ee307c to
0f8f433
Compare
harry-rhesis
left a comment
harry-rhesis
left a comment
There was a problem hiding this comment.
Nice work on this. The design is sound, the testing is thorough, and the router cleanup is satisfying to read. None of the items below are blockers.
Double-logging on a few services.py paths (non-blocking)
services.py:248-250 (ModelConfigurationError) and services.py:468-470 (RuntimeError) both log with exc_info=True and then raise a 500 HTTPException without rhesis_logged = True. The global handler logs the same error a second time with a traceback. Same thing at line 557-559 (recent activities), though that one uses a fixed-string detail so it's safe either way.
All three are pre-existing and behaviorally correct (the global handler masks the detail). But replacing them with internal_error would cut the duplicate log lines. Fine to defer.
handle_execution_error fallback still constructs the leak pattern (non-blocking)
execution_validation.py:139-142:
error_msg = str(error) if str(error) else "An unexpected error occurred"
return HTTPException(status_code=500, detail=f"Failed to {operation}: {error_msg}")The global handler masks this today. But if a future caller uses handle_execution_error outside the request lifecycle (background task, CLI), the leak surfaces. The AST guard can't see through function calls, so it won't catch it. I know the ValueError bleed paths are already tracked for follow-up; the broad fallback at the end of this function is a separate instance of the same concern.
Header-duplication test could cover UpstreamHTTPException (non-blocking)
test_request_id_header_is_not_duplicated checks /boom, /server-error, and /bad-request. Adding /upstream to the list would confirm no duplication on the upstream exemption path too.
Thanks for the thorough writeup in the PR description, especially the middleware-ordering rationale and the note on the three latent bugs this surfaced.
Routers raised 531 HTTPExceptions with no catch-all handler behind them, so an uncaught error became a bare 500 that logged nothing. 36 sites also put an exception caught by a broad `except Exception` straight into `detail=`, handing callers SQLAlchemy statements, OAuth provider internals and connection targets. Adds the shared pieces the per-router cleanup builds on: - RequestIDMiddleware assigns each request an id, echoed as X-Request-ID. Pure ASGI, not BaseHTTPMiddleware, so the ContextVar stays readable both downstream in log records and back up in the exception handlers. - unhandled_exception_handler logs the traceback and returns a generic message plus error_id. http_exception_handler does the same for 5xx and leaves sub-500s untouched, so deliberate validation messages still reach the user. - internal_error() for routers that genuinely add context before giving up. - Log records carry request_id, so service and crud lines correlate too. test_router_error_leaks.py AST-scans routers for the leak and holds the remaining 36 in an allowlist that may only shrink. 5xx response bodies are now generic; the real detail moves to the logs under the error_id.
The guard only followed direct references, so `error_msg = str(e)` ahead of
`detail=f"...{error_msg}"` slipped past it. Taint now propagates through
local assignments to a fixpoint.
That exposed five more leaks the batches could not have seen:
services.py (4) was reported clean by the old scan, and test.py's 404 branch
still returned a broad exception's text after its 500 branch was fixed.
KNOWN_LEAKS is now empty -- every router is clean under the stricter scan.
The OWASP router landed on main while this branch was in flight and shipped the same pattern: a broad handler putting str(e) into detail= on a 502 and a 500. The guard flagged both on merge, which is the regression it exists for.
POST /tests/bulk raises its own HTTPException(400, "No tests provided in request") inside a try whose broad handler catches it -- str() of that exception is "400: No tests provided in request", which fails the "not found" check and falls through to a 500. Pre-existing, but this branch made it worse: the old code at least echoed the real message inside the 500 body, and internal_error now strips it, so the caller got a bare "An unexpected error occurred." The new route test fails with `assert 500 == 400` without the fix.
Blanket 5xx masking was too blunt in one direction and the handler had four defects. From a code review of this branch. Users' own upstream failures are no longer masked. POST /endpoints/test and invoke_endpoint exist to report what is wrong with the *caller's* endpoint -- a refused connection, a rejected token -- and "An unexpected error occurred." told them nothing while protecting nothing of ours. UpstreamHTTPException marks those; the global handler passes their detail through. The exemption is deliberately narrow, because the obvious version of it would have been a new leak: - EndpointService wraps our own exceptions in EndpointInvocationError as error_type="internal_error", so the router branches on that discriminator rather than trusting the type. - testing.py's try covered endpoint construction and input enrichment as well as the invocation, so the exemption is scoped to the invoke() call alone and setup failures still go through internal_error. Also fixed: - CORS expose_headers omitted X-Request-ID, so browser JS could never read the id the middleware exists to emit. - internal_error(status_code=400) answered "An unexpected error occurred.", describing a client error in server-error words. - internal_error logged, then the handler logged the same failure again -- two tracebacks per error, the second one saying less. - Both the middleware and the handler set X-Request-ID, so every 5xx HTTPException carried it twice, plus a redundant X-Error-Id.
internal_error always logged a full traceback at error level, including at the two call sites that return 400 (OAuth login and callback). A denied consent or an expired state is the caller's problem, not a stack we need. The level now follows the status: 5xx keeps the traceback, 4xx gets one warning line. The endpoint test path had the same shape from the other direction. Every invocation failure logged a warning with a traceback and the global handler then added an error line on top, so a mistyped API key in the user's own endpoint config produced three entries. It now logs once and marks itself logged.
The patterns treated only - and _ as separators, so "api key: sk-live-..." went through untouched. Upstream services phrase their own auth failures that way in prose, and UpstreamHTTPException now passes those bodies back to us verbatim, which is how such a line reaches a log at all. A literal space rather than \s: \s would let a match run past the end of a line. Every pattern still requires a : or = after the name, so prose like "the api key was missing" is left readable.
0f8f433 to
0090906
Compare
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid hardening overall: request correlation id, global 5xx masking, and the AST leak-guard look like a big reliability/security win.
Improvement: log format now requires request_prefix, which can KeyError if anything logs before set_logger() installs _WorkerContextFilter.
Question: invoke_endpoint’s upstream-detail passthrough is fail-open if EndpointInvocationError.error_type ever changes/unsets.
Found 3 issues (0 critical, 2 improvements, 1 question).
| # EndpointService wraps *our* failures in this same type as | ||
| # error_type="internal_error" (services/endpoint/service.py), so the | ||
| # discriminator is what separates the user's endpoint from our bug. | ||
| if e.error_type == "internal_error": |
There was a problem hiding this comment.
Question: In invoke_endpoint, the decision to expose str(e) via UpstreamHTTPException hinges on e.error_type != "internal_error". If error_type is ever unset/renamed, this becomes a fail-open leak path (internal errors returned to the client).
Fix: consider making this fail-closed by only passing through detail for an allowlisted upstream discriminator (e.g.
error_type == "upstream_error"), and otherwise route tointernal_error(...).
There was a problem hiding this comment.
Good catch on the follow-up commits, but I think this is still slightly fail-open: EndpointInvocationError.error_type is optional (can be None/""), so if e.error_type == "internal_error" will treat an unset/empty discriminator as upstream and pass str(e) through.
Fix: make this fail-closed for the unset case too, e.g.
if not e.error_type or e.error_type == "internal_error": ...internal_error...(and only raiseUpstreamHTTPExceptionwhenerror_typeis present + known to be caller-facing).
| @@ -17,7 +18,7 @@ | |||
| JSON_LOGGER_ENABLED = application_settings.json_logger_enabled | |||
| DEV_MODE = application_settings.dev_mode | |||
| # role_prefix is "" for API; Celery workers get "[MAIN] - " via the filter. | |||
There was a problem hiding this comment.
Improvement: LOG_FORMAT now includes %(request_prefix)s, which is only present when _WorkerContextFilter is installed. If any logs are emitted before set_logger() runs (early imports / some tests), this format can raise a KeyError and drop logs.
Fix: make the formatter resilient (e.g. a custom
Formatterthat sets missing attrs to ""; or keepLOG_FORMATwithoutrequest_prefixand prepend in the filter by mutatingrecord.name/record.msg; or ensureset_logger()runs before any logging config uses this format).
There was a problem hiding this comment.
Follow-up commits largely addressed the intent here by ensuring the filter sets request_prefix and by explicitly passing extra={"request_id": ...} on the unhandled path.
One remaining sharp edge: LOG_FORMAT still hard-requires request_prefix. If any logging config uses this format without _WorkerContextFilter (e.g. Alembic’s fileConfig, or a test setting up handlers manually), it can still KeyError.
Fix: either (a) make
request_prefixoptional at formatting time (custom Formatter that defaults missing attrs to ""), or (b) avoid adding%(request_prefix)stoLOG_FORMATand instead prepend it by mutatingrecord.name/record.msgin the filter.
| class UpstreamHTTPException(HTTPException): | ||
| """A failure of the *caller's* system, not ours -- detail is theirs to see. | ||
|
|
||
| Endpoint testing and invocation exist to report what is wrong with a user's |
There was a problem hiding this comment.
Improvement: UpstreamHTTPException passes detail through even for 5xx. That’s intended for endpoint testing/invocation, but it’s easy for future code to misuse (e.g. raising it for internal failures).
Fix: consider renaming to something more specific (
EndpointUpstreamHTTPException) and/or adding a brief note in the class docstring about where it is allowed to be raised, plus a test asserting only those routers use it.
There was a problem hiding this comment.
The new PublicHTTPException + docstring clarification makes this a lot safer and addresses the main concern (there’s now a “deliberate 5xx detail survives” type, and tests pin its behavior).
I think this thread can be resolved once the invoke_endpoint discriminator is made fail-closed (so UpstreamHTTPException can’t accidentally become a generic “escape hatch” for internal errors).
Masking every 5xx detail left no way to keep a message that was written for the caller on purpose. "Garak package is not installed" and "No Celery workers available" name the one thing the caller can act on and reveal nothing, but they were replaced by the generic string with the literals left in the code, so it read as if they still arrived. PublicHTTPException passes its detail through and logs at WARNING with no stack -- a deliberate 5xx is not a fault to debug, and the traceback only points back at the raise. UpstreamHTTPException becomes one case of it. internal_error gains public_detail for the same reason at 4xx, and returns a PublicHTTPException when given one, since a plain HTTPException would have been masked by the handler it just asked to pass through. Also drops the hand-written request id prefix, which the log formatter already stamps, and passes the id explicitly on the unhandled path: ServerErrorMiddleware runs after RequestIDMiddleware clears the ContextVar, so the field was landing null on exactly the lines an error_id points at.
The handler logged every validation error at ERROR with a traceback immediately before calling log_validation_error, whose own docstring says a 422 is the caller's mistake and not a server fault.
JsonLogFormatter built its payload from getMessage() and never read
record.exc_info, and every deployed environment sets
jsonLoggerEnabled: true. So logger.exception produced one line with no
stack, no frames and no exception class -- the client was told nothing
and the log kept only str(exc). That is the half of "mask the response,
log the reason" that was silently not happening, and it bit hardest
where broad handlers were deleted on the grounds that the global handler
would log the same traceback.
Redaction was also wrong in both directions. It mangled ordinary
diagnostics ("api key: not configured" -> "api key: [REDACTED]
configured") because any word after the separator counted as a secret,
while missing the secrets that actually reach us: provider errors read
"Incorrect API key provided: sk-...", with filler between the keyword
and the value, and header dumps put a quote between the name and
"Bearer ...". The value now has to look like a secret, and key shapes
are matched without needing a keyword at all. The URL pattern never
matched SQLAlchemy's real postgresql:// scheme, and the Celery broker is
redis://; both are covered now.
The context filter no longer overwrites a request_id passed via extra,
which the unhandled-exception path depends on.
A wrong Rhesis API key was always fine -- 4xx pass through untouched. A wrong credential for a service the user connected themselves was not. handle_mcp_exception deliberately remaps an MCP 401/403 to a 502 so the reason survives and the frontend does not clear the session, and the new handler masked exactly that, so a bad Notion token read as our service being down. It now returns UpstreamHTTPException, and only for MCPApplicationError, whose detail really is the tool's parsed response; a message our own code wrote gets a literal instead. services.py stripped provider reasons at 400 -- a status the contract says passes through -- so "invalid api key" and "context length exceeded" became "Failed to generate content", and each was logged at ERROR with a stack for a response it returned as the caller's fault. Failures that carry a provider status now keep the reason at 400; ours go to internal_error at 500 with a traceback. Kept at 400 rather than 502 because the SDK retries 5xx, and retrying a bad key four times helps nobody. tools.py reports an unreachable instance again: wrong credentials already came back as 200, but a refused connection became a masked 500 in a drawer whose whole job is showing why the connection failed. MCP providers reaching that endpoint were escaping uncaught entirely. auth.py reported IdP outages and bugs in our own callback as 400 "The request could not be processed.", logged as one stackless warning where it used to be an ERROR with a traceback -- both axes wrong at once. Those are 500s again, a provider's deliberate 400 survives, a rejected email address says why, and an invalid token no longer logs at ERROR.
testing.py wrapped invoke() in a bare except Exception and declared whatever it caught to be upstream detail, shipping str(exc) to the client on a 500 -- the opposite of what this branch is for. The catch was far wider than the exemption: the invokers *return* an error response for real upstream failures, so what it actually caught was config errors, SQLAlchemy failures carrying the statement and its bound params, and bugs in our own mapping code. A Starlette HTTPException also stringifies to "400: ...", so the client got a status code inside a 500 body. The wrapper is gone; HTTPException keeps its own status. rest_invoker's unexpected_error arm did the same thing through a channel nothing was watching: str(e) in an HTTP 200 body, invisible to the leak guard because there is no detail=. service.py labelled everything it did not recognise as internal_error, and the router masked on that discriminator, so "Failed to get client credentials token" -- the user's own OAuth server rejecting their secret -- came back generic. It now re-raises HTTPException. Its 502 arm no longer catches bare OSError, which was routing our own FileNotFoundError to the client with a server path. test_endpoint_mapping was left out of the exemption even though its only job is explaining what is wrong with the mappings the user just typed. The invokers' caller-actionable literals become public, and the token failure is a 502 upstream error rather than a masked 500. One connection-refused used to produce three ERROR records and two tracebacks; it is now one warning. model_connection stops narrating our own bugs in a 200 body while still passing provider messages through, and stops logging the provider's reply, which can hold the user's key.
The garak, Celery and SDK 503/500 literals go through PublicHTTPException, so "Garak package is not installed" reaches the caller again instead of "The service is temporarily unavailable." A deployment without garak was worse than vague on one route: _get_module_info swallows the ImportError and returns None, so get_probe_module_detail answered 404 "Probe module not found" -- a wrong answer, not a missing reason. Two small helpers map the real cause instead, and the existing except RuntimeError arms are dropped because neither could ever see a missing package. handle_execution_error still built detail from a broad exception, safe only because the global handler happened to mask it, and invisible to the guard for living outside routers/. It uses internal_error now, and the test that asserted the leaked text asserts the masked one. POST /test_sets/bulk had no 4xx mapping at all, and the service laundered IntegrityError and pydantic ValidationError into a bare Exception, so an uploaded file with a bad owner_id told the user nothing. The decorator handles the DB half and the validation half reports which field failed, without echoing the input value back. Three sites persisted str(exc) on the record and then masked the same text in the response, so the message was gone from the API but readable in the UI field next to it. That text comes from task_launcher, where a broker failure carries the connection string, so the persisted field gets a caller-safe summary and the reason stays in the log. The bulk 404 names the missing test set again and drops its traceback.
The guard let 14 of 20 crafted leaks through. The most likely
regression was the simplest: only detail= keywords were checked, so
HTTPException(500, f"...{e}") with a positional detail was invisible.
BROAD_EXCEPTIONS held only Exception and BaseException, which treated
SQLAlchemyError and httpx.HTTPError as narrow and deliberate despite
carrying SQL text and connection strings. Taint tracked plain
assignment only, so +=, tuples, subscripts and attributes all escaped,
as did assigning to .detail after construction.
It also could not see the channel most of the remaining leaks use: a
200 response with the reason in the body, or a result object, neither of
which has a detail= at all. And it scanned only routers/, missing
handle_execution_error one directory over -- reached by following taint
from a parameter annotated as a broad exception.
Scanning now covers routers, utils and services recursively.
services/invokers is exempt: every string there reports on the user's own
endpoint. 15 pre-existing 200-body leaks across 8 files are ratcheted so
the count can only fall.
One deliberate narrowing: a 4xx detail built from an exception is no
longer flagged. Sub-500 details pass through by design and three live
sites depend on it, but this is less coverage than before, not a
refactor.
5a6b26c to
8edc8e7
Compare
There was a problem hiding this comment.
Looks good overall — the global masking/logging contract + request correlation id + leak guard are solid, and the new PublicHTTPException/public_detail path is a good way to keep intentional 5xx messages public.
Two small follow-ups remain from the existing threads:
invoke_endpoint: the upstream passthrough still hinges one.error_type != "internal_error"(fail-open if that field ever goes missing/renamed).logging_config.LOG_FORMAT: relying onrequest_prefixexisting via a filter could still blow up if anything logs through that formatter before the filter is attached; easiest fix is attaching_WorkerContextFilterto handlers before adding them (or making the formatter tolerant).
There was a problem hiding this comment.
[Critical] invoke_endpoint still has a fail-open path: EndpointInvocationError.error_type can be unset/empty, and the current if e.error_type == "internal_error" will treat that as upstream and pass str(e) to the client via UpstreamHTTPException.
Fix: make the decision fail-closed (treat missing/unknown
error_typeas internal), or only pass through for an allowlisted caller-facing discriminator.
[Improvement] LOG_FORMAT now requires %(request_prefix)s; if any logging config uses this format without _WorkerContextFilter, it can raise KeyError and drop logs (e.g. non-API contexts like Alembic/tests).
Fix: make formatting resilient to missing attrs (default
request_prefix=""), or avoid addingrequest_prefixtoLOG_FORMATand instead prepend in the filter.
Found 2 issues (1 critical, 1 improvement).
2 participants
Purpose
Router error handling had two problems. We were blind: 98 broad
except Exceptionblocks across the routers, 27 of which logged nothing at all, and no catch-all handler behind any of them — an uncaught error became a bare Starlette 500 that left no trace. There was no request id, so even a logged error couldn't be tied to a user's report. We leaked: 43 sites fed an exception caught by a broad handler straight intodetail=, handing callers SQLAlchemy statements, OAuth provider internals, Celery broker URLs and connection targets.auth.pyreturnedf"Authentication failed: {str(e)}";job.pyreturnedf"Celery health check failed: {str(e)}".An unexpected error should now tell the user nothing and the logs everything, tied together by an id the user can quote.
What Changed
Foundation (
3a10c76c5)RequestIDMiddlewareassigns every request an id, echoed asX-Request-ID. Pure ASGI rather thanBaseHTTPMiddleware— the latter runs the downstream app in a new anyio task, and the id has to stay readable both downstream in log records and back up in the exception handlers.Exceptionhandler logs the full traceback and returns{"detail": "An unexpected error occurred.", "error_id": "..."}. A 5xxHTTPExceptionhandler does the same. Sub-500s pass through completely untouched, so deliberate validation messages like "Test set name already exists" still reach users unchanged.internal_error(exc, context=...)for the cases where a router genuinely adds context a global handler can't infer.request_id, so service- and crud-level lines correlate to the same request.tests/backend/security/test_router_error_leaks.pyAST-scans every router for the leak pattern and fails on regressions.Router cleanup (43 leaks removed across 4 commits)
Most of the fix is deletion — once a global handler exists, a broad handler that logs nothing and re-wraps the error is pure noise. Net −436 lines in the routers.
Additional Context
Three latent bugs surfaced. Broad handlers were swallowing the endpoints' own deliberate
HTTPExceptions and re-wrapping them as 500s:job.py's503 "No Celery workers available"and404 "Task not found", andtest_run.py's404 "Test run not found". Deleting the handlers fixes all three.Behavior change: all 5xx response bodies are now generic, including hand-written ones like
"Failed to send test request to SDK". Nothing is lost — the original moves to the logs under theerror_id— but it is visible. Two tests asserted the old behavior and were updated. The narrower alternative (genericise only exception-derived text) was rejected because it lets every new leak ship freely until someone notices.The guard already caught a regression. The OWASP router (#2202) landed on
mainwhile this branch was in flight, shipping the same pattern on a 502 and a 500. The guard flagged both on merge — that is the case it exists for.Known gap:
model.py::test_model_connectionreturns HTTP 200 with{"status": "error", "message": str(e)}. The guard cannot see it — there is nodetail=. Fixing it would change the response contract, so it is left for a follow-up.Frontend needs no change to keep working; it reads
.detail, which still exists. Surfacingerror_idin error toasts is a separate follow-up.Testing
3907 backend tests pass, 42 skipped. Zero leaks across all 52 routers.
cd apps/backend uv sync --extra all --extra ee uv run pytest ../../tests/backend/security/ ../../tests/backend/routes/ \ ../../tests/backend/app/ ../../tests/backend/auth/ ../../tests/backend/utils/ \ ../../tests/backend/tasks/ -q -p no:randomlyTo confirm the guard actually bites, reintroduce a leak and watch it fail:
End to end:
./rh dev statusfor this checkout's ports, then trigger a failing endpoint and confirm the response body is generic,X-Request-IDis set, and the same id appears in the logs beside a full traceback.Update: code-review fixes (commits 7–8)
A
/code-review highpass over this branch found eight issues. Six are fixed here; two are deliberately deferred.The blanket 5xx masking was too blunt in one direction.
POST /endpoints/testandinvoke_endpointexist to report what is wrong with the caller's endpoint — a refused connection, a rejected token — and masking those told the user nothing while protecting nothing of ours.UpstreamHTTPExceptionnow marks that case and the global handler passes its detail through.The exemption is deliberately narrow, because the obvious version of it would have introduced a new leak. Two places where the boundary was not where it looked:
EndpointServicewraps our own exceptions inEndpointInvocationErroraserror_type="internal_error"(services/endpoint/service.py:348), so the router branches on that discriminator rather than trusting the exception type.testing.py'strycovered endpoint construction and input enrichment as well as the invocation, so the exemption is scoped to theinvoke()call alone and setup failures still go throughinternal_error.A test asserts a plain 500 is still masked, so the exemption cannot quietly widen.
Also fixed: CORS
expose_headersomittedX-Request-ID, so browser JS could never read the id the middleware exists to emit;internal_error(status_code=400)answered "An unexpected error occurred.", describing a client error in server-error words;internal_errorlogged and then the handler logged the same failure again; both the middleware and the handler setX-Request-ID, so every 5xx carried it twice plus a redundantX-Error-Id; andPOST /tests/bulkwith an empty list returned 500 instead of 400 (pre-existing, but this branch stripped the message that made it diagnosable).Deferred, tracked for follow-up:
ValueErrorbleed paths — five sites inservices/launder a provider or parser error into aValueErrormessage that narrow router handlers then hand to the user (raise ValueError(f"LLM generation failed: {e}")). Same concern as this PR, one hop removed, and invisible to the guard because it crosses araise/catchboundary. Also deferred: inboundX-Request-IDis adopted from any caller, so a client can reuse an id and make correlation ambiguous.Revised review order
error_handlers.py+utils/request_context.py(commits 1, 8) — the contract. The middleware-ordering argument for reading the id from the request scope, thestatus_code < 500passthrough, and theUpstreamHTTPExceptionexemption and its two guards.test_router_error_leaks.py(commit 5) — defines what counts as a leak.garak.pyis the riskiest (unwrappedtryblocks with dedented bodies).Verification
5795 backend tests pass, 43 skipped. Zero leaks across all 52 routers.
Update: second review pass (seven follow-up commits)
A review of the whole branch found the masking went too far in one direction and not far enough in another. Both are fixed here, along with a bug that made this PR's central promise false in every deployed environment.
Logs held no traceback in production.
JsonLogFormatterbuilt its payload fromrecord.getMessage()and never readrecord.exc_info, andjsonLoggerEnabled: trueis set in dev, stg and prd. Sologger.exceptionemitted one line with no stack, no frames and no exception class — the client was told nothing and the log kept onlystr(exc). That is the half of "mask the response, log the reason" that was silently not happening, and it bit hardest exactly where this branch deleted broad handlers on the grounds that the global handler would log the same traceback. Tracebacks now land in astack_tracefield, redacted. The request id is also passed explicitly on the unhandled path, becauseServerErrorMiddlewareruns afterRequestIDMiddlewareclears the ContextVar — the field had been null on precisely the lines anerror_idpoints at.Deliberate 5xx messages had no way through.
PublicHTTPExceptionpasses a literal detail through and logs at WARNING with no stack, since a hand-written 503 is not a fault to debug and its traceback only points back at the raise.UpstreamHTTPExceptionbecomes one case of it. This restores "Garak package is not installed", "No Celery workers available" and "Failed to send test request to SDK" — strings this branch left in the code while the handler replaced them, so the code read as if they still arrived.internal_errorgainspublic_detailfor the same reason at 4xx.A wrong credential for a service the user connected themselves was masked.
handle_mcp_exceptiondeliberately remaps an MCP 401/403 to a 502 so the reason survives and the frontend does not clear the session — and the global handler masked exactly that, so a bad Notion token read as our service being down. It returnsUpstreamHTTPExceptionnow, and only forMCPApplicationError, whose detail really is the tool's parsed response.services.pyhad stripped provider reasons at 400, a status this PR's own contract says passes through, turning "invalid api key" into "Failed to generate content" while logging it at ERROR with a stack as though it were ours.tools.pyreports an unreachable instance again, in a drawer whose only job is showing why a connection failed.The branch had introduced a leak of its own.
services/endpoint/testing.pywrappedinvoke()in a bareexcept Exceptionand declared whatever it caught to be upstream detail, shippingstr(exc)on a 500 — including SQLAlchemy errors carrying the statement and its bound parameters, andHTTPExceptions that stringify to"400: ...", so a status code arrived inside a 500 body. The invokers return an error response for genuine upstream failures, so almost nothing that block caught was upstream at all.rest_invokerdid the same thing through a channel nothing was watching:str(e)in an HTTP 200 body, invisible to the guard because there is nodetail=.test_endpoint_mapping, whose only job is explaining what is wrong with the mappings the user just typed, had been left out of the exemption entirely.Caller faults were logged as server faults. 422s logged at ERROR with a traceback immediately before the function whose docstring says a 422 is not a server fault. Login failures reported an IdP outage as a 400 with one stackless warning, where
mainlogged an ERROR with a traceback — both axes wrong at once. A single connection-refused against a user's endpoint produced three ERROR records and two tracebacks; it is now one warning.Redaction was wrong in both directions. It mangled ordinary diagnostics —
api key: not configuredbecameapi key: [REDACTED] configured— while missing the secrets that actually arrive: provider errors readIncorrect API key provided: sk-…, with filler between the keyword and the value, and header dumps put a quote between the name andBearer …. The URL pattern never matched SQLAlchemy's realpostgresql://scheme, and the Celery broker isredis://. Both directions are fixed, with key shapes matched without needing a keyword at all.The guard let 14 of 20 crafted leaks through. The likeliest regression was the simplest: only
detail=keywords were inspected, soHTTPException(500, f"...{e}")with a positional detail was invisible.BROAD_EXCEPTIONSheld onlyExceptionandBaseException, treatingSQLAlchemyErrorandhttpx.HTTPErroras narrow and deliberate despite carrying SQL text and connection strings. Taint followed plain assignment only, so+=, tuples, subscripts and attributes all escaped. Scanning now coversrouters/,utils/andservices/recursively;services/invokers/is exempt because every string there reports on the user's own endpoint.Two things worth pushing back on
The guard no longer flags a 4xx
detailbuilt from an exception, which it used to. Sub-500 details pass through by design and three live sites depend on it, but this is less coverage than before, not a refactor.get_client_credentials_tokenchanged from 500 to 502, with two tests updated. More accurate — it is the caller's own token endpoint failing — but it is a status change on a public path.Known gaps, deliberately not fixed here
15 pre-existing leaks across 8 files put
str(e)in a 200 body or a result object. They are the same bug this PR removes, through a channel it never examined, and they are ratcheted inKNOWN_LEAKSso the count can only fall. Separately:routers/file_import.pyordersexcept ValueErrorahead ofexcept ValidationError, so a caller fault lands in the 404 arm; the SDK'smcp/agent.pywraps any exception asMCPValidationErrorat 422, a sub-500 that passes through unmasked;services/github.pyanswers 200 with an empty body for an invalidrepo_url; and an unhandled 500 comes fromServerErrorMiddleware, outsideCORSMiddleware, so browser JS can read neither its body norX-Request-ID— meaning theexpose_headerschange earlier in this PR does not help that path. A test now pins that behaviour so it cannot change silently.Verification (current)
6165 passed, 43 skipped, 1 xfailed. Ruff clean on all changed files apart from six findings that reproduce on
origin/main.cd apps/backend uv sync --extra all --extra ee uv run pytest ../../tests/backend/security/ ../../tests/backend/routes/ ../../tests/backend/app/ \ ../../tests/backend/auth/ ../../tests/backend/utils/ ../../tests/backend/tasks/ \ ../../tests/backend/services/ -q -p no:randomlyThe traceback fix is the one thing no test in the earlier rounds would have caught, so it is worth seeing directly: