fix(control): classify native error frames authored as a bare SQLSTATE - #241
Conversation
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.
|
Pushed
Repointing the key is the whole fix. It does not widen the gate: swapping Worth noting what the fix is not: adding an exact-site Happy to split this into its own PR if you would rather land the CI fix separately. |
|
Static gates has a second pre-existing failure behind the first one, the authorized-dispatch gate. Same check: it fails identically on unmodified Fixed in Not fixed, needs a maintainer decision: the six The gate matches forbidden APIs by name only. Bisected: 1 violation from Adding the six to Two fixes that keep the gate strict, both maintainer calls since they touch the security surface rather than my change:
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 |
1491327 to
9384161
Compare
|
Moved the CI work out of this PR. #242 now carries all three Static gates fixes (the two allowlist repoints plus the Static gates will stay red on this PR until #242 lands, since the failures are on |
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 numericndb_codeon 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 == 0is documented as "peer predates that field" and folds tointernalon arrival, sois_not_found(),is_auth_denied()andis_retriable()all answer wrongly for it.5ef7fc13candee71c2cdareached every frame rendered from a classifiedError. They did not reach the frames that never held one:DdlErroris{ sqlstate, message }, authored that way in ~600 places across the DDL layer, and has no numeric code to carry.ddl_result_to_nativetherefore built its frame withNativeResponse::error, whose own doc says a frame built there "collapses to a generic internal error on the far side". SoDROP TABLE does_not_existarrived asErrorCode(9000)/Internalwhile the identicalSELECT * FROM does_not_existarrived asCollectionNotFound.direct_ops,graph_match,session/auth,session/request,transaction_savepoint,sql_admin,single_task,streamingandrunreject a request with a literal SQLSTATE. An authorization denial on a direct op, a tenant-isolation refusal and a database-admission refusal were allInternalon 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_wirestates, and deliberately so. On the client every SQLSTATE arrives through one funnel, so a reverse mapping would have to resolve23505into 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:53400is four NodeDB variants,0A000is three. A caller that knows which one it is passes the code explicitly instead (see the two admission refusals below).42P07,42704,25P02,3B001. Typing these needs newErrorCode/ErrorDetailsvariants plus msgpack tags andfrom_wirearms; 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.28P01precisely so a caller cannot tell a wrong password from an unknown user.ilp_auth'swrite_safe_failureis 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 classifiedErrorbut render a more specific SQLSTATE than the error implies (a plan that cannot be built is42601to a SQL client, an RLS refusal is42501). These keep the site's SQLSTATE and take the code fromerror_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_authpassDATABASE_QUOTA_EXCEEDED/TENANT_QUOTA_EXCEEDED, since the shared53400cannot say which pool refused. A rejected frame inrun.rspassesBAD_REQUEST, since54000cannot say the request was malformed.INVALID_CATALOG_NAME(3D000) is added tonodedb-types' SQLSTATE constants, which had every other class the server emits but not that one.Tests
native_error_code_classification.rs, alongside the existing ones:DROP TABLEnaming an absent collection now carries42P01+COLLECTION_NOT_FOUND; a duplicateCREATE COLLECTIONstill carries42P07, its message verbatim andndb_code == 0, pinning that the unmapped fallback is byte-identical to what shipped before.sqlstate_codeunit tests cover the mapped codes, the40001case reconstructing as retriable throughfrom_wire(the retry-loop bug this issue opened on), and each unmapped group staying at0.conversion.rsunit 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 --checkclean.cargo clippy --no-deps -p nodedb-types -p nodedb --all-targets --all-features -- -D warnings: the only failures are 7 pre-existingdead_codeerrors indata/executor/handlers/join/grace_spill.rstest helpers, verified identical on unmodifiedmain(git stash+ rerun). The workspace-wide run also fails innodedb-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 --libfor the touched modules: 9/9.cargo deny checknot run (cargo-denynot installed here); no dependencies are added or changed.Deferring to CI for the authoritative full-suite result.