Skip to content

Commit 65ad77d

Browse files
os-billclaude
andauthored
fix(spec): refuse an off-vocabulary driver id in the config registry instead of answering with a truthy non-schema (#17457)
`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), the card's six-row table reproduces exactly: `getDriverConfigJsonSchemaById('constructor')` ran `Object()` and returned `{}` — an EMPTY JSON Schema that accepts every config it is asked to judge — `'toString'` returned the string '[object Object]' out of a signature that promises an object, `'valueOf'` returned the registry itself, and only `'__proto__'` and a plainly absent word threw. The same measurement found the defect twice more in the same file, in the same class, and one of those two is reachable without a plain-JS consumer: `resolveDriverId('constructor')` returned the `Object` FUNCTION and `resolveDriverId('__proto__')` returned `Object.prototype` — truthy non-ids out of a signature that admits only `BuiltinDriverId | undefined`. The CLI's `resolveStorageDriver` 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 that refusal; `driverHasLocalDefault` failed from the other end, returning `undefined` for `constructor` and `__proto__` out of a function declared `boolean` whose own doc promises `true` for an id the table does not know. `toString` and `valueOf` escaped the resolvers only because `.toLowerCase()` maps them onto nothing — an accident of casing, which did not cover the two words already lowercase. All three lookups now go through an `Object.prototype.hasOwnProperty.call` check — the same spelling the sibling guard in `src/shared/value-domain.zod.ts` uses. It 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. No published signature changes. `getDriverConfigJsonSchemaById` refuses by THROWING rather than by widening to an optional: its own doc exists to say a caller enumerating drivers must not be able to get a quiet `undefined` out of it, `getDriverConfigSchema` is already the optional alias-following door, and both ids that already threw threw a `TypeError`, so keeping that class leaves every existing caller's catch unmoved and only improves the message. 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<Record<...>>` annotation at all (TS2353), and the `Object.assign(Object.create(null), ...)` spelling that does compile silently costs that annotation — in a probe of exactly that shape, a table missing a driver stopped failing to compile (TS2741). Deleting a compile-time exhaustiveness guarantee to close a runtime hole is a bad trade. The pins that existed could not have caught any of this: every one of them iterates `BUILTIN_DRIVER_IDS`, `DRIVER_ID_ALIASES` or a hand-written canonical spelling — exactly the population that behaves. The new pins put `constructor`, `toString`, `valueOf`, `hasOwnProperty`, `isPrototypeOf`, `propertyIsEnumerable`, `__proto__` and plainly absent words into the population, hold that population honest, and keep the canonical answers pinned as the controls that must not move. Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0f38407 commit 65ad77d

3 files changed

Lines changed: 292 additions & 2 deletions

File tree

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
---
2+
'@objectstack/spec': patch
3+
---
4+
5+
fix(spec): the driver-config registry refuses an off-vocabulary id instead of answering with a truthy non-schema
6+
7+
`DRIVER_CONFIG_JSON_SCHEMAS`, `DRIVER_ID_ALIASES` and `DATABASE_DRIVER_ALIASES`
8+
are plain object literals, so all three inherit `Object.prototype`, and every
9+
lookup into them was a bare index. Measured against the built artifact
10+
(`dist/data/index.mjs`) on the repo's Node 22 baseline (v22.22.2), an id that
11+
names an inherited member resolved that member and was handed onward as if it
12+
were a driver:
13+
14+
| call | before | after |
15+
|:--|:--|:--|
16+
| `getDriverConfigJsonSchemaById('memory')` | the JSON Schema | the JSON Schema — unmoved |
17+
| `getDriverConfigJsonSchemaById('constructor')` | `{}` — an EMPTY JSON Schema that accepts every config | `TypeError` naming the id and the legal vocabulary |
18+
| `getDriverConfigJsonSchemaById('toString')` | `'[object Object]'` — a **string**, where the signature promises an object | `TypeError` |
19+
| `getDriverConfigJsonSchemaById('valueOf')` | the registry object itself | `TypeError` |
20+
| `getDriverConfigJsonSchemaById('__proto__')` | `TypeError: … is not a function` | `TypeError`, now naming the id |
21+
| `getDriverConfigJsonSchemaById('nope')` | `TypeError: … is not a function` | `TypeError`, now naming the id |
22+
| `resolveDriverId('constructor')` | the `Object` **function** — truthy, not a driver id | `undefined` |
23+
| `resolveDriverId('__proto__')` | `Object.prototype` — a truthy object | `undefined` |
24+
| `resolveDatabaseDriverId('constructor')` | the `Object` **function** | `undefined` |
25+
| `driverHasLocalDefault('constructor')` | `undefined`, out of a function declared `boolean` | `true`, as its doc promises for an unknown id |
26+
| `resolveDriverId('pg')` / `resolveDriverId(' PostgreSQL ')` | `'postgres'` | `'postgres'` — unmoved |
27+
28+
`getDriverConfigJsonSchemaById` handing back `{}` is the worst of these: an
29+
empty JSON Schema validates anything, so a Studio connection form or a
30+
`DriverDefinitionSchema.configSchema` consumer that asked "what shape must this
31+
config have" was told "any shape at all" and reported success.
32+
33+
The resolvers' half is reachable without a plain-JS consumer. The CLI refuses an
34+
unclaimed operator selection with `if (driverType && !kind)` after calling
35+
`resolveDatabaseDriverId`, so `OS_DATABASE_DRIVER=constructor` produced a truthy
36+
`kind` that is not a driver id and walked past the refusal.
37+
38+
All three lookups now go through an `Object.prototype.hasOwnProperty.call` check.
39+
This narrows and widens nothing: every legal spelling is an own key of its table,
40+
so no value accepted before is refused now, and only answers that were never
41+
inside the declared return types move. The declared signatures are unchanged —
42+
`getDriverConfigJsonSchemaById` stays `(id: BuiltinDriverId) => Record<string, unknown>`
43+
and both resolvers stay `(driver: unknown) => BuiltinDriverId | undefined`.
44+
45+
A null-prototype table was the other available shape and was measured rather than
46+
assumed: a `__proto__: null` object literal does not type-check against the
47+
`Readonly<Record<…>>` annotation at all (TS2353), and the
48+
`Object.assign(Object.create(null), …)` spelling that does compile silently costs
49+
that annotation — a table missing a driver stopped failing to compile (TS2741).

packages/spec/src/data/driver/config-registry.test.ts

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,12 @@ import { describe, it, expect } from 'vitest';
55
import { DatasourceSchema } from '../datasource.zod';
66
import {
77
BUILTIN_DRIVER_IDS,
8+
type BuiltinDriverId,
89
DATABASE_DRIVER_SELECTION_ALIASES,
910
DATABASE_DRIVER_SELECTION_IDS,
1011
DRIVER_CONFIG_SCHEMAS,
1112
DRIVER_ID_ALIASES,
13+
driverHasLocalDefault,
1214
getDriverConfigJsonSchemaById,
1315
getDriverConfigSchema,
1416
resolveDatabaseDriverId,
@@ -222,3 +224,149 @@ describe('DATABASE_DRIVER_SELECTION_IDS — what a boot flag may offer (#6969)',
222224
expect(Object.isFrozen(DATABASE_DRIVER_SELECTION_IDS)).toBe(true);
223225
});
224226
});
227+
/**
228+
* The OFF-VOCABULARY population — the one the pins above cannot reach (#16903).
229+
*
230+
* ⚠ Every existing case in this file iterates `BUILTIN_DRIVER_IDS`,
231+
* `DRIVER_ID_ALIASES` or a hand-written canonical spelling: exactly the
232+
* population that behaves. They were all green while
233+
* `getDriverConfigJsonSchemaById('constructor')` returned `{}` — an EMPTY JSON
234+
* Schema that accepts every config it is asked to judge — and while
235+
* `resolveDriverId('constructor')` returned the `Object` FUNCTION out of a
236+
* signature that says `BuiltinDriverId | undefined`. A pin over canonical ids
237+
* alone would be green before and after the guard and would prove nothing, so
238+
* the population is the point of this describe.
239+
*/
240+
describe('driver lookups — an OFF-vocabulary id is refused, never answered with a non-schema (#16903)', () => {
241+
/**
242+
* The consumer that actually reaches these, spelled as the cast it is.
243+
* `getDriverConfigJsonSchemaById` and both resolvers are published
244+
* (`packages/spec/api-surface/data.json`), so "unreachable in-repo" is not
245+
* "unreachable": a plain-JS consumer arrives with zero type checking, and for
246+
* the resolvers the id can also arrive from `OS_DATABASE_DRIVER` or from
247+
* authored `datasource.driver` metadata — which is exactly where
248+
* `constructor` and `toString` show up.
249+
*/
250+
const untypedJsonSchema = (id: string): unknown => getDriverConfigJsonSchemaById(id as BuiltinDriverId);
251+
252+
/**
253+
* Words that resolve an INHERITED member, grouped by what the bare lookup did
254+
* with each before the own-property guard. Measured against the built
255+
* artifact (`dist/data/index.mjs`) on the Node 22 baseline (v22.22.2).
256+
*/
257+
const PROTOTYPE_RESOLVABLE_CALLABLE = [
258+
// Returned a truthy NON-schema — the silent wrong answers, and the reason
259+
// this is a bug rather than a tidy-up. `constructor` ran `Object()` and gave
260+
// `{}`; `toString` gave the STRING '[object Object]'; `valueOf` gave the
261+
// registry object itself.
262+
'constructor',
263+
'toString',
264+
'valueOf',
265+
// Returned a `boolean` (each called with no argument, receiver = the
266+
// registry) where the signature promises an object — which is why a
267+
// truthiness assertion alone cannot catch this family either.
268+
'hasOwnProperty',
269+
'isPrototypeOf',
270+
'propertyIsEnumerable',
271+
];
272+
273+
/**
274+
* Words with no own key AND nothing callable behind them: `__proto__`
275+
* resolved `Object.prototype`, the rest resolved `undefined`. Both spellings
276+
* already threw a `TypeError` off a non-callable, so these are the CONTROLS —
277+
* the guard must not invent a new failure for input that already failed.
278+
*/
279+
const ALREADY_THREW = ['__proto__', 'nope', '', 'com.vendor.snowflake'];
280+
281+
const OFF_VOCABULARY = [...PROTOTYPE_RESOLVABLE_CALLABLE, ...ALREADY_THREW];
282+
283+
it('holds this population HONEST — every word above is outside the vocabulary', () => {
284+
// Without this, a spelling promoted into the table would leave every pin
285+
// below asserting a refusal for a LEGAL id, and they would go on passing
286+
// while meaning the opposite of what they say.
287+
for (const word of OFF_VOCABULARY) {
288+
expect(BUILTIN_DRIVER_IDS as readonly string[], word).not.toContain(word);
289+
expect(Object.prototype.hasOwnProperty.call(DRIVER_ID_ALIASES, word), word).toBe(false);
290+
}
291+
});
292+
293+
it('throws for an id naming a callable Object.prototype member, instead of returning a non-schema', () => {
294+
for (const id of PROTOTYPE_RESOLVABLE_CALLABLE) {
295+
expect(() => untypedJsonSchema(id), id).toThrow(TypeError);
296+
}
297+
});
298+
299+
it('still throws for a plainly absent id, exactly as it always did', () => {
300+
// The control: `__proto__` and an unknown word threw a `TypeError` before
301+
// the guard too. The guard is a narrowing, so this assertion must be green
302+
// on both sides of it — if it moves, the change did more than close a hole.
303+
for (const id of ALREADY_THREW) {
304+
expect(() => untypedJsonSchema(id), id).toThrow(TypeError);
305+
}
306+
});
307+
308+
it('names the offending id and the legal vocabulary in every refusal', () => {
309+
// A bare `.toThrow()` is not a refusal assertion here: two of these words
310+
// already threw. What distinguishes a REFUSAL from the old incidental
311+
// `… is not a function` is that the message names the subject and what was
312+
// expected instead.
313+
for (const id of OFF_VOCABULARY) {
314+
let message = '';
315+
try {
316+
untypedJsonSchema(id);
317+
} catch (error) {
318+
message = (error as Error).message;
319+
}
320+
expect(message, id).toContain('getDriverConfigJsonSchemaById');
321+
expect(message, id).toContain(JSON.stringify(id));
322+
for (const canonical of BUILTIN_DRIVER_IDS) {
323+
expect(message, `${id}${canonical}`).toContain(canonical);
324+
}
325+
}
326+
});
327+
328+
it('still answers every canonical id with its own JSON Schema, unmoved', () => {
329+
// The narrowing must stop at the vocabulary edge: the guard refuses more and
330+
// accepts nothing new, so every in-vocabulary answer is byte-identical.
331+
for (const id of BUILTIN_DRIVER_IDS) {
332+
const json = getDriverConfigJsonSchemaById(id) as { type?: string; properties?: object };
333+
expect(json.type, id).toBe('object');
334+
expect(json.properties, id).toBeTruthy();
335+
}
336+
// …and the memoised identity survives the guard.
337+
expect(getDriverConfigJsonSchemaById('postgres')).toBe(getDriverConfigJsonSchemaById('postgres'));
338+
});
339+
340+
it('resolves an off-vocabulary spelling to `undefined`, never to a truthy non-id', () => {
341+
// `resolveDriverId('constructor')` returned the `Object` FUNCTION and
342+
// `resolveDriverId('__proto__')` returned `Object.prototype` — both truthy,
343+
// neither a `BuiltinDriverId`, out of a signature that admits only
344+
// `BuiltinDriverId | undefined`.
345+
for (const word of OFF_VOCABULARY) {
346+
expect(resolveDriverId(word), word).toBeUndefined();
347+
expect(resolveDatabaseDriverId(word), word).toBeUndefined();
348+
}
349+
});
350+
351+
it('answers `true` for an off-vocabulary driver in driverHasLocalDefault, never `undefined`', () => {
352+
// The declared return is `boolean` and the doc promises `true` for an id the
353+
// table does not know. A truthy non-id from `resolveDriverId` used to index
354+
// `DRIVER_LOCAL_DEFAULT` to `undefined`, so `constructor` and `__proto__`
355+
// came back `undefined` out of a function declared `boolean`.
356+
for (const word of OFF_VOCABULARY) {
357+
expect(typeof driverHasLocalDefault(word), word).toBe('boolean');
358+
expect(driverHasLocalDefault(word), word).toBe(true);
359+
}
360+
});
361+
362+
it('still answers every canonical id from the vocabulary table, unmoved', () => {
363+
// The other side of the same edge, for the resolvers.
364+
for (const id of BUILTIN_DRIVER_IDS) {
365+
expect(resolveDriverId(id), id).toBe(id);
366+
expect(resolveDatabaseDriverId(id), id).toBe(id);
367+
}
368+
expect(resolveDriverId(' PostgreSQL ')).toBe('postgres');
369+
expect(resolveDriverId('sqlite3')).toBe('sqlite');
370+
expect(resolveDatabaseDriverId('sqlite3')).toBeUndefined();
371+
});
372+
});

