From 0fad76341222c14ac83b029745b717b293edde96 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 14:39:01 +0000 Subject: [PATCH] fix(spec): refuse an off-vocabulary domain in isValueDomainMember instead of failing open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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>` 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. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x --- ...omain-membership-off-vocabulary-refusal.md | 54 +++++++++++ packages/spec/src/shared/value-domain.test.ts | 92 +++++++++++++++++++ packages/spec/src/shared/value-domain.zod.ts | 32 +++++++ 3 files changed, 178 insertions(+) create mode 100644 .changeset/value-domain-membership-off-vocabulary-refusal.md diff --git a/.changeset/value-domain-membership-off-vocabulary-refusal.md b/.changeset/value-domain-membership-off-vocabulary-refusal.md new file mode 100644 index 0000000000..06127d79ed --- /dev/null +++ b/.changeset/value-domain-membership-off-vocabulary-refusal.md @@ -0,0 +1,54 @@ +--- +'@objectstack/spec': patch +--- + +fix(spec): `isValueDomainMember` refuses an off-vocabulary domain instead of failing OPEN on `Object.prototype` names + +`DOMAIN_MEMBERSHIP` is an object literal, so it inherits `Object.prototype`, and +`isValueDomainMember` indexed it with no own-property guard. Measured against the +built artifact (`dist/shared/index.mjs`) on the repo's Node 22 baseline (v22.22.2), +an off-vocabulary `domain` did one of two wrong things — and one of them was a +membership FALSE POSITIVE out of a predicate whose whole job is to refuse +non-members: + +| `domain` | before | after | +|:--|:--|:--| +| `iana_time_zone` (in vocabulary) | `true` for `UTC` | `true` for `UTC` — unmoved | +| `toString` | `'[object Object]'` — a truthy **string** | `false` | +| `valueOf` | a truthy **object** | `false` | +| `constructor` | a truthy **object** | `false` | +| `__proto__` | threw a `TypeError` | `false` | +| `nope`, `''` | threw a `TypeError` | `false` | + +**Why it is reachable.** "Unreachable in-repo" is not "unreachable". The parameter +is typed `ValueDomain` and every in-repo call site names a member, but +`isValueDomainMember` is **published** on `@objectstack/spec/shared` (it is in +`packages/spec/api-surface/shared.json`). A plain-JS consumer, or any caller +handing over a domain string read from **metadata** rather than written in source, +reaches it with no type checking at all — and metadata-sourced strings are exactly +where `constructor` and `toString` show up. + +**This narrows and widens nothing, measured rather than asserted.** Every accepted +`domain` is an own key of the record, so no value that was accepted before is +refused now; the three real domains answer from their own definitions, unmoved. +The change is one `Object.prototype.hasOwnProperty.call` guard — the same spelling +the `iso_4217_currency` definition in the same module already uses — returning +`false` for a domain that is not an own key. A **null-prototype record** was the +other shape available and was not taken: it converts the truthy answers into +throws rather than into `false`, and it costs the `Readonly>` +annotation that makes a vocabulary member added without a definition fail to +compile. + +**Unknown domain answers `false`; it does not throw.** `false` is the narrowing +reading — it refuses more and accepts nothing new — whereas a thrown refusal would +change published behaviour for callers who today receive a truthy value. This is +the same third branch a sister ruling settled for the same defect family: list +reject / own-member value / prototype-resolvable ⇒ reject. + +The pin that existed did not cover this, and the fix is as much about its +POPULATION as about the guard: the totality pin asserted the return `typeof` was +`boolean` but iterated `ValueDomainSchema.options` **only** — exactly the domains +that behave. The 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 every one of them is still outside the vocabulary. diff --git a/packages/spec/src/shared/value-domain.test.ts b/packages/spec/src/shared/value-domain.test.ts index 7bb400bfba..53b79c7e4c 100644 --- a/packages/spec/src/shared/value-domain.test.ts +++ b/packages/spec/src/shared/value-domain.test.ts @@ -19,6 +19,7 @@ import { ValueDomainSchema, ISO_3166_ALPHA2_CODES, isValueDomainMember, + type ValueDomain, } from './value-domain.zod'; import { SpecifierValueDomainSchema } from '../system/settings-manifest.zod'; import { CURRENCY_FRACTION_DIGITS } from '../data/currency-fraction-digits'; @@ -168,9 +169,100 @@ describe('isValueDomainMember — iso_3166_alpha2 is the explicit 249-code list' describe('isValueDomainMember — every vocabulary member has a definition', () => { it('answers a boolean for each member, never throws, never returns undefined', () => { + // ⚠ The POPULATION here is `ValueDomainSchema.options` — exactly the + // domains that behave. That is the right population for THIS claim (every + // member has a definition), and the wrong one for "an unknown domain is + // refused": the describe below carries that claim over the population this + // loop cannot reach. for (const domain of ValueDomainSchema.options) { expect(typeof isValueDomainMember(domain, 'definitely-not-a-member')).toBe('boolean'); expect(isValueDomainMember(domain, 'definitely-not-a-member')).toBe(false); } }); }); + +describe('isValueDomainMember — an OFF-vocabulary domain is refused, never answered truthy', () => { + /** + * The published contract, exercised the way a consumer actually reaches it. + * `isValueDomainMember` is in `packages/spec/api-surface/shared.json`, so + * "unreachable in-repo" is not "unreachable": a plain-JS consumer, or any + * caller handing over a domain string read from METADATA rather than written + * in source, arrives with zero type checking — and metadata-sourced strings + * are exactly where `constructor` and `toString` show up. The cast is that + * caller, not a way around the type. + */ + const untyped = (domain: string, value: string): unknown => + isValueDomainMember(domain as ValueDomain, value); + + /** + * Domain words that are NOT in the vocabulary, grouped by what each one did + * before the own-property guard. `DOMAIN_MEMBERSHIP` is an object literal, so + * it inherits `Object.prototype`. + */ + const PROTOTYPE_RESOLVABLE = [ + // Answered TRUTHY — the membership false positives, the reason this is a + // bug and not a tidy-up: `toString` gave the string '[object Object]', + // `valueOf` and `constructor` gave objects. + 'toString', + 'valueOf', + 'constructor', + // Answered a boolean `false` by accident (`hasOwnProperty` called with + // `DOMAIN_MEMBERSHIP` as its receiver), which is why a `typeof` assertion + // alone is not enough to catch this family. + 'hasOwnProperty', + 'isPrototypeOf', + 'propertyIsEnumerable', + // Resolved to `Object.prototype` itself — not callable, so it THREW a + // TypeError. `false` now, like every other non-member. + '__proto__', + ]; + + /** No own key and no prototype member either: these threw a TypeError too. */ + const PLAINLY_ABSENT = ['nope', '', 'iana_timezone', 'iso_8601_date', 'bcp47_locale', 'ZZ']; + + const OFF_VOCABULARY = [...PROTOTYPE_RESOLVABLE, ...PLAINLY_ABSENT]; + + // Values spanning all three real domains plus junk, so a leaked definition + // would be caught whichever domain it leaked from. + const VALUES = ['UTC', 'USD', 'US', '', 'definitely-not-a-member']; + + it('answers exactly `false` for a domain naming an Object.prototype member', () => { + for (const domain of PROTOTYPE_RESOLVABLE) { + for (const value of VALUES) { + const answer = untyped(domain, value); + const at = `${JSON.stringify(domain)} / ${JSON.stringify(value)}`; + expect(typeof answer, at).toBe('boolean'); + expect(answer, at).toBe(false); + } + } + }); + + it('answers exactly `false` for a plainly absent domain, where it used to throw', () => { + for (const domain of PLAINLY_ABSENT) { + for (const value of VALUES) { + const answer = untyped(domain, value); + const at = `${JSON.stringify(domain)} / ${JSON.stringify(value)}`; + expect(typeof answer, at).toBe('boolean'); + expect(answer, at).toBe(false); + } + } + }); + + it('holds this population HONEST — every word above is outside the vocabulary', () => { + // Without this, a word promoted into `ValueDomainSchema` would leave the + // two pins above asserting `false` for a legal domain, and they would go on + // passing while meaning the opposite of what they say. + for (const domain of OFF_VOCABULARY) { + expect(ValueDomainSchema.safeParse(domain).success, domain).toBe(false); + expect(ValueDomainSchema.options as readonly string[], domain).not.toContain(domain); + } + }); + + it('still answers the three real members from their own definitions', () => { + // The narrowing must stop at the vocabulary edge: the guard refuses more + // and accepts nothing new, so every in-vocabulary answer is unmoved. + expect(isValueDomainMember('iana_time_zone', 'UTC')).toBe(true); + expect(isValueDomainMember('iso_4217_currency', 'USD')).toBe(true); + expect(isValueDomainMember('iso_3166_alpha2', 'US')).toBe(true); + }); +}); diff --git a/packages/spec/src/shared/value-domain.zod.ts b/packages/spec/src/shared/value-domain.zod.ts index 04e7a369fe..16909a0b9a 100644 --- a/packages/spec/src/shared/value-domain.zod.ts +++ b/packages/spec/src/shared/value-domain.zod.ts @@ -171,7 +171,39 @@ const DOMAIN_MEMBERSHIP: Readonly boolean * two code domains are exact uppercase). Element-wise iteration over a * multi-value carrier, and the prose a refusal message needs, are the * caller's: this function answers membership and nothing else. + * + * Total over the vocabulary AND closed outside it: a `domain` that is not a + * member — including one that names an `Object.prototype` member such as + * `toString`, `valueOf`, `constructor` or `__proto__` — answers `false`. It + * never throws and never answers a non-boolean, so a caller reaching this + * published export from plain JS or from metadata cannot get a membership + * false positive out of an unknown domain word. See the guard's own comment. */ export function isValueDomainMember(domain: ValueDomain, value: string): boolean { + // ⛔ The own-property guard is load-bearing, not defensive noise. `domain` is + // typed, but this function is PUBLISHED (`api-surface/shared.json`), so a + // plain-JS consumer — or any caller handing over a domain string read from + // METADATA rather than written in source — arrives with no type checking at + // all, and metadata-sourced strings are exactly where `constructor` and + // `toString` show up. `DOMAIN_MEMBERSHIP` is an object literal, so it + // inherits `Object.prototype` and a bare `DOMAIN_MEMBERSHIP[domain]` resolves + // a prototype member for such a word: measured on the baseline, `toString` + // answered `'[object Object]'` (a truthy STRING), `valueOf` and `constructor` + // answered truthy OBJECTS, and `__proto__`, `nope` and `''` threw a + // `TypeError` off a non-callable. A predicate whose whole job is to refuse + // non-members therefore failed OPEN on three of them. + // + // The guard collapses every off-vocabulary domain onto one answer — `false`, + // never truthy and never a throw — whether it is prototype-resolvable or + // plainly absent. It narrows: nothing that was accepted before is refused + // now, because every accepted `domain` is an own key. It is the same + // spelling the `iso_4217_currency` definition above already uses. + // + // ⛔ Not `Object.create(null)` for the record: the null prototype would turn + // the truthy answers into throws rather than into `false`, and it would cost + // the `Readonly>` annotation that makes a vocabulary + // member added without a definition fail to COMPILE — the guarantee the + // record's own doc comment above exists to state. + if (!Object.prototype.hasOwnProperty.call(DOMAIN_MEMBERSHIP, domain)) return false; return DOMAIN_MEMBERSHIP[domain](value); }