diff --git a/.changeset/error-leak-shipped-dialect-phrasings.md b/.changeset/error-leak-shipped-dialect-phrasings.md new file mode 100644 index 0000000000..28d80dca10 --- /dev/null +++ b/.changeset/error-leak-shipped-dialect-phrasings.md @@ -0,0 +1,24 @@ +--- +'@objectstack/types': patch +--- + +Withhold the Postgres and bare-SQLite phrasings of a driver failure from HTTP error bodies + +`looksLikeInternalErrorLeak` recognised SQLite's `SQLITE_ERROR: no such table: sys_metadata` +but not the Postgres phrasing of the same condition, `relation "sys_metadata" does not exist`. +The result was that one failure disclosed a physical table name or not depending on which +engine was underneath, from every boundary that applies the predicate — `HttpDispatcher.error`, +the declarative endpoint executor, the dispatcher plugin, the direct-mount package door and the +Hono auth-config route. + +The predicate now also recognises, for the engines this repo actually runs: + +- Postgres `relation "…" does not exist` and `column "…" does not exist` (42P01/42703), which + covers the `… of relation "…"` sub-object family as a superstring; +- Postgres `permission denied for table|relation|sequence|database …` (42501); +- SQLite/libsql `no such table:` / `no such column:` in their bare, un-prefixed form. + +Each phrasing is anchored on the driver's own template — a quoted identifier, or the trailing +colon — never on the bare tail, so ordinary business messages such as "user does not exist" are +still returned to the caller unchanged. The predicate is applied only where the outcome is +already a 5xx, and the full text still reaches the server log and the error reporter. diff --git a/packages/rest/src/package-door-5xx-message-sanitization.test.ts b/packages/rest/src/package-door-5xx-message-sanitization.test.ts index ab01860720..8be77af17c 100644 --- a/packages/rest/src/package-door-5xx-message-sanitization.test.ts +++ b/packages/rest/src/package-door-5xx-message-sanitization.test.ts @@ -232,29 +232,31 @@ describe('[#8086] a real sys_metadata failure, walked in process through this do }, 60_000); /** - * ⚠️ The CEILING of option B, measured and pinned rather than papered over. + * [#8132] Was the residual; now the second green. * - * The ruled fix applies the SHARED predicate, which is a heuristic over the - * message and recognises no Postgres "relation … does not exist" phrasing — - * measured false, asserted below. So that dialect's line still travels - * through this door after this change, exactly as it travels through the - * dispatcher twin, which runs the same predicate (#3867). The two doors - * therefore still AGREE, which is what this card was about; what remains is a - * property of the heuristic, shared by every boundary that applies it. + * This case was added by #8086 as a deliberately-red-in-future pin: the + * shared predicate was a heuristic over the message and knew no Postgres + * `relation … does not exist` phrasing, so that dialect's line still + * travelled through this door while SQLite's was withheld — the same + * condition, disclosed or not depending on which engine was underneath. It + * asserted that gap positively so the day it closed would be visible. * - * This is not an argument for widening the predicate here — that would be a - * new rule at one door, re-creating the divergence this closes. It is the - * argument for **option C**: `metadata-protocol` should not interpolate - * driver text into client-facing messages at all, which is the only fix that - * does not depend on recognising a dialect's phrasing. Filed separately. + * #8132 closed it in the predicate, where it belonged — so the assertion is + * INVERTED here rather than deleted, and the pair above/below now proves the + * property that actually matters: this door answers the same withheld + * envelope for BOTH dialects of one failure. * - * This case goes RED the day the shared predicate learns this phrasing or C - * lands — which is precisely when a reader should come back and re-read the - * paragraph above, instead of consuming a green suite as proof that the door - * is covered. + * ⚠️ Still not the structural cure, and this comment is the reason the + * pointer survives the flip. The predicate now recognises the two engines + * this repo runs; it is a phrasing test, and a phrasing test can only ever + * know the dialects someone has met. **Option C** — `metadata-protocol` not + * interpolating driver text into client-facing messages at all — is the fix + * whose correctness does not depend on that, and is tracked as #8136. The + * anti-vacuity guard at the top of this describe block goes red when C + * lands, which is the intended signal to revisit this whole section. */ - it('the residual: Postgres phrasing trips no keyword, so it still travels (option C is the cure)', async () => { - expect(looksLikeInternalErrorLeak(PG_NO_RELATION)).toBe(false); + it('the Postgres phrasing of the same failure is withheld too, by the shared predicate', async () => { + expect(looksLikeInternalErrorLeak(PG_NO_RELATION)).toBe(true); const protocol = await bootRealProtocol(PG_NO_RELATION); const captured = await drive( @@ -267,10 +269,13 @@ describe('[#8086] a real sys_metadata failure, walked in process through this do const error = expectDeclaredEnvelope(captured); expect(captured.status).toBe(500); expect(error.code).toBe('INTERNAL_ERROR'); - // Stated as the fact it is: withheld would be better, and the predicate - // cannot tell. Asserted positively so the day it changes is visible. - expect(error.message).toContain('does not exist'); - expect(error.message).not.toBe(INTERNAL_ERROR_MESSAGE); + // The same positive shape the SQLite case asserts, which is the point of + // the flip: one door, one envelope, regardless of the engine underneath. + expect(error.message).toBe(INTERNAL_ERROR_MESSAGE); + + const wire = JSON.stringify(captured.body); + expect(wire).not.toContain('does not exist'); + expect(wire).not.toContain('sys_metadata'); }, 60_000); }); diff --git a/packages/types/src/error-leak.test.ts b/packages/types/src/error-leak.test.ts index e84f29ef62..3dff23db28 100644 --- a/packages/types/src/error-leak.test.ts +++ b/packages/types/src/error-leak.test.ts @@ -87,6 +87,81 @@ describe('looksLikeInternalErrorLeak', () => { }); }); +/** + * [#8132] The shipped dialects' phrasings, both directions. + * + * The gap this pins: the keyword set caught SQLite's `SQLITE_ERROR: no such + * table: sys_metadata` (via the `sqlite_` limb) while the Postgres phrasing of + * *the same condition* — `relation "sys_metadata" does not exist` — returned + * FALSE and shipped a physical table name from every boundary that applies the + * predicate. + * + * Scope is deliberately the dialects this repo actually RUNS (SQLite/libsql and + * Postgres, via `driver-sql`), not a census of MySQL/MSSQL/Oracle spellings + * nobody here has met — the unbounded-list trap the module note argues against. + * + * The negative half is the load-bearing half. A bare `includes('does not + * exist')` would have matched "user does not exist" and started replacing + * ordinary business answers with "Internal server error", so every phrasing is + * anchored on the driver's own template (a QUOTED identifier, or the trailing + * colon) and the near-miss cases below are what prove that anchor is real + * rather than incidental. + */ +describe('looksLikeInternalErrorLeak — shipped-dialect phrasings (#8132)', () => { + it.each([ + // Postgres 42P01. The exact string measured false on the shipping predicate. + ['postgres missing relation', 'relation "sys_metadata" does not exist'], + [ + 'postgres missing relation wrapped in a producer sentence', + 'Failed to delete customization overlay: relation "sys_metadata" does not exist', + ], + // Postgres 42703, read path — no relation named, so the sub-object + // helpers in `relation-sub-object.ts` deliberately do not see it. + ['postgres missing column (read path)', 'column "bogus" does not exist'], + // Postgres 42703 write path / 42704: these carry a complete missing-TABLE + // phrase as a substring. For a LEAK verdict that overlap is harmless — + // both spellings are a leak — which is why this predicate needs none of + // the ordering care `matchMissingColumnOfRelation` exists to provide. + ['postgres missing column of relation', 'column "label" of relation "sys_team" does not exist'], + [ + 'postgres missing constraint of relation', + 'constraint "uq_sys_team_name" of relation "sys_team" does not exist', + ], + // Postgres 42501 — names a physical table the caller never asked about. + ['postgres permission denied for table', 'permission denied for table sys_user'], + ['postgres permission denied for relation', 'permission denied for relation sys_user'], + // SQLite/libsql message-only errors: the same conditions with NO + // `SQLITE_` prefix to trip the existing limb. Measured shapes in this + // repo — `metadata/src/utils/schema-sync-errors.ts` documents both. + ['sqlite bare missing table', 'no such table: sys_metadata'], + ['sqlite bare missing table with a schema prefix', 'no such table: main.sys_metadata_history'], + ['sqlite bare missing column', 'no such column: bogus'], + ])('catches %s', (_label, message) => { + expect(looksLikeInternalErrorLeak(message)).toBe(true); + }); + + /** + * ⛔ The false-positive guard. Every one of these contains the tail of a + * phrasing above and is an ordinary message a caller is entitled to read. + * If someone later relaxes an anchor to a bare `includes(...)`, these go red + * — which is the whole point of writing them down. + */ + it.each([ + ['a business message about a missing user', 'user does not exist'], + ['a business message about a missing record', 'record does not exist'], + ['a sentence a hook author wrote', 'The customer you selected does not exist'], + // The quote anchor, stated as a test: unquoted prose that uses the same + // NOUN is not a driver line. The looser `includes('relation') && + // includes('does not exist')` reading would match this one. + ['prose merely using the word relation', 'This relation does not exist in the diagram'], + ['prose merely using the word column', 'The column layout does not exist'], + // No physical object kind, so not Postgres' ACL template. + ['an ordinary permission refusal', 'Permission denied for this operation'], + ])('leaves %s alone', (_label, message) => { + expect(looksLikeInternalErrorLeak(message)).toBe(false); + }); +}); + /** * [#5811] The declaration half. `looksLikeInternalErrorLeak` asks whether a * message SOUNDS internal; this asks whether the producer SAID it was a server diff --git a/packages/types/src/error-leak.ts b/packages/types/src/error-leak.ts index f484a9960c..779690732d 100644 --- a/packages/types/src/error-leak.ts +++ b/packages/types/src/error-leak.ts @@ -35,18 +35,71 @@ /** Generic replacement text for a message that trips {@link looksLikeInternalErrorLeak}. */ export const INTERNAL_ERROR_MESSAGE = 'Internal server error'; +/** + * [#8132] The phrasings of the dialects this repo actually RUNS, each anchored + * on the driver's own errmsg template rather than on its tail. + * + * The gap that forced these: the keyword set below caught SQLite's + * `SQLITE_ERROR: no such table: sys_metadata` through the `sqlite_` limb, while + * the Postgres phrasing of *the same condition* — + * `relation "sys_metadata" does not exist` — matched nothing and shipped a + * physical table name to the client from every boundary that applies the + * predicate. + * + * **Why anchored, and never on the bare tail.** `does not exist` is ordinary + * business English: "user does not exist", "record does not exist". Matching + * that substring would replace legitimate answers with `Internal server error`, + * so each pattern requires what the DRIVER always emits and prose usually does + * not — a quoted identifier, or the trailing colon of SQLite's template. The + * negative cases in `error-leak.test.ts` pin that distinction. + * + * **Why the list stops here.** The module note above argues against growing a + * driver taxonomy, and it is right that the list is unbounded *across dialects* + * — MySQL/MSSQL/Oracle each phrase all of this differently and nobody here runs + * them. These are not a census: they are the two engines `driver-sql`, + * `driver-turso` and `driver-sqlite-wasm` actually reach. A dialect this repo + * does not run gets no entry, and {@link declaresServerFault} remains the + * answer that does not depend on phrasing at all. + * + * ⚠️ Related but NOT reusable: `relation-sub-object.ts` owns the same Postgres + * sentence for two other questions (which column? / is this a sub-object?), and + * its note warns that its two widths must never be collapsed. Neither answers + * "is this a leak", and its central problem does not arise here: a message like + * `column "label" of relation "sys_team" does not exist` contains a complete + * missing-TABLE phrase as a substring, which is a hazard when you are deciding + * WHICH object is missing and a non-issue when the verdict is "leak" either way. + * That is why this asks its own question with its own patterns. + */ +const DIALECT_LEAK_PHRASINGS: readonly RegExp[] = [ + // Postgres 42P01 / 42703 (and, as a superstring, the `… of relation "…"` + // sub-object family: 42704 and friends). The quotes are required because + // Postgres always emits them here. + /\b(?:relation|column)\s+["'`][^"'`]+["'`]\s+does not exist/i, + // Postgres 42501. Restricted to physical object kinds: `schema`, `view`, + // `function` and `column` are all ObjectStack AUTHORING vocabulary, so a + // product message could legitimately use them and a miss is the cheap + // direction (the outcome is already a 5xx). + /\bpermission denied for (?:table|relation|sequence|database)\b/i, + // SQLite/libsql, message-only form. The `sqlite_` limb below catches these + // only when the driver prefixed its code; `better-sqlite3` and libsql both + // raise them bare, which is the shape measured across this repo. + /\bno such (?:table|column):/i, +]; + /** * Whether `message` looks like a raw SQL statement or driver/engine dump that * must not be returned to an API client. * * Matches: dialect error codes (`SQLSTATE`, `sqlite_*`), bare statements * (a message that *starts* as `SELECT`/`INSERT INTO`/`UPDATE`/`DELETE FROM` — - * drivers prefix the offending SQL to their message), and constraint-violation - * dumps, which name physical tables and columns. + * drivers prefix the offending SQL to their message), constraint-violation + * dumps, which name physical tables and columns, and the + * {@link DIALECT_LEAK_PHRASINGS} of the engines this repo ships. * * Does NOT match ordinary business or validation messages, which is why the - * statement forms are anchored with `startsWith`: a legitimate message may - * *mention* "update" without being one. + * statement forms are anchored with `startsWith` and the dialect phrasings on + * the driver's template: a legitimate message may *mention* "update", or say + * "does not exist" about a business record, without being either. */ export function looksLikeInternalErrorLeak(message: string | undefined | null): boolean { if (!message) return false; @@ -60,7 +113,8 @@ export function looksLikeInternalErrorLeak(message: string | undefined | null): lower.startsWith('delete from ') || lower.includes('constraint failed') || lower.includes('unique constraint') || - lower.includes('foreign key') + lower.includes('foreign key') || + DIALECT_LEAK_PHRASINGS.some((pattern) => pattern.test(lower)) ); }