Skip to content

fix(control): classify native error frames authored as a bare SQLSTATE - #241

Open
laksamanakeris wants to merge 1 commit into
NodeDB-Lab:mainfrom
laksamanakeris:fix/native-sqlstate-frames-carry-ndb-code
Open

fix(control): classify native error frames authored as a bare SQLSTATE#241
laksamanakeris wants to merge 1 commit into
NodeDB-Lab:mainfrom
laksamanakeris:fix/native-sqlstate-frames-carry-ndb-code

Conversation

@laksamanakeris

Copy link
Copy Markdown
Contributor

Fixes the part of #239 that survived 5ef7fc13c / ee71c2cda. Replaces #240, which is closed: its approach (rebuild the type from SQLSTATE on the client) was superseded by the numeric ndb_code on the wire, which is the better mechanism.

What is wrong

A native error frame carries the stable numeric NodeDB code so the client can rebuild a typed error. A frame that ships ndb_code == 0 is documented as "peer predates that field" and folds to internal on arrival, so is_not_found(), is_auth_denied() and is_retriable() all answer wrongly for it.

5ef7fc13c and ee71c2cda reached every frame rendered from a classified Error. They did not reach the frames that never held one:

  • DDL refusals. DdlError is { sqlstate, message }, authored that way in ~600 places across the DDL layer, and has no numeric code to carry. ddl_result_to_native therefore built its frame with NativeResponse::error, whose own doc says a frame built there "collapses to a generic internal error on the far side". So DROP TABLE does_not_exist arrived as ErrorCode(9000) / Internal while the identical SELECT * FROM does_not_exist arrived as CollectionNotFound.
  • Guard rejections. About 25 sites in direct_ops, graph_match, session/auth, session/request, transaction_savepoint, sql_admin, single_task, streaming and run reject a request with a literal SQLSTATE. An authorization denial on a direct op, a tenant-isolation refusal and a database-admission refusal were all Internal on the client.

What this changes

sqlstate_code — one SQLSTATE-to-numeric-code table, used only where the SQLSTATE is the sole classification the server ever produced.

This is the inverse of the rule NodeDbError::from_wire states, and deliberately so. On the client every SQLSTATE arrives through one funnel, so a reverse mapping would have to resolve 23505 into either a unique violation or a duplicate idempotency key with no way to tell them apart; that is why the numeric code exists. Here the lookup runs on the server at the site that chose the SQLSTATE, and the table only carries SQLSTATEs whose classification is unambiguous whatever site emitted them.

Everything else maps to 0, which is exactly the frame shipped today, so no path this table does not cover is made worse. Three groups stay unmapped on purpose:

  • Overloaded SQLSTATEs. 53400 is four NodeDB variants, 0A000 is three. A caller that knows which one it is passes the code explicitly instead (see the two admission refusals below).
  • SQLSTATEs with no NodeDB variant. 42P07, 42704, 25P02, 3B001. Typing these needs new ErrorCode / ErrorDetails variants plus msgpack tags and from_wire arms; that is a public-API change, kept out of this PR and recorded on Native client discards the server's SQLSTATE: every server error becomes ErrorCode(9000) Internal, and retriability is lost #239.
  • Deliberately undistinguished SQLSTATEs. Every credential failure renders as 28P01 precisely so a caller cannot tell a wrong password from an unknown user. ilp_auth's write_safe_failure is untouched for the same reason. Typing them would rebuild the oracle that collapsing removed.

error_to_native_with_sqlstate — for the sites that still hold a classified Error but render a more specific SQLSTATE than the error implies (a plan that cannot be built is 42601 to a SQL client, an RLS refusal is 42501). These keep the site's SQLSTATE and take the code from error_classify, never from the SQLSTATE they just chose, so the one classification table stays the source.

Explicit codes where the site knows more than the SQLSTATE. The two admission refusals in handle_auth pass DATABASE_QUOTA_EXCEEDED / TENANT_QUOTA_EXCEEDED, since the shared 53400 cannot say which pool refused. A rejected frame in run.rs passes BAD_REQUEST, since 54000 cannot say the request was malformed.

