Skip to content

Commit bbd86ed

Browse files
os-steveclaude
andauthored
fix(scripts): anchor check-org-identifier on session provenance, not the receiver name (#9810)
* fix(scripts): anchor check-org-identifier on session PROVENANCE, not the receiver name (#9691) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja * fix(service-storage,plugin-audit): read the caller org under the blessed name in the access-hook session fallback (#9691) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 526a436 commit bbd86ed

7 files changed

Lines changed: 613 additions & 49 deletions

File tree

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
---
2+
'@objectstack/service-storage': patch
3+
'@objectstack/plugin-audit': patch
4+
---
5+
6+
Attachment access hooks: read the caller's org under the blessed `organizationId` name
7+
8+
`callerContext()` in the `sys_attachment` access kit built its fallback
9+
execution envelope from `session.tenantId` — an alias removed from the
10+
hook/action session surface in v11 (#3290). `HookContextSchema` strips a
11+
`tenantId` key and the engine's `buildSession` only ever emits
12+
`organizationId`, so on every call that reached the session fallback (no
13+
execution context riding along) the envelope handed to
14+
`ISharingService.canEdit` carried **no organization at all**. Parent-record
15+
access for attachments was therefore evaluated without the caller's active
16+
org on that path. It now reads `session.organizationId`, matching the
17+
`sys_comment` kit, which already did.
18+
19+
The `sys_comment` kit's own `callerContext()` had the same read as a dead
20+
first arm (`s.tenantId ?? s.organizationId`); the arm is removed. That half
21+
is behaviour-neutral — the fallback already carried the value.
22+
23+
Both kits gain coverage of the session-fallback path in both directions: the
24+
blessed name is read, and a stray removed-alias key does not become the org.

packages/plugins/plugin-audit/src/audit-writers.test.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1657,8 +1657,13 @@ describe('audit writers — the writer reads the session key the engine emits (#
16571657
// A session in the REMOVED dialect. The engine cannot produce one, so the
16581658
// only way this shape reaches the writer is a caller that is itself broken
16591659
// — and honouring it here would hide that. This pin goes red the day
1660-
// `sess.tenantId` is reintroduced as a fallback arm; `pnpm check:org-identifier`
1661-
// cannot see that reintroduction when the receiver is spelled `sess`.
1660+
// `sess.tenantId` is reintroduced as a fallback arm. It used to be the ONLY
1661+
// thing that would: `pnpm check:org-identifier` was anchored on the literal
1662+
// receiver name `session` and scored zero on `sess`. Since #9691 that gate
1663+
// resolves the receiver's PROVENANCE instead — a local filled from a
1664+
// `.session` expression is a session whatever it is called — so the
1665+
// reintroduction is caught in both places now. Keep this pin anyway: the
1666+
// gate cannot see a wrong VALUE, only a removed-alias read.
16621667
await fire('afterInsert', {
16631668
object: 'crm_lead',
16641669
input: { id: 'lead-1' },

packages/plugins/plugin-audit/src/comment-access-hooks.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,4 +495,40 @@ describe('#7141 — caller envelope forwarded to the sharing gate', () => {
495495
).rejects.toMatchObject({ code: 'RECORD_NOT_ACCESSIBLE', status: 403 });
496496
expect((canEdit.mock.calls[0]![2] as any).__writeScope).toBeUndefined();
497497
});
498+
499+
// ── The session fallback, and the org name it reads (#9691) ───────────
500+
//
501+
// The kit had no coverage of the no-execution-context path at all, so the
502+
// dead `s.tenantId ?? s.organizationId` first arm was invisible in both
503+
// directions: nothing proved the blessed name was read, and nothing would
504+
// have noticed if the fallback had been dropped. Both directions are pinned
505+
// here, on the session shape `ObjectQLEngine.buildSession` actually emits.
506+
it('falls back to the session snapshot and reads the caller org under the BLESSED name (#9691)', async () => {
507+
const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true);
508+
const { beforeDelete } = install({ comments: [row], sharing: { canEdit } });
509+
await beforeDelete({
510+
object: 'sys_comment',
511+
event: 'beforeDelete',
512+
input: { id: 'c1' },
513+
session: { userId: 'u1', organizationId: 'org_1', positions: ['p1'] },
514+
api: apiFor(['crm_opportunity/opp1']),
515+
});
516+
// `tenantId` on the way OUT is `ExecutionContext`'s driver-layer name for
517+
// the same value — the separate axis #3290 deliberately left alone.
518+
expect(canEdit.mock.calls[0]![2]).toEqual({ userId: 'u1', tenantId: 'org_1', positions: ['p1'] });
519+
});
520+
521+
it('does not resurrect the removed `session.tenantId` alias if one ever reaches a hook (#9691)', async () => {
522+
const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true);
523+
const { beforeDelete } = install({ comments: [row], sharing: { canEdit } });
524+
await beforeDelete({
525+
object: 'sys_comment',
526+
event: 'beforeDelete',
527+
input: { id: 'c1' },
528+
// A key `HookContextSchema` strips (#3290). It is not the caller's org.
529+
session: { userId: 'u1', tenantId: 'stale_org', positions: ['p1'] } as any,
530+
api: apiFor(['crm_opportunity/opp1']),
531+
});
532+
expect((canEdit.mock.calls[0]![2] as any).tenantId).toBeUndefined();
533+
});
498534
});

packages/plugins/plugin-audit/src/comment-access-hooks.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,13 @@ function callerContext(ctx: any): ExecutionContext {
252252
return withoutOperationPrivateKeys(exec as Record<string, unknown>);
253253
}
254254
const s = ctx?.session ?? {};
255-
return { userId: s.userId, tenantId: s.tenantId ?? s.organizationId, positions: s.positions };
255+
// [#9691] The `s.tenantId` arm was DEAD, not a fallback: `HookContextSchema`
256+
// strips a `tenantId` key from the session (#3290) and the engine's
257+
// `buildSession` only ever emits `organizationId`, so the first arm answered
258+
// `undefined` on every call and the second one carried the value. Dropping it
259+
// is byte-for-byte the same envelope; it is removed because a dead read of a
260+
// removed alias is what an author copies out of a reference body.
261+
return { userId: s.userId, tenantId: s.organizationId, positions: s.positions };
256262
}
257263

