Repository navigation
Fix/security scan findings - #24
Merged
Merged
Conversation
Error envelopes returned the raw upstream exception, disclosing the
query engine's name, the cloud region and internal job identifiers to
anonymous callers.
`APIError` gains a `detail` field: `message` is what the caller sees,
`detail` is operator-only and reaches the log alone. Inverting the split
this way makes the safe path the default - a new `ServerError("...")`
without `detail` cannot leak. `str(exc)` still folds in the detail, so
tracebacks stay useful.
Every engine raise that interpolated an exception into `message` is
converted, plus two of the same shape found by scanning for the pattern:
the SQL-download dry run and the CKAN transport error, the latter
carrying the internal cluster address.
`_classify_bq_error` additionally maps BigQuery's caller-error reasons
(`invalidQuery`, `invalid`, `notFound`) to a 400 rather than a 500 - an
unknown column or a type-mismatched filter is the caller's mistake. The
reason is read from `errors[0]`, not the `reason` attribute, which is
None on a real `BadRequest`.
5xx now logs at ERROR with the detail and the original traceback; it was
DEBUG, so in production the detail reached the client and nothing else.
A `resource_id` containing a null byte was passed straight through to
CKAN, which 500'd on it, and the API surfaced that as `Internal Error`.
Malformed input is the caller's mistake and belongs in a 400 raised
before any upstream call.
Allow-list rather than block-list: `^[A-Za-z0-9_-]{1,200}$`. Blocking
the null byte would only invite blocking the newline next, then the
semicolon, and so on. The pattern covers both shapes actually in use -
CKAN UUIDs and the table-style names `datastore_create` accepts and the
docs' own examples use - so it rejects traversal, quotes, separators,
control characters and over-length values without narrowing the API. A
UUID-only rule, as the report suggested, would have broken documented
behaviour.
Applied to every `resource_id` field, to the `id` alias on info/delete
(validating only one of the pair would leave a bypass), and to the
`dump/{resource_id}` path parameter, which no request model covers.
The rejection message states what is allowed rather than echoing the
input, so control characters do not reach the response body or the logs.
A V4 signed URL carries the signer's service-account email in `X-Goog-Credential`, where it is public. Signing with the rw account advertised the write identity on every download redirect, and issued a grant from an account that could also overwrite the object it points at. A blob signs through whichever client listed it, and the export path lists through rw, so `_signed_urls` re-binds each blob to the ro bucket before signing. Both the cache-hit and cache-miss paths converge on this one call, so neither can sign as rw. Only signing moves. The export, compose, publish and GC steps still run as rw, which they must. Requires an IAM change before deploy: the ro service account needs `iam.serviceAccountTokenCreator` on itself (IAM `signBlob` under workload identity) plus `storage.objects.get`. Signing will fail at runtime until that grant lands. Section 5.3's IAM table is updated to match - it previously documented signing as an rw operation. The service-account email cannot be removed from a signed URL; that is inherent to the signing scheme. What changes is that the account it names can only read.
Parquet exported `*`, keeping BigQuery's native TIMESTAMP, while csv/ndjson cast to a whole-second ISO string. The same row therefore read back as `2026-09-11T07:08:35` from a csv download and `2026-09-11T07:08:35.908256` from a parquet one. Parquet now applies the same `format_select_column` cast as the other formats, keeping its JSON-to-string cast, which BigQuery still requires. `*` is retained when a table has neither timestamp nor JSON columns, so the common case keeps the short form. Affects both download endpoints, which share this select-list builder. Tradeoff: parquet now carries timestamps as strings rather than a native TIMESTAMP column, which is what matching csv exactly costs - csv can only carry text. If typed timestamps matter more than cross-format equality, the alternative is widening the csv format string to microseconds instead.
A request to a path with no route, or with the wrong method, came back
as FastAPI's default `{"detail": "Not Found"}` rather than the
`{help, success, error:{__type, message}}` envelope every other error
uses, so a client parsing the documented shape needed a special case.
`fastapi.HTTPException` subclasses Starlette's, and it is Starlette's
that the router raises for 404 and 405 - registering only the subclass
never caught them. Registering the base class alongside it covers both
without changing any existing behaviour.
`sort=", ,"` parsed to an empty list and was silently ignored, so a client that built the string wrong got unsorted rows and a 200 rather than being told. `"person_id,,first_name"` had the same problem mid- string, silently dropping nothing but hiding the typo. An empty term between commas is now a ValueError, which surfaces as a 400. A wholly blank `sort` still means "no sort" - that is absence rather than a malformed value, and remains accepted.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
No description provided.