Skip to content

fix(security): route /v1/search's internal-exception branch through buildErrorBody - #15951

Merged
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.52from
jonlwheat2-gif:fix/15159-w1-3-search-internal-error-sanitize
Oct 9, 2026
Merged

diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.52from
jonlwheat2-gif:fix/15159-w1-3-search-internal-error-sanitize

Conversation

@jonlwheat2-gif

Copy link
Copy Markdown
Contributor

Summary

Wave 1.3 of the #15159 audit.

src/app/api/v1/search/route.ts caught any unexpected internal exception and forwarded its raw message to the client:

log.error("SEARCH", `Unexpected error: ${err.message}`);
const errorPayload = toJsonErrorPayload(err.message, "Internal search error");

The leak is that toJsonErrorPayload was handed an internal exception, not an upstream provider body. An internal exception's message is our own text: it can carry an absolute path, a SQL fragment, a credential echoed from a failed connection, or a stack tail.

The sibling alpha/search/route.ts already routed this branch through buildErrorBody and carried a comment explaining why — one file apart, fixed on one side only. This converges the two.

Scope held deliberately

  • The SearchError branch above it keeps its existing toJsonErrorPayload call — that message is our own controlled provider-failure text.
  • The log.error stays unsanitized so the operator keeps the full diagnostic on the log path. Sanitization belongs on the response path, not the log path.

Testing

TDD, per Hard Rule #18: 3 tests watched red before the fix, 7 green after. 59 search tests pass. typecheck:core, lint, check:error-helper clean.

Guard is deliberately stronger than regex-only

The alpha/search precedent notes that source-matching tests "could not have caught" this class while falsely asserting errors were sanitized. So:

Reverting the fix and re-running caught a false-green in my own first draft. The convergence assertion passed on a prose comment naming buildErrorBody rather than a call to it — the exact false-green this guard must not have. catchTail now strips line comments, and all three source-shape assertions fail on revert.

Two behavioural assertions additionally prove the builder still redacts a credential, an absolute path and a stack frame, so the guard fails if either half regresses:

buildErrorBody -> {"error":{"message":"ENOENT: ... open '<path>' api_key=[REDACTED]",
                            "type":"internal_server_error","code":"internal_server_error"}}
leaks key? false   leaks path? false   leaks srv? false   leaks frame? false

⚠️ base-red inherited: #15306

…uildErrorBody

Wave 1.3 of the diegosouzapw#15159 audit.

src/app/api/v1/search/route.ts caught any unexpected internal exception and
forwarded its raw message to the client via
toJsonErrorPayload(err.message, "Internal search error").
toJsonErrorPayload is built for upstream provider bodies and returned an
unparseable plain string straight through with zero sanitization. An internal
exception's message is our OWN text: it can carry an absolute path, a SQL
fragment, a credential echoed from a failed connection, or a stack tail.

The sibling alpha/search/route.ts already routed this branch through
buildErrorBody and carried a comment explaining why - one file apart, fixed on
one side only. This converges the two. The SearchError branch above it keeps
its existing toJsonErrorPayload call, because that message is our own
controlled provider-failure text, and the log.error stays unsanitized so the
operator keeps the full diagnostic on the log path.

Guard is deliberately stronger than regex-only: reverting the fix and re-running
caught a false-green in my own first draft, where the convergence assertion
satisfied itself on a prose comment naming buildErrorBody instead of a call to
it. catchTail now strips line comments, so all three source-shape assertions
fail on revert. Two behavioural assertions additionally prove the builder still
redacts a credential, an absolute path and a stack frame - the gap the alpha
search precedent flagged, where source matching "could not have caught" the
leak while asserting errors were sanitized.

Verified: 3 tests watched red on revert, 7 green after; 59 search tests pass;
typecheck:core, lint and check:error-helper clean.
@diegosouzapw
diegosouzapw merged commit a0bcbbe into diegosouzapw:release/v3.8.52 Oct 9, 2026
29 of 51 checks passed
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks @jonlwheat2-gif — the /v1/search internal-exception response now goes through buildErrorBody, so the client body is sanitized. This is in the upcoming release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants