Hide SQL details in production JSON/NDJSON/SSE/CSV error responses#1308
Merged
lovasoa merged 1 commit intoJun 11, 2026
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf94b4525c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0ab1fe3 to
cc50183
Compare
…format In production, error responses must not leak the SQL statement, the source file path, the raw database error, environment values, or configuration, for ANY output format. In development the full detail is shown, and the full error is always logged server-side. This centralizes the dev-vs-production decision in a single place. An internal error stays a full `anyhow::Error` everywhere; the only place that turns it into a user-facing representation is `ClientError::new` in `src/webserver/error.rs`, which is also the only caller of `DevOrProd::is_prod`. Every renderer (HTML, JSON, NDJSON, SSE, CSV) and the header/pre-body path obtains a `ClientError` from that one function and only formats it; none of them inspect the environment. Leaking is therefore impossible by construction: a renderer cannot emit what it never receives. The error type, the production message, `get_backtrace_as_strings`, and the error-component data construction live in `error.rs`; `render.rs` only renders components and formats a `ClientError`.
cc50183 to
07d2aef
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
hide SQL details in production error responses for every output format
In
production, the HTML renderer already hid DB errors behind a generic message, but the JSON, NDJSON, SSE, and CSV renderers wrote the full error (the.sqlpath, the exact SQL statement, and the raw DB error) into the response body once streaming had started. Since the format is picked from theAcceptheader, a visitor could request an HTML page withAccept: application/jsonand read back its SQL.Design
One centralized censoring point, no environment checks in the rendering logic:
anyhow::Erroreverywhere. Producers never think about dev/prod.ClientError::newinerror.rs. It is also the only caller ofDevOrProd::is_prod: in production it strips detail to a generic message, in development it keeps message/backtrace/hint. The full error is always logged server-side.ClientErrorand only formats it. None inspectenvironment. A new output format is safe by construction: it can only renderClientError's already-censored fields.render.rsnow renders components;error.rsowns turning errors into user-facing representations. Moved intoerror.rs: theClientErrortype, the production message,get_backtrace_as_strings, and the error-component data construction.Verify
The single environment check:
render.rsshrank to mostly renderer dispatch; the error type and policy live inerror.rs:Behavior is covered by tests in
tests/data_formats/mod.rs: production JSON, CSV, CSV-before-first-row, and an HTML-only page requested as JSON all return the generic message and leak nothing; a unit test inerror.rsasserts the productionClientErrorexposes no sensitive substring at the single boundary.