From 12ab76f3f420fe48941d337ff07f925a6377fecf Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 07:49:23 +0000 Subject: [PATCH] refactor(core,plugin-audit,service-storage,plugin-reports): give the `__` operation-private-key convention a single owner (#7284) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `withoutOperationPrivateKeys` and its `OPERATION_PRIVATE_KEY_PREFIX` had been hand-copied into three packages — plugin-audit's comment access hooks (#7141), service-storage's attachment access hooks (#7145) and plugin-reports' report service (#7204). All three were byte-equivalent in behaviour; their doc blocks had already diverged in prose. The rule now lives once in `@objectstack/core` (`security/operation-private-keys.ts`), beside `assemble-execution-context.ts` — that file owns where an ExecutionContext is BUILT at a transport entry point, this one owns where it is stripped before being forwarded to a question it was not resolved for. Home chosen by dependency measurement: all three consumers already depend on `@objectstack/core`, none depends on `plugin-security` (the producer, and the most honest owner, but a string-prefix filter does not justify three new dependency edges onto a plugin), and `@objectstack/spec` is fenced by Prime Directive #2. Core is the only zero-new-edge candidate. The reasoning moved with the code rather than being thinned; each consumer keeps only its own local half and points at the shared home. Pins: `operation-private-keys.test.ts` asserts the rule's own behaviour, which no package-level test had asserted directly, and `operation-private-keys.pin.test.ts` turns red if a fourth file declares its own copy. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017AD2nx7MRuje3kLqoLBHPM --- .../operation-private-keys-single-owner.md | 39 ++++++ packages/core/src/security/index.ts | 8 ++ .../operation-private-keys.pin.test.ts | 127 ++++++++++++++++++ .../security/operation-private-keys.test.ts | 112 +++++++++++++++ .../src/security/operation-private-keys.ts | 122 +++++++++++++++++ .../plugin-audit/src/comment-access-hooks.ts | 50 ++----- .../plugin-reports/src/report-service.ts | 53 +++----- .../src/attachment-access-hooks.ts | 50 ++----- 8 files changed, 452 insertions(+), 109 deletions(-) create mode 100644 .changeset/operation-private-keys-single-owner.md create mode 100644 packages/core/src/security/operation-private-keys.pin.test.ts create mode 100644 packages/core/src/security/operation-private-keys.test.ts create mode 100644 packages/core/src/security/operation-private-keys.ts diff --git a/.changeset/operation-private-keys-single-owner.md b/.changeset/operation-private-keys-single-owner.md new file mode 100644 index 0000000000..261951e334 --- /dev/null +++ b/.changeset/operation-private-keys-single-owner.md @@ -0,0 +1,39 @@ +--- +"@objectstack/core": patch +"@objectstack/plugin-audit": patch +"@objectstack/service-storage": patch +"@objectstack/plugin-reports": patch +--- + +refactor(core,plugin-audit,service-storage,plugin-reports): give the `__` operation-private-key convention a single owner (#7284) + +`withoutOperationPrivateKeys` — the rule that a consumer forwarding a caller's +execution envelope to a question about a DIFFERENT object must first drop the +`__`-prefixed keys plugin-security stamped for the operation in flight — had been +hand-copied into three packages: `plugin-audit`'s comment access hooks (#7141), +`service-storage`'s attachment access hooks (#7145) and `plugin-reports`' report +service (#7204). Each carried its own `OPERATION_PRIVATE_KEY_PREFIX` and its own +doc block, and the prose had already diverged while the code still agreed — the +shape that makes a later divergence in behaviour hard to notice. + +The helper now lives once, in `@objectstack/core` +(`security/operation-private-keys.ts`), exported from the package root. Core is +the only candidate all three consumers already depend on: `plugin-security` is +the producer of the convention and the most honest owner, but none of the three +depends on it and a string-prefix filter does not justify three new dependency +edges onto a plugin; `@objectstack/spec` is fenced off by Prime Directive #2. The +new home sits beside `assemble-execution-context.ts`, which owns the other end of +the same lifecycle — that file is where an `ExecutionContext` is built at a +transport entry point, this one is where it is stripped back down before being +forwarded. + +The full reasoning moved with the code rather than being thinned: which keys the +middleware stamps and why each is a widening input, why they are dropped by +PREFIX and never by a name list, and why the fresh copy is load-bearing in both +directions. Each consumer keeps only its own local half — which object *its* +gates actually ask about — and points at the shared home. + +No behaviour change: the three copies were byte-equivalent, and all three +packages' suites pass unchanged. Two new pins at the home cover it — the rule's +own behaviour, which no package-level test had ever asserted directly, and a +repository-shape pin that turns red if a fourth file declares its own copy. diff --git a/packages/core/src/security/index.ts b/packages/core/src/security/index.ts index 47fc70e8f9..597660534a 100644 --- a/packages/core/src/security/index.ts +++ b/packages/core/src/security/index.ts @@ -131,3 +131,11 @@ export { // ADR-0091 D1/D2 — grant validity windows, the shared resolution-time predicate. export { isGrantActive, isGrantExpired, type GrantValidityWindow } from './grant-validity.js'; + +// #7284 — the `__` operation-private-key convention, the CONSUMER half of the +// ExecutionContext lifecycle `assemble-execution-context.ts` opens. One owner +// for the rule three packages had hand-copied (#7141 / #7145 / #7204). +export { + OPERATION_PRIVATE_KEY_PREFIX, + withoutOperationPrivateKeys, +} from './operation-private-keys.js'; diff --git a/packages/core/src/security/operation-private-keys.pin.test.ts b/packages/core/src/security/operation-private-keys.pin.test.ts new file mode 100644 index 0000000000..965d01fb9b --- /dev/null +++ b/packages/core/src/security/operation-private-keys.pin.test.ts @@ -0,0 +1,127 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7284] The `__` operation-private-key convention has exactly ONE owner. + * + * This is the pin the extraction is worth having. The three copies #7284 found + * were byte-equivalent in behaviour and each was covered by its own package's + * tests, so nothing in the repository went red while the rule was being copied + * by hand a third time — the finding was made by a human reading three diffs + * months apart. A fourth consumer is written the same way the first three were: + * by opening the nearest existing one and copying the block out of it. Extracting + * the helper without pinning it just resets that counter to one. + * + * So the assertion is about the SHAPE of the repository, not about behaviour: no + * file outside this module may declare its own `OPERATION_PRIVATE_KEY_PREFIX` or + * its own `withoutOperationPrivateKeys`. A fourth author who copies the block + * turns this red the first time they run the suite, with a message naming the + * import to use instead. + * + * ⛔ Scope, deliberately narrow — this pin does NOT try to detect "a consumer + * that should have used the helper and did not". That is the interesting + * question and it is not decidable by scanning: forwarding an envelope is + * spelled a dozen ways, and a regex ambitious enough to catch them all would be + * a false-red generator, which is worse than the gap (an inert or noisy gate + * reads as a gate that is watching — `validate-security-posture.ts`'s hazard). + * What IS decidable is redeclaration, which is exactly how all three copies got + * here. + * + * Reworded freely: the pin matches DECLARATIONS, not mentions. Documentation, + * comments and tests may name either symbol as much as they like. + */ + +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { dirname, join, relative, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { describe, it, expect } from 'vitest'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +/** …/packages/core/src/security → repo root */ +const REPO_ROOT = resolve(HERE, '../../../..'); +const PACKAGES = join(REPO_ROOT, 'packages'); + +/** The one file allowed to declare the convention. */ +const HOME = join(HERE, 'operation-private-keys.ts'); + +/** + * A DECLARATION of either symbol — `const OPERATION_PRIVATE_KEY_PREFIX =` or + * `function withoutOperationPrivateKeys(`, with or without `export`. + * + * Anchored at a statement start so that imports (`import { … }`), re-exports + * (`export { … } from`), calls and prose never match. Both spellings a copy + * could plausibly take are covered: a `function` declaration is what all three + * copies used, and `const … =` catches the arrow-function rewrite. + */ +const DECLARATION = + /^\s*(?:export\s+)?(?:const|let|var|function)\s+(OPERATION_PRIVATE_KEY_PREFIX|withoutOperationPrivateKeys)\b\s*[=(<]/gm; + +const SKIP_DIRS = new Set(['node_modules', 'dist', 'build', '.turbo', 'coverage', '.next']); + +/** Every `.ts`/`.tsx` file under `packages/`, excluding build output. */ +function sourceFiles(dir: string, out: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + if (SKIP_DIRS.has(entry)) continue; + const full = join(dir, entry); + if (statSync(full).isDirectory()) sourceFiles(full, out); + else if (/\.tsx?$/.test(entry) && !entry.endsWith('.d.ts')) out.push(full); + } + return out; +} + +describe('the `__` operation-private-key convention has one owner (#7284)', () => { + it('is declared in exactly one file, and that file is the shared home', () => { + const offenders: string[] = []; + + for (const file of sourceFiles(PACKAGES)) { + if (file === HOME) continue; + const text = readFileSync(file, 'utf8'); + DECLARATION.lastIndex = 0; + if (DECLARATION.test(text)) offenders.push(relative(REPO_ROOT, file)); + } + + expect( + offenders, + offenders.length === 0 + ? '' + : [ + 'These files declare their own copy of the `__` operation-private-key convention:', + ...offenders.map((f) => ` - ${f}`), + '', + 'That rule has a single owner since #7284. Import it instead:', + '', + " import { withoutOperationPrivateKeys } from '@objectstack/core';", + '', + 'The reasoning — why a consumer must drop these keys, why by PREFIX and', + 'never by a name list, and why the copy is load-bearing in both', + 'directions — lives at packages/core/src/security/operation-private-keys.ts.', + 'If you are adding a consumer, add it to that header\'s "Known consumers"', + 'list rather than re-deriving the argument locally.', + ].join('\n'), + ).toEqual([]); + }); + + it('the home really does declare both symbols — the scan cannot pass vacuously', () => { + // #4690: a check that finds nothing because it is looking in the wrong place + // reads exactly like a check that found no violations. Anchor it. + const text = readFileSync(HOME, 'utf8'); + const found = [...text.matchAll(DECLARATION)].map((m) => m[1]).sort(); + + expect(found).toEqual(['OPERATION_PRIVATE_KEY_PREFIX', 'withoutOperationPrivateKeys']); + }); + + it('the scan reaches the packages that used to hold the copies', () => { + // The second half of the same anti-vacuity guard: prove the walker actually + // descends into the three consumer packages, so a future refactor of + // SKIP_DIRS or the walk cannot silently narrow the scan to `packages/core`. + const scanned = sourceFiles(PACKAGES).map((f) => relative(REPO_ROOT, f)); + + for (const consumer of [ + 'packages/plugins/plugin-audit/src/comment-access-hooks.ts', + 'packages/services/service-storage/src/attachment-access-hooks.ts', + 'packages/plugins/plugin-reports/src/report-service.ts', + ]) { + expect(scanned).toContain(consumer); + } + }); +}); diff --git a/packages/core/src/security/operation-private-keys.test.ts b/packages/core/src/security/operation-private-keys.test.ts new file mode 100644 index 0000000000..d516b9478b --- /dev/null +++ b/packages/core/src/security/operation-private-keys.test.ts @@ -0,0 +1,112 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7284] Behaviour of the `__` operation-private-key convention at its home. + * + * The three packages that hand-copied this helper each covered it only through + * their own gates — `sys_comment`'s access hooks, `sys_attachment`'s, the report + * runner's — so the RULE itself was never asserted anywhere, only its effect on + * three particular call sites. These are the assertions that belong to the rule. + */ + +import { describe, it, expect } from 'vitest'; + +import { + OPERATION_PRIVATE_KEY_PREFIX, + withoutOperationPrivateKeys, +} from './operation-private-keys.js'; + +describe('withoutOperationPrivateKeys', () => { + it('drops every key carrying the operation-private prefix', () => { + const out = withoutOperationPrivateKeys({ + userId: 'u1', + tenantId: 't1', + __readScope: 'org', + __writeScope: 'org', + __delegatorReadScope: 'unit', + __delegatorWriteScope: 'unit', + __expandRead: true, + __referentialFieldClear: true, + }); + + expect(out).toEqual({ userId: 'u1', tenantId: 't1' }); + }); + + it('preserves every principal field — it strips, it does not project', () => { + // The defect half of the five-field projections this helper replaced + // (#7141 / #7145 / #7204): these decide the verdict the gate then trusts. + const envelope = { + userId: 'u1', + tenantId: 't1', + positions: ['p1'], + permissions: ['read'], + isSystem: false, + onBehalfOf: { userId: 'agent-owner' }, + principalKind: 'agent', + systemPermissions: ['x'], + accessible_org_ids: ['o1', 'o2'], + org_user_ids: ['u1'], + posture: 'group', + audience: 'api', + rlsMembership: { unit: 'u' }, + timezone: 'Asia/Shanghai', + __readScope: 'org', + }; + + const out = withoutOperationPrivateKeys(envelope) as Record; + + const { __readScope: _dropped, ...everythingElse } = envelope; + expect(out).toEqual(everythingElse); + }); + + it('returns a FRESH object even when there is nothing to strip', () => { + // ⛔ The copy is the point, not an optimisation to skip on a clean envelope: + // a callee that stamps its own `__writeScope` onto what it receives must not + // be able to write back into the caller's operation context. + const envelope = { userId: 'u1' }; + const out = withoutOperationPrivateKeys(envelope) as Record; + + expect(out).not.toBe(envelope); + expect(out).toEqual(envelope); + + out.__writeScope = 'org'; + expect(envelope).toEqual({ userId: 'u1' }); + }); + + it('is a SHALLOW copy — nested values are forwarded by reference', () => { + // Stated so the boundary is a decision rather than an accident: the hazard + // this closes is a callee stamping TOP-LEVEL keys, which is all the + // middleware ever does. + const nested = { userId: 'agent-owner' }; + const out = withoutOperationPrivateKeys({ onBehalfOf: nested }) as Record; + + expect(out.onBehalfOf).toBe(nested); + }); + + it('drops a key the middleware has not stamped yet, by prefix alone', () => { + // The whole reason the rule is a prefix and not a name list: a seventh + // operation-private key must be dropped by every consumer on the day it is + // stamped, with no consumer edited. + const out = withoutOperationPrivateKeys({ userId: 'u1', __someFutureMarker: true }); + + expect(out).toEqual({ userId: 'u1' }); + }); + + it('leaves keys that merely CONTAIN the prefix, and single-underscore keys', () => { + const out = withoutOperationPrivateKeys({ + _private: 1, + 'field__with__dunders': 2, + org_user_ids: ['o1'], + }); + + expect(out).toEqual({ _private: 1, 'field__with__dunders': 2, org_user_ids: ['o1'] }); + }); + + it('tolerates an empty envelope', () => { + expect(withoutOperationPrivateKeys({})).toEqual({}); + }); + + it('pins the prefix itself — consumers and the middleware agree on `__`', () => { + expect(OPERATION_PRIVATE_KEY_PREFIX).toBe('__'); + }); +}); diff --git a/packages/core/src/security/operation-private-keys.ts b/packages/core/src/security/operation-private-keys.ts new file mode 100644 index 0000000000..a249987ae9 --- /dev/null +++ b/packages/core/src/security/operation-private-keys.ts @@ -0,0 +1,122 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7284] The `__` operation-private-key convention — one owner, on the + * CONSUMER side. + * + * `assemble-execution-context.ts` next door is the single place an + * `ExecutionContext` is BUILT at a transport entry point (#6216). This file is + * its counterpart at the other end: the single place one is stripped back down + * before being forwarded to a question it was not resolved for. + * + * ## What a `__` key is + * + * plugin-security's middleware STAMPS keys onto the operation context, resolved + * for the object of the operation IN FLIGHT. They are middleware-private + * vocabulary, not fields of `ExecutionContext`, and every one of them is read as + * a WIDENING input by whoever consumes it: + * + * - the ADR-0057 D1 access DEPTH the sharing owner-match expands to — + * `__readScope` / `__writeScope`, plus the ADR-0090 D10 delegator halves + * `__delegatorReadScope` / `__delegatorWriteScope`, stamped in place by + * `security-plugin.ts` (`sc.__readScope = …`); + * - 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. + * + * plugin-security is the PRODUCER of that vocabulary and would be the most + * honest owner of the rule for consuming it, but none of the three consumers + * depends on it and a string-prefix filter does not justify three new dependency + * edges onto a plugin (the trade the filing card priced, #7284). `@objectstack/ + * spec` is fenced off by Prime Directive #2. `@objectstack/core` is the only + * candidate every consumer already depends on, so the rule lives here and the + * producer stays free of reverse edges. + * + * ## Why a consumer must drop them + * + * A caller's envelope carries a depth resolved for the object the middleware + * last saw. A consumer that forwards that envelope to ask about a DIFFERENT + * object applies one object's widening 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`). + * + * The leak is not hypothetical and does not require the consumer to be careless: + * plugin-security only OVERWRITES `__readScope` when it actually resolves + * permission sets for the new object (`if (permissionSets.length > 0)`), so a + * stale depth SURVIVES into a question it was never resolved for whenever that + * branch does not fire. A REST request that touched another object before + * reaching, say, `/reports/:id/run` hands over an envelope the middleware has + * already written into. + * + * Dropping is safe in the one direction that matters: the middleware re-stamps + * the depth for THIS object when it resolves any set, so the only thing dropping + * can do is leave the sharing owner-match at its narrowest (`own`) — the safe + * direction. + * + * ## Why by PREFIX and never by a name list + * + * The `__` convention is what marks a key as belonging to the operation in + * flight. A hand-maintained list of the six names above would go stale the day + * the middleware stamps a seventh, and it would go stale SILENTLY — a forwarded + * key nobody remembered to add reads exactly like a key that was meant to be + * forwarded. The prefix is the contract; the names are its current membership. + * + * ⛔ The corollary, for whoever changes the middleware: a key that is + * operation-private MUST carry the `__` prefix. Stamping one without it makes it + * invisible to every consumer at once, and there is no compiler error. + * + * ## Known consumers + * + * `plugin-audit` (`comment-access-hooks.ts`, #7141), `service-storage` + * (`attachment-access-hooks.ts`, #7145) and `plugin-reports` + * (`report-service.ts`, #7204) — each forwarding a caller envelope to a gate or + * a read that asks about a parent/target object rather than about the object the + * middleware resolved for. Each of the three grew its own byte-equivalent copy + * of this file by hand before #7284 gave the rule a home; `operation-private- + * keys.pin.test.ts` is what now catches a fourth. + */ + +import type { ExecutionContext } from '@objectstack/spec/kernel'; + +/** + * The prefix marking a key as private to the operation plugin-security has in + * flight. See this module's header for why the convention is a prefix and not a + * list of names. + */ +export const OPERATION_PRIVATE_KEY_PREFIX = '__'; + +/** + * The caller's execution envelope, minus the operation-private keys. + * + * A FRESH object every time, and that is load-bearing in BOTH directions: + * + * - outbound — 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 the caller + * was handed; + * - inbound — the engine's middleware stamps a depth for the object it is about + * to read onto whatever it is handed, so forwarding a caller's envelope BY + * REFERENCE would write that depth back into the request context the route + * goes on using. + * + * ⛔ Never `return exec;` on the "nothing to strip" path. The copy is the point, + * not an optimisation to skip when the envelope happens to be clean — the two + * hazards above are about the callee's future writes, not about the current + * contents. + * + * Note what this deliberately does NOT do: it strips, it never SYNTHESISES a + * depth for the new object. Absent depth leaves the sharing owner-match at its + * narrowest (`own`), which is the safe direction and byte-for-byte what the + * five-field projections these call sites replaced produced. + * + * @param exec the caller's envelope, as a bare record + * @returns a new envelope carrying only the non-operation-private keys + */ +export 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; +} diff --git a/packages/plugins/plugin-audit/src/comment-access-hooks.ts b/packages/plugins/plugin-audit/src/comment-access-hooks.ts index e63462f638..2d219c7403 100644 --- a/packages/plugins/plugin-audit/src/comment-access-hooks.ts +++ b/packages/plugins/plugin-audit/src/comment-access-hooks.ts @@ -55,6 +55,7 @@ * load-bearing. */ +import { withoutOperationPrivateKeys } from '@objectstack/core'; import type { ISharingService } from '@objectstack/spec/contracts'; import type { ExecutionContext } from '@objectstack/spec/kernel'; @@ -181,45 +182,22 @@ function asIdList(id: unknown): Array | null { } /** - * Keys plugin-security's middleware STAMPS onto the operation context, resolved - * for the object of the CURRENT operation — `sys_comment` here. + * Why this module strips the operation-private keys before forwarding an + * envelope — the LOCAL half of the argument. * - * 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`, `security-plugin.ts` — `sc.__readScope = …`), - * 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). + * plugin-security's middleware stamps `__`-prefixed keys onto the operation + * context resolved for the object of the CURRENT operation, which here is + * `sys_comment`. Every gate in this module asks about the PARENT record's + * object, never about `sys_comment`, so carrying any of them across is one + * object's widening applied to another object's question. * - * Every gate in this module asks about the PARENT record's object, never about - * `sys_comment`, 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. + * [#7141] The general rule — which keys those are, why they are dropped by + * PREFIX rather than by a name list, and why the copy is load-bearing in both + * directions — is `withoutOperationPrivateKeys` in `@objectstack/core`. It was + * hand-copied into this file, `service-storage`'s attachment kit and + * `plugin-reports` before #7284 gave it one owner; ⛔ import it, never re-derive + * it locally (`operation-private-keys.pin.test.ts` catches the fourth copy). */ -const OPERATION_PRIVATE_KEY_PREFIX = '__'; - -/** - * The caller's execution envelope, minus the operation-private keys above. - * - * [#7141] 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. diff --git a/packages/plugins/plugin-reports/src/report-service.ts b/packages/plugins/plugin-reports/src/report-service.ts index effb28986e..3dd5e53d44 100644 --- a/packages/plugins/plugin-reports/src/report-service.ts +++ b/packages/plugins/plugin-reports/src/report-service.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { withoutOperationPrivateKeys } from '@objectstack/core'; import type { IReportService, SavedReport, @@ -177,47 +178,25 @@ function renderSubject(template: string | undefined, vars: Record 0)`). A stale depth therefore survives into a - * question it was never resolved for. 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 another - * one. Same shape as `plugin-audit`'s and `service-storage`'s kits (#7141 / - * #7145). - */ -const OPERATION_PRIVATE_KEY_PREFIX = '__'; - -/** - * The caller's execution envelope, minus the operation-private keys above. + * object the caller's envelope last carried a `__`-prefixed depth for: a REST + * request that touched another object before reaching `/reports/:id/run` hands + * over an envelope plugin-security's middleware has already written into, and + * that middleware only OVERWRITES `__readScope` when it resolves permission sets + * for the new object (`if (permissionSets.length > 0)`). A stale depth therefore + * survives into a question it was never resolved for. * - * A FRESH object every time, in both directions: the engine's middleware - * stamps `__readScope` for `report.object_name` onto whatever it is handed, so - * forwarding the caller's own envelope by reference would write that depth - * back into the REQUEST context the route goes on using. + * [#7204] The general rule — which keys those are, why they are dropped by + * PREFIX rather than by a name list, and why the copy is load-bearing in both + * directions — is `withoutOperationPrivateKeys` in `@objectstack/core`. It was + * hand-copied into this file, `plugin-audit`'s comment kit and + * `service-storage`'s attachment kit before #7284 gave it one owner; ⛔ import + * it, never re-derive it locally (`operation-private-keys.pin.test.ts` catches + * the fourth copy). */ -function withoutOperationPrivateKeys(exec: Record): Record { - const out: Record = {}; - for (const [key, value] of Object.entries(exec)) { - if (key.startsWith(OPERATION_PRIVATE_KEY_PREFIX)) continue; - out[key] = value; - } - return out; -} // ─── Service ────────────────────────────────────────────────────── diff --git a/packages/services/service-storage/src/attachment-access-hooks.ts b/packages/services/service-storage/src/attachment-access-hooks.ts index 44c8043f95..8e57f0d885 100644 --- a/packages/services/service-storage/src/attachment-access-hooks.ts +++ b/packages/services/service-storage/src/attachment-access-hooks.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { withoutOperationPrivateKeys } from '@objectstack/core'; import type { ExecutionContext } from '@objectstack/spec/kernel'; import type { @@ -69,45 +70,22 @@ function asIdList(id: unknown): Array | null { } /** - * Keys plugin-security's middleware STAMPS onto the operation context, resolved - * for the object of the CURRENT operation — `sys_attachment` here. + * Why this module strips the operation-private keys before forwarding an + * envelope — the LOCAL half of the argument. * - * 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). + * plugin-security's middleware stamps `__`-prefixed keys onto the operation + * context resolved for the object of the CURRENT operation, which here is + * `sys_attachment`. Every gate in this module asks about the PARENT record's + * object, never about `sys_attachment`, so carrying any of them across is one + * object's widening applied to another object's question. * - * 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. + * [#7145] The general rule — which keys those are, why they are dropped by + * PREFIX rather than by a name list, and why the copy is load-bearing in both + * directions — is `withoutOperationPrivateKeys` in `@objectstack/core`. It was + * hand-copied into this file, `plugin-audit`'s comment kit and `plugin-reports` + * before #7284 gave it one owner; ⛔ import it, never re-derive it locally + * (`operation-private-keys.pin.test.ts` catches the fourth copy). */ -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.