From bf422b783874ffcdbe79b30bfa84ded0a2e9ddc2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 10:55:32 +0000 Subject: [PATCH] fix(app-shell): answer `id: null` for a signed-out predicate identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildExpressionUser` carried `id` on its signed-in branch only, so `'id' in buildExpressionUser(null)` was `false`. An absent key is not `false`: a CEL predicate naming `ctx.user.id` / `current_user.id` / `os.user.id` hit an unbound key for a signed-out visitor and FAULTED, and a faulting visibility predicate fails OPEN, so the gated field rendered for exactly the principal it was written to exclude — at every mount site, silently. `null` rather than `undefined` or absent: settled by precedent on this same object (present-and-always-undefined "is the shape that teaches the wrong thing"), and absent IS the defect. `null` is a value a predicate can compare against, so the gate now resolves FALSE and bites. Fail-open on a predicate that DOES fault is untouched, deliberately. This removes a reason to fault; it does not change the fault policy. The `RECORDS:`-named pin recorded the defect as a passing fact and is inverted here; the five-key anonymous shape pin becomes six keys, and gains a key-set-equality assertion so a future edit that adds a key to one branch and forgets the other fails whichever branch it forgets. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q --- .../anonymous-expression-user-id-6534.md | 45 ++++++++++ .../AppContent.expressionUserShape.test.ts | 83 +++++++++++++++++++ .../expressionUser.mountParity.test.tsx | 54 +++++++----- .../app-shell/src/providers/expressionUser.ts | 37 ++++++++- 4 files changed, 198 insertions(+), 21 deletions(-) create mode 100644 .changeset/anonymous-expression-user-id-6534.md diff --git a/.changeset/anonymous-expression-user-id-6534.md b/.changeset/anonymous-expression-user-id-6534.md new file mode 100644 index 0000000000..c3c8459aae --- /dev/null +++ b/.changeset/anonymous-expression-user-id-6534.md @@ -0,0 +1,45 @@ +--- +'@object-ui/app-shell': patch +--- + +The predicate identity bound for a signed-out visitor now carries `id: null`, so a +`ctx.user.id` visibility gate BITES instead of failing open (objectui#6534). + +`buildExpressionUser` has two branches, and only the signed-in one carried `id`. +So `'id' in buildExpressionUser(null)` was `false`, and an absent key is not +`false`: a CEL predicate naming `ctx.user.id` / `current_user.id` / `os.user.id` +hit an unbound key for a signed-out visitor and FAULTED. A faulting visibility +predicate fails OPEN (`evaluateVisibility`), so the gated field or action rendered +for exactly the principal it was written to exclude, with nothing on screen to say +the gate had not bitten — silently, for every signed-out visitor. + +Because the defect was in the shared normaliser rather than at a mount site, it +reached EVERY mount site, including `AppContent` and the console's +`InternalFormRoute`, both of which have always called the normaliser correctly. +This is the same fault-open mechanism objectui#6515 fixed one level up, where +`RecordFormPage` hand-rolled a descriptor missing `id` and `isPlatformAdmin`. + +`null` rather than `undefined`, and rather than leaving the key absent, is settled +by precedent on this exact object rather than chosen here. objectui#5424 removed +`roles` from it because a present-and-always-`undefined` key "is the shape that +teaches the wrong thing" — the context answers rather than being plainly absent, +and the answer is silently wrong; `undefined` here would reproduce that defect one +key over. Leaving it absent IS the defect. `null` is a VALUE a CEL author can +compare against, so `ctx.user.id == '…'` resolves to a clean FALSE. Measured, not +assumed: at a real mount site with `authState.user = null` and a field gated on +`ctx.user.id == 'u_admin'`, the field is now filtered OUT of the schema handed to +`ObjectForm`, where before it was present. + +This also closes the last asymmetry between the two branches. Both now advertise +the same six keys, which is the symmetry objectui#5424 was closing when it removed +`roles` — and the shape pin now asserts the key sets are equal, so a future edit +that adds a key to one branch and forgets the other fails whichever branch it +forgets. + +NOT CHANGED, deliberately: fail-open on a predicate that DOES fault. That is +shipped permission-boundary policy (objectui#6443 / #6487 / #6445) and remains +exactly as it was — an unevaluable `visible` still renders. This change removes a +REASON to fault; it does not touch what happens once a predicate has. No accept set +is widened, no gate is relaxed and no fallback is added: the only behavioural +movement is that an id-gated surface which used to render for anonymous visitors +now hides from them. diff --git a/packages/app-shell/src/console/__tests__/AppContent.expressionUserShape.test.ts b/packages/app-shell/src/console/__tests__/AppContent.expressionUserShape.test.ts index 9263f1d7b6..45fe8662ec 100644 --- a/packages/app-shell/src/console/__tests__/AppContent.expressionUserShape.test.ts +++ b/packages/app-shell/src/console/__tests__/AppContent.expressionUserShape.test.ts @@ -113,7 +113,21 @@ describe('objectui#5424 — `buildExpressionUser` stops advertising the retired const anonymous = buildExpressionUser(null); expect('roles' in anonymous).toBe(false); + // objectui#6534 widened this from five keys to six by adding `id: null`. + // That is a TIGHTENING of the pin, not a weakening of it, and it was graded + // as part of the fix rather than a breach — triage, 2026-08-26, verbatim: + // + // "Updating `AppContent.expressionUserShape.test.ts`'s five-key pin is + // part of the fix, not a breach" + // + // The pin's job is unchanged and its strictness is unchanged: it still + // enumerates the WHOLE object with `toStrictEqual`, so an added key, a + // dropped key or a key written as explicit `undefined` all still fail here. + // Only the enumerated set moved, and it moved to CLOSE the last asymmetry + // between the two branches — which is the same symmetry objectui#5424 was + // closing when it removed `roles`. expect(anonymous).toStrictEqual({ + id: null, name: 'Anonymous', email: '', role: 'guest', @@ -122,3 +136,72 @@ describe('objectui#5424 — `buildExpressionUser` stops advertising the retired }); }); }); + +/** + * objectui#6534 — the anonymous branch ANSWERS `id` instead of omitting it. + * + * `id` was signed-in-only, so `'id' in buildExpressionUser(null)` was `false`. + * An absent key is not `false`: `ctx.user.id == '…'` FAULTS for a signed-out + * visitor, and a faulting visibility predicate fails OPEN + * (`evaluateVisibility`), so an id-gated field rendered for exactly the + * principal it was written to exclude — at EVERY mount site, including the two + * that have always called this normaliser correctly. + * + * ## Why `null` and not `undefined` + * + * Settled by precedent on this very object, not chosen here. objectui#5424 + * removed `roles` because present-and-always-`undefined` "is the shape that + * teaches the wrong thing" — the context answers rather than being plainly + * absent, and the answer is silently wrong. `null` is a VALUE: a CEL author can + * compare against it and the comparison resolves. `undefined` would reproduce + * exactly the defect objectui#5424 measured, one key over. + * + * ## Why `in` and not just a value assertion + * + * Same reason the `roles` pin above uses it: `{ id: undefined }` and `{}` both + * read as `undefined` at `user.id`, so only key PRESENCE distinguishes the + * three candidate shapes. The `in` check and the value check together are what + * separate `null` from both rejected alternatives — neither alone does. + * + * ## Fenced boundary + * + * This is NOT a change to fail-open. A predicate that faults still renders + * (objectui#6443 / #6487 / #6445 — deliberate, and untouched by this card). + * What changed is that this predicate no longer faults. + */ +describe('objectui#6534 — the anonymous branch answers `id` rather than omitting it', () => { + it('advertises `id` on the anonymous branch', () => { + // The pin. This is the assertion that was `false` before the fix, and it is + // the one that moves between all three candidate shapes. + expect('id' in buildExpressionUser(null)).toBe(true); + }); + + it('answers `null` there — a value a predicate can compare against', () => { + const anonymous = buildExpressionUser(null); + + expect(anonymous.id).toBeNull(); + // Explicitly NOT `undefined`: that is the shape objectui#5424 rejected on + // this object, and `toBeNull` alone would not catch it being reintroduced + // as `undefined` on a future edit — `toBeUndefined` would pass on `{}` too. + expect(anonymous.id).not.toBeUndefined(); + }); + + it('agrees with the signed-in branch on the key set — the asymmetry is closed', () => { + // objectui#5424 removed `roles` to make the two branches agree on one + // shape; `id` was the last key they still disagreed on. A future edit that + // adds a key to one branch and forgets the other fails HERE, whichever + // branch it forgets, without needing to know which key was added. + const signedIn = buildExpressionUser(PROTOCOL_17_USER); + const anonymous = buildExpressionUser(null); + + expect(Object.keys(anonymous).sort()).toEqual(Object.keys(signedIn).sort()); + }); + + it('still distinguishes anonymous from a signed-in user by VALUE, not by key set', () => { + // The converse guard: converging the key sets must not make the two + // branches indistinguishable. A gate excluding a signed-out visitor reads + // the value, and the values still differ. + expect(buildExpressionUser(null).id).toBeNull(); + expect(buildExpressionUser(PROTOCOL_17_USER).id).toBe('u_1'); + }); +}); diff --git a/packages/app-shell/src/providers/expressionUser.mountParity.test.tsx b/packages/app-shell/src/providers/expressionUser.mountParity.test.tsx index e48372dff0..f7457e08bb 100644 --- a/packages/app-shell/src/providers/expressionUser.mountParity.test.tsx +++ b/packages/app-shell/src/providers/expressionUser.mountParity.test.tsx @@ -58,9 +58,11 @@ * `toContain` — the fault fails OPEN, so the field is PRESENT; * - the same for the signed-IN `ctx.user.id` case, and for the signed-out * `isPlatformAdmin` case; - * - `RECORDS: a ctx.user.id gate STILL fails open for a signed-out visitor` - * stays GREEN both ways — it measures the normaliser's own anonymous - * branch, which this card does not touch; + * - `hides a ctx.user.id-gated field from a signed-out visitor` stays GREEN + * both ways — it measures the normaliser's own anonymous branch, which + * THIS card does not touch. (It was named `RECORDS: a ctx.user.id gate + * STILL fails open for a signed-out visitor` and asserted the opposite + * until objectui#6534 fixed that branch; see the case body.) * - `grants ... to a platform admin` stays GREEN — fail-open and a correct * `true` are indistinguishable at the call site, which is the whole reason * the excluded-user cases carry the pin. @@ -259,29 +261,41 @@ describe('objectui#6515 — the record form page publishes the normaliser’s `c expect(lastFields()).not.toContain('plan_canonical'); }); - it('RECORDS: a `ctx.user.id` gate STILL fails open for a signed-out visitor', async () => { - // Measurement, not endorsement — and NOT something this card changes. + it('hides a `ctx.user.id`-gated field from a signed-out visitor', async () => { + // objectui#6534 — this case is the inverted successor of objectui#6515's + // `RECORDS: a ctx.user.id gate STILL fails open for a signed-out visitor`. // - // `buildExpressionUser(null)` returns `{ name, email, role, - // isPlatformAdmin, positions }`. There is no `id` key on that branch, so - // `ctx.user.id == '…'` faults for a signed-out visitor and fails OPEN, at - // EVERY mount site — `AppContent` and `InternalFormRoute` included, both of - // which have always called the normaliser. That asymmetry is in the - // normaliser's own anonymous branch, not in this page, and closing it means - // changing the shape `AppContent.expressionUserShape.test.ts` pins, which - // is a decision of its own (`id: null`? `id: undefined`? — objectui#5424 - // rejected present-and-undefined as the shape that teaches the wrong - // thing). Filed separately rather than widened into this PR. + // That case recorded the defect as a passing fact: `'id' in + // buildExpressionUser(null)` was `false` and the excluded field was + // `toContain`-present. Both assertions are flipped below, and the flip is + // this card's reverse verification — the pin was GREEN on `origin/main` + // (measured: 13/13 passing) and went RED the instant `id: null` landed, + // which is what proves the normaliser's anonymous branch is the thing that + // moved. // - // Pinned so the follow-up has a red test to turn green, and so this stays - // a measured fact rather than an assumption. The signed-IN case above is - // the one this card fixes: there `id` is present and the gate bites. + // ⚠️ The SECOND assertion is the load-bearing one and it is not + // interchangeable with the first. `'id' in …` only proves the key exists; + // it says nothing about whether CEL can compare against `null` rather than + // faulting on it a second way. Only the rendered field list proves the gate + // actually BITES — that `ctx.user.id == 'u_admin'` resolves to a clean + // FALSE for an anonymous visitor and the field is filtered out. If `null` + // merely relocated the fault, the first assertion would still pass and this + // one would still find the field present. + // + // Fenced boundary (objectui#6443 / #6487 / #6445): fail-open on a predicate + // that DOES fault is deliberate and is untouched. This asserts that this + // predicate no longer faults, not that a faulting one now fails closed. authState.user = null; renderPage(); await waitFor(() => expect(formSchemas.length).toBeGreaterThan(0)); - expect('id' in buildExpressionUser(null)).toBe(false); - expect(lastFields()).toContain('self_note'); + expect('id' in buildExpressionUser(null)).toBe(true); + expect(buildExpressionUser(null).id).toBeNull(); + // Ungated fields are untouched — this narrowed exactly one gate, and the + // filter still lets everything else by. Without this line a change that + // dropped ALL fields would pass the assertion below. + expect(lastFields()).toContain('name'); + expect(lastFields()).not.toContain('self_note'); }); it('grants the `isPlatformAdmin` gate to a platform admin', async () => { diff --git a/packages/app-shell/src/providers/expressionUser.ts b/packages/app-shell/src/providers/expressionUser.ts index 805b2084b7..d23ec20f75 100644 --- a/packages/app-shell/src/providers/expressionUser.ts +++ b/packages/app-shell/src/providers/expressionUser.ts @@ -51,6 +51,25 @@ * two branches agree on one shape. Both branches carry `isPlatformAdmin` and * `positions` for the same reason: a predicate naming either must evaluate to * FALSE, not fault. + * + * `id` is on BOTH branches for that same reason, and it is the last place the + * two disagreed (objectui#6534). It used to be signed-in-only, so + * `'id' in buildExpressionUser(null)` was `false` and a gate naming + * `ctx.user.id` / `current_user.id` / `os.user.id` faulted for every signed-out + * visitor — measured at a real mount site in + * `expressionUser.mountParity.test.tsx`, where the excluded field was PRESENT + * in the schema handed to `ObjectForm`. Anonymous now answers `null`, which is + * a VALUE a CEL author can compare against, so the predicate resolves FALSE + * instead of faulting. + * + * ## What this module does NOT decide + * + * Fail-open on a predicate that DOES fault stays deliberate policy + * (objectui#6443 / #6487 / #6445): an unevaluable `visible` still renders, and + * that verdict is `evaluateVisibility`'s to make, not this normaliser's. Every + * shape rule above works the same way — it removes REASONS to fault, so fewer + * predicates ever reach that policy. None of them touches what the policy does + * once one has. */ export function buildExpressionUser(user: unknown): Record { const u = user as @@ -58,7 +77,23 @@ export function buildExpressionUser(user: unknown): Record { | null | undefined; if (!u) { - return { name: 'Anonymous', email: '', role: 'guest', isPlatformAdmin: false, positions: [] }; + return { + // `null`, not absent — objectui#6534. An ABSENT key is not `false`: it + // makes `ctx.user.id == '…'` FAULT, and a faulting visibility predicate + // fails OPEN (`evaluateVisibility`), so an id-gated field rendered for + // exactly the signed-out visitor it was written to exclude, silently, at + // EVERY mount site. `null` ANSWERS: the comparison is a clean FALSE and + // the gate bites. Not `undefined` — objectui#5424 measured on this same + // object that a present-and-always-`undefined` key "is the shape that + // teaches the wrong thing", which is why `roles` is absent rather than + // forwarded as `undefined`. + id: null, + name: 'Anonymous', + email: '', + role: 'guest', + isPlatformAdmin: false, + positions: [], + }; } return { id: u.id,