fix(api): scrub driver errors from the preview endpoint and answer 400, not 500 - #258
Merged
Merged
Conversation
…0, not 500
POST /reports/{name}/preview caught only ConfigurationException. It opens a live
connection and runs SQL, so a bad filter value (say Equals "abc" against an
integer column) surfaced the raw SqlException/PostgresException/OracleException —
which names the host, port and database — as an unhandled 500, and on a host
running in Development the developer exception page renders the whole stack
trace. Every sibling endpoint that touches a source already routes failures
through SchemaProblem (logged server-side, generic to the caller); this one now
does too.
That alone would have turned a second bug into a misleading 502: a code-first
report's name is only checked for non-blank, so "sales.daily" is legal, while a
config store validates its argument against the dynamic-name pattern and throws
for it. ReportPreviewRunner probed the store before deciding the report was
typed, so that ArgumentException escaped as a 500. A name the store cannot hold
is definitively not a dynamic report, so it is now recognised as such and the
endpoint returns the clear 400 it always intended.
The regression test initially passed for the wrong reason - TestApp registers no
config store unless AddDynamicReports is called, so the runner short-circuited
before reaching the name check. Hardened to register a real file-backed store;
it now fails without the guard (502 instead of 400).
The Sonar gate flagged new_coverage 0%: the two new lines are the catch and its SchemaProblem return, and nothing exercised them — the existing regression test takes the name-guard path, which returns 400 before any driver call. So the security fix itself was untested. Add a filter translator that fails the way a provider does, with a message naming the connection target, and assert the response is the scrubbed 502 and carries neither the host, the database, nor the driver text. Verified to fail without the catch (the exception escapes with the connection string intact).
|
|
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
1 participant
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.
What
POST /reports/{name}/previewcaught onlyConfigurationException. It opens a live connection and runs SQL, so a bad filter value (Equals "abc"against an integer column) surfaced the rawSqlException/PostgresException/OracleException— naming the host, port and database — as an unhandled 500. On a host running inDevelopment, the developer exception page renders the full stack trace too.Every sibling endpoint that touches a source already routes failures through
SchemaProblem(logged server-side, generic 502 to the caller). This one now does the same — it was the only data-plane endpoint left unscrubbed after WP5.A second bug the first fix would have masked
A code-first report's name is only checked for non-blank, so
sales.dailyis legal — while a config store validates its argument against the dynamic-name pattern and throws for it.ReportPreviewRunnerprobed that store before deciding the report was typed, so theArgumentExceptionescaped as another 500.Adding only the catch-all would have converted it into a misleading 502 ("the source's database could not be read") for what is really "this report isn't filterable". A name no config store can hold is definitively not a dynamic report, so it is now recognised as such and the endpoint returns the clear 400 it always intended.
Verification
Filters_on_a_typed_report_whose_name_the_config_store_cannot_hold_return_400.TestAppregisters noIReportConfigStoreunlessAddDynamicReportsis called, so the runner short-circuited onconfigStore is nulland never reached the name check. Hardened to register a real file-backed store; it now fails without the guard (502 instead of 400).Found by the API-layer bug hunt; remaining findings in
docs/STATUS-AND-BACKLOG.md§6.