258264
/** Can the CALLER read `(object, recordId)`? A caller-scoped `findOne` through

packages/services/service-storage/src/attachment-access-hooks.test.ts

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -711,17 +711,45 @@ describe('#7145 — caller envelope forwarded to the sharing gate', () => {
711711
expect((canEdit.mock.calls[0]![2] as any).__writeScope).toBeUndefined();
712712
});
713713

714-
// ── The session fallback is unchanged ─────────────────────────────────
715-
it('still falls back to the session snapshot when no execution context rides along', async () => {
714+
// ── The session fallback, and the org name it reads (#9691) ───────────
715+
//
716+
// ⚠️ This case used to hand the hook a session spelling `tenantId: 'org_1'`
717+
// and assert the same key came back out. That is a session the engine cannot
718+
// produce: `HookContextSchema` STRIPS a `tenantId` key (#3290, pinned in
719+
// `packages/spec/src/data/hook.test.ts`) and `buildSession` only ever emits
720+
// `organizationId`. So the fixture pinned the removed-alias arm itself — it
721+
// passed for exactly as long as `callerContext` read the dead name, and could
722+
// only have started failing if the code became right, which is what happened.
723+
// Replaced rather than respelled: the fixture below is the envelope a real
724+
// transport builds.
725+
it('falls back to the session snapshot and reads the caller org under the BLESSED name (#9691)', async () => {
716726
const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true);
717727
const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } });
718728
await beforeDelete({
719729
object: 'sys_attachment',
720730
event: 'beforeDelete',
721731
input: { id: 'a1' },
722-
session: { userId: 'u1', tenantId: 'org_1', positions: ['p1'] },
732+
// Exactly what `ObjectQLEngine.buildSession` emits.
733+
session: { userId: 'u1', organizationId: 'org_1', positions: ['p1'] },
723734
api: apiFor([]),
724735
});
736+
// `tenantId` on the way OUT is `ExecutionContext`'s driver-layer name for
737+
// the same value — the separate axis #3290 deliberately left alone.
725738
expect(canEdit.mock.calls[0]![2]).toEqual({ userId: 'u1', tenantId: 'org_1', positions: ['p1'] });
726739
});
740+
741+
it('does not resurrect the removed `session.tenantId` alias if one ever reaches a hook (#9691)', async () => {
742+
const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true);
743+
const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } });
744+
await beforeDelete({
745+
object: 'sys_attachment',
746+
event: 'beforeDelete',
747+
input: { id: 'a1' },
748+
// A key the schema strips. Reaching for it is how this seam handed the
749+
// sharing service an envelope with no org at all for several majors.
750+
session: { userId: 'u1', tenantId: 'stale_org', positions: ['p1'] } as any,
751+
api: apiFor([]),
752+
});
753+
expect((canEdit.mock.calls[0]![2] as any).tenantId).toBeUndefined();
754+
});
727755
});

packages/services/service-storage/src/attachment-access-hooks.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,17 @@ function callerContext(ctx: any): ExecutionContext {
134134
return withoutOperationPrivateKeys(exec as Record<string, unknown>);
135135
}
136136
const s = ctx?.session ?? {};
137-
return { userId: s.userId, tenantId: s.tenantId, positions: s.positions };
137+
// [#9691] `s.organizationId`, NOT `s.tenantId`. The hook session's org key is
138+
// `organizationId` (engine `buildSession`; `HookContextSchema` STRIPS a
139+
// `tenantId` key outright, pinned in `packages/spec/src/data/hook.test.ts`),
140+
// so the removed alias read here answered `undefined` on every call and this
141+
// fallback handed `ISharingService.canEdit` an envelope with no org at all.
142+
// The target field keeps its `tenantId` spelling: that is `ExecutionContext`'s
143+
// driver-layer name for the same value, a separate axis #3290 deliberately
144+
// left alone. The comment kit's `callerContext` already read the blessed name
145+
// (`s.tenantId ?? s.organizationId`), so this is the #7145 parity that kit's
146+
// card asked for, completed.
147+
return { userId: s.userId, tenantId: s.organizationId, positions: s.positions };
138148
}
139149

140150
export function installAttachmentAccessHooks(

0 commit comments

Comments
 (0)