diff --git a/.changeset/org-identifier-session-provenance.md b/.changeset/org-identifier-session-provenance.md new file mode 100644 index 0000000000..e66af0a2d8 --- /dev/null +++ b/.changeset/org-identifier-session-provenance.md @@ -0,0 +1,24 @@ +--- +'@objectstack/service-storage': patch +'@objectstack/plugin-audit': patch +--- + +Attachment access hooks: read the caller's org under the blessed `organizationId` name + +`callerContext()` in the `sys_attachment` access kit built its fallback +execution envelope from `session.tenantId` — an alias removed from the +hook/action session surface in v11 (#3290). `HookContextSchema` strips a +`tenantId` key and the engine's `buildSession` only ever emits +`organizationId`, so on every call that reached the session fallback (no +execution context riding along) the envelope handed to +`ISharingService.canEdit` carried **no organization at all**. Parent-record +access for attachments was therefore evaluated without the caller's active +org on that path. It now reads `session.organizationId`, matching the +`sys_comment` kit, which already did. + +The `sys_comment` kit's own `callerContext()` had the same read as a dead +first arm (`s.tenantId ?? s.organizationId`); the arm is removed. That half +is behaviour-neutral — the fallback already carried the value. + +Both kits gain coverage of the session-fallback path in both directions: the +blessed name is read, and a stray removed-alias key does not become the org. diff --git a/packages/plugins/plugin-audit/src/audit-writers.test.ts b/packages/plugins/plugin-audit/src/audit-writers.test.ts index bc88c088e7..79decd9bbc 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.test.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.test.ts @@ -1657,8 +1657,13 @@ describe('audit writers — the writer reads the session key the engine emits (# // A session in the REMOVED dialect. The engine cannot produce one, so the // only way this shape reaches the writer is a caller that is itself broken // — and honouring it here would hide that. This pin goes red the day - // `sess.tenantId` is reintroduced as a fallback arm; `pnpm check:org-identifier` - // cannot see that reintroduction when the receiver is spelled `sess`. + // `sess.tenantId` is reintroduced as a fallback arm. It used to be the ONLY + // thing that would: `pnpm check:org-identifier` was anchored on the literal + // receiver name `session` and scored zero on `sess`. Since #9691 that gate + // resolves the receiver's PROVENANCE instead — a local filled from a + // `.session` expression is a session whatever it is called — so the + // reintroduction is caught in both places now. Keep this pin anyway: the + // gate cannot see a wrong VALUE, only a removed-alias read. await fire('afterInsert', { object: 'crm_lead', input: { id: 'lead-1' }, diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts index a62076fcff..05c78fa3a7 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.test.ts @@ -495,4 +495,40 @@ describe('#7141 — caller envelope forwarded to the sharing gate', () => { ).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 }); expect((canEdit.mock.calls[0]![2] as any).__writeScope).toBeUndefined(); }); + + // ── The session fallback, and the org name it reads (#9691) ─────────── + // + // The kit had no coverage of the no-execution-context path at all, so the + // dead `s.tenantId ?? s.organizationId` first arm was invisible in both + // directions: nothing proved the blessed name was read, and nothing would + // have noticed if the fallback had been dropped. Both directions are pinned + // here, on the session shape `ObjectQLEngine.buildSession` actually emits. + it('falls back to the session snapshot and reads the caller org under the BLESSED name (#9691)', async () => { + const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true); + const { beforeDelete } = install({ comments: [row], sharing: { canEdit } }); + await beforeDelete({ + object: 'sys_comment', + event: 'beforeDelete', + input: { id: 'c1' }, + session: { userId: 'u1', organizationId: 'org_1', positions: ['p1'] }, + api: apiFor(['crm_opportunity/opp1']), + }); + // `tenantId` on the way OUT is `ExecutionContext`'s driver-layer name for + // the same value — the separate axis #3290 deliberately left alone. + expect(canEdit.mock.calls[0]![2]).toEqual({ userId: 'u1', tenantId: 'org_1', positions: ['p1'] }); + }); + + it('does not resurrect the removed `session.tenantId` alias if one ever reaches a hook (#9691)', async () => { + const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true); + const { beforeDelete } = install({ comments: [row], sharing: { canEdit } }); + await beforeDelete({ + object: 'sys_comment', + event: 'beforeDelete', + input: { id: 'c1' }, + // A key `HookContextSchema` strips (#3290). It is not the caller's org. + session: { userId: 'u1', tenantId: 'stale_org', positions: ['p1'] } as any, + api: apiFor(['crm_opportunity/opp1']), + }); + expect((canEdit.mock.calls[0]![2] as any).tenantId).toBeUndefined(); + }); }); diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.ts index 2d219c7403..0c3807f9ef 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.ts @@ -252,7 +252,13 @@ function callerContext(ctx: any): ExecutionContext { return withoutOperationPrivateKeys(exec as Record); } const s = ctx?.session ?? {}; - return { userId: s.userId, tenantId: s.tenantId ?? s.organizationId, positions: s.positions }; + // [#9691] The `s.tenantId` arm was DEAD, not a fallback: `HookContextSchema` + // strips a `tenantId` key from the session (#3290) and the engine's + // `buildSession` only ever emits `organizationId`, so the first arm answered + // `undefined` on every call and the second one carried the value. Dropping it + // is byte-for-byte the same envelope; it is removed because a dead read of a + // removed alias is what an author copies out of a reference body. + return { userId: s.userId, tenantId: s.organizationId, positions: s.positions }; } /** Can the CALLER read `(object, recordId)`? A caller-scoped `findOne` through diff --git a/packages/services/service-storage/src/attachment-access-hooks.test.ts b/packages/services/service-storage/src/attachment-access-hooks.test.ts index ce8d3509ce..82bc990925 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.test.ts @@ -546,17 +546,45 @@ describe('#7145 — caller envelope forwarded to the sharing gate', () => { expect((canEdit.mock.calls[0]![2] as any).__writeScope).toBeUndefined(); }); - // ── The session fallback is unchanged ───────────────────────────────── - it('still falls back to the session snapshot when no execution context rides along', async () => { + // ── The session fallback, and the org name it reads (#9691) ─────────── + // + // ⚠️ This case used to hand the hook a session spelling `tenantId: 'org_1'` + // and assert the same key came back out. That is a session the engine cannot + // produce: `HookContextSchema` STRIPS a `tenantId` key (#3290, pinned in + // `packages/spec/src/data/hook.test.ts`) and `buildSession` only ever emits + // `organizationId`. So the fixture pinned the removed-alias arm itself — it + // passed for exactly as long as `callerContext` read the dead name, and could + // only have started failing if the code became right, which is what happened. + // Replaced rather than respelled: the fixture below is the envelope a real + // transport builds. + it('falls back to the session snapshot and reads the caller org under the BLESSED name (#9691)', async () => { const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true); const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } }); await beforeDelete({ object: 'sys_attachment', event: 'beforeDelete', input: { id: 'a1' }, - session: { userId: 'u1', tenantId: 'org_1', positions: ['p1'] }, + // Exactly what `ObjectQLEngine.buildSession` emits. + session: { userId: 'u1', organizationId: 'org_1', positions: ['p1'] }, api: apiFor([]), }); + // `tenantId` on the way OUT is `ExecutionContext`'s driver-layer name for + // the same value — the separate axis #3290 deliberately left alone. expect(canEdit.mock.calls[0]![2]).toEqual({ userId: 'u1', tenantId: 'org_1', positions: ['p1'] }); }); + + it('does not resurrect the removed `session.tenantId` alias if one ever reaches a hook (#9691)', async () => { + const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true); + const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } }); + await beforeDelete({ + object: 'sys_attachment', + event: 'beforeDelete', + input: { id: 'a1' }, + // A key the schema strips. Reaching for it is how this seam handed the + // sharing service an envelope with no org at all for several majors. + session: { userId: 'u1', tenantId: 'stale_org', positions: ['p1'] } as any, + api: apiFor([]), + }); + expect((canEdit.mock.calls[0]![2] as any).tenantId).toBeUndefined(); + }); }); diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 8e57f0d885..9c2f3f99d0 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.ts @@ -131,7 +131,17 @@ function callerContext(ctx: any): ExecutionContext { return withoutOperationPrivateKeys(exec as Record); } const s = ctx?.session ?? {}; - return { userId: s.userId, tenantId: s.tenantId, positions: s.positions }; + // [#9691] `s.organizationId`, NOT `s.tenantId`. The hook session's org key is + // `organizationId` (engine `buildSession`; `HookContextSchema` STRIPS a + // `tenantId` key outright, pinned in `packages/spec/src/data/hook.test.ts`), + // so the removed alias read here answered `undefined` on every call and this + // fallback handed `ISharingService.canEdit` an envelope with no org at all. + // The target field keeps its `tenantId` spelling: that is `ExecutionContext`'s + // driver-layer name for the same value, a separate axis #3290 deliberately + // left alone. The comment kit's `callerContext` already read the blessed name + // (`s.tenantId ?? s.organizationId`), so this is the #7145 parity that kit's + // card asked for, completed. + return { userId: s.userId, tenantId: s.organizationId, positions: s.positions }; } export function installAttachmentAccessHooks( diff --git a/scripts/check-org-identifier.mjs b/scripts/check-org-identifier.mjs index 115e3c62c5..22a2f5d4fa 100644 --- a/scripts/check-org-identifier.mjs +++ b/scripts/check-org-identifier.mjs @@ -9,8 +9,9 @@ // reads `ctx.user.organizationId` / `ctx.session.organizationId`, matching the // `organization_id` column and `current_user.organizationId` in RLS. The old // `ctx.session.tenantId` was a deprecated alias; #3290 REMOVED it from the -// hook/action `ctx.session` surface entirely (v11 major), so any `session.tenantId` -// read in an authoring body now resolves to `undefined` and is simply a bug. +// hook/action `ctx.session` surface entirely (v11 major), so any session-borne +// `tenantId` read in an authoring body now resolves to `undefined` and is +// simply a bug. // // This is a hard-fail guard, not a ratchet: the scanned surfaces carry ZERO // occurrences today, so any match is a NEW one and fails. It is deliberately @@ -21,24 +22,28 @@ // the ObjectQL audit-stamp plugin were migrated to `organizationId`), so // packages/ is now held to the same bar as reference apps -- an author or AI // copying a package example body will not find the removed name. -// • The generic DRIVER-LAYER tenancy knob is untouched and never matched: the -// pattern anchors on the `session` receiver, so `execCtx.tenantId` / -// `opts.tenantId` / `DriverOptions.tenantId` (a configurable isolation -// column, legitimately an *environment* id in database-per-tenant kernels) -// do not trip it. For the rare genuine driver-layer `session.tenantId`, add -// an `os-allow-tenant-id` comment on the same line. +// • The generic DRIVER-LAYER tenancy knob is untouched and never matched: a +// finding requires a receiver this gate has SHOWN to be a session, so +// `execCtx.tenantId` / `opts.tenantId` / `DriverOptions.tenantId` (a +// configurable isolation column, legitimately an *environment* id in +// database-per-tenant kernels) do not trip it. For the rare genuine +// driver-layer `session.tenantId`, add an `os-allow-tenant-id` comment on +// the same line. // • Test/spec files are EXCLUDED: they legitimately reference the removed // token to assert its ABSENCE (`expect(session.tenantId).toBeUndefined()`), // and are not reference bodies an author copies a hook from. // • Comments are SKIPPED -- a migration note that NAMES the removed alias to // explain its removal is documentation, not an executable read. Which spans // ARE comments is decided by the ONE shared string-, template- and regex- -// aware scanner (`scripts/js-comment-mask.mjs`, #9367), not by this gate. +// aware scanner (`scripts/js-comment-mask.mjs`, #9367) for the TEXT rule, +// and by the parser itself for the BINDING rule (comments are trivia, so a +// syntax tree cannot mistake one for code). // • skills/ and content/docs/ are EXCLUDED: prose there may still name the // removed alias when documenting the migration. // // node scripts/check-org-identifier.mjs // node scripts/check-org-identifier.mjs --self-test +// node scripts/check-org-identifier.mjs --list-bindings (the discovered population) // // Scope: tracked sources under examples/, apps/, and packages/ (git ls-files). // @@ -62,34 +67,135 @@ // as live code: the gate would have MANUFACTURED a finding out of prose. Both // directions are gone with one shared mask, and both are pinned in `--self-test`. // +// ## Why a RECEIVER NAME is not the anchor any more (#9691) +// +// Until #9691 the whole detector was one line: +// +// const PATTERN = /\bsession\s*\??\.\s*tenantId\b/; // receiver must be spelled `session` +// +// and the header above claimed "the scanned surfaces carry ZERO occurrences +// today". That was never an observation about the tree. A hook body binds its +// session to a local before reading it, and the shipped offender that #9516 +// removed spelled that local `sess`: +// +// const sess: any = (ctx as any).session ?? {}; +// const tenantId = recordOrgId ?? sess.tenantId; // gate: no match +// +// CENSUS on 11b779e0f, over the 2057 scanned files (`--list-bindings` reproduces +// the binding half): +// +// .tenantId property reads, all receivers ......................... 111 +// ... whose receiver is spelled `session` (what the old PATTERN saw) 0 +// ... reached through a local bound from `….session` ................. 2 +// locals bound from an expression ending in `.session` ............... 8 +// ... distinct SPELLINGS of that local .... 4 (session x3, sess x2, s x2, adminSession) +// destructuring escapes (`const { tenantId } = ….session`) ........... 0 +// +// So the old pattern matched NOTHING in the corpus, and the two reads it could +// not see are BOTH live on `main` -- one of them defective (see below). A green +// run was an artifact of the receiver spelling, on every PR, for the whole life +// of the guard. +// +// ⛔ The obvious repair -- widen to a vocabulary `session|sess|s|ctx|…` -- is +// the one this gate must NOT take, and the census says why in one row: the most +// common alias after `session` is the single letter `s`. A vocabulary +// containing `s` fires on every unrelated one-letter receiver in the tree +// (`s.tenantId` where `s` is a driver option bag is exactly the axis the +// bullet above promises never to match), and a vocabulary WITHOUT it misses +// two of the eight real bindings. Either way the next alias is invisible again, +// which is the defect, not a symptom of it. +// +// ⚠️ And the cheapest way to satisfy the OLD gate was actively harmful, the +// #9657 shape: a red `session.tenantId` was silenced by binding the session to +// a differently-named local and reading that. The code stays dead, the gate +// goes quiet, and the escape is permanent. A name-anchored gate rewards the +// rename it cannot see. +// +// ## What the anchor is instead +// +// A structural one: a receiver is a session because this file SHOWED it being +// filled from a `.session` expression, not because of how it is spelled. Two +// rules run over every scanned file and their findings are merged by line: +// +// TEXT rule -- `maskComments` + PATTERN, unchanged. It still owns the +// literal `session.tenantId` / `session?.tenantId` / +// `this.session . tenantId` shapes AND the authoring-sample +// case (a removed alias taught inside a string or template is +// a finding: an author copies what they read). +// BINDING rule -- a syntax tree. A local (`const/let/var`) or a same-file +// function PARAMETER whose value came from an expression +// ending in `.session` is session-valued; a `.tenantId` read +// off such a receiver is a finding. Propagation is transitive +// (`const a = ctx.session; const b = a;`) and follows +// same-file call sites one function at a time, so the +// 2-hop `stampData(…, hookCtx.session, …) -> applyToRecord` +// shape in `packages/objectql/src/plugin.ts` is inside the +// population rather than beyond it. +// +// ⛔ A TYPE-based anchor was priced and is NOT available here. All 8 binding +// sites in the census reach their session through an `any`: 7 are `(ctx: any)` +// hook handlers or `(ctx as any).session`, and the 8th +// (`record-change-trigger.ts`) annotates a hand-written inline literal type, +// not `HookContext['session']`. A checker-based rule would resolve every one of +// them to `any` and grade nothing -- it would be a THIRD blind gate, not a +// stronger one. The provenance anchor works precisely because it does not need +// the declared type. +// +// ## Known limits, stated rather than discovered later (#9747) +// +// • Flow ACROSS files is not followed. A helper in another module that takes +// a session and reads `.tenantId` is invisible to both rules. Measured: 7 +// call sites in the corpus pass a `.session` expression as an argument, all +// 7 same-file, 0 of them reading `.tenantId` today. +// • A session reaching a receiver through a container (array element, map +// value, spread into a new object) is not followed. +// • The BINDING rule reads code, not strings: an aliased read taught inside a +// template literal is seen by neither rule (the TEXT rule catches only the +// literal spelling there). +// These are the shapes to widen to if one ever goes live. They are named here +// so a future green is read as "clean where this gate can see", never as proof. +// +// ## The population invariant -- zero is a broken scan, not a clean repo +// +// The BINDING rule is only as good as the population it discovers, and this +// gate spent its whole life certifying a corpus it could not read. So the +// discovered count is PRINTED on every run and a count of ZERO is a FAILURE: +// hook and action bodies bind their session before reading it, so a corpus +// with no session binding at all means the resolver stopped working, not that +// the tree got clean. (The germ is `check-engine-double-contract`'s DISCOVERED +// invariant -- "Zero is not a clean repo, it is a broken scan" -- generalised +// here to the one population this gate depends on.) Measured stability: 8 +// bindings today, and the two files that carry the only `.tenantId` reads have +// not changed shape since #7141 / #7145 introduced them. +// // ## LIVE or LATENT, measured on 51a46a440's successor (af2a989be) // // Corpus: 2051 author-facing source files. Projections (this gate's old strip // vs `maskComments`) disagree on the text of **271** files, but the gate's // VERDICT changes on **0** -- every one of the 10 corpus lines that names // `session.tenantId` today is inside a comment, and old and new agree on all -// ten. So the defect is LATENT here, and structurally so: this is a hard-fail -// ZERO-occurrence guard, so a corpus that contains the hazard is a corpus in -// which the gate is already red. The intersection the card asks for can only -// ever be empty while the gate is green; the measurement that means something -// is the NEAR MISS, and that half is everywhere -- **665 lines across 181 -// files** carry a doubled slash inside a string, template or regex literal. -// The day one of them also carries the removed read, the old gate goes quiet. +// ten. So the #9444 defect is LATENT here; the #9691 defect above was LIVE. +// The measurement that means something for the comment half is the NEAR MISS: +// **665 lines across 181 files** carry a doubled slash inside a string, +// template or regex literal. The day one of them also carries the removed read, +// the old gate goes quiet. // // ## Why `maskComments` and not `stripComments` // -// This gate reports a FILE and a LINE, so it takes the blanking projection: the -// masked text stays byte-for-byte aligned with the source and line `i` is still -// line `i`. Cost measured over the same 2051 files (best of 3, scan + match -// only): old per-line strip 195ms, `maskComments` 2088ms, `stripComments` +// The TEXT rule reports a FILE and a LINE, so it takes the blanking projection: +// the masked text stays byte-for-byte aligned with the source and line `i` is +// still line `i`. Cost measured over the same 2051 files (best of 3, scan + +// match only): old per-line strip 195ms, `maskComments` 2088ms, `stripComments` // 1275ms. #9367 measured a 51x cliff (6.4s -> 5m27s) when a LAZY `[\s\S]*?` // matcher was dragged across the whitespace blanking leaves behind; this gate's // matcher is a short anchored pattern run per line, so it is not exposed to -// that, and ~2s is the scanner's own linear cost over 29MB. +// that. The BINDING rule parses only the files that contain the word `session` +// at all -- 254 of 2057, +1.2s -- so the whole gate stays around 3s. import { execFileSync } from 'node:child_process'; import { readFileSync } from 'node:fs'; import { join } from 'node:path'; +import ts from 'typescript'; import { maskComments } from './js-comment-mask.mjs'; const ROOTS = ['examples', 'apps', 'packages']; @@ -99,13 +205,235 @@ const EXCLUDED = /(^|\/)(node_modules|dist|build|\.next|\.turbo)\//; const TEST_FILE = /(\.(test|spec)\.[cm]?[jt]sx?$)|((^|\/)__tests__\/)/; // `ctx.session.tenantId`, `session?.tenantId`, `this.session . tenantId`, … -- -// the `session` receiver immediately before `.tenantId`. Anchored on the -// `session` word so `execCtx.tenantId` / `opts.tenantId` never match. +// the literal `session` receiver immediately before `.tenantId`. Anchored on the +// `session` word so `execCtx.tenantId` / `opts.tenantId` never match. This is +// the TEXT half only; a receiver under any OTHER name is the BINDING rule's job. const PATTERN = /\bsession\s*\??\.\s*tenantId\b/; const ALLOW_MARKER = 'os-allow-tenant-id'; +const SESSION_PROP = 'session'; +const TENANT_PROP = 'tenantId'; +/** Transitive propagation depth. The deepest real chain in the corpus is 2 + * hops (`stampData` -> `applyToRecord`); the cap only bounds pathological input. */ +const FIXPOINT_ROUNDS = 6; + +// ── the binding rule ────────────────────────────────────────────────────── + +/** Peel the wrappers that carry a value through unchanged. */ +function unwrap(node) { + let e = node; + while ( + e && + (ts.isParenthesizedExpression(e) || + ts.isAsExpression(e) || + ts.isTypeAssertionExpression?.(e) || + e.kind === ts.SyntaxKind.NonNullExpression || + e.kind === ts.SyntaxKind.SatisfiesExpression) + ) { + e = e.expression; + } + return e; +} + +/** The property being read, for `a.b` and `a['b']` alike; undefined otherwise. */ +function readProperty(node) { + if (ts.isPropertyAccessExpression(node)) return node.name.text; + if (ts.isElementAccessExpression(node) && ts.isStringLiteralLike(node.argumentExpression)) { + return node.argumentExpression.text; + } + return undefined; +} + +/** + * Does this expression EVALUATE to a session? + * + * `isSessionIdent(identifierNode)` answers the same question for a bare name, + * which is how the fixpoint feeds itself. Note what is deliberately NOT here: + * the receiver's spelling. `ctx?.session ?? {}`, `(ctx as any).session`, + * `ctx['session']` and `a` (where `a` was already shown to be one) all qualify; + * `opts`, `execCtx` and every other name qualify only by provenance. + */ +function isSessionValued(expr, isSessionIdent) { + const e = unwrap(expr); + if (!e) return false; + if (ts.isBinaryExpression(e)) { + const op = e.operatorToken.kind; + // `x.session ?? {}` / `x.session || {}` -- the fallback is the empty-object + // guard every one of these bindings writes; either arm being a session is + // enough, because the read that follows is the read either way. + if (op === ts.SyntaxKind.QuestionQuestionToken || op === ts.SyntaxKind.BarBarToken) { + return isSessionValued(e.left, isSessionIdent) || isSessionValued(e.right, isSessionIdent); + } + return false; + } + if (ts.isConditionalExpression(e)) { + return isSessionValued(e.whenTrue, isSessionIdent) || isSessionValued(e.whenFalse, isSessionIdent); + } + if (ts.isIdentifier(e)) return isSessionIdent(e); + return readProperty(e) === SESSION_PROP; +} + +/** The node a declaration's name is visible inside -- block, function or file. */ +function scopeOf(node) { + let p = node.parent; + while (p) { + if ( + ts.isBlock(p) || + ts.isSourceFile(p) || + ts.isModuleBlock(p) || + ts.isCaseBlock(p) || + ts.isFunctionLike(p) + ) { + return p; + } + p = p.parent; + } + return node.getSourceFile(); +} + +function contains(scope, node) { + return node.pos >= scope.pos && node.end <= scope.end; +} + +/** + * Every session-valued binding in one parsed file, plus every declaration that + * SHADOWS one of those names, so a read resolves to the nearest declaration + * rather than to any same-named binding anywhere in the file. + * + * ⛔ Per-FILE name matching (what #9691's sketch proposed) is the version of + * this that manufactures findings: a file that binds `const s = ctx.session` in + * one function and takes an unrelated `s` in another would report the second. + * Resolving to the innermost enclosing declaration costs one ancestor walk and + * removes the whole class -- pinned in `--self-test`. + */ +function collectBindings(sf) { + const sessionBindings = []; // { name, scope, decl } + const allDeclarations = []; // { name, scope } -- session or not + const localFunctions = new Map(); // name -> [functionLikeNode] + + const declare = (name, node) => allDeclarations.push({ name, scope: scopeOf(node) }); + + const walk = (node) => { + if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name)) { + declare(node.name.text, node); + const init = node.initializer && unwrap(node.initializer); + if (init && (ts.isFunctionExpression(init) || ts.isArrowFunction(init))) { + const list = localFunctions.get(node.name.text) ?? []; + list.push(init); + localFunctions.set(node.name.text, list); + } + } else if (ts.isParameter(node) && ts.isIdentifier(node.name)) { + declare(node.name.text, node); + } else if (ts.isFunctionDeclaration(node) && node.name) { + const list = localFunctions.get(node.name.text) ?? []; + list.push(node); + localFunctions.set(node.name.text, list); + } + ts.forEachChild(node, walk); + }; + walk(sf); + + const isSessionIdent = (ident) => { + let best = null; + for (const b of sessionBindings) { + if (b.name !== ident.text || !contains(b.scope, ident)) continue; + if (!best || b.scope.pos > best.scope.pos) best = b; + } + if (!best) return false; + // A nearer declaration of the same name shadows the session binding. + for (const d of allDeclarations) { + if (d.name !== ident.text || !contains(d.scope, ident)) continue; + if (d.scope.pos > best.scope.pos) return false; + } + return true; + }; + + const known = new Set(); + const remember = (name, scope, decl) => { + const key = `${name}@${decl.pos}`; + if (known.has(key)) return false; + known.add(key); + sessionBindings.push({ name, scope, decl }); + return true; + }; + + // Fixpoint: a binding can be session-valued only because ANOTHER one already + // is, and a call site can only be read after its callee's params are known. + for (let round = 0; round < FIXPOINT_ROUNDS; round++) { + let grew = false; + const visit = (node) => { + if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name) && node.initializer) { + if (isSessionValued(node.initializer, isSessionIdent)) { + grew = remember(node.name.text, scopeOf(node), node) || grew; + } + } + // Same-file call: `helper(ctx.session)` makes `helper`'s parameter a + // session for the whole of its body. Only a name declared as a function + // EXACTLY ONCE in this file is followed -- an overloaded or reassigned + // name is not something a syntax tree can resolve honestly. + if (ts.isCallExpression(node) && ts.isIdentifier(node.expression)) { + const fns = localFunctions.get(node.expression.text); + if (fns && fns.length === 1) { + const fn = fns[0]; + node.arguments.forEach((arg, i) => { + const param = fn.parameters[i]; + if (!param || !ts.isIdentifier(param.name)) return; + if (isSessionValued(arg, isSessionIdent)) { + grew = remember(param.name.text, fn, param) || grew; + } + }); + } + } + ts.forEachChild(node, visit); + }; + visit(sf); + if (!grew) break; + } + + return { sessionBindings, isSessionIdent }; +} + /** - * Every removed-alias read in one source, as `{ file, line, text }`. + * `.tenantId` reads in one parsed file. + * + * The receiver must be a bare identifier this file showed being filled from a + * `.session` expression. A property-access receiver (`ctx.session.tenantId`) + * is the TEXT rule's job and is skipped here so the two rules do not both + * claim the same line for different reasons. + */ +function collectBoundReads(sf, isSessionIdent) { + const hits = []; + const visit = (node) => { + if (readProperty(node) === TENANT_PROP) { + const recv = unwrap(node.expression); + if (recv && ts.isIdentifier(recv) && isSessionIdent(recv)) { + hits.push({ + line: sf.getLineAndCharacterOfPosition(node.getStart(sf)).line + 1, + via: recv.text, + }); + } + } + ts.forEachChild(node, visit); + }; + visit(sf); + return hits; +} + +function parse(text, file) { + // `scriptKind` left to the parser so `.tsx` / `.jsx` / `.mjs` are inferred + // from the name. The parser is error-tolerant: a file it cannot fully parse + // still yields the nodes around the failure rather than throwing. + return ts.createSourceFile(file, text, ts.ScriptTarget.Latest, /* setParentNodes */ true); +} + +// ── the gate ────────────────────────────────────────────────────────────── + +/** + * Every removed-alias read in one source, as `{ file, line, text, via }`. + * + * `via` names the receiver the BINDING rule resolved, or `session` for a TEXT + * hit, so the message can tell the author WHY this line is a finding when the + * word `session` is nowhere on it. * * The comment/code split comes from `maskComments`, once per file, and the * masked text is index-aligned with the raw text, so a finding still quotes the @@ -118,17 +446,48 @@ const ALLOW_MARKER = 'os-allow-tenant-id'; */ export function findOffenders(text, file) { const lines = text.split('\n'); + const byLine = new Map(); + const code = maskComments(text).split('\n'); - const offenders = []; for (let i = 0; i < lines.length; i++) { - const line = lines[i]; - if (line.includes(ALLOW_MARKER)) continue; if (!PATTERN.test(code[i] ?? '')) continue; - offenders.push({ file, line: i + 1, text: line.trim() }); + byLine.set(i + 1, { file, line: i + 1, text: lines[i].trim(), via: SESSION_PROP }); } + + let bindings = 0; + if (text.includes(SESSION_PROP)) { + const sf = parse(text, file); + const { sessionBindings, isSessionIdent } = collectBindings(sf); + bindings = sessionBindings.length; + for (const hit of collectBoundReads(sf, isSessionIdent)) { + if (byLine.has(hit.line)) continue; + byLine.set(hit.line, { + file, + line: hit.line, + text: (lines[hit.line - 1] ?? '').trim(), + via: hit.via, + }); + } + } + + const offenders = [...byLine.values()] + .filter((o) => !(lines[o.line - 1] ?? '').includes(ALLOW_MARKER)) + .sort((a, b) => a.line - b.line); + offenders.bindings = bindings; return offenders; } +/** The discovered session-binding population of one source (population invariant). */ +export function countSessionBindings(text, file) { + if (!text.includes(SESSION_PROP)) return []; + const sf = parse(text, file); + return collectBindings(sf).sessionBindings.map((b) => ({ + file, + line: sf.getLineAndCharacterOfPosition(b.decl.getStart(sf)).line + 1, + name: b.name, + })); +} + /** * The shapes, not the corpus. * @@ -136,7 +495,14 @@ export function findOffenders(text, file) { * and for a zero-occurrence hard-fail guard it can prove nothing else -- so * these cases ARE this gate's contract. `BLIND` marks a case the pre-#9444 strip * got wrong by MISSING a real read; `FABRICATE` marks one it got wrong by - * inventing a finding out of prose. + * inventing a finding out of prose; `RENAMED` marks one the pre-#9691 + * name-anchored PATTERN could not see at all. + * + * ⛔ The `RENAMED` cases are deliberately spelled with receivers no vocabulary + * would ever contain (`hookState`, `zzz`, an anonymous parameter). #9750's dev + * measured a harness that passed `284/284` with the guarantee absent; a case + * that a widened alias list could satisfy would be that harness. These can be + * satisfied only by resolving where the value came from. */ function selfTest() { const BT = String.fromCharCode(96); // backtick, kept out of the literals below @@ -170,6 +536,36 @@ function selfTest() { 'a docblock does not swallow the code line under it'], ['const sample = ' + BT + 'ctx.session.tenantId' + BT + ';', 1, 'a string is not a comment: an authoring sample teaching the removed alias is a finding'], + + // ── #9691: the receiver name is not the anchor ────────────────────── + ['function h(ctx) {\n const sess = (ctx).session ?? {};\n return recordOrgId ?? sess.tenantId;\n}', 1, + 'RENAMED: the shipped pre-#9516 shape -- `sess`, which the old PATTERN scored zero'], + ['function h(ctx) {\n const hookState = ctx.session;\n return hookState.tenantId;\n}', 1, + 'RENAMED: a receiver no alias vocabulary would ever list'], + ['function h(ctx) {\n const a = ctx.session;\n const zzz = a;\n return zzz.tenantId;\n}', 1, + 'RENAMED: transitive -- the session is two rebinds away from the read'], + ['function h(ctx) {\n const sess =\n (ctx).session ?? {};\n return sess.tenantId;\n}', 1, + 'RENAMED: the binding spans two lines -- a single-line scan returns a false zero (#9681)'], + ["function h(ctx) {\n const s = ctx['session'];\n return s['tenantId'];\n}", 1, + 'RENAMED: element access on both hops'], + ['function pick(anything) {\n return anything.tenantId;\n}\nfunction h(ctx) {\n return pick(ctx.session);\n}', 1, + 'RENAMED: a same-file helper PARAMETER fed a session -- the receiver has no session-ish name at all'], + ['function h(ctx) {\n const s = ctx.session;\n return s.tenantId; // os-allow-tenant-id: driver isolation column\n}', 0, + 'the waiver marker exempts an ALIASED read too'], + + // ── #9691: and it must not manufacture findings ───────────────────── + ['function h(opts) {\n const s = opts.driverOptions;\n return s.tenantId;\n}', 0, + 'NO FALSE RED: a one-letter receiver that is NOT a session -- the vocabulary answer would fire here'], + ['function a(ctx) {\n const s = ctx.session;\n return s.organizationId;\n}\nfunction b(opts) {\n const s = opts;\n return s.tenantId;\n}', 0, + 'NO FALSE RED: same name, different scope -- the read resolves to the nearer declaration'], + ['function h(ctx) {\n const s = ctx.session;\n return s.organizationId;\n}', 0, + 'the blessed name on a session receiver is the whole point and is never a finding'], + ['function pick(o) {\n return o.tenantId;\n}\nfunction h(opts) {\n return pick(opts);\n}', 0, + 'NO FALSE RED: the same helper fed DRIVER options does not become a session'], + ['function h(ctx) {\n const sess = ctx.session;\n // sess.tenantId was removed in v11 (#3290)\n return sess.organizationId;\n}', 0, + 'a comment naming an ALIASED read is documentation -- the tree never sees it'], + ['function h(ctx) {\n const execCtx = ctx.input.options.context;\n return execCtx.tenantId;\n}', 0, + 'NO FALSE RED: the driver-layer envelope is not reached through `.session`'], ]; let failed = 0; @@ -180,25 +576,33 @@ function selfTest() { failed++; } } + + // The population invariant is itself a contract: a resolver that discovers + // nothing must not be able to pass this harness. + const populated = countSessionBindings( + 'function h(ctx) {\n const sess = ctx.session ?? {};\n return sess.organizationId;\n}', + 'self-test.ts', + ); + if (populated.length !== 1) { + console.error( + ` ✗ self-test "the binding population is discovered at all": expected 1 binding, got ${populated.length}`, + ); + failed++; + } + if (failed) { console.error(`\n✗ check-org-identifier self-test failed (${failed} case(s)).`); process.exit(1); } - console.log(`✓ check-org-identifier self-test: ${cases.length} cases pass.`); + console.log(`✓ check-org-identifier self-test: ${cases.length + 1} cases pass.`); } -function main() { - if (process.argv.includes('--self-test')) return selfTest(); - - const root = execFileSync('git', ['rev-parse', '--show-toplevel'], { - encoding: 'utf8', - }).trim(); - +function sourceFiles(root) { // Newline-delimited on purpose (not `-z`): tracked paths under these roots // never contain a newline, and avoiding the NUL delimiter keeps this very // script free of any raw NUL byte (which would make it invisible to grep -- the // exact #3127 failure mode this repo already guards with check:nul-bytes). - const files = execFileSync('git', ['ls-files', '--', ...ROOTS], { + return execFileSync('git', ['ls-files', '--', ...ROOTS], { cwd: root, encoding: 'utf8', maxBuffer: 64 * 1024 * 1024, @@ -208,15 +612,60 @@ function main() { .filter((f) => EXTENSIONS.some((ext) => f.endsWith(ext))) .filter((f) => !EXCLUDED.test(f)) .filter((f) => !TEST_FILE.test(f)); +} + +function repoRoot() { + return execFileSync('git', ['rev-parse', '--show-toplevel'], { encoding: 'utf8' }).trim(); +} + +function main() { + if (process.argv.includes('--self-test')) return selfTest(); + + const root = repoRoot(); + const files = sourceFiles(root); + + if (process.argv.includes('--list-bindings')) { + const all = []; + for (const file of files) { + all.push(...countSessionBindings(readFileSync(join(root, file), 'utf8'), file)); + } + const spellings = new Map(); + for (const b of all) spellings.set(b.name, (spellings.get(b.name) ?? 0) + 1); + for (const b of all) console.log(` ${b.file}:${b.line} ${b.name}`); + console.log( + `\n${all.length} session binding(s), ${spellings.size} distinct spelling(s): ` + + [...spellings].sort((a, b) => b[1] - a[1]).map(([n, c]) => `${n} x${c}`).join(', '), + ); + process.exit(0); + } const offenders = []; + let bindings = 0; for (const file of files) { - offenders.push(...findOffenders(readFileSync(join(root, file), 'utf8'), file)); + const found = findOffenders(readFileSync(join(root, file), 'utf8'), file); + bindings += found.bindings ?? 0; + offenders.push(...found); + } + + // ⛔ Zero is not a clean repo, it is a broken scan. The BINDING rule certifies + // nothing if it discovered no session to anchor on, and this gate spent its + // whole life printing OK over a population it could not read (#9691). + if (bindings === 0) { + console.error( + 'check-org-identifier: the session-binding resolver discovered ZERO session-valued\n' + + 'receivers in the scan roots. Hook and action bodies bind their session before\n' + + 'reading it, so zero means this gate stopped being able to read the tree -- NOT\n' + + 'that the tree is clean. Run `node scripts/check-org-identifier.mjs --list-bindings`\n' + + 'and `--self-test`; a green result here would be the exact silent under-reporting\n' + + 'the guard exists to prevent.', + ); + process.exit(1); } if (offenders.length === 0) { console.log( - `check-org-identifier: OK (${files.length} author-facing source file(s), no removed session.tenantId alias).`, + `check-org-identifier: OK (${files.length} author-facing source file(s), ` + + `${bindings} session binding(s) resolved, no removed session.tenantId alias).`, ); process.exit(0); } @@ -226,7 +675,8 @@ function main() { `check-org-identifier: ${offenders.length} removed \`session.tenantId\` ${plural} in author-facing code\n`, ); for (const o of offenders) { - console.error(` • ${o.file}:${o.line} ${o.text}`); + const how = o.via === SESSION_PROP ? '' : ` [receiver \`${o.via}\` was bound from a \`.session\` expression]`; + console.error(` • ${o.file}:${o.line} ${o.text}${how}`); } console.error(` \`session.tenantId\` was REMOVED from the hook/action ctx.session surface (#3290); @@ -237,7 +687,12 @@ org under the blessed name instead: It matches the \`organization_id\` column and \`current_user.organizationId\` in RLS. For a genuine driver-layer use (a configurable isolation column, not the -caller's org), add an \`${ALLOW_MARKER}\` comment on the line.`); +caller's org), add an \`${ALLOW_MARKER}\` comment on the line. + +⛔ Renaming the receiver is NOT a fix. This gate resolves where the value came +from, not what it is spelled, so \`const s = ctx.session; s.tenantId\` is the +same finding under a different name -- and before #9691 that rename was the +cheapest way to silence it while leaving the dead read in place.`); process.exit(1); }