fix(spec,objectql): internal field flag — stop sys_api_key.key serializing on the data API (#7728) - #7920
Conversation
…n the generic data path (#7728) ADR-0100 names three credential channels. The third — the auth subsystem's one-way hashes, which live in ordinary `text` columns — has had no read protection at all, and not by oversight: the engine's credential collectors key off the field TYPE, so a `text` column is collected by nothing regardless of `managedBy`. Retyping is not available either (`secret` encrypts at rest and destroys the verifier's hash lookup; `password` is defined as plaintext at rest, which a one-way hash is not), so the channel needs its own opt-in, type-independent declaration. `internal: true` is that declaration: "the declared value is never returned on the generic data path". Storage, filtering and indexing are explicitly out of scope, 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. Carries its ADR-0049 obligation in the same commit: the liveness ledger entry is `live` from landing day, with the enforcing call sites as evidence and both directions of the proof named. The authorable-surface baseline and the liveness state counts are regenerated, not hand-edited. This commit is deliberately separable from the `objectql` / `platform-objects` halves — `packages/spec` is normally the `domain:spec` seat's surface, and a cross-seat declaration is open on #6017 until merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RwFDN5B6EZ2vTDpkVhkxxF
…key.key` serializing (#7728) `sys_api_key.key` — the stored SHA-256 hash — declared "never exposed to clients" and serialized anyway. Measured on a real engine at origin/main, it came back on FOUR surfaces: get-by-id, list, an explicit `?select=id,key` projection, and the PATCH 200 body. `hidden: true` was never the broken contract (spec defines it as "Hidden from default UI"); the field's own description was. The omit is honoured at `Engine.maskSecretFields`' new collector branch and at the two write-response sites the #7642 `__search` strip already occupies — the create body and the by-id update body. The PATCH surface is not decoration: `sys_api_key` has `update` open (#7727) and its revoke/restore row actions PATCH it, so revoking a key handed the hash back until this commit. Omission rather than masking: `key` is `required: true`, so a "a value is set" mask carries zero bits while still shipping a value under a field that promises none. `?select=` is closed by construction — the strip acts on result rows, not on the projection — because `select` gates only on whether a field is known. Nothing below the response changes, which the tests assert in both directions: the value stays in storage, the verifier's `where: { key: <hash> }` lookup still resolves a principal, and `POST /api/v1/keys` still returns the raw secret once at creation. No system-caller carve-out, deliberately — unlike its sibling strip this flag has no system reader to protect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RwFDN5B6EZ2vTDpkVhkxxF
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 3 package(s): 109 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
|
ACCEPT — This is a credential-exposure fix, so I did not take the report's word on the two claims that decide whether it works. Both are the kind that a one-directional test passes while the product breaks. I read the sources myself. ⛔ The two load-bearing gates, measured independently1. Does anything read
2. Does the show-once mint return the ROW or the generated plaintext?
⇒ Both invariants hold by construction, not by test coverage. That is the right kind of safety here, and it is what licenses the no-carve-out design. 3. Everything else I checked
⭐ The ordering detail is right and easy to get wrong: What I most want on the recordThe PATCH surface was not in the card, and finding it is the difference between a fix and a half-fix. The no-carve-out decision is argued from a measurement, not a preference. The sibling ⭐ The out-of-scope observation is now a card: #7922Standing rule on this seat — the existence test for a split is an issue number, and ⛔ a credential PR is the worst possible place to ride an unrequested extra. You were right to record and not fix. #7922: Repricing for #7823 — accepted, and it changes the next dispatchYour reframing is the useful half. #7823's first question is no longer "does it serialize" (answered, replay-proven) but "does any server-side reader take Landing11/24 checks concluded, 0 failed, the rest running. Flipping ready; auto-merge arms once every job concludes — ⛔ Nothing to change. This is the strongest report this seat has reviewed: the premise was tested rather than assumed, a fourth leaking surface was found that the card never mentioned, both break-the-product directions were asserted, and the two things that should not have been fixed here weren't. Generated by Claude Code |
|
⛔ PATCH ROUND — CI red. The ACCEPT stands on the merits; this is mechanical and it is in your new test file. Auto-merge was not armed. All 26 jobs have now concluded: 24 success, 1 skipped, 1 failure. The failure is The exact failure
What to do — ⛔ and what not toFix the three errors in your own test file. They are new, they are yours, and they are three. ⛔ Do NOT raise the TEST_DEBT entry. The gate allows raising only for errors "genuinely irreducible today", and errors introduced by a file added in this same PR are the opposite of that. Raising the ledger to admit a brand-new file is precisely the "permission slip" #5278 exists to refuse, and it would leave a 3-error hole through which unrelated regressions can land silently (the log names driver-mongodb's 33 swallowing a whole signature reversion). ⛔ Do NOT run ⇒ The whole fix is: make Re-verify with the gate that actually caught this
and confirm it exits 0 with no ⛔ Do not re-run any other gate you already ran, and ⛔ do not touch anything outside My share of thisMy dispatch brief named the suites to run but not this ratchet, even though the card was always going to add a test file to Generated by Claude Code |
…ratchet holds at 355 (#7728) The new test file added 3 raw `tsc --noEmit` errors to `@objectstack/objectql`'s hidden test layer (355 → 358). The package excludes `**/*.test.ts` from its own tsconfig, so `pnpm typecheck` was green over a layer it never reads — the ratchet (#5278) is the only thing that sees it. Three errors, all in this file, all real rather than cosmetic: - two TS2554 at the `registerObject` calls — `packageId` is a REQUIRED second parameter, and passing one argument was wrong, not merely untyped; - one TS6133 — a `created` binding the repeated-read test never used. Fixed at the source: the fixtures are annotated `ServiceObject` (they satisfy it as written), the calls pass a `packageId`, and the unused binding is gone. Back to exactly 355 with zero errors from this file, and the 10 tests still pass. ⛔ The TEST_DEBT entry is NOT raised — raising a frozen number to admit a file added in the same PR is the "permission slip" #5278 exists to refuse, and it would leave a 3-error hole for unrelated regressions. No other ledger entry is touched either; the nine informational "can be lowered" lines are pre-existing and none of them is this PR's to close. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RwFDN5B6EZ2vTDpkVhkxxF
Patch round —
|
2 participants
Fixes #7728
Implements the maintainer ruling of 2026-08-12 — (a) the new spec flag: approved; (b) omit, not mask — against the prior dev's report, which the ruling designates as the implementation spec (
dev 的报告即实现规格). The design is not redesigned here; it is built, measured, and registered.The defect, measured before touching anything
sys_api_key.key— the stored SHA-256 hash — declareddescription: 'Hashed API key value — never exposed to clients'and serialized anyway. Run on a real engine (bootStack(showcaseStack)) against a full build of unmodifiedorigin/main@e3a6f6e:GET /data/sys_api_key/{id}expected [ 'id', 'created_at', …(9) ] to not include 'key'GET /data/sys_api_key(list)GET /data/sys_api_key/{id}?select=id,keyexpected [ 'id', 'key' ] to not include 'key'PATCH /data/sys_api_key/{id}200 bodyexpected [ 'id', 'created_at', …(9) ] to not include 'key'The
?select=row confirms the PM's premise #1 on this object, not only on #7823's sibling: a strip that only touched the default projection would have shipped looking complete.The PATCH row is new — it was not in the card, and it is not incidental.
sys_api_keyis one of the few identity objects with a write verb open (apiMethods: ['get','list','update'], #7727), and its declaredrevoke_api_key/restore_api_keyrow actions PATCH it. So revoking a key handed the stored hash back in the success body.hidden: trueis not the contract that was broken — spec defines it as "Hidden from default UI", never as "stripped from serialization". The broken contract is the field's own description.Why nothing existing could carry it (re-verified, not inherited)
ADR-0100 names three credential channels; the third — the auth subsystem's one-way hashes in ordinary
textcolumns — had no read protection at all, and structurally so:collectMaskedReadFieldscollects by field TYPE, so atextcolumn is collected by nothing regardless ofmanagedBy. The better-auth exemption is the second barrier, not the first.Field.secretencrypts at rest and replaces the column with asys_secretref → destroyswhere: { key: hashApiKey(raw) }inresolveApiKeyPrincipal(packages/core/src/security/api-key.ts:152). It would break authentication to fix a disclosure.Field.passwordis defined as plaintext at rest; a one-way hash is not. Adopting it swaps one false declaration for another — and it is inert here anyway, per the type-collector fact above.requiredPermissionsmask per caller; the requirement is "nobody, ever", as the admin persona that could previously see it.The change
internal: true— an opt-in, type-independent field declaration: the declared value is never returned on the generic data path.Honoured at
Engine.maskSecretFields' new collector branch (the choke point the ruling names), and at the two write-response sites the #7642stripSearchCompanionprecedent already occupies — the 201 create body and the by-id update body. #7642 faced this exact question and answered it in its own comment: "leaving it out would mean POST and PATCH on one object disagreed about whether a client-invisible column is visible." The measurement above makes that concrete rather than theoretical here.Omit, not mask.
keyisrequired: true, so it is always set — the mask's "a value is set" signal carries zero bits while still shipping a value under a field that promises none. Omission also leaves the description string untouched, so the four generated translation bundles that mirror it do not churn.?select=is closed by construction. The strip acts on the result rows, not on the projection, so naming the column returns a 200 without it.selectgates only on whether a field is known, and a flagged column is known.No system-caller carve-out — a deliberate divergence from the precedent, stated rather than silent.
stripSearchCompanionFromReadkeeps__searchfor a system caller that names it by projection, because it has such a reader (the pinyin backfill) whose comparison would otherwise rewrite every row on every run. This flag has none: the verifier uses the column as awherefilter and never reads it off the result (it takesexpires_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. If a legitimate system reader ever appears, it should read through a purpose-built privileged accessor the wayresolveSecretdoes forsecret— that is written into the code comment so the next author is told.Verification record
Both directions, because a strip is trivially satisfiable by breaking the feature.
Real engine / real driver —
packages/qa/dogfood/test/api-key-hash-not-serialized.dogfood.test.ts, RED against a full build of unmodifiedorigin/main, GREEN after a fullpnpm buildwith the change (a source-only revert would have measured nothing —packages/runtimesuites resolve throughdist):key, keepsname/prefixkeyon every row?select=id,keyomits it, honours the rest of the projectionkey.internal === trueon the registered schema; stilltype: 'text'; description unchangedPOST /api/v1/keysstill returns the raw secret onceThe last two rows are the load-bearing ones: a change that stripped the column everywhere would pass every "absent" assertion and break the product. #7727's existing
api-key-revoke-lifecycle.dogfood.test.ts(6 tests, incl. mint → authenticate → revoke → 401 → restore → authenticate) also still passes unmodified.Engine level —
packages/objectql/src/internal-fields.test.ts(10 tests): the collector is flag-keyed and ignoresmanagedBy; strictly opt-in (internal: 1/'false'do not enrol a field); omit on find/findOne/create-body/update-body; the?select=case; unflagged objects untouched; and the negative direction — the value stays in storage, thewhere: { key: <hash> }filter still resolves the row, a wrong hash still misses, and repeated reads are not cumulative.Gates run locally, all green:
@objectstack/spec(10009),@objectstack/objectql(3325),@objectstack/platform-objects(339),@objectstack/runtime(2096),@objectstack/client(282),@objectstack/qa-dogfood(593 passed / 3 skipped), typecheck on spec + objectql + platform-objects + runtime + dogfood,check:type-check-debt(exit 0, 33 ledger entries re-measured, none above its recorded number),check:nul-bytes,check:error-code-casing,check:adr-0087-registration,check:empty-changeset, eslint on every changed file, and the spec generated-artifact family (check:authorable-surface,check:generated,check:docs,check:api-surface,check:skill-refs,check:skill-docs,check:spec-changes,check:migration-registry,check:variant-docs,check:liveness). Every regenerated artifact is committed as generated output, never hand-edited.Obligations discharged in this PR
packages/spec/liveness/field.jsongains aninternalentry,status: livefrom landing day,verifiedAt: 2026-08-12, evidence pointing at the collector and the four enforcing call sites, and both directions of the proof named.state-counts.mdregenerated (field66 → 67).not-required (no-migration-prescription). Purely additive — one new optional key, nothing renamed, retired or tombstoned, so there is no conversion to register and no consumer action to prescribe.check:adr-0087-registrationandcheck:spec-changesboth pass.Scope
One declaration site, per the ruling.
sys_session.tokenis #7823 andsys_account.passwordis a later card; neither is adopted here.packages/specchange is the first commit and is cleanly separable (62be724), per the declaration on #6017. If thedomain:specseat vetoes, that commit lifts out at the seam without touching the objectql commits.Repricing input for #7823
This makes #7823 substantially simpler, with one named measurement that decides whether it is a one-liner or a real card. The mechanism is now built, published, registered and proven, and the
?select=bypass it measured is closed by construction — so its spec and engine work is zero and it becomesinternal: trueon one field plus its own two-direction test. But:sys_api_key.keyis only ever awherefilter and is never read off a result, which is what licensed the no-carve-out design. #7823's first question is therefore no longer "does it serialize" (already answered, replay-proven) but "does any server-side reader takesys_session.tokenoff a find/findOne result?" — better-auth's session object carriestoken, so this is a live possibility. If no, one line. If yes, it needs either a privileged accessor for that reader or a ruling on adding a carve-out to this contract — which should come back as a ruling, not a silent divergence.Out-of-scope observation (not fixed, not a rider)
rejectCredentialAggregation(engine.ts) keys offcollectCredentialFields, i.e. thesecret/passwordtypes, so aninternal-flagged column is not covered by it. No reachable route exposes this forsys_api_keytoday (there is no/data/:object/aggregate, and analytics needs a declared dataset), so it is recorded rather than fixed — but foldinginternalinto that gate is the natural companion card, and it would matter more for a live credential than for a hash.Generated by Claude Code