INVALID_CATALOG_NAME (3D000) is added to nodedb-types' SQLSTATE constants, which had every other class the server emits but not that one.

Tests

  • End-to-end, at the frame level in native_error_code_classification.rs, alongside the existing ones: DROP TABLE naming an absent collection now carries 42P01 + COLLECTION_NOT_FOUND; a duplicate CREATE COLLECTION still carries 42P07, its message verbatim and ndb_code == 0, pinning that the unmapped fallback is byte-identical to what shipped before.
  • sqlstate_code unit tests cover the mapped codes, the 40001 case reconstructing as retriable through from_wire (the retry-loop bug this issue opened on), and each unmapped group staying at 0.
  • conversion.rs unit tests cover the DDL frame and the site-chosen-SQLSTATE case, where the assertion is specifically that the code comes from the error and not from the SQLSTATE.

Gates

  • cargo fmt --all --check clean.
  • cargo clippy --no-deps -p nodedb-types -p nodedb --all-targets --all-features -- -D warnings: the only failures are 7 pre-existing dead_code errors in data/executor/handlers/join/grace_spill.rs test helpers, verified identical on unmodified main (git stash + rerun). The workspace-wide run also fails in nodedb-vector/src/quantize/pq.rs (nonminimal_bool) on this toolchain, likewise pre-existing.
  • cargo nextest run -p nodedb --test native_error_code_classification: 7/7, including the 2 added here.
  • cargo nextest run -p nodedb --test native_protocol --test native_transactions_savepoint --test native_sql_authorization --test native_handshake_e2e --test startup_gate_native: 27/27, the suites covering the rewritten guard sites.
  • cargo nextest run -p nodedb-client-tests --test native_typed_error_classification --all-features: 3/3, the client half of the contract.
  • cargo nextest run -p nodedb --lib for the touched modules: 9/9.
  • cargo deny check not run (cargo-deny not installed here); no dependencies are added or changed.

Deferring to CI for the authoritative full-suite result.

Native error frames carry the stable numeric NodeDB code so the client can
rebuild a typed error; a frame that ships ndb_code == 0 collapses on arrival
into a generic internal failure, so is_not_found(), is_auth_denied() and
is_retriable() all answer wrongly for it.

Frames rendered from a classified Error already carried their code. The ones
that never held an Error did not: a DDL refusal (DdlError is authored as a
SQLSTATE plus a message in ~600 places and has no numeric code to carry), and
the session and dispatch guards that reject a request with a literal SQLSTATE.
DROP TABLE naming an absent collection therefore reached the client as an
internal failure while the identical SELECT arrived typed.

Adds sqlstate_code, a table from SQLSTATE to numeric code, used only where the
SQLSTATE is the sole classification the server ever produced. It is the inverse
of the client-side rule in NodeDbError::from_wire and deliberately so: there
every SQLSTATE arrives through one funnel and 23505 cannot be resolved back to
a unique violation or a duplicate idempotency key, while here the lookup runs
at the site that chose the SQLSTATE. Overloaded SQLSTATEs (53400, 0A000),
those with no NodeDB variant (42P07, 42704, 25P02, 3B001) and the deliberately
undistinguished credential failures (28P01) map to 0, which is exactly the
frame shipped before, so an unmapped SQLSTATE is never worse off.

Sites that still hold a classified Error but render a more specific SQLSTATE
than the error implies (a plan that cannot be built is 42601, an RLS refusal
is 42501) go through error_to_native_with_sqlstate, which keeps the site's
SQLSTATE and takes the code from error_classify. The two admission refusals in
the auth path pass their code explicitly, since the shared 53400 cannot say
whether the database or the tenant pool refused.

ILP auth is left alone: every failure there renders as one code and one
message so a caller cannot tell a wrong password from an unknown user, and
typing them would rebuild the oracle that collapsing removed.
@laksamanakeris

Copy link
Copy Markdown
Contributor Author

Pushed e07236889, which fixes the Static gates failure. It is not from this PR: the reconstructed-SQL gate fails identically on unmodified main (python3 scripts/ci/check_reconstructed_sql.py on 5a41a5962, no local changes).