packages/spec/src/data/driver/config-registry.zod.ts

Lines changed: 95 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -227,13 +227,66 @@ export const DATABASE_DRIVER_SELECTION_ALIASES: readonly string[] = Object.freez
227227
VOCABULARY_ROWS.flatMap((entry) => [...entry.aliases]),
228228
);
229229

230+
/**
231+
* The ONE alias lookup both resolvers go through — an own-property check, then
232+
* the read.
233+
*
234+
* ## Why the bare `TABLE[spelling]` it replaces was wrong
235+
*
236+
* Both alias tables are built by `Object.fromEntries`, so both inherit
237+
* `Object.prototype`, and a bare index resolves an INHERITED member for a
238+
* spelling that names one. Both resolvers declare `BuiltinDriverId | undefined`
239+
* and both are published (`packages/spec/api-surface/data.json`), so what came
240+
* back was neither: measured against the built artifact on the Node 22 baseline
241+
* (v22.22.2), `constructor` answered the `Object` FUNCTION and `__proto__`
242+
* answered `Object.prototype` — two truthy non-ids out of a pair of functions
243+
* whose `undefined` is the entire "this driver is not ours" signal.
244+
*
245+
* That signal has consumers that are not plain-JS callers. The CLI's
246+
* `resolveStorageDriver` (`packages/cli/src/utils/storage-driver.ts`) refuses an
247+
* unclaimed operator selection with `if (driverType && !kind)`, so
248+
* `OS_DATABASE_DRIVER=constructor` walked PAST the refusal #6345 fork 1 exists
249+
* to be — a truthy `kind` that is not a driver id. {@link driverHasLocalDefault}
250+
* failed the same way from the other end: a truthy non-id indexed
251+
* `DRIVER_LOCAL_DEFAULT` to `undefined`, so a function DECLARED `boolean`
252+
* returned `undefined` for `constructor` and `__proto__` where its own doc
253+
* promises `true`.
254+
*
255+
* `toString` and `valueOf` escaped only by accident — `.toLowerCase()` maps them
256+
* to `tostring` / `valueof`, which name nothing. An accident of casing is not a
257+
* guard, and the two words that ARE already lowercase were not covered by it.
258+
*
259+
* ## What it changes, and what it cannot
260+
*
261+
* It NARROWS, strictly: every legal spelling is an own key of its table, so no
262+
* value accepted before is refused now, and the only answers that move are the
263+
* ones that were never `BuiltinDriverId | undefined` in the first place.
264+
*
265+
* ⛔ Not a null-prototype table, for the reason the sibling guard in
266+
* `src/shared/value-domain.zod.ts` records and this file re-measured: a
267+
* `__proto__: null` object literal does not type-check against the
268+
* `Readonly<Record<…>>` annotation at all (TS2353), and the
269+
* `Object.assign(Object.create(null), …)` spelling that does compile silently
270+
* COSTS the annotation — a table missing a driver stopped failing to compile
271+
* (TS2741) in a probe of exactly that shape. Deleting a compile-time
272+
* exhaustiveness guarantee to close a runtime hole is a bad trade.
273+
*/
274+
function lookupDriverId(
275+
table: Readonly<Record<string, BuiltinDriverId>>,
276+
driver: string,
277+
): BuiltinDriverId | undefined {
278+
const spelling = driver.trim().toLowerCase();
279+
if (!Object.prototype.hasOwnProperty.call(table, spelling)) return undefined;
280+
return table[spelling];
281+
}
282+
230283
/**
231284
* Resolve an authored `datasource.driver` onto its canonical id, or `undefined`
232285
* when the platform ships no contract for it (a plugin-contributed driver).
233286
*/
234287
export function resolveDriverId(driver: unknown): BuiltinDriverId | undefined {
235288
if (typeof driver !== 'string') return undefined;
236-
return DRIVER_ID_ALIASES[driver.trim().toLowerCase()];
289+
return lookupDriverId(DRIVER_ID_ALIASES, driver);
237290
}
238291

