From 36ca4c4eee24b95e336f90f4a61fa2695cc7e571 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 02:37:16 +0000 Subject: [PATCH] fix(service-storage): forward the caller's execution envelope to the sys_attachment sharing gates (#7145) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `callerContext()` rebuilt a five-field projection of the caller's ExecutionContext before handing it to `ISharingService.canEdit`, whose contract declares the full envelope and forbids callers rebuilding a subset of it (#6523 / the #6206 ruling). Same defect PR #7143 fixed for the sys_comment kit (#7141), one package over. The projection did two jobs and only one was correct: dropping the middleware-private `__`-prefixed keys (preserved, now by PREFIX) vs dropping the principal fields (the defect — `onBehalfOf` disarmed the fail-closed `hasWriteBypass` guard, `principalKind` resolved an ADR-0090 D10 agent principal as a human). Applied to both canEdit call sites and to the read middleware's parent-visibility probe. No parent access DEPTH is synthesised: absent depth leaves the owner-match at `own`, byte-for-byte what the projection produced (the #7144 decision is untouched). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016R9de1FqP7NvwKvqXi92Gh --- .changeset/attachment-gate-caller-envelope.md | 61 ++++ .../src/attachment-access-hooks.test.ts | 292 ++++++++++++++++++ .../src/attachment-access-hooks.ts | 105 ++++++- .../src/attachment-read-visibility.test.ts | 42 ++- 4 files changed, 489 insertions(+), 11 deletions(-) create mode 100644 .changeset/attachment-gate-caller-envelope.md diff --git a/.changeset/attachment-gate-caller-envelope.md b/.changeset/attachment-gate-caller-envelope.md new file mode 100644 index 0000000000..337f662674 --- /dev/null +++ b/.changeset/attachment-gate-caller-envelope.md @@ -0,0 +1,61 @@ +--- +"@objectstack/service-storage": patch +--- + +fix(service-storage): forward the caller's full execution envelope to the `sys_attachment` sharing gates (#7145) + +`callerContext()` in `attachment-access-hooks.ts` rebuilt a five-field +projection of the caller's `ExecutionContext` (`userId` / `tenantId` / +`positions` / `permissions` / `isSystem`) before handing it to +`ISharingService.canEdit`, whose contract declares the **full** envelope and +whose doc block tells callers they "MUST NOT rebuild a subset of it" (#6523 / +the #6206 ruling). This is the same defect PR #7143 fixed for the `sys_comment` +kit (#7141), one package over — the attachment kit is what the comment kit was +derived from. + +The projection was doing two jobs at once and only one of them was correct: + +- **Dropping the middleware-private keys was correct**, and is preserved. + plugin-security's middleware stamps the access DEPTH it resolved for the + object of the operation in flight — `sys_attachment` here — onto the context + in place (`sc.__readScope = …`), while these gates ask the sharing service + about the **parent record's** object. Forwarding that whole would hand one + object's widening to another object's owner-match, the stale-scope leak + `resolveWriteScopeForSharing` was extracted to prevent. The keys are now + dropped by the `__` **prefix** rather than by name, which also covers the + engine's other operation-private markers on that channel (`__expandRead` + waives the object-level CRUD check, `__referentialFieldClear` the + referential-clear write) and cannot go stale when a fifth key is added. +- **Dropping the principal fields was the defect.** Two of them decide the + verdict these gates then trust: + - `onBehalfOf` — `ISecurityService.hasWriteBypass`, the `modifyAllRecords` + probe `SharingService.canEdit` consults last, is documented to fail CLOSED + on a delegated context and implements that by reading exactly + `context?.onBehalfOf?.userId`. Stripped, the guard could never fire on the + attachment path, and the `/mcp` OAuth agent principal that + `resolve-execution-context` builds *with* the delegation link reached the + bypass probe looking like an ordinary direct call. + - `principalKind` — `resolvePermissionSetsForContext` keys the ADR-0090 D10 + rule "an agent's grants are EXACTLY its scope-derived ceiling" on + `principalKind === 'agent'`. Stripped, the additive human baseline was + appended to an agent's ceiling here, so the sets the bypass probe evaluated + were a superset of what the user consented to. + + `systemPermissions`, `accessible_org_ids`, `posture`, `audience` and + `rlsMembership` were dropped by the same projection and are forwarded now for + the same reason. + +Both `canEdit` call sites are covered — the `beforeInsert` parent gate and the +`beforeDelete` per-row authorization loop — and the same +envelope-minus-private-keys rule is applied to the read middleware's +parent-visibility probe, which spread the whole operation context into a `find` +on a different object. + +No access depth is synthesised for the parent object: absent depth leaves the +sharing owner-match at its narrowest (`own`), which is the safe direction and +byte-for-byte what the projection produced. Resolving the parent's own depth +would WIDEN these gates and is deliberately left to the separate decision +tracked as #7144. + +Enforcement effect: a delegated (`onBehalfOf`-carrying) principal is now refused +where the contract says it is refused. No caller gains access. 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 dab3567a52..c0f6b6176a 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.test.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.test.ts @@ -268,3 +268,295 @@ describe('attachment access — beforeDelete (uploader or parent editor)', () => }); }); }); + +// ───────────────────────────────────────────────────────────────────────── +// #7145 — what the gate FORWARDS to the sharing service +// +// The mirror of #7141 / PR #7143 for the attachment kit: `callerContext()` +// rebuilt a five-field projection of the caller's execution envelope before +// handing it to `ISharingService.canEdit`, whose contract declares the FULL +// envelope and whose doc block tells callers they "MUST NOT rebuild a subset +// of it" (#6523 / the #6206 ruling). +// ───────────────────────────────────────────────────────────────────────── + +/** + * The caller's execution envelope as a real transport builds it — an OAuth MCP + * agent principal acting on behalf of a human (`resolve-execution-context.ts` + * is the live producer of `principalKind: 'agent'` + `onBehalfOf`) — with the + * middleware-private keys plugin-security stamps for the object of the + * operation in flight (`sys_attachment`) riding along, because that is exactly + * what `sc.__readScope = …` leaves on the context these hooks receive. + */ +const DELEGATED_ENVELOPE = { + userId: 'human_1', + tenantId: 'org_1', + email: 'human@example.com', + positions: [], + permissions: ['mcp_agent_data_write'], + systemPermissions: [], + principalKind: 'agent', + onBehalfOf: { userId: 'human_1', principalKind: 'human' }, + audience: 'internal', + posture: 'authenticated', + accessible_org_ids: ['org_1'], + rlsMembership: { team: ['t1'] }, + isSystem: false, + // Middleware-private, resolved for `sys_attachment` — NOT for the parent. + __readScope: 'org', + __writeScope: 'org', + __delegatorReadScope: 'org', + __delegatorWriteScope: 'org', + __expandRead: true, +} as const; + +/** The principal half of {@link DELEGATED_ENVELOPE} — everything that must + * survive the forward, and nothing that must not. */ +const DELEGATED_PRINCIPAL_FIELDS = { + userId: 'human_1', + tenantId: 'org_1', + email: 'human@example.com', + positions: [], + permissions: ['mcp_agent_data_write'], + systemPermissions: [], + principalKind: 'agent', + onBehalfOf: { userId: 'human_1', principalKind: 'human' }, + audience: 'internal', + posture: 'authenticated', + accessible_org_ids: ['org_1'], + rlsMembership: { team: ['t1'] }, + isSystem: false, +}; + +const OPERATION_PRIVATE_KEYS = [ + '__readScope', + '__writeScope', + '__delegatorReadScope', + '__delegatorWriteScope', + '__expandRead', +]; + +/** An insert ctx carrying an explicit execution envelope. */ +const envelopeInsertCtx = (data: any, exec: Record) => ({ + object: 'sys_attachment', + event: 'beforeInsert', + input: { data, options: { context: exec } }, + session: { userId: exec.userId as string }, + api: apiFor([]), +}); + +/** A delete ctx carrying an explicit execution envelope. */ +const envelopeDeleteCtx = (input: any, exec: Record) => ({ + object: 'sys_attachment', + event: 'beforeDelete', + input: { ...input, options: { ...(input.options ?? {}), context: exec } }, + session: { userId: exec.userId as string }, + api: apiFor([]), +}); + +/** The deployment's `fallbackPermissionSet` (ADR-0056 D7: an app's `isDefault` + * profile, else the built-in `member_default`). */ +const DEPLOYMENT_BASELINE_SET = 'app_default_profile'; + +/** + * `ISecurityService.hasWriteBypass` as plugin-security implements it + * (`security-plugin.ts`) — the three guard lines, then the `modifyAllRecords` + * set probe. A DOUBLE, not a copy of production logic: service-storage does not + * depend on plugin-security (it consults a duck-typed `AttachmentSharingLike`), + * so the only way to pin the OUTCOME on this side of the seam is to model the + * contract the gate is documented to be talking to. `setsWithBypass` names + * which permission sets carry the bit in the modelled deployment. + */ +function hasWriteBypassDouble(context: any, setsWithBypass: string[]): boolean { + if (context?.isSystem) return true; + if (!context?.userId) return false; + if (context?.onBehalfOf?.userId) return false; // documented fail-CLOSED on delegation + // `resolvePermissionSetsForContext`: positions + explicit sets, plus the + // ADDITIVE human baseline — which an ADR-0090 D10 agent principal must NOT + // receive (its grants are exactly its scope-derived ceiling). + const requested = [...(context?.positions ?? []), ...(context?.permissions ?? [])]; + const resolved = + context?.principalKind === 'agent' ? requested : [...requested, DEPLOYMENT_BASELINE_SET]; + return resolved.some((name: string) => setsWithBypass.includes(name)); +} + +/** + * `SharingService.checkEdit`'s positive bases, in order: ownership widened by + * the middleware-stamped write DEPTH (`matchesOwnerScope` — `__writeScope === + * 'org'` short-circuits to true), then the `modifyAllRecords` bypass. The share + * branch is omitted (no grants in these fixtures). + */ +function sharingCanEditDouble(opts: { ownerId: string; setsWithBypass?: string[] }) { + return vi.fn(async (_object: string, _recordId: string, callerCtx: any) => { + if (callerCtx?.isSystem) return true; + if (!callerCtx?.userId) return false; + if ((callerCtx as any).__writeScope === 'org') return true; // depth fast-exit + if (String(callerCtx.userId) === opts.ownerId) return true; + return hasWriteBypassDouble(callerCtx, opts.setsWithBypass ?? []); + }); +} + +describe('#7145 — caller envelope forwarded to the sharing gate', () => { + const attRow = { + id: 'a1', + file_id: 'f1', + parent_object: 'att_secret', + parent_id: 'r1', + uploaded_by: 'someone_else', + }; + + // ── The forward itself, on BOTH call sites ──────────────────────────── + it('beforeInsert forwards the whole envelope MINUS the operation-private keys', async () => { + const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true); + const { beforeInsert } = install({ sharing: { canEdit } }); + await beforeInsert( + envelopeInsertCtx({ parent_object: 'att_case', parent_id: 'r1', file_id: 'f1' }, { + ...DELEGATED_ENVELOPE, + }), + ); + + const forwarded = canEdit.mock.calls[0]![2] as unknown as Record; + // Every principal field survives — the #6523 contract's unit is the + // envelope, and #6206 forbids rebuilding a subset of it. `uploaded_by` + // stamping does not touch the context. + expect(forwarded).toEqual(DELEGATED_PRINCIPAL_FIELDS); + // …and every middleware-private key resolved for `sys_attachment` is gone. + for (const key of OPERATION_PRIVATE_KEYS) expect(forwarded).not.toHaveProperty(key); + }); + + it('beforeDelete forwards the whole envelope MINUS the operation-private keys', async () => { + const canEdit = vi.fn(async (_o: string, _r: string, _c: any) => true); + const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } }); + await beforeDelete(envelopeDeleteCtx({ id: 'a1' }, { ...DELEGATED_ENVELOPE })); + + const forwarded = canEdit.mock.calls[0]![2] as unknown as Record; + expect(forwarded).toEqual(DELEGATED_PRINCIPAL_FIELDS); + for (const key of OPERATION_PRIVATE_KEYS) expect(forwarded).not.toHaveProperty(key); + }); + + it('hands the service a COPY, so a callee stamping its own depth cannot write back', async () => { + const exec: Record = { ...DELEGATED_ENVELOPE }; + const canEdit = vi.fn(async (_o: string, _r: string, callerCtx: any) => { + // What plugin-security does right before it calls the sharing service. + callerCtx.__writeScope = 'unit'; + return true; + }); + const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } }); + await beforeDelete(envelopeDeleteCtx({ id: 'a1' }, exec)); + + expect(canEdit.mock.calls[0]![2]).not.toBe(exec); + expect(exec.__writeScope).toBe('org'); // untouched: still sys_attachment's own + }); + + // ── What the restored fields BUY: the two verdict-deciding ones ─────── + it('REFUSES a delegated principal whose sets carry modifyAllRecords (fail-closed, #7145)', async () => { + // The exploit shape the card names: an OAuth agent on the `/mcp` surface + // presenting sets that carry the super-user write bypass. `hasWriteBypass` + // is documented to fail CLOSED on `onBehalfOf` — it can only do that if the + // field reaches it. + const canEdit = sharingCanEditDouble({ + ownerId: 'other_owner', + setsWithBypass: ['admin_full_access'], + }); + const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } }); + await expect( + beforeDelete( + envelopeDeleteCtx({ id: 'a1' }, { + ...DELEGATED_ENVELOPE, + permissions: ['admin_full_access'], + }), + ), + ).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED', status: 403 }); + expect(canEdit).toHaveBeenCalledTimes(1); + }); + + it('REFUSES the same delegated principal on the beforeInsert parent gate too', async () => { + // Both `canEdit` call sites read the same `callerContext()`, so the guard + // has to arrive on the attach path as well as the detach one. + const canEdit = sharingCanEditDouble({ + ownerId: 'other_owner', + setsWithBypass: ['admin_full_access'], + }); + const { beforeInsert } = install({ sharing: { canEdit } }); + await expect( + beforeInsert( + envelopeInsertCtx({ parent_object: 'att_secret', parent_id: 'r1', file_id: 'f1' }, { + ...DELEGATED_ENVELOPE, + permissions: ['admin_full_access'], + }), + ), + ).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403 }); + expect(canEdit).toHaveBeenCalledTimes(1); + }); + + it('keeps an AGENT principal capped at its ceiling — no additive human baseline (ADR-0090 D10)', async () => { + // `resolvePermissionSetsForContext` keys that rule on `principalKind`, which + // the old projection dropped: the agent was resolved as a human and the + // deployment's default profile was appended to its consented ceiling. + const canEdit = sharingCanEditDouble({ + ownerId: 'other_owner', + setsWithBypass: [DEPLOYMENT_BASELINE_SET], + }); + const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } }); + await expect( + beforeDelete( + envelopeDeleteCtx({ id: 'a1' }, { + ...DELEGATED_ENVELOPE, + onBehalfOf: undefined, // isolate the ceiling rule from the delegation guard + }), + ), + ).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED', status: 403 }); + }); + + // ── The half of the old projection that was CORRECT ─────────────────── + it("does NOT carry sys_attachment's access DEPTH into the parent's owner-match", async () => { + // The context carries `__writeScope: 'org'` resolved for `sys_attachment`, + // and the gate asks about `att_secret`. Forwarding it whole would widen one + // object's question with another object's answer — a plain member would + // detach any file on any record in the org. + const canEdit = sharingCanEditDouble({ ownerId: 'other_owner' }); + const { beforeDelete } = install({ attachments: [attRow], sharing: { canEdit } }); + await expect( + beforeDelete( + envelopeDeleteCtx({ id: 'a1' }, { + ...DELEGATED_ENVELOPE, + principalKind: 'human', + onBehalfOf: undefined, + userId: 'plain_member', + permissions: [], + }), + ), + ).rejects.toMatchObject({ code: 'ATTACHMENT_DELETE_DENIED', status: 403 }); + expect((canEdit.mock.calls[0]![2] as any).__writeScope).toBeUndefined(); + }); + + it('does not carry the depth into the beforeInsert parent gate either', async () => { + const canEdit = sharingCanEditDouble({ ownerId: 'other_owner' }); + const { beforeInsert } = install({ sharing: { canEdit } }); + await expect( + beforeInsert( + envelopeInsertCtx({ parent_object: 'att_secret', parent_id: 'r1', file_id: 'f1' }, { + ...DELEGATED_ENVELOPE, + principalKind: 'human', + onBehalfOf: undefined, + userId: 'plain_member', + permissions: [], + }), + ), + ).rejects.toMatchObject({ code: 'ATTACHMENT_PARENT_ACCESS', status: 403 }); + 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 () => { + 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'] }, + api: apiFor([]), + }); + expect(canEdit.mock.calls[0]![2]).toEqual({ userId: 'u1', tenantId: 'org_1', positions: ['p1'] }); + }); +}); diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 537363d8c7..44c8043f95 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.ts @@ -1,5 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import type { ExecutionContext } from '@objectstack/spec/kernel'; + import type { AttachmentLifecycleEngine, AttachmentLifecycleLogger, @@ -66,18 +68,89 @@ function asIdList(id: unknown): Array | null { return null; } +/** + * Keys plugin-security's middleware STAMPS onto the operation context, resolved + * for the object of the CURRENT operation — `sys_attachment` here. + * + * They are middleware-private vocabulary, not fields of `ExecutionContext`, and + * they are all read as WIDENING inputs by whoever consumes them: the ADR-0057 + * D1 access DEPTH the sharing owner-match expands to (`__readScope` / + * `__writeScope`, plus the ADR-0090 D10 delegator halves + * `__delegatorReadScope` / `__delegatorWriteScope` — `sc.__readScope = …`, + * `security-plugin.ts`), and the engine's internal privilege markers on the + * same channel (`__expandRead` waives the object-level CRUD check for a lookup + * expansion, `__referentialFieldClear` the referential-clear write). + * + * Every gate in this module asks about the PARENT record's object, never about + * `sys_attachment`, so carrying any of these across is one object's widening + * applied to another object's question — the exact stale-scope leak + * `resolveWriteScopeForSharing` was extracted to prevent ("a stale value can + * never leak in through a spread", `security-plugin.ts`). They are therefore + * dropped by PREFIX rather than by a name list: the `__` convention is what + * marks a key as belonging to the operation in flight, and a list would go + * stale the day the middleware stamps a fifth one. + */ +const OPERATION_PRIVATE_KEY_PREFIX = '__'; + +/** + * The caller's execution envelope, minus the operation-private keys above. + * + * [#7145] A FRESH object every time, so a callee that stamps its own + * `__writeScope` onto what it receives (which is exactly what plugin-security + * does before it calls the sharing service) can never write back into the + * operation context this hook was handed. + */ +function withoutOperationPrivateKeys(exec: Record): ExecutionContext { + const out: Record = {}; + for (const [key, value] of Object.entries(exec)) { + if (key.startsWith(OPERATION_PRIVATE_KEY_PREFIX)) continue; + out[key] = value; + } + return out as ExecutionContext; +} + /** The caller's ExecutionContext rides on the operation options — the - * session snapshot lacks `permissions`, which sharing bypasses need. */ -function callerContext(ctx: any): Record { + * session snapshot lacks `permissions`, which sharing bypasses need. + * + * [#7145] Forwarded as the full envelope, which is what `ISharingService` + * declares for every parameter this value is handed to and what the #6206 + * ruling requires of every caller: they "MUST NOT rebuild a subset of it" + * (#6523). The five-field projection this replaced (`userId` / `tenantId` / + * `positions` / `permissions` / `isSystem`) was doing two jobs at once, and + * only one of them was correct — same defect, same kit, one package over from + * `comment-access-hooks.ts` (#7141 / PR #7143), which this mirrors: + * + * - dropping the middleware-private keys — CORRECT, and preserved above by + * {@link withoutOperationPrivateKeys}: `return exec;` would hand + * `sys_attachment`'s access depth to the parent object's owner-match; + * - dropping the PRINCIPAL fields — the defect. Two of them decide the + * verdict the gate then trusts: + * * `onBehalfOf` — `ISecurityService.hasWriteBypass`, the `modifyAllRecords` + * probe `SharingService.canEdit` consults last, is documented to fail + * CLOSED on a delegated context and implements that by reading exactly + * `context?.onBehalfOf?.userId` (`security-plugin.ts`). Stripped, that + * guard could never fire here, and a `/mcp` OAuth agent principal (which + * `resolve-execution-context.ts` builds WITH the delegation link) reached + * the bypass probe looking like an ordinary direct call. + * * `principalKind` — `resolvePermissionSetsForContext` keys the ADR-0090 + * D10 rule "an agent's grants are EXACTLY its scope-derived ceiling" on + * `principalKind === 'agent'`; stripped, the additive human baseline was + * appended to an agent's ceiling on this path, so the sets the bypass + * probe evaluated were a SUPERSET of what the user consented to. + * + * `systemPermissions`, `accessible_org_ids`, `posture`, `audience` and + * `rlsMembership` were dropped by the same projection; they are forwarded + * now for the same reason — the envelope is the contract's unit. + * + * Note what deliberately did NOT change: no access DEPTH is synthesised for the + * parent object. Absent depth leaves the sharing owner-match at its narrowest + * (`own`) — the safe direction, and byte-for-byte the behaviour the projection + * produced. Resolving the parent's own depth would WIDEN this gate and is a + * separate decision, tracked as #7144. */ +function callerContext(ctx: any): ExecutionContext { const exec = ctx?.input?.options?.context; if (exec && typeof exec === 'object') { - return { - userId: exec.userId, - tenantId: exec.tenantId, - positions: exec.positions, - permissions: exec.permissions, - isSystem: exec.isSystem, - }; + return withoutOperationPrivateKeys(exec as Record); } const s = ctx?.session ?? {}; return { userId: s.userId, tenantId: s.tenantId, positions: s.positions }; @@ -340,6 +413,18 @@ async function computeParentVisibilityFilter( // 2. Per parent_object, the visible id subset via the CALLER's context — // the parent object's own RLS/OWD/sharing applies. + // + // [#7145] The caller's envelope, minus the operation-private keys: this + // probe reads a DIFFERENT object than the one the middleware resolved its + // depth for, and `__readScope` / `__expandRead` are widening inputs that + // would arrive attached to the wrong question (the security middleware + // re-stamps the depth for THIS object when it resolves any set, so the + // only thing dropping them can do is leave the owner-match at its + // narrowest — the safe direction). Same rule as `callerContext` above, + // and the same half of #7141 / PR #7143 the comment kit already carries. + const callerEnvelope = withoutOperationPrivateKeys( + (ctx.context ?? {}) as Record, + ); const clauses: Array> = []; for (const [parentObject, idSet] of byObject) { const ids = [...idSet]; @@ -349,7 +434,7 @@ async function computeParentVisibilityFilter( where: { id: { $in: ids } }, fields: ['id'], limit: ids.length, - context: { ...ctx.context }, + context: { ...callerEnvelope }, }); visible = rows.map((r) => String(r.id)).filter(Boolean); } catch { diff --git a/packages/services/service-storage/src/attachment-read-visibility.test.ts b/packages/services/service-storage/src/attachment-read-visibility.test.ts index 3cfdd42ded..7832def980 100644 --- a/packages/services/service-storage/src/attachment-read-visibility.test.ts +++ b/packages/services/service-storage/src/attachment-read-visibility.test.ts @@ -88,7 +88,11 @@ function install(opts: { visible: Record>; }) { let mw!: (ctx: AttachmentReadMiddlewareCtx, next: () => Promise) => Promise; - const calls = { parentFinds: [] as Array<{ object: string; ids: string[]; userId?: string }> }; + const calls = { + parentFinds: [] as Array<{ object: string; ids: string[]; userId?: string }>, + /** [#7145] The context each parent probe was handed, verbatim. */ + parentFindContexts: [] as any[], + }; const engine: AttachmentLifecycleEngine = { registerHook: () => {}, @@ -105,6 +109,7 @@ function install(opts: { const userId = options?.context?.userId as string | undefined; const ids: string[] = (options?.where?.id?.$in ?? []).map(String); calls.parentFinds.push({ object, ids, userId }); + calls.parentFindContexts.push(options?.context); const vis = opts.visible[object]?.[userId ?? ''] ?? []; return ids.filter((id) => vis.includes(id)).map((id) => ({ id })) as any; }, @@ -277,6 +282,41 @@ describe('installAttachmentReadVisibility', () => { expect(where).toEqual({ id: '__attachment_parent_denied__' }); expect(calls.parentFinds).toHaveLength(0); }); + + // [#7145] The parent probe reads a DIFFERENT object than the one the security + // middleware resolved its depth for, so the caller's envelope crosses over + // but the operation-private keys must not: `__readScope` is + // `sys_attachment`'s access DEPTH and `__expandRead` waives the object-level + // CRUD check. Mirrors the same half of #7141 / PR #7143 in the comment kit. + it('probes the parent with the caller ENVELOPE, minus the operation-private keys', async () => { + const { mw, calls } = install(dataset); + await runRead(mw, { + context: { + userId: 'u1', + principalKind: 'agent', + onBehalfOf: { userId: 'human_1', principalKind: 'human' }, + accessible_org_ids: ['org_1'], + posture: 'authenticated', + __readScope: 'org', + __delegatorReadScope: 'org', + __expandRead: true, + } as any, + }); + const probed = calls.parentFindContexts; + expect(probed.length).toBeGreaterThan(0); + for (const c of probed) { + expect(c).toMatchObject({ + userId: 'u1', + principalKind: 'agent', + onBehalfOf: { userId: 'human_1', principalKind: 'human' }, + accessible_org_ids: ['org_1'], + posture: 'authenticated', + }); + for (const key of ['__readScope', '__delegatorReadScope', '__expandRead']) { + expect(c).not.toHaveProperty(key); + } + } + }); }); /**