043d76090 (refactor(control): split scatter_gather into a directory) moved build_graph_traverse_sql and its canonical_direction_sql / canonical_label_sql helpers from control/scatter_gather.rs to control/scatter_gather/remote_sql.rs, but the gate's PATH_CANONICAL_HELPERS key stayed on the old path. direct_canonical only treats a bare helper call as canonical when its name is in that per-path set, so the two helper-built fragments started reading as unquoted interpolation. The quoting in that file never changed. Since Static gates runs on pull_request only, main carries the break with nothing to report it, and every PR opened since inherits the failure.

Repointing the key is the whole fix. It does not widen the gate: swapping quote_literal(node_id) for a bare node_id in that same format! still fails, so the site keeps its teeth for anything the two allowlisted helpers do not cover. Self-tests pass and the full scan is clean.

Worth noting what the fix is not: adding an exact-site // reconstructed-sql: parser-only comment there would be wrong twice over. That marker exists for parse_sql sites that only validate syntax, and this text is shipped to a remote shard and re-planned, so it is an execution sink; and the marker suppresses the check at that site, so a genuinely unquoted argument added later would pass unnoticed. Marking a site as an exception to hide an allowlist that rotted out of date would trade a stale path for a permanently blind one.

Happy to split this into its own PR if you would rather land the CI fix separately.

@laksamanakeris

Copy link
Copy Markdown
Contributor Author

Static gates has a second pre-existing failure behind the first one, the authorized-dispatch gate. Same check: it fails identically on unmodified main @ 5a41a5962. It is two unrelated findings, and I have only fixed the first.

Fixed in 14913273c (allowlist rot). 7102813b8 moved the native DROP ARRAY path out of sql_loop.rs into the new sql_dispatch_task.rs, taking its authorize_native_task(..)? -> into_physical_task() pair along. sql_loop.rs is in ALLOWED_REFERENCES for that exact seam and the new file was not, so the moved call has been flagged since that commit. The capability is still consumed before the raw task is reached, which is the condition the allowlist documents.

Not fixed, needs a maintainer decision: the six into_scope references plus the public-definition violation. I believe these are a false positive, and the right fix is not an allowlist entry.

The gate matches forbidden APIs by name only. into_scope is on that list for the authorization-capability type, which is why the three control/array_sync/* sites (authorization.into_scope(), consuming the capability) are individually allowlisted. cc3b19989 then added an unrelated method with the same name, ClientRequestScope::into_scope at control/security/request_scope/client_scope.rs:113, whose body is self.scope and whose doc says "Consume the binding once admission has run, keeping the scope". It unwraps a peer-address binding; it consumes no capability and grants no authority. Every transport that unwraps it (native, pgwire, RESP, and three HTTP routes) now trips a rule written about a different type.

Bisected: 1 violation from e00d371c0 (2026-08-09, the moved seam above), 8 from cc3b19989 (2026-08-10).

Adding the six to ALLOWED_REFERENCES would be the wrong fix: those entries are keyed (path, name), so exempting into_scope in pgwire/handler/routing/planning.rs also exempts a genuine capability into_scope appearing in that file later. That trades a false positive for a permanent blind spot in six transport files, which is the opposite of what this gate is for.

Two fixes that keep the gate strict, both maintainer calls since they touch the security surface rather than my change:

  1. Rename the new accessor (into_resolved_scope, say) at its definition and its six call sites. Smallest diff, no gate change, the forbidden name goes back to meaning one thing.
  2. Make the gate distinguish the two by receiver or defining type instead of by bare name.

Happy to prepare either as a separate PR. Flagging rather than doing it, because silencing a security gate is not a call I should make inside an unrelated error-classification PR.

Note that Static gates runs on pull_request only, so neither of these failures had anywhere to report on main, and every PR opened since 2026-08-09 inherits them.

@laksamanakeris

Copy link
Copy Markdown
Contributor Author

Moved the CI work out of this PR. #242 now carries all three Static gates fixes (the two allowlist repoints plus the ClientRequestScope::into_scope rename), and this branch is force-pushed back to just the error-classification commit, so the diff here is only what the title says.

Static gates will stay red on this PR until #242 lands, since the failures are on main and not from this change. Everything else here is unaffected.

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.

1 participant