239292
/** Selection-face lookup, built once so {@link resolveDatabaseDriverId} is a hash hit. */
@@ -255,7 +308,7 @@ const DATABASE_DRIVER_ALIASES: Readonly<Record<string, BuiltinDriverId>> = Objec
255308
*/
256309
export function resolveDatabaseDriverId(driver: unknown): BuiltinDriverId | undefined {
257310
if (typeof driver !== 'string') return undefined;
258-
return DATABASE_DRIVER_ALIASES[driver.trim().toLowerCase()];
311+
return lookupDriverId(DATABASE_DRIVER_ALIASES, driver);
259312
}
260313

261314
/**
@@ -377,8 +430,48 @@ const DRIVER_CONFIG_JSON_SCHEMAS: Readonly<Record<BuiltinDriverId, () => Record<
377430
* Takes a CANONICAL id (not an alias) so a caller enumerating drivers cannot
378431
* quietly get `undefined` for a spelling it thought was covered; use
379432
* {@link resolveDriverId} first when the id came from authored metadata.
433+
*
434+
* Total over {@link BUILTIN_DRIVER_IDS} and closed outside it: an id that is not
435+
* one of them — including one that names an `Object.prototype` member such as
436+
* `constructor`, `toString` or `valueOf` — THROWS a `TypeError` naming the legal
437+
* ids. It never answers a non-schema, so a caller reaching this published export
438+
* from plain JS, or with an id read from METADATA rather than written in source,
439+
* cannot be handed an empty schema that accepts everything. See the guard's own
440+
* comment for what each of those words used to return.
380441
*/
381442
export function getDriverConfigJsonSchemaById(id: BuiltinDriverId): Record<string, unknown> {
443+
// ⛔ The own-property guard is load-bearing, not defensive noise, and the
444+
// refusal it enables is a THROW rather than an `undefined` on purpose.
445+
//
446+
// `DRIVER_CONFIG_JSON_SCHEMAS` is an object literal, so it inherits
447+
// `Object.prototype`, and the bare `[id]()` this replaces CALLED whatever an
448+
// off-vocabulary id resolved to. Measured against the built artifact
449+
// (`dist/data/index.mjs`) on the Node 22 baseline (v22.22.2): `constructor`
450+
// ran `Object()` and handed back `{}` — an EMPTY JSON Schema, which accepts
451+
// every config it is ever asked to judge; `toString` handed back the STRING
452+
// '[object Object]' where the signature promises an object; `valueOf` handed
453+
// back the registry itself. Only `__proto__` and a plainly absent word threw.
454+
// Three quiet wrong answers and two throws, from one lookup.
455+
//
456+
// The guard collapses all five onto the throw, so the function is TOTAL: a
457+
// canonical id gets its schema, and everything else gets a refusal naming the
458+
// legal ids. `undefined` was the other in-band spelling and is NOT taken —
459+
// this accessor's own doc above exists to say that a caller enumerating
460+
// drivers must not be able to get a quiet `undefined` out of it, and
461+
// {@link getDriverConfigSchema} is already the optional, alias-following door
462+
// for a driver the platform may not know. Widening this return to an optional
463+
// would erase the distinction between the two and change a published
464+
// signature to do it.
465+
//
466+
// `TypeError` rather than this module's usual `Error`: the two ids that
467+
// already threw threw a `TypeError`, so the class every existing caller can
468+
// catch is unmoved and only the message improves.
469+
if (!Object.prototype.hasOwnProperty.call(DRIVER_CONFIG_JSON_SCHEMAS, id)) {
470+
throw new TypeError(
471+
`getDriverConfigJsonSchemaById: ${JSON.stringify(String(id))} is not a built-in driver id ` +
472+
`(expected one of ${BUILTIN_DRIVER_IDS.join(', ')}); resolve an authored spelling with resolveDriverId first.`,
473+
);
474+
}
382475
return DRIVER_CONFIG_JSON_SCHEMAS[id]();
383476
}
384477

0 commit comments

Comments
 (0)