diff --git a/.changeset/internal-field-flag-api-key-hash.md b/.changeset/internal-field-flag-api-key-hash.md new file mode 100644 index 0000000000..6ac566ca60 --- /dev/null +++ b/.changeset/internal-field-flag-api-key-hash.md @@ -0,0 +1,70 @@ +--- +"@objectstack/spec": minor +"@objectstack/objectql": minor +"@objectstack/platform-objects": patch +--- + +feat(spec): `internal: true` — a field whose value is never returned on the generic data path, applied to `sys_api_key.key` (#7728) + + + +`sys_api_key.key` — the stored **SHA-256 hash** of an API key — declared +`description: 'Hashed API key value — never exposed to clients'` and then +serialized anyway. Measured on a real engine at `origin/main`, the hash came back +on **four** surfaces: get-by-id, list, an explicit `?select=id,key` projection, +and the `PATCH` 200 body. + +`hidden: true` was not the broken contract — spec defines `hidden` as "Hidden +from default UI", never as "stripped from serialization". The broken contract was +the field's own description, and there was no mechanism to honour it. + +**Why no existing mechanism fit.** ADR-0100 names three credential channels, and +the third — the auth subsystem's one-way hashes, which live in ordinary `text` +columns — had no read protection at all. The engine's credential mask collects by +field **TYPE** (`collectMaskedReadFields` walks for `secret` / `password`), so a +`text` column is collected by nothing, *regardless* of `managedBy`; the +better-auth exemption is the second barrier, not the first. Retyping is not +available either: `Field.secret` encrypts at rest and replaces the column with a +`sys_secret` ref, which destroys the `where: { key: hashApiKey(raw) }` lookup the +API-key verifier depends on — it would break authentication in order to fix a +disclosure — and `Field.password` is defined as *plaintext at rest*, which a +one-way hash is not, so adopting it would swap one false declaration for another. + +**The new flag.** `internal: true` is an opt-in, type-independent field +declaration meaning *the declared value is never returned on the generic data +path*. The engine omits the key from the rows it hands back at the four post-hook +positions the `__search` companion strip (#7642) already occupies: `find`, +`findOne`, the 201 create body and the by-id update body. + +**Omission, not masking.** The credential mask signals "a value is set" without +leaking it. `key` is `required: true`, so it is always set — the signal carries +zero bits here, while still shipping a value under a field whose declaration +promises none. Omitting also leaves the description string untouched, so the four +generated translation bundles that mirror it do not churn. + +**`?select=` is closed by construction, and that half is load-bearing.** The strip +acts on the result rows rather than on the projection, so a client that spells the +column out gets a 200 without it. `select` only gates on whether a field is +*known*, and a flagged column is known — a projection-aware strip would have +shipped looking complete while leaking to anyone who named the column. + +**What is deliberately untouched**, because the flag would be unusable otherwise: +storage and encryption; filtering and indexing, so the verifier's hash lookup +still resolves a principal (the strip runs *after* the driver has evaluated the +predicate); and the show-once mint path — `POST /api/v1/keys` still returns the +raw secret exactly once at creation. + +Unlike its sibling `stripSearchCompanionFromRead`, this strip has **no +system-caller carve-out**. That one keeps the `__search` column for a system +reader that names it by projection, because it has such a reader whose backfill +would otherwise rewrite every row on every run. This flag has none: the verifier +uses the column as a filter and never reads it off the result, and the mint path +returns the plaintext it generated rather than the row it inserted. An escape +hatch nobody needs is a hole in a non-exposure guarantee. + +Scope is one declaration site. `sys_session.token` is tracked separately as #7823 +and `sys_account.password` is a later card; neither is adopted here. diff --git a/content/docs/references/data/field.mdx b/content/docs/references/data/field.mdx index 03d66f5dd3..9e69804feb 100644 --- a/content/docs/references/data/field.mdx +++ b/content/docs/references/data/field.mdx @@ -105,6 +105,7 @@ const result = CurrencyConfigSchema.parse(data); | **conditionalRequired** | `never` | optional | [REMOVED] `conditionalRequired` was removed in @objectstack/spec 17 (#3855) — use `requiredWhen`. Rename the key; the value (a CEL predicate) is unchanged. Run `os migrate meta --from 16` to rewrite existing sources automatically. | | **widget** | `string` | optional | Form widget override — names a registered field component (resolved as `field:`) to render this field instead of the `type` default. Degrades to the `type` renderer when unregistered. e.g. "object-ref", "filter-condition", "recipient-picker". | | **hidden** | `boolean` | optional | Hidden from default UI | +| **internal** | `boolean` | optional | [#7728] Never return this field's value on the generic data path — the engine OMITS the key from `find`/`findOne` results, the 201 create body and the by-id update body, on the default projection AND when a client names the field in `?select=`. Storage, filtering and indexing are untouched, so a server-side verifier can still match on the column and a purpose-built mint route can still return the value once at creation. The read protection for ADR-0100's third credential channel (auth-subsystem one-way hashes on `text` columns). Omission, not masking: a mask signals 'a value is set', which carries no information on a `required` column. | | **readonly** | `boolean` | optional | Read-only — never editable in forms, AND server-enforced on BOTH write paths: a non-system write to this field is silently dropped from the payload on UPDATE (#2948/#3003) and on INSERT (#3043; a create can no longer directly seed e.g. `approval_status: "approved"`), symmetric with `readonlyWhen`. A stripped INSERT field still falls back to its `defaultValue`. Exempt from the strip on BOTH paths: `isSystem` writes (seed replay, migration). Exempt on the UPDATE path ONLY: an opt-in "historical" import (`preserveAudit`, #3493) — which admits a whitelist (the audit/timestamp family plus author-declared business `readonly` fields). On INSERT the exemption does NOT apply (#6640): a non-system create that requests `preserveAudit` still has its readonly fields stripped, and is warned loudly that the exemption is UPDATE-only — replaying archival readonly facts on create requires a system context. A normal (non-system) import is NOT system-context and still strips. | | **requiredPermissions** | `string[]` | optional | [ADR-0066 D3] Capabilities required to read/edit this field (mask on read, deny on write; AND-gate). | | **ackPlaintextMasking** | `boolean` | optional | [ADR-0100] Affirm a generic `password` field's plaintext-at-rest / masked-on-read contract is intended, silencing the author-time warning (#3420). No effect on non-password fields. | diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index fede7a2155..14990be244 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -122,6 +122,7 @@ import type { ICryptoProvider, CryptoHandle } from '@objectstack/spec/contracts' import { collectSecretFields, collectMaskedReadFields, + collectInternalReadFields, collectCredentialFields, makeSecretRef, parseSecretRef, @@ -4740,19 +4741,82 @@ export class ObjectQL implements IObjectQLEngine { * `null`. Privileged callers that genuinely need a secret's plaintext use * {@link resolveSecret} against the stored ref; a `password` field is stored * as plaintext at rest, so its cleartext is only ever reachable off this path. + * + * [#7728] Second collector branch, same choke point: a field declared + * `internal: true` is OMITTED rather than masked — see + * {@link omitInternalFields} for why the two dispositions differ. */ private maskSecretFields(object: string, rows: any): void { if (!rows) return; const schema = this._registry.getObject(object); const maskedFields = collectMaskedReadFields(schema); - if (maskedFields.length === 0) return; + if (maskedFields.length > 0) { + const list = Array.isArray(rows) ? rows : [rows]; + for (const row of list) { + if (!row || typeof row !== 'object') continue; + for (const field of maskedFields) { + if (!(field in row)) continue; + row[field] = row[field] == null ? null : SECRET_MASK; + } + } + } + // Runs AFTER the mask, so a field that is somehow both `secret`-typed and + // `internal` ends up omitted rather than masked — the stricter disposition + // wins, which is the only safe way for the two to compose. + this.omitInternalFields(object, rows); + } + + /** + * [#7728] Drop every field declared `internal: true` from the rows the engine + * hands back — "the declared value is never returned on the generic data + * path". This is the read protection for ADR-0100's third credential channel: + * auth-subsystem one-way hashes stored in `text` columns, which the two + * type-keyed credential collectors structurally cannot reach. + * + * **OMIT, not mask** (maintainer ruling 2026-08-12 on #7728). The credential + * mask exists to signal "a value is set" without leaking it. The column this + * was minted for — `sys_api_key.key` — is `required: true`, so it is ALWAYS + * set: the signal carries zero bits, while still shipping a value under a + * field whose own description says it is "never exposed to clients". Omission + * also leaves that description string untouched, so the four generated + * translation bundles that mirror it do not churn. + * + * **`?select=` is covered by construction, and that is load-bearing.** The + * strip acts on the RESULT ROWS, not on the projection, so a client that + * spells the column out (`?select=id,key`) gets a 200 without it rather than + * a bypass. `select` only gates on whether a field is KNOWN + * (`assertProjectionFieldsExist`) and a flagged column is known, so a + * projection-aware strip would have shipped looking complete and still leaked + * to any caller who named the column — measured on the sibling column in + * #7823, and reproduced here on `sys_api_key.key` before the fix. + * + * **No system carve-out**, and this is where the shape deliberately diverges + * from its sibling {@link stripSearchCompanionFromRead}. That one keeps the + * `__search` companion for a system caller who names it by projection, + * because it has exactly one such reader whose backfill comparison would + * otherwise rewrite every row on every run. This flag has no such reader: the + * API-key verifier uses the column as a `where` FILTER and never reads it off + * the result (`resolveApiKeyPrincipal` takes `expires_at` / `user_id` / + * `organization_id` / `scopes`), and the mint path returns the plaintext it + * generated, not the row it inserted. An escape hatch nobody needs is a hole + * in a non-exposure guarantee, so there isn't one — if a legitimate system + * reader ever appears, it reads the column through a purpose-built privileged + * accessor, the way {@link resolveSecret} does for `secret`. + * + * Nothing below storage is touched. The strip runs on rows the driver has + * already produced, so the predicate has been evaluated and the index used + * before this method sees anything — which is precisely why authentication + * keeps working. + */ + private omitInternalFields(object: string, rows: any): void { + if (!rows) return; + const schema = this._registry.getObject(object); + const internalFields = collectInternalReadFields(schema); + if (internalFields.length === 0) return; const list = Array.isArray(rows) ? rows : [rows]; for (const row of list) { if (!row || typeof row !== 'object') continue; - for (const field of maskedFields) { - if (!(field in row)) continue; - row[field] = row[field] == null ? null : SECRET_MASK; - } + for (const field of internalFields) delete row[field]; } } @@ -7779,6 +7843,12 @@ export class ObjectQL implements IObjectQLEngine { // AFTER the hook dispatch, matching the read path: `afterInsert` // handlers still observe the whole stored row. stripSearchCompanion(rowCtx.result); + // [#7728] Same position, same reason, for `internal` fields. A write + // has no projection to consult here either, so the omit is + // unconditional. This does NOT touch the show-once mint path: that + // route reads only `id` off the insert result and returns the + // plaintext it generated itself. + this.omitInternalFields(object, rowCtx.result); } // Roll-up: recompute parent summary fields that aggregate this object. @@ -8716,6 +8786,16 @@ export class ObjectQL implements IObjectQLEngine { // an affected-row COUNT (#4639), which the strip skips as a // non-object. stripSearchCompanion(hookContext.result); + // [#7728] …and the same for `internal` fields, on the identical + // argument. This is not a hypothetical symmetry: `sys_api_key` is + // one of the few identity objects with a write verb open + // (`apiMethods: ['get','list','update']`, #7727) and its declared + // revoke/restore row actions PATCH it, so before this line a client + // revoking a key got the stored hash back in the 200 body — measured, + // and the fourth leaking surface on the object #7728 was filed + // against. A predicate update resolves to an affected-row COUNT + // (#4639), which the omit skips as a non-object. + this.omitInternalFields(object, hookContext.result); // The record IS updated; a summary that could not recompute after // retries must surface, not stay silent (framework#3147). if (summaryFailures.length > 0) throw new SummaryRecomputeError(summaryFailures, hookContext.result); diff --git a/packages/objectql/src/internal-fields.test.ts b/packages/objectql/src/internal-fields.test.ts new file mode 100644 index 0000000000..3606811cbc --- /dev/null +++ b/packages/objectql/src/internal-fields.test.ts @@ -0,0 +1,283 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7728 — the `internal: true` field flag, against a REAL {@link ObjectQL} + * engine + a minimal stub driver. + * + * The flag declares "the value is never returned on the generic data path". It + * exists because the two credential collectors key off the field **TYPE** + * (`secret` / `password`), so a one-way hash living in a `text` column — + * ADR-0100's third channel — is collected by neither, regardless of `managedBy`. + * + * Every assertion here is paired with its negative, because a strip is trivially + * satisfiable by breaking the feature: + * + * - absent from the RESPONSE ⇄ still present in STORAGE + * - absent from the RESPONSE ⇄ still usable as a `where` FILTER (this is the + * verifier's `where: { key: }`, the thing that must not break) + * - the flagged field omitted ⇄ every other field survives + * + * The `?select=` case is its own test rather than a variation: `select` gates + * only on whether a field is KNOWN, so a projection-aware strip would leak to + * any caller who spelled the column out. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { collectInternalReadFields, SECRET_MASK } from './secret-fields.js'; +import type { ServiceObject } from '@objectstack/spec/data'; + +// ---- minimal stub driver (equality-only WHERE) ---------------------------- +// Rows leave the driver as COPIES, as a real driver's do — see the note in +// `secret-fields.test.ts` (#7799): handing out the live stored object would let +// the engine's own strip mutate storage, which reads exactly like an engine bug. +function makeStubDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const copy = (r: T): T => (r == null ? r : ({ ...r } as T)); + const project = (row: any, fields?: string[]) => { + if (!row || !Array.isArray(fields) || fields.length === 0) return copy(row); + const out: Record = {}; + for (const f of fields) if (f in row) out[f] = row[f]; + return out; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()) + .filter((r) => matches(r, ast?.where)) + .map((r) => project(r, ast?.fields)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return project(r, ast?.fields); + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return copy(row); + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) throw new Error(`not found: ${object}/${id}`); + const updated = { ...cur, ...data, id }; + s.set(id, updated); + return copy(updated); + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + +/** + * Shaped after `sys_api_key`: a one-way hash in a `text` column on a + * better-auth-managed identity object. `managedBy` is set on purpose — it is + * what makes `password` retyping inert, and the flag must work in spite of it. + */ +const tokenObject: ServiceObject = { + name: 'itest_api_key', + label: 'API Key', + managedBy: 'better-auth', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + prefix: { name: 'prefix', label: 'Prefix', type: 'text' as const }, + revoked: { name: 'revoked', label: 'Revoked', type: 'boolean' as const }, + key: { + name: 'key', label: 'Hashed Key', type: 'text' as const, + required: true, hidden: true, readonly: true, internal: true, + }, + }, +}; + +/** No flagged field — the fast path, and the proof the flag is opt-in. */ +const plainObject: ServiceObject = { + name: 'itest_plain', + label: 'Plain', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const }, + key: { name: 'key', label: 'Key', type: 'text' as const }, + }, +}; + +async function buildEngine() { + const engine = new ObjectQL(); + const { driver, stores } = makeStubDriver(); + engine.registerDriver(driver, true); + await engine.init(); + // `packageId` is required — these fixtures own their objects outright. + engine.registry.registerObject(tokenObject, 'internal-fields-test'); + engine.registry.registerObject(plainObject, 'internal-fields-test'); + return { engine, stores }; +} + +const HASH = 'sha256:deadbeefcafe'; + +describe('#7728: the `internal` field flag omits a value from the generic data path', () => { + let ctx: Awaited>; + beforeEach(async () => { ctx = await buildEngine(); }); + + const seed = async () => + ctx.engine.insert('itest_api_key', { + name: 'k1', prefix: 'osk_', revoked: false, key: HASH, + }, { context: { isSystem: true } } as any); + + describe('the collector', () => { + it('collects by FLAG, not by type, and ignores `managedBy`', () => { + // The two facts that make the flag necessary at all: the column is + // `text` (so no type-keyed collector sees it) on a better-auth object + // (so the password exemption would have skipped it anyway). + expect(collectInternalReadFields(tokenObject as any)).toEqual(['key']); + expect(collectInternalReadFields(plainObject as any)).toEqual([]); + expect(collectInternalReadFields(undefined)).toEqual([]); + expect(collectInternalReadFields({ name: 'x' } as any)).toEqual([]); + }); + + it('is strictly opt-in — a truthy-but-not-true value does not enrol a field', () => { + // A non-exposure guarantee must not be switchable by accident. `1` / + // `'false'` are the shapes a loose `!!def.internal` would silently accept. + const loose = { name: 'o', fields: { a: { internal: 1 }, b: { internal: 'false' }, c: { internal: false } } }; + expect(collectInternalReadFields(loose as any)).toEqual([]); + }); + }); + + describe('the read path', () => { + it('omits the flagged field from find and findOne — the key is absent, not null', async () => { + const created = await seed(); + + const viaFind = (await ctx.engine.find('itest_api_key', { where: { id: created.id } }))[0] as any; + expect(Object.keys(viaFind)).not.toContain('key'); + // Not masked either — omit was ruled over mask (#7728 (b)). + expect(Object.values(viaFind)).not.toContain(SECRET_MASK); + // Falsifiability: this is a real row, not an emptied one. + expect(viaFind.name).toBe('k1'); + expect(viaFind.prefix).toBe('osk_'); + + const viaOne = await ctx.engine.findOne('itest_api_key', { where: { id: created.id } }) as any; + expect(Object.keys(viaOne)).not.toContain('key'); + expect(viaOne.name).toBe('k1'); + }); + + it('an EXPLICIT projection naming the field does not bypass the omit', async () => { + // The #7823 bypass. `select`/`fields` only gates on whether a field is + // KNOWN, and a flagged field is known — so naming it must be served + // WITHOUT it rather than refused, and the rest of the projection honoured. + const created = await seed(); + + const rows = await ctx.engine.find('itest_api_key', { + where: { id: created.id }, fields: ['id', 'key', 'prefix'], + }); + expect(rows).toHaveLength(1); + expect(Object.keys(rows[0] as any)).not.toContain('key'); + expect((rows[0] as any).id).toBe(created.id); + expect((rows[0] as any).prefix).toBe('osk_'); + + const one = await ctx.engine.findOne('itest_api_key', { + where: { id: created.id }, fields: ['id', 'key'], + }) as any; + expect(Object.keys(one)).not.toContain('key'); + expect(one.id).toBe(created.id); + }); + + it('leaves an unflagged object completely alone (opt-in, and the fast path)', async () => { + const created = await ctx.engine.insert('itest_plain', { key: 'visible' }); + const row = await ctx.engine.findOne('itest_plain', { where: { id: created.id } }) as any; + expect(row.key).toBe('visible'); + }); + }); + + describe('the write-response surfaces', () => { + it('omits the flagged field from the create body', async () => { + const created = await seed(); + expect(Object.keys(created)).not.toContain('key'); + // The create still returns a usable record — the mint path reads `id` + // off exactly this value. + expect(created.id).toBeTruthy(); + }); + + it('omits the flagged field from the by-id update body', async () => { + // The surface measured leaking on `sys_api_key` itself: that object has + // `update` open (#7727) and its revoke/restore row actions PATCH it. + const created = await seed(); + const updated = await ctx.engine.update('itest_api_key', { id: created.id, revoked: true }, { + context: { isSystem: true }, + } as any); + expect(Object.keys(updated)).not.toContain('key'); + expect(updated.revoked).toBe(true); + }); + }); + + describe('the negative direction — nothing below the response changes', () => { + it('keeps the value in STORAGE', async () => { + const created = await seed(); + await ctx.engine.find('itest_api_key', { where: { id: created.id } }); + await ctx.engine.findOne('itest_api_key', { where: { id: created.id } }); + + // Read straight out of the driver's store, past the engine. If the strip + // mutated the stored row instead of the response copy, the credential is + // destroyed and the object is unrecoverable. + const stored = ctx.stores.get('itest_api_key')!.get(created.id) as any; + expect(stored.key).toBe(HASH); + }); + + it('keeps the field usable as a WHERE filter — the verifier lookup', async () => { + // `resolveApiKeyPrincipal` does exactly this: match the at-rest hash, + // then read `expires_at`/`user_id`/`scopes` off the row. If the flag were + // implemented by dropping the column from the QUERY instead of from the + // response, this returns nothing and authentication breaks platform-wide. + const created = await seed(); + + const found = await ctx.engine.find('itest_api_key', { where: { key: HASH, revoked: false }, limit: 1 }); + expect(found).toHaveLength(1); + expect((found[0] as any).id).toBe(created.id); + // …and the matched row still hands back the columns the verifier reads, + // while withholding the one it only ever filters on. + expect((found[0] as any).name).toBe('k1'); + expect(Object.keys(found[0] as any)).not.toContain('key'); + + // A wrong hash still misses — the filter is real, not ignored. + expect(await ctx.engine.find('itest_api_key', { where: { key: 'sha256:wrong' } })).toHaveLength(0); + }); + + it('survives repeated reads — the strip is not cumulative on storage', async () => { + await seed(); + for (let i = 0; i < 3; i++) { + await ctx.engine.find('itest_api_key', { where: { key: HASH } }); + } + const found = await ctx.engine.find('itest_api_key', { where: { key: HASH } }); + expect(found).toHaveLength(1); + }); + }); +}); diff --git a/packages/objectql/src/secret-fields.ts b/packages/objectql/src/secret-fields.ts index 15f743e254..4c968b77b7 100644 --- a/packages/objectql/src/secret-fields.ts +++ b/packages/objectql/src/secret-fields.ts @@ -23,6 +23,18 @@ * `text` column) off the generic CRUD path. Objects it owns carry * `managedBy: 'better-auth'` and are exempt from password masking so login reads * still see the stored hash. + * + * [#7728] That third channel had **no read protection at all**, and the reason is + * structural rather than an oversight in the exemption: the two collectors above + * key off the field **TYPE**, so a `text` column is never collected *regardless* + * of `managedBy` — the better-auth exemption is the second barrier, not the + * first. Retyping is not the fix either, because `secret` rewrites the column to + * a `sys_secret` ref (destroying the `where: { key: }` lookup the API-key + * verifier depends on) and `password` is declared plaintext-at-rest, which a + * one-way hash is not. So the channel gets its own opt-in, type-independent + * declaration — the `internal` field flag ({@link collectInternalReadFields}), + * which OMITS rather than masks. See ADR-0100 / ADR-0049 and the maintainer + * ruling of 2026-08-12 on #7728. */ import type { ServiceObject } from '@objectstack/spec/data'; @@ -98,6 +110,38 @@ export function collectMaskedReadFields(schema: ServiceObject | undefined | null return out; } +/** + * [#7728] Collect the names of fields declared `internal: true` — "the declared + * value is never returned on the generic data path". + * + * Three differences from {@link collectMaskedReadFields}, all deliberate: + * + * - **It collects by FLAG, not by TYPE.** That is the whole point: the columns + * this protects are one-way hashes living in `text` columns, which no + * type-keyed collector can ever reach. + * - **No `managedBy` exemption.** The password exemption exists so login reads + * still see the stored hash; `internal` is opt-in *per field*, so an object + * that needs a column readable simply does not flag it. An exemption here + * would silently disable the flag on exactly the identity objects it was + * minted for. + * - **The caller OMITS the key rather than masking it** (see + * {@link SECRET_MASK}). The mask signals "a value is set"; on a `required` + * column that is zero bits of information, and shipping it would still put a + * value under a field whose declaration promises none. + * + * Returns an empty array when the schema has no fields or none are flagged, so + * callers can fast-path on `length === 0`. + */ +export function collectInternalReadFields(schema: ServiceObject | undefined | null): string[] { + const fields = (schema as any)?.fields as Record | undefined; + if (!fields) return []; + const out: string[] = []; + for (const [name, def] of Object.entries(fields)) { + if (def && def.internal === true) out.push(name); + } + return out; +} + /** * Collect the names of every credential-bearing field on an object — `secret` * OR `password` — **unconditionally**, ignoring `managedBy`. diff --git a/packages/platform-objects/src/identity/sys-api-key.object.ts b/packages/platform-objects/src/identity/sys-api-key.object.ts index c19c099971..f92733085c 100644 --- a/packages/platform-objects/src/identity/sys-api-key.object.ts +++ b/packages/platform-objects/src/identity/sys-api-key.object.ts @@ -199,11 +199,26 @@ export const SysApiKey = ObjectSchema.create({ }), // ── Secret (hidden by default) ────────────────────────────── + // + // [#7728] `internal: true` is what makes the description below TRUE. It was + // false on every build before this flag existed: `hidden` is a UI contract + // ("Hidden from default UI"), never a serialization one, and the engine's + // credential read mask collects by field TYPE — so this `text` column was + // collected by nothing and the stored SHA-256 hash came back on get-by-id, + // on list, on an explicit `?select=id,key` and in the PATCH body. + // + // Still `text`, deliberately. `Field.secret` would encrypt at rest and + // replace the column with a `sys_secret` ref, destroying the + // `where: { key: hashApiKey(raw) }` lookup `resolveApiKeyPrincipal` uses — + // i.e. it would break authentication to fix a disclosure. `internal` is + // read-side only: storage, the index and the verifier's filter are + // untouched, and `POST /api/v1/keys` still returns the raw secret once. key: Field.text({ label: 'Hashed Key', required: true, hidden: true, readonly: true, + internal: true, description: 'Hashed API key value — never exposed to clients', group: 'Secret', }), diff --git a/packages/qa/dogfood/test/api-key-hash-not-serialized.dogfood.test.ts b/packages/qa/dogfood/test/api-key-hash-not-serialized.dogfood.test.ts new file mode 100644 index 0000000000..e4c159ff4b --- /dev/null +++ b/packages/qa/dogfood/test/api-key-hash-not-serialized.dogfood.test.ts @@ -0,0 +1,185 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #7728 — `sys_api_key.key` (the stored SHA-256 hash) must not come back on the + * generic read path, because its own declaration says it never does: + * + * description: 'Hashed API key value — never exposed to clients' + * + * On `origin/main` that sentence was false. The engine's read path masks only + * `secret`- and `password`-TYPED columns (`collectMaskedReadFields`), and `key` + * is `text`, so nothing collected it and the hash serialized on get-by-id and + * list. `hidden: true` is not the contract that was broken — spec defines it as + * "Hidden from default UI", not "stripped from serialization" — the broken + * contract is the field's own description. The fix is the new `internal: true` + * field flag, honoured at the same post-hook choke point as the credential mask. + * + * **This file has to drive BOTH directions**, and the negative one is the + * load-bearing half. A change that strips the column everywhere would satisfy + * every "absent" assertion below and still break the product: + * + * - the verifier resolves a principal with `where: { key: hashApiKey(raw) }`, + * so authentication must keep working (`keyStillAuthenticates`); + * - `POST /api/v1/keys` returns the raw secret ONCE at mint, and that is the + * only time a client ever sees the credential. + * + * The third pin is `?select=`. #7823 measured the sibling column coming back by + * EXPLICIT projection as well as on the default one, so a strip that only + * touched the default projection would ship looking complete and still leak to + * any client that spells the column out. `select` gates on whether a field is + * KNOWN, and `key` is known — so the explicit-projection case is its own test. + * + * Falsifiability: `prefix` / `name` are asserted PRESENT throughout. Without + * them a "delete every column" bug reads as a pass. + * + * Refusal/absence cases assert `code` AND `status` where a refusal is involved + * (ADR-0112); a bare status check stays green against a naked `Error`. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; + +describe('#7728: sys_api_key.key (hash) never serializes on the generic read path', () => { + let stack: VerifyStack; + let token: string; + + /** Mint through the ONE mint path; returns the row id + the show-once secret. */ + const mintKey = async (name: string): Promise<{ id: string; raw: string }> => { + const res = await stack.apiAs(token, 'POST', '/keys', { name }); + expect(res.status).toBe(201); + const body: any = await res.json(); + expect(body?.data?.id).toBeTruthy(); + // The show-once mint path is NOT the generic read path and must keep + // returning the credential — this is the negative direction, asserted at + // the moment it would break. + expect(typeof body?.data?.key).toBe('string'); + expect(body.data.key.length).toBeGreaterThan(8); + return { id: String(body.data.id), raw: String(body.data.key) }; + }; + + /** + * Does this key still authenticate? Asked through a real authenticated read + * with NO bearer token, so the key is the only credential present. + */ + const keyStillAuthenticates = async (raw: string): Promise => { + const res = await stack.api('/data/sys_api_key', { headers: { 'x-api-key': raw } }); + if (res.status === 200) return true; + expect(res.status).toBe(401); + return false; + }; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, {}); + token = await stack.signIn(); + }, 120_000); + + afterAll(async () => { await stack?.stop?.(); }); + + it('get-by-id omits `key` on the default projection, and keeps the other columns', async () => { + const { id, raw } = await mintKey('hash-getbyid'); + + const res = await stack.apiAs(token, 'GET', `/data/sys_api_key/${id}`); + expect(res.status).toBe(200); + const record = ((await res.json()) as any).record ?? {}; + + // OMIT, not mask (maintainer ruling 2026-08-12 (b)): `key` is + // `required: true`, so a "a value is set" mask carries zero bits here while + // still shipping a value under a field whose description promises none. + // `toBeUndefined()` alone would pass on a masked value of `undefined`; the + // key must be absent from the object. + expect(Object.keys(record)).not.toContain('key'); + + // Falsifiability: the read still works and still carries its other columns. + expect(record.id).toBe(id); + expect(record.name).toBe('hash-getbyid'); + expect(record.prefix).toBeTruthy(); + + // Negative direction: stripping a column from a RESPONSE must not disturb + // the stored row or the verifier's `where: { key: }` lookup. + expect(await keyStillAuthenticates(raw)).toBe(true); + }); + + it('list omits `key` on every row', async () => { + const { raw } = await mintKey('hash-list'); + + const res = await stack.apiAs(token, 'GET', '/data/sys_api_key'); + expect(res.status).toBe(200); + const rows = ((await res.json()) as any).records ?? []; + expect(rows.length).toBeGreaterThan(0); + + for (const row of rows) expect(Object.keys(row)).not.toContain('key'); + // …and the rows are real rows, not empty objects. + expect(rows.every((r: any) => typeof r.id === 'string')).toBe(true); + + expect(await keyStillAuthenticates(raw)).toBe(true); + }); + + it('an EXPLICIT `?select=id,key` projection does not bypass the strip', async () => { + // The bypass #7823 measured on the sibling column. `select` only gates on + // whether a field is KNOWN (`assertProjectionFieldsExist`) and `key` is + // known, so naming it is a legal request that must come back WITHOUT it — + // stripped, not refused, so a client asking for a legal-but-omitted column + // still gets its other columns. + const { id, raw } = await mintKey('hash-select'); + + const byId = await stack.apiAs(token, 'GET', `/data/sys_api_key/${id}?select=id,key`); + expect(byId.status).toBe(200); + const record = ((await byId.json()) as any).record ?? {}; + expect(Object.keys(record)).not.toContain('key'); + expect(record.id).toBe(id); + + const list = await stack.apiAs(token, 'GET', '/data/sys_api_key?select=id,key,prefix'); + expect(list.status).toBe(200); + const rows = ((await list.json()) as any).records ?? []; + expect(rows.length).toBeGreaterThan(0); + for (const row of rows) expect(Object.keys(row)).not.toContain('key'); + // The projection is otherwise honoured — proves the request was served, + // not silently downgraded to something that never contained `key` anyway. + expect(rows.every((r: any) => typeof r.prefix === 'string')).toBe(true); + + expect(await keyStillAuthenticates(raw)).toBe(true); + }); + + it('the revoke lifecycle still works, and its PATCH response carries no `key` either', async () => { + // `sys_api_key` is one of the few identity objects with a write verb open + // (`apiMethods: ['get','list','update']`, #7727), so PATCH is a response + // surface a real client hits on this exact object. Read it explicitly + // rather than assuming the read-path strip covers it. + const { id, raw } = await mintKey('hash-patch'); + expect(await keyStillAuthenticates(raw)).toBe(true); + + const patched = await stack.apiAs(token, 'PATCH', `/data/sys_api_key/${id}`, { revoked: true }); + expect(patched.status).toBe(200); + const body: any = await patched.json(); + const echoed = body?.record ?? body?.data ?? {}; + if (echoed && typeof echoed === 'object') { + expect(Object.keys(echoed)).not.toContain('key'); + } + + // The write itself still lands — the strip did not turn a real update into + // a no-op. + expect(await keyStillAuthenticates(raw)).toBe(false); + }); + + it('the declaration that makes all of the above required is still on the registered schema', async () => { + // The original defect was a DECLARATION disagreeing with the runtime, so + // pin the declaration from the REGISTERED schema — what the runtime serves, + // not what the source file says. If `internal` is dropped from `key`, every + // assertion above breaks anyway, but this one says WHY in one line. + const engine = await stack.kernel.getServiceAsync('objectql'); + const schema = engine?.getSchema?.('sys_api_key'); + expect(schema, 'sys_api_key schema must be registered').toBeTruthy(); + + const key = schema.fields?.key; + expect(key, 'sys_api_key.key must stay declared').toBeTruthy(); + expect(key.internal).toBe(true); + // The description this card exists to make true — unchanged by the fix + // (omit was ruled over mask partly to avoid churning the four generated + // translation bundles that mirror this string). + expect(String(key.description)).toContain('never exposed to clients'); + // Still a plain `text` column: the fix does NOT retype it, because + // `secret` would encrypt at rest and destroy the verifier's hash lookup. + expect(key.type).toBe('text'); + }); +}); diff --git a/packages/spec/authorable-surface/data.json b/packages/spec/authorable-surface/data.json index 7e9b4ace97..d2e10eb556 100644 --- a/packages/spec/authorable-surface/data.json +++ b/packages/spec/authorable-surface/data.json @@ -381,6 +381,7 @@ "data/Field:inlineEdit", "data/Field:inlineHelpText", "data/Field:inlineTitle", + "data/Field:internal", "data/Field:label", "data/Field:language", "data/Field:lookupColumns", diff --git a/packages/spec/liveness/field.json b/packages/spec/liveness/field.json index 0e0dd03f99..1400f6803a 100644 --- a/packages/spec/liveness/field.json +++ b/packages/spec/liveness/field.json @@ -104,6 +104,12 @@ "status": "live", "note": "renderer." }, + "internal": { + "status": "live", + "verifiedAt": "2026-08-12", + "evidence": "packages/objectql/src/secret-fields.ts (collectInternalReadFields — collects by FLAG, not by field type); packages/objectql/src/engine.ts (Engine.omitInternalFields deletes the collected keys from the result rows at four post-hook sites: find, findOne, the 201 create body and the by-id update body)", + "note": "[#7728] ADR-0100 named three credential channels and channel 3 — auth-subsystem one-way hashes on text columns — had NO read protection: collectMaskedReadFields collects the secret/password TYPES, so a text column is never collected regardless of managedBy. sys_api_key.key (a SHA-256 hash declared 'never exposed to clients') therefore serialized on get-by-id, on list, on an explicit ?select=id,key projection and in the PATCH body — all four measured on a real engine before the fix. This flag is that channel's read protection: OMIT rather than SECRET_MASK (key is required:true, so a 'a value is set' mask carries zero bits) applied at the same post-hook position the credential mask and the #7642 __search companion strip already occupy. Storage, the verifier's where:{key:} lookup and the show-once POST /api/v1/keys mint path are untouched by construction — the strip acts on RESULT ROWS, after the driver has evaluated the predicate. Proven in packages/qa/dogfood/test/api-key-hash-not-serialized.dogfood.test.ts (both directions: absent from all four surfaces AND the key still authenticates AND mint still returns it once) and packages/objectql/src/internal-fields.test.ts (engine-level, including the ?select= projection case and non-flagged columns surviving)." + }, "widget": { "status": "live", "verifiedAt": "2026-08-09", diff --git a/packages/spec/liveness/state-counts.md b/packages/spec/liveness/state-counts.md index e275c3cb2d..77d201a851 100644 --- a/packages/spec/liveness/state-counts.md +++ b/packages/spec/liveness/state-counts.md @@ -28,7 +28,7 @@ for both corollaries. | Type | live | exp | dead | planned | classified | |---|---|---|---|---|---| | `object` | 49 | 0 | 0 | 1 | 50 | -| `field` | 66 | 0 | 0 | 0 | 66 | +| `field` | 67 | 0 | 0 | 0 | 67 | | `flow` | 34 | 0 | 6 | 0 | 40 | | `action` | 42 | 0 | 2 | 0 | 44 | | `hook` | 18 | 0 | 2 | 0 | 20 | @@ -57,4 +57,4 @@ for both corollaries. | `api` | 25 | 0 | 0 | 2 | 27 | | `capability` | 12 | 0 | 0 | 0 | 12 | | `qa` | 4 | 0 | 5 | 0 | 9 | -| **total** | **776** | **6** | **52** | **5** | **839** | +| **total** | **777** | **6** | **52** | **5** | **840** | diff --git a/packages/spec/src/data/field.zod.ts b/packages/spec/src/data/field.zod.ts index 803bba87a8..850a0a1dda 100644 --- a/packages/spec/src/data/field.zod.ts +++ b/packages/spec/src/data/field.zod.ts @@ -860,6 +860,40 @@ export const FieldSchema = lazySchema(() => strictObject({ /** Security & Visibility */ hidden: z.boolean().default(false).describe('Hidden from default UI'), + + /** + * [#7728] "The declared value is never returned on the generic data path." + * + * Opt-in, and deliberately NOT a synonym for anything already here. `hidden` + * is a UI contract ("Hidden from default UI") and has never governed + * serialization; `readonly` governs the WRITE path; `requiredPermissions` + * masks per CALLER, so it cannot express "nobody, ever". This flag is the + * read-side complement: the engine OMITS the key from the rows it hands back + * — on `find`, `findOne`, the 201 create body and the by-id update body, on + * the default projection AND when a client names the field in `?select=`. + * + * OMIT, not mask (maintainer ruling 2026-08-12). The credential mask exists + * to signal "a value is set" without leaking it; on a `required` column that + * signal carries zero bits, while still shipping a value under a field whose + * declaration promises none. So the property is dropped, not replaced. + * + * What it deliberately does NOT do — the flag would be unusable otherwise: + * - it does not touch STORAGE or encryption (that is `Field.secret`, which + * rewrites the column to a `sys_secret` ref); + * - it does not touch FILTERING or indexing, so a server-side verifier can + * still match on the column (`where: { key: }`) — the strip + * runs on the RESULT ROWS, after the driver has evaluated the predicate; + * - it does not touch a purpose-built issue/mint route that returns the + * value once at creation off the generic path. + * + * This is the read protection for ADR-0100's third credential channel — + * auth-subsystem one-way hashes on `text` columns, which `secret`/`password` + * masking cannot reach because `collectMaskedReadFields` collects by TYPE and + * a `text` column is never collected. ADR-0049: enforced from landing day, at + * `Engine.maskSecretFields`. + */ + internal: z.boolean().optional().describe("[#7728] Never return this field's value on the generic data path — the engine OMITS the key from `find`/`findOne` results, the 201 create body and the by-id update body, on the default projection AND when a client names the field in `?select=`. Storage, filtering and indexing are untouched, so a server-side verifier can still match on the column and a purpose-built mint route can still return the value once at creation. The read protection for ADR-0100's third credential channel (auth-subsystem one-way hashes on `text` columns). Omission, not masking: a mask signals 'a value is set', which carries no information on a `required` column."), + readonly: z.boolean().default(false).describe('Read-only — never editable in forms, AND server-enforced on BOTH write paths: a non-system write to this field is silently dropped from the payload on UPDATE (#2948/#3003) and on INSERT (#3043; a create can no longer directly seed e.g. `approval_status: "approved"`), symmetric with `readonlyWhen`. A stripped INSERT field still falls back to its `defaultValue`. Exempt from the strip on BOTH paths: `isSystem` writes (seed replay, migration). Exempt on the UPDATE path ONLY: an opt-in "historical" import (`preserveAudit`, #3493) — which admits a whitelist (the audit/timestamp family plus author-declared business `readonly` fields). On INSERT the exemption does NOT apply (#6640): a non-system create that requests `preserveAudit` still has its readonly fields stripped, and is warned loudly that the exemption is UPDATE-only — replaying archival readonly facts on create requires a system context. A normal (non-system) import is NOT system-context and still strips.'), /**