From 56d95f352e8208006a70daaf07685b8b3c45fd4e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 23:59:17 +0000 Subject: [PATCH 1/3] fix(scripts): anchor check-org-identifier on session PROVENANCE, not the receiver name (#9691) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detector was one line — `/\bsession\s*\??\.\s*tenantId\b/` — so it graded a receiver only when it was literally spelled `session`. Census over the 2057 scanned files: 111 `.tenantId` reads, ZERO of them spelled `session`, and 2 reached through a local bound from `….session`. The gate matched nothing in the corpus for its whole life while printing "no removed session.tenantId alias". Adds a second rule anchored on where the value came from: a local or a same-file function parameter filled from an expression ending in `.session` is a session, whatever it is called, and a `.tenantId` read off one is a finding. Propagation is transitive and scope-resolved, so the same name in another function is not a false red. The text rule is unchanged and still owns the literal spelling and the authoring-sample-inside-a-string case. Also prints the discovered binding population and fails at zero — this gate certified a corpus it could not read, so an empty population is a broken scan, not a clean repo. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja --- scripts/check-org-identifier.mjs | 539 ++++++++++++++++++++++++++++--- 1 file changed, 497 insertions(+), 42 deletions(-) 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); } From aaf99d508415cfc535bf58cbe80c65304f31f08a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 00:06:35 +0000 Subject: [PATCH 2/3] fix(service-storage,plugin-audit): read the caller org under the blessed name in the access-hook session fallback (#9691) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two live reads the widened check:org-identifier now sees. Both spell the receiver `s`, bound from `ctx?.session`, which is why the name-anchored gate scored zero on them for their whole life. - service-storage attachment kit: `tenantId: s.tenantId` had NO fallback, so the envelope forwarded to ISharingService.canEdit carried no org at all on the session-fallback path. Genuinely defective, not merely invisible. - plugin-audit comment kit: `s.tenantId ?? s.organizationId` — dead first arm, behaviour-neutral removal. The attachment kit's existing coverage of that path pinned the dead arm: it handed the hook a session spelling `tenantId`, a shape HookContextSchema strips and buildSession never emits, so it passed for exactly as long as the code was wrong. Replaced with the envelope a real transport builds, plus a mirror pin in both kits that a stray removed-alias key does not become the org. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja --- .../org-identifier-session-provenance.md | 24 +++++++++++++ .../src/comment-access-hooks.test.ts | 36 +++++++++++++++++++ .../plugin-audit/src/comment-access-hooks.ts | 8 ++++- .../src/attachment-access-hooks.test.ts | 34 ++++++++++++++++-- .../src/attachment-access-hooks.ts | 12 ++++++- 5 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 .changeset/org-identifier-session-provenance.md 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/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( From f7ea776e097903b384a4c0da5217eed6b142cd2d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 00:36:46 +0000 Subject: [PATCH 3/3] docs(plugin-audit): correct the audit-writers pin note that named check:org-identifier as blind to `sess` (#9691) The comment was written when the gate anchored on the literal receiver name. It now resolves the receiver's provenance, so the claim it makes is false as of the commit that introduced this line's own subject. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja --- packages/plugins/plugin-audit/src/audit-writers.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) 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' },