diff --git a/.changeset/driver-config-registry-off-vocabulary-lookup-guard.md b/.changeset/driver-config-registry-off-vocabulary-lookup-guard.md new file mode 100644 index 0000000000..704dca4544 --- /dev/null +++ b/.changeset/driver-config-registry-off-vocabulary-lookup-guard.md @@ -0,0 +1,49 @@ +--- +'@objectstack/spec': patch +--- + +fix(spec): the driver-config registry refuses an off-vocabulary id instead of answering with a truthy non-schema + +`DRIVER_CONFIG_JSON_SCHEMAS`, `DRIVER_ID_ALIASES` and `DATABASE_DRIVER_ALIASES` +are plain object literals, so all three inherit `Object.prototype`, and every +lookup into them was a bare index. Measured against the built artifact +(`dist/data/index.mjs`) on the repo's Node 22 baseline (v22.22.2), an id that +names an inherited member resolved that member and was handed onward as if it +were a driver: + +| call | before | after | +|:--|:--|:--| +| `getDriverConfigJsonSchemaById('memory')` | the JSON Schema | the JSON Schema — unmoved | +| `getDriverConfigJsonSchemaById('constructor')` | `{}` — an EMPTY JSON Schema that accepts every config | `TypeError` naming the id and the legal vocabulary | +| `getDriverConfigJsonSchemaById('toString')` | `'[object Object]'` — a **string**, where the signature promises an object | `TypeError` | +| `getDriverConfigJsonSchemaById('valueOf')` | the registry object itself | `TypeError` | +| `getDriverConfigJsonSchemaById('__proto__')` | `TypeError: … is not a function` | `TypeError`, now naming the id | +| `getDriverConfigJsonSchemaById('nope')` | `TypeError: … is not a function` | `TypeError`, now naming the id | +| `resolveDriverId('constructor')` | the `Object` **function** — truthy, not a driver id | `undefined` | +| `resolveDriverId('__proto__')` | `Object.prototype` — a truthy object | `undefined` | +| `resolveDatabaseDriverId('constructor')` | the `Object` **function** | `undefined` | +| `driverHasLocalDefault('constructor')` | `undefined`, out of a function declared `boolean` | `true`, as its doc promises for an unknown id | +| `resolveDriverId('pg')` / `resolveDriverId(' PostgreSQL ')` | `'postgres'` | `'postgres'` — unmoved | + +`getDriverConfigJsonSchemaById` handing back `{}` is the worst of these: an +empty JSON Schema validates anything, so a Studio connection form or a +`DriverDefinitionSchema.configSchema` consumer that asked "what shape must this +config have" was told "any shape at all" and reported success. + +The resolvers' half is reachable without a plain-JS consumer. The CLI refuses an +unclaimed operator selection with `if (driverType && !kind)` after calling +`resolveDatabaseDriverId`, so `OS_DATABASE_DRIVER=constructor` produced a truthy +`kind` that is not a driver id and walked past the refusal. + +All three lookups now go through an `Object.prototype.hasOwnProperty.call` check. +This narrows and widens nothing: every legal spelling is an own key of its table, +so no value accepted before is refused now, and only answers that were never +inside the declared return types move. The declared signatures are unchanged — +`getDriverConfigJsonSchemaById` stays `(id: BuiltinDriverId) => Record` +and both resolvers stay `(driver: unknown) => BuiltinDriverId | undefined`. + +A null-prototype table was the other available shape and was measured rather than +assumed: a `__proto__: null` object literal does not type-check against the +`Readonly>` annotation at all (TS2353), and the +`Object.assign(Object.create(null), …)` spelling that does compile silently costs +that annotation — a table missing a driver stopped failing to compile (TS2741). diff --git a/packages/spec/src/data/driver/config-registry.test.ts b/packages/spec/src/data/driver/config-registry.test.ts index e95cd67b78..9ccafc3eed 100644 --- a/packages/spec/src/data/driver/config-registry.test.ts +++ b/packages/spec/src/data/driver/config-registry.test.ts @@ -5,10 +5,12 @@ import { describe, it, expect } from 'vitest'; import { DatasourceSchema } from '../datasource.zod'; import { BUILTIN_DRIVER_IDS, + type BuiltinDriverId, DATABASE_DRIVER_SELECTION_ALIASES, DATABASE_DRIVER_SELECTION_IDS, DRIVER_CONFIG_SCHEMAS, DRIVER_ID_ALIASES, + driverHasLocalDefault, getDriverConfigJsonSchemaById, getDriverConfigSchema, resolveDatabaseDriverId, @@ -222,3 +224,149 @@ describe('DATABASE_DRIVER_SELECTION_IDS — what a boot flag may offer (#6969)', expect(Object.isFrozen(DATABASE_DRIVER_SELECTION_IDS)).toBe(true); }); }); +/** + * The OFF-VOCABULARY population — the one the pins above cannot reach (#16903). + * + * ⚠ Every existing case in this file iterates `BUILTIN_DRIVER_IDS`, + * `DRIVER_ID_ALIASES` or a hand-written canonical spelling: exactly the + * population that behaves. They were all green while + * `getDriverConfigJsonSchemaById('constructor')` returned `{}` — an EMPTY JSON + * Schema that accepts every config it is asked to judge — and while + * `resolveDriverId('constructor')` returned the `Object` FUNCTION out of a + * signature that says `BuiltinDriverId | undefined`. A pin over canonical ids + * alone would be green before and after the guard and would prove nothing, so + * the population is the point of this describe. + */ +describe('driver lookups — an OFF-vocabulary id is refused, never answered with a non-schema (#16903)', () => { + /** + * The consumer that actually reaches these, spelled as the cast it is. + * `getDriverConfigJsonSchemaById` and both resolvers are published + * (`packages/spec/api-surface/data.json`), so "unreachable in-repo" is not + * "unreachable": a plain-JS consumer arrives with zero type checking, and for + * the resolvers the id can also arrive from `OS_DATABASE_DRIVER` or from + * authored `datasource.driver` metadata — which is exactly where + * `constructor` and `toString` show up. + */ + const untypedJsonSchema = (id: string): unknown => getDriverConfigJsonSchemaById(id as BuiltinDriverId); + + /** + * Words that resolve an INHERITED member, grouped by what the bare lookup did + * with each before the own-property guard. Measured against the built + * artifact (`dist/data/index.mjs`) on the Node 22 baseline (v22.22.2). + */ + const PROTOTYPE_RESOLVABLE_CALLABLE = [ + // Returned a truthy NON-schema — the silent wrong answers, and the reason + // this is a bug rather than a tidy-up. `constructor` ran `Object()` and gave + // `{}`; `toString` gave the STRING '[object Object]'; `valueOf` gave the + // registry object itself. + 'constructor', + 'toString', + 'valueOf', + // Returned a `boolean` (each called with no argument, receiver = the + // registry) where the signature promises an object — which is why a + // truthiness assertion alone cannot catch this family either. + 'hasOwnProperty', + 'isPrototypeOf', + 'propertyIsEnumerable', + ]; + + /** + * Words with no own key AND nothing callable behind them: `__proto__` + * resolved `Object.prototype`, the rest resolved `undefined`. Both spellings + * already threw a `TypeError` off a non-callable, so these are the CONTROLS — + * the guard must not invent a new failure for input that already failed. + */ + const ALREADY_THREW = ['__proto__', 'nope', '', 'com.vendor.snowflake']; + + const OFF_VOCABULARY = [...PROTOTYPE_RESOLVABLE_CALLABLE, ...ALREADY_THREW]; + + it('holds this population HONEST — every word above is outside the vocabulary', () => { + // Without this, a spelling promoted into the table would leave every pin + // below asserting a refusal for a LEGAL id, and they would go on passing + // while meaning the opposite of what they say. + for (const word of OFF_VOCABULARY) { + expect(BUILTIN_DRIVER_IDS as readonly string[], word).not.toContain(word); + expect(Object.prototype.hasOwnProperty.call(DRIVER_ID_ALIASES, word), word).toBe(false); + } + }); + + it('throws for an id naming a callable Object.prototype member, instead of returning a non-schema', () => { + for (const id of PROTOTYPE_RESOLVABLE_CALLABLE) { + expect(() => untypedJsonSchema(id), id).toThrow(TypeError); + } + }); + + it('still throws for a plainly absent id, exactly as it always did', () => { + // The control: `__proto__` and an unknown word threw a `TypeError` before + // the guard too. The guard is a narrowing, so this assertion must be green + // on both sides of it — if it moves, the change did more than close a hole. + for (const id of ALREADY_THREW) { + expect(() => untypedJsonSchema(id), id).toThrow(TypeError); + } + }); + + it('names the offending id and the legal vocabulary in every refusal', () => { + // A bare `.toThrow()` is not a refusal assertion here: two of these words + // already threw. What distinguishes a REFUSAL from the old incidental + // `… is not a function` is that the message names the subject and what was + // expected instead. + for (const id of OFF_VOCABULARY) { + let message = ''; + try { + untypedJsonSchema(id); + } catch (error) { + message = (error as Error).message; + } + expect(message, id).toContain('getDriverConfigJsonSchemaById'); + expect(message, id).toContain(JSON.stringify(id)); + for (const canonical of BUILTIN_DRIVER_IDS) { + expect(message, `${id} → ${canonical}`).toContain(canonical); + } + } + }); + + it('still answers every canonical id with its own JSON Schema, unmoved', () => { + // The narrowing must stop at the vocabulary edge: the guard refuses more and + // accepts nothing new, so every in-vocabulary answer is byte-identical. + for (const id of BUILTIN_DRIVER_IDS) { + const json = getDriverConfigJsonSchemaById(id) as { type?: string; properties?: object }; + expect(json.type, id).toBe('object'); + expect(json.properties, id).toBeTruthy(); + } + // …and the memoised identity survives the guard. + expect(getDriverConfigJsonSchemaById('postgres')).toBe(getDriverConfigJsonSchemaById('postgres')); + }); + + it('resolves an off-vocabulary spelling to `undefined`, never to a truthy non-id', () => { + // `resolveDriverId('constructor')` returned the `Object` FUNCTION and + // `resolveDriverId('__proto__')` returned `Object.prototype` — both truthy, + // neither a `BuiltinDriverId`, out of a signature that admits only + // `BuiltinDriverId | undefined`. + for (const word of OFF_VOCABULARY) { + expect(resolveDriverId(word), word).toBeUndefined(); + expect(resolveDatabaseDriverId(word), word).toBeUndefined(); + } + }); + + it('answers `true` for an off-vocabulary driver in driverHasLocalDefault, never `undefined`', () => { + // The declared return is `boolean` and the doc promises `true` for an id the + // table does not know. A truthy non-id from `resolveDriverId` used to index + // `DRIVER_LOCAL_DEFAULT` to `undefined`, so `constructor` and `__proto__` + // came back `undefined` out of a function declared `boolean`. + for (const word of OFF_VOCABULARY) { + expect(typeof driverHasLocalDefault(word), word).toBe('boolean'); + expect(driverHasLocalDefault(word), word).toBe(true); + } + }); + + it('still answers every canonical id from the vocabulary table, unmoved', () => { + // The other side of the same edge, for the resolvers. + for (const id of BUILTIN_DRIVER_IDS) { + expect(resolveDriverId(id), id).toBe(id); + expect(resolveDatabaseDriverId(id), id).toBe(id); + } + expect(resolveDriverId(' PostgreSQL ')).toBe('postgres'); + expect(resolveDriverId('sqlite3')).toBe('sqlite'); + expect(resolveDatabaseDriverId('sqlite3')).toBeUndefined(); + }); +}); diff --git a/packages/spec/src/data/driver/config-registry.zod.ts b/packages/spec/src/data/driver/config-registry.zod.ts index b9f76e4b6d..bc36311d30 100644 --- a/packages/spec/src/data/driver/config-registry.zod.ts +++ b/packages/spec/src/data/driver/config-registry.zod.ts @@ -227,13 +227,66 @@ export const DATABASE_DRIVER_SELECTION_ALIASES: readonly string[] = Object.freez VOCABULARY_ROWS.flatMap((entry) => [...entry.aliases]), ); +/** + * The ONE alias lookup both resolvers go through — an own-property check, then + * the read. + * + * ## Why the bare `TABLE[spelling]` it replaces was wrong + * + * Both alias tables are built by `Object.fromEntries`, so both inherit + * `Object.prototype`, and a bare index resolves an INHERITED member for a + * spelling that names one. Both resolvers declare `BuiltinDriverId | undefined` + * and both are published (`packages/spec/api-surface/data.json`), so what came + * back was neither: measured against the built artifact on the Node 22 baseline + * (v22.22.2), `constructor` answered the `Object` FUNCTION and `__proto__` + * answered `Object.prototype` — two truthy non-ids out of a pair of functions + * whose `undefined` is the entire "this driver is not ours" signal. + * + * That signal has consumers that are not plain-JS callers. The CLI's + * `resolveStorageDriver` (`packages/cli/src/utils/storage-driver.ts`) refuses an + * unclaimed operator selection with `if (driverType && !kind)`, so + * `OS_DATABASE_DRIVER=constructor` walked PAST the refusal #6345 fork 1 exists + * to be — a truthy `kind` that is not a driver id. {@link driverHasLocalDefault} + * failed the same way from the other end: a truthy non-id indexed + * `DRIVER_LOCAL_DEFAULT` to `undefined`, so a function DECLARED `boolean` + * returned `undefined` for `constructor` and `__proto__` where its own doc + * promises `true`. + * + * `toString` and `valueOf` escaped only by accident — `.toLowerCase()` maps them + * to `tostring` / `valueof`, which name nothing. An accident of casing is not a + * guard, and the two words that ARE already lowercase were not covered by it. + * + * ## What it changes, and what it cannot + * + * It NARROWS, strictly: every legal spelling is an own key of its table, so no + * value accepted before is refused now, and the only answers that move are the + * ones that were never `BuiltinDriverId | undefined` in the first place. + * + * ⛔ Not a null-prototype table, for the reason the sibling guard in + * `src/shared/value-domain.zod.ts` records and this file re-measured: a + * `__proto__: null` object literal does not type-check against the + * `Readonly>` annotation at all (TS2353), and the + * `Object.assign(Object.create(null), …)` spelling that does compile silently + * COSTS the annotation — a table missing a driver stopped failing to compile + * (TS2741) in a probe of exactly that shape. Deleting a compile-time + * exhaustiveness guarantee to close a runtime hole is a bad trade. + */ +function lookupDriverId( + table: Readonly>, + driver: string, +): BuiltinDriverId | undefined { + const spelling = driver.trim().toLowerCase(); + if (!Object.prototype.hasOwnProperty.call(table, spelling)) return undefined; + return table[spelling]; +} + /** * Resolve an authored `datasource.driver` onto its canonical id, or `undefined` * when the platform ships no contract for it (a plugin-contributed driver). */ export function resolveDriverId(driver: unknown): BuiltinDriverId | undefined { if (typeof driver !== 'string') return undefined; - return DRIVER_ID_ALIASES[driver.trim().toLowerCase()]; + return lookupDriverId(DRIVER_ID_ALIASES, driver); } /** Selection-face lookup, built once so {@link resolveDatabaseDriverId} is a hash hit. */ @@ -255,7 +308,7 @@ const DATABASE_DRIVER_ALIASES: Readonly> = Objec */ export function resolveDatabaseDriverId(driver: unknown): BuiltinDriverId | undefined { if (typeof driver !== 'string') return undefined; - return DATABASE_DRIVER_ALIASES[driver.trim().toLowerCase()]; + return lookupDriverId(DATABASE_DRIVER_ALIASES, driver); } /** @@ -377,8 +430,48 @@ const DRIVER_CONFIG_JSON_SCHEMAS: Readonly Record< * Takes a CANONICAL id (not an alias) so a caller enumerating drivers cannot * quietly get `undefined` for a spelling it thought was covered; use * {@link resolveDriverId} first when the id came from authored metadata. + * + * Total over {@link BUILTIN_DRIVER_IDS} and closed outside it: an id that is not + * one of them — including one that names an `Object.prototype` member such as + * `constructor`, `toString` or `valueOf` — THROWS a `TypeError` naming the legal + * ids. It never answers a non-schema, so a caller reaching this published export + * from plain JS, or with an id read from METADATA rather than written in source, + * cannot be handed an empty schema that accepts everything. See the guard's own + * comment for what each of those words used to return. */ export function getDriverConfigJsonSchemaById(id: BuiltinDriverId): Record { + // ⛔ The own-property guard is load-bearing, not defensive noise, and the + // refusal it enables is a THROW rather than an `undefined` on purpose. + // + // `DRIVER_CONFIG_JSON_SCHEMAS` is an object literal, so it inherits + // `Object.prototype`, and the bare `[id]()` this replaces CALLED whatever an + // off-vocabulary id resolved to. Measured against the built artifact + // (`dist/data/index.mjs`) on the Node 22 baseline (v22.22.2): `constructor` + // ran `Object()` and handed back `{}` — an EMPTY JSON Schema, which accepts + // every config it is ever asked to judge; `toString` handed back the STRING + // '[object Object]' where the signature promises an object; `valueOf` handed + // back the registry itself. Only `__proto__` and a plainly absent word threw. + // Three quiet wrong answers and two throws, from one lookup. + // + // The guard collapses all five onto the throw, so the function is TOTAL: a + // canonical id gets its schema, and everything else gets a refusal naming the + // legal ids. `undefined` was the other in-band spelling and is NOT taken — + // this accessor's own doc above exists to say that a caller enumerating + // drivers must not be able to get a quiet `undefined` out of it, and + // {@link getDriverConfigSchema} is already the optional, alias-following door + // for a driver the platform may not know. Widening this return to an optional + // would erase the distinction between the two and change a published + // signature to do it. + // + // `TypeError` rather than this module's usual `Error`: the two ids that + // already threw threw a `TypeError`, so the class every existing caller can + // catch is unmoved and only the message improves. + if (!Object.prototype.hasOwnProperty.call(DRIVER_CONFIG_JSON_SCHEMAS, id)) { + throw new TypeError( + `getDriverConfigJsonSchemaById: ${JSON.stringify(String(id))} is not a built-in driver id ` + + `(expected one of ${BUILTIN_DRIVER_IDS.join(', ')}); resolve an authored spelling with resolveDriverId first.`, + ); + } return DRIVER_CONFIG_JSON_SCHEMAS[id](); }