Skip to content

Commit 7a7fb03

Browse files
claude[bot]claude
andauthored
fix(spec): refuse an off-vocabulary domain in isValueDomainMember instead of failing open (#16899)
`DOMAIN_MEMBERSHIP` is an object literal, so it inherits `Object.prototype`, and the predicate indexed it with no own-property guard. Measured against the built artifact on the Node 22 baseline (v22.22.2), a `domain` outside the vocabulary did one of two wrong things, and one of them was a membership FALSE POSITIVE out of a function whose whole job is to refuse non-members: `toString` answered the truthy string '[object Object]', `valueOf` and `constructor` answered truthy objects, while `__proto__`, `nope` and '' threw a TypeError off a non-callable. The parameter is typed and every in-repo call site names a member, but "unreachable in-repo" is not "unreachable": `isValueDomainMember` is published on `@objectstack/spec/shared` (`packages/spec/api-surface/shared.json`), so a plain-JS consumer, or any caller passing a domain string read from metadata rather than written in source, reaches it with no type checking at all. An `Object.prototype.hasOwnProperty.call` guard — the same spelling the `iso_4217_currency` definition in this module already uses — now returns `false` for a domain that is not an own key. This narrows and widens nothing: every accepted domain is an own key, so no previously accepted value is refused, and the three real domains answer from their own definitions unmoved. `false` rather than a thrown refusal is the narrowing reading; throwing would change published behaviour for callers who today receive a truthy value. A null-prototype record was the other available shape and was not taken: it converts the truthy answers into throws rather than into `false`, and it costs the `Readonly<Record<ValueDomain, ...>>` annotation that makes a vocabulary member added without a definition fail to compile. The pin that existed could not have caught this. It asserted the return `typeof` was `boolean` but iterated `ValueDomainSchema.options` only — exactly the domains that behave. The fix is as much about the pin's POPULATION as about the guard: new pins put `toString`, `valueOf`, `constructor`, `hasOwnProperty`, `isPrototypeOf`, `propertyIsEnumerable`, `__proto__` and plainly absent words into the population, and a third pin holds that population honest by asserting each is still outside the vocabulary. Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x Co-authored-by: Claude <noreply@anthropic.com>
1 parent ed6579b commit 7a7fb03

3 files changed

Lines changed: 178 additions & 0 deletions

File tree

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
---
2+
'@objectstack/spec': patch
3+
---
4+
5+
fix(spec): `isValueDomainMember` refuses an off-vocabulary domain instead of failing OPEN on `Object.prototype` names
6+
7+
`DOMAIN_MEMBERSHIP` is an object literal, so it inherits `Object.prototype`, and
8+
`isValueDomainMember` indexed it with no own-property guard. Measured against the
9+
built artifact (`dist/shared/index.mjs`) on the repo's Node 22 baseline (v22.22.2),
10+
an off-vocabulary `domain` did one of two wrong things — and one of them was a
11+
membership FALSE POSITIVE out of a predicate whose whole job is to refuse
12+
non-members:
13+
14+
| `domain` | before | after |
15+
|:--|:--|:--|
16+
| `iana_time_zone` (in vocabulary) | `true` for `UTC` | `true` for `UTC` — unmoved |
17+
| `toString` | `'[object Object]'` — a truthy **string** | `false` |
18+
| `valueOf` | a truthy **object** | `false` |
19+
| `constructor` | a truthy **object** | `false` |
20+
| `__proto__` | threw a `TypeError` | `false` |
21+
| `nope`, `''` | threw a `TypeError` | `false` |
22+
23+
**Why it is reachable.** "Unreachable in-repo" is not "unreachable". The parameter
24+
is typed `ValueDomain` and every in-repo call site names a member, but
25+
`isValueDomainMember` is **published** on `@objectstack/spec/shared` (it is in
26+
`packages/spec/api-surface/shared.json`). A plain-JS consumer, or any caller
27+
handing over a domain string read from **metadata** rather than written in source,
28+
reaches it with no type checking at all — and metadata-sourced strings are exactly
29+
where `constructor` and `toString` show up.
30+
31+
**This narrows and widens nothing, measured rather than asserted.** Every accepted
32+
`domain` is an own key of the record, so no value that was accepted before is
33+
refused now; the three real domains answer from their own definitions, unmoved.
34+
The change is one `Object.prototype.hasOwnProperty.call` guard — the same spelling
35+
the `iso_4217_currency` definition in the same module already uses — returning
36+
`false` for a domain that is not an own key. A **null-prototype record** was the
37+
other shape available and was not taken: it converts the truthy answers into
38+
throws rather than into `false`, and it costs the `Readonly<Record<ValueDomain, …>>`
39+
annotation that makes a vocabulary member added without a definition fail to
40+
compile.
41+
42+
**Unknown domain answers `false`; it does not throw.** `false` is the narrowing
43+
reading — it refuses more and accepts nothing new — whereas a thrown refusal would
44+
change published behaviour for callers who today receive a truthy value. This is
45+
the same third branch a sister ruling settled for the same defect family: list
46+
reject / own-member value / prototype-resolvable ⇒ reject.
47+
48+
The pin that existed did not cover this, and the fix is as much about its
49+
POPULATION as about the guard: the totality pin asserted the return `typeof` was
50+
`boolean` but iterated `ValueDomainSchema.options` **only** — exactly the domains
51+
that behave. The new pins put `toString`, `valueOf`, `constructor`,
52+
`hasOwnProperty`, `isPrototypeOf`, `propertyIsEnumerable`, `__proto__` and plainly
53+
absent words into the population, and a third pin holds that population honest by
54+
asserting every one of them is still outside the vocabulary.

packages/spec/src/shared/value-domain.test.ts

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
ValueDomainSchema,
2020
ISO_3166_ALPHA2_CODES,
2121
isValueDomainMember,
22+
type ValueDomain,
2223
} from './value-domain.zod';
2324
import { SpecifierValueDomainSchema } from '../system/settings-manifest.zod';
2425
import { CURRENCY_FRACTION_DIGITS } from '../data/currency-fraction-digits';
@@ -168,9 +169,100 @@ describe('isValueDomainMember — iso_3166_alpha2 is the explicit 249-code list'
168169

169170
describe('isValueDomainMember — every vocabulary member has a definition', () => {
170171
it('answers a boolean for each member, never throws, never returns undefined', () => {
172+
// ⚠ The POPULATION here is `ValueDomainSchema.options` — exactly the
173+
// domains that behave. That is the right population for THIS claim (every
174+
// member has a definition), and the wrong one for "an unknown domain is
175+
// refused": the describe below carries that claim over the population this
176+
// loop cannot reach.
171177
for (const domain of ValueDomainSchema.options) {
172178
expect(typeof isValueDomainMember(domain, 'definitely-not-a-member')).toBe('boolean');
173179
expect(isValueDomainMember(domain, 'definitely-not-a-member')).toBe(false);
174180
}
175181
});
176182
});
183+
184+
describe('isValueDomainMember — an OFF-vocabulary domain is refused, never answered truthy', () => {
185+
/**
186+
* The published contract, exercised the way a consumer actually reaches it.
187+
* `isValueDomainMember` is in `packages/spec/api-surface/shared.json`, so
188+
* "unreachable in-repo" is not "unreachable": a plain-JS consumer, or any
189+
* caller handing over a domain string read from METADATA rather than written
190+
* in source, arrives with zero type checking — and metadata-sourced strings
191+
* are exactly where `constructor` and `toString` show up. The cast is that
192+
* caller, not a way around the type.
193+
*/
194+
const untyped = (domain: string, value: string): unknown =>
195+
isValueDomainMember(domain as ValueDomain, value);
196+
197+
/**
198+
* Domain words that are NOT in the vocabulary, grouped by what each one did
199+
* before the own-property guard. `DOMAIN_MEMBERSHIP` is an object literal, so
200+
* it inherits `Object.prototype`.
201+
*/
202+
const PROTOTYPE_RESOLVABLE = [
203+
// Answered TRUTHY — the membership false positives, the reason this is a
204+
// bug and not a tidy-up: `toString` gave the string '[object Object]',
205+
// `valueOf` and `constructor` gave objects.
206+
'toString',
207+
'valueOf',
208+
'constructor',
209+
// Answered a boolean `false` by accident (`hasOwnProperty` called with
210+
// `DOMAIN_MEMBERSHIP` as its receiver), which is why a `typeof` assertion
211+
// alone is not enough to catch this family.
212+
'hasOwnProperty',
213+
'isPrototypeOf',
214+
'propertyIsEnumerable',
215+
// Resolved to `Object.prototype` itself — not callable, so it THREW a
216+
// TypeError. `false` now, like every other non-member.
217+
'__proto__',
218+
];
219+
220+
/** No own key and no prototype member either: these threw a TypeError too. */
221+
const PLAINLY_ABSENT = ['nope', '', 'iana_timezone', 'iso_8601_date', 'bcp47_locale', 'ZZ'];
222+
223+
const OFF_VOCABULARY = [...PROTOTYPE_RESOLVABLE, ...PLAINLY_ABSENT];
224+
225+
// Values spanning all three real domains plus junk, so a leaked definition
226+
// would be caught whichever domain it leaked from.
227+
const VALUES = ['UTC', 'USD', 'US', '', 'definitely-not-a-member'];
228+
229+
it('answers exactly `false` for a domain naming an Object.prototype member', () => {
230+
for (const domain of PROTOTYPE_RESOLVABLE) {
231+
for (const value of VALUES) {
232+
const answer = untyped(domain, value);
233+
const at = `${JSON.stringify(domain)} / ${JSON.stringify(value)}`;
234+
expect(typeof answer, at).toBe('boolean');
235+
expect(answer, at).toBe(false);
236+
}
237+
}
238+
});
239+
240+
it('answers exactly `false` for a plainly absent domain, where it used to throw', () => {
241+
for (const domain of PLAINLY_ABSENT) {
242+
for (const value of VALUES) {
243+
const answer = untyped(domain, value);
244+
const at = `${JSON.stringify(domain)} / ${JSON.stringify(value)}`;
245+
expect(typeof answer, at).toBe('boolean');
246+
expect(answer, at).toBe(false);
247+
}
248+
}
249+
});
250+
251+
it('holds this population HONEST — every word above is outside the vocabulary', () => {
252+
// Without this, a word promoted into `ValueDomainSchema` would leave the
253+
// two pins above asserting `false` for a legal domain, and they would go on
254+
// passing while meaning the opposite of what they say.
255+
for (const domain of OFF_VOCABULARY) {
256+
expect(ValueDomainSchema.safeParse(domain).success, domain).toBe(false);
257+
expect(ValueDomainSchema.options as readonly string[], domain).not.toContain(domain);
258+
}
259+
});
260+
261+
it('still answers the three real members from their own definitions', () => {
262+
// The narrowing must stop at the vocabulary edge: the guard refuses more
263+
// and accepts nothing new, so every in-vocabulary answer is unmoved.
264+
expect(isValueDomainMember('iana_time_zone', 'UTC')).toBe(true);
265+
expect(isValueDomainMember('iso_4217_currency', 'USD')).toBe(true);
266+
expect(isValueDomainMember('iso_3166_alpha2', 'US')).toBe(true);
267+
});
268+
});

packages/spec/src/shared/value-domain.zod.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,39 @@ const DOMAIN_MEMBERSHIP: Readonly<Record<ValueDomain, (value: string) => boolean
171171
* two code domains are exact uppercase). Element-wise iteration over a
172172
* multi-value carrier, and the prose a refusal message needs, are the
173173
* caller's: this function answers membership and nothing else.
174+
*
175+
* Total over the vocabulary AND closed outside it: a `domain` that is not a
176+
* member — including one that names an `Object.prototype` member such as
177+
* `toString`, `valueOf`, `constructor` or `__proto__` — answers `false`. It
178+
* never throws and never answers a non-boolean, so a caller reaching this
179+
* published export from plain JS or from metadata cannot get a membership
180+
* false positive out of an unknown domain word. See the guard's own comment.
174181
*/
175182
export function isValueDomainMember(domain: ValueDomain, value: string): boolean {
183+
// ⛔ The own-property guard is load-bearing, not defensive noise. `domain` is
184+
// typed, but this function is PUBLISHED (`api-surface/shared.json`), so a
185+
// plain-JS consumer — or any caller handing over a domain string read from
186+
// METADATA rather than written in source — arrives with no type checking at
187+
// all, and metadata-sourced strings are exactly where `constructor` and
188+
// `toString` show up. `DOMAIN_MEMBERSHIP` is an object literal, so it
189+
// inherits `Object.prototype` and a bare `DOMAIN_MEMBERSHIP[domain]` resolves
190+
// a prototype member for such a word: measured on the baseline, `toString`
191+
// answered `'[object Object]'` (a truthy STRING), `valueOf` and `constructor`
192+
// answered truthy OBJECTS, and `__proto__`, `nope` and `''` threw a
193+
// `TypeError` off a non-callable. A predicate whose whole job is to refuse
194+
// non-members therefore failed OPEN on three of them.
195+
//
196+
// The guard collapses every off-vocabulary domain onto one answer — `false`,
197+
// never truthy and never a throw — whether it is prototype-resolvable or
198+
// plainly absent. It narrows: nothing that was accepted before is refused
199+
// now, because every accepted `domain` is an own key. It is the same
200+
// spelling the `iso_4217_currency` definition above already uses.
201+
//
202+
// ⛔ Not `Object.create(null)` for the record: the null prototype would turn
203+
// the truthy answers into throws rather than into `false`, and it would cost
204+
// the `Readonly<Record<ValueDomain, …>>` annotation that makes a vocabulary
205+
// member added without a definition fail to COMPILE — the guarantee the
206+
// record's own doc comment above exists to state.
207+
if (!Object.prototype.hasOwnProperty.call(DOMAIN_MEMBERSHIP, domain)) return false;
176208
return DOMAIN_MEMBERSHIP[domain](value);
177209
}

0 commit comments

Comments
 (0)