From 944066ed18f0afbf3cea1c016daf410d010b4a4c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Jul 2026 15:05:24 +0000 Subject: [PATCH 1/3] =?UTF-8?q?docs(adr):=20ADR-0096=20=E2=80=94=20executi?= =?UTF-8?q?on-surface=20identity=20admission?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalizes #2849 from an action-shaped bug to its class: metadata-driven execution surfaces that reach the data engine without an ExecutionContext silently inherit full authority via the security plugin's empty-principal fall-open (#2308 and #2849 are two instances found only by adversarial review). Decides one invariant and four mechanisms: every engine call carries a real principal or an explicit audited SystemGrant (D1); a single elevation constructor systemContext(reason) (D2); identity-required engine signatures so forgetting context is a compile error, not a fall-open (D3); an execution-surface admission test wired into the authz conformance matrix with a CI meta-test (D4); and a staged fail-closed strict mode (D5). Re-bases #2849 Phase 2/3 as D6: action-level runAs adopts ADR-0073's three-posture vocabulary and lands as this mechanism's first application. Staged per ADR-0049: v1 is the decision record + systemContext + honest matrix rows + the ai.exposed/trusted-body lint; signature migration and the strict-mode default flip are evidence-gated M2+. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0155bjzJZxZaZTKSMSvg6Aum --- ...96-execution-surface-identity-admission.md | 220 ++++++++++++++++++ 1 file changed, 220 insertions(+) create mode 100644 docs/adr/0096-execution-surface-identity-admission.md diff --git a/docs/adr/0096-execution-surface-identity-admission.md b/docs/adr/0096-execution-surface-identity-admission.md new file mode 100644 index 0000000000..a17db9cafa --- /dev/null +++ b/docs/adr/0096-execution-surface-identity-admission.md @@ -0,0 +1,220 @@ +# ADR-0096: Execution-Surface Identity Admission — no data-engine call without an explicit principal or an explicit, audited elevation + +**Status**: Proposed (2026-07-15) +**Deciders**: ObjectStack Protocol Architects +**Builds on**: [ADR-0049](./0049-no-unenforced-security-properties.md) (enforce-or-remove gate; stage by whether the feature exists), [ADR-0056](./0056-permission-model-landing-verification.md) D10 (authz conformance matrix — the durable-encoding idiom this ADR extends), [ADR-0066](./0066-unified-authorization-model.md) (capability gate; action `requiredPermissions`), [ADR-0073](./0073-automation-execution-identity.md) (`runAs` as authorization **posture** — `user` / `automation` / `system`; no anonymous run; M2 gated on the first real consumer), [ADR-0088](./0088-metadata-kind-admission-and-retirement.md) (the admission-test idiom, at the kind level), [ADR-0090](./0090-permission-model-v2-concept-convergence.md) D10 (agent ceiling∩user intersection), [ADR-0095](./0095-authz-kernel-tenant-layer-and-posture-ladder.md) (layered authz kernel) +**Consumers**: `@objectstack/objectql` (engine call contract), `@objectstack/runtime` (action/facade dispatch, MCP bridge), `@objectstack/plugin-security` (the empty-principal seam), `@objectstack/service-automation`, trigger packages, `@objectstack/dogfood` (conformance matrix + meta-test), every future package that adds an execution surface +**Tracking**: framework#2849 (the trigger instance; its Phase 2/3 become applications of this mechanism) + +**Premise**: pre-launch (ADR-0049 idiom) — specify the end-state invariant now, land only the non-speculative slice, and gate every runtime phase on evidence (audit telemetry), not on dates. + +> **Trigger**: #2849 (fixed in #2964 at the invoke-time boundary) found that business-action bodies dispatch through an engine facade whose `insert/update/delete/find` carry **no `ExecutionContext`**. A context-less call hits the SecurityPlugin's empty-principal skip and runs with **ambient full authority** — no RLS, no FLS, no CRUD, no tenant scoping, no ADR-0090 D10 agent intersection. #2308 had already found the *same shape* on schedule-triggered flows (`runAs:'user'` with no user → UNSCOPED). Two independent surfaces, one root cause. This ADR is about the class, not the instances. + +--- + +## TL;DR + +Today the platform's data-security middleware **fails open on missing identity**: a call that carries no positions, no permission sets, and no `userId` skips every check. That was a deliberate auth-layering choice, but it has a structural consequence: **every new execution surface that forgets to thread the caller's context silently becomes a privilege-escalation primitive** — no error, no failing test, nothing until an adversarial review finds it (#2308, #2849). As the metadata platform grows surfaces (action bodies, flows, triggers, AI tools, scheduled jobs, import pipelines, custom endpoints), the number of chances to re-introduce this hole grows with it. + +This ADR inverts the default with one invariant and four mechanisms: + +> **The invariant (D1)**: every data-engine invocation carries either a **real principal** (`ExecutionContext`) or an **explicit, reasoned, audited system grant**. "No context" is a *defect*, never an *authorization*. + +1. **D2 — One elevation door**: a single typed constructor (`systemContext(reason)`) becomes the only sanctioned way to run elevated. Elevation becomes grep-able, reasoned, and audited — never ambient. +2. **D3 — The compiler enforces threading**: engine entry points take a **required** identity argument (`ExecutionContext | SystemGrant`). Forgetting to pass context stops being a silent fall-open and becomes a compile error. +3. **D4 — Execution-surface admission test**: the ADR-0088 idiom applied to *surfaces* instead of *kinds*. A surface that can reach the data engine earns admission only by declaring its identity posture (ADR-0073 vocabulary), registering in the authz conformance matrix with an enforcement site, and passing strict-mode CI. A meta-test fails CI on unregistered surfaces. +4. **D5 — Strict mode, staged to fail-closed**: a mode that *denies* non-system context-less calls — ON in CI/dogfood first to surface every violator, default-ON at a major once telemetry says the migration is done. + +**Also decided (D6)**: action-level `runAs` (the #2849 Phase 2/3 plan) adopts ADR-0073's three-posture vocabulary (`user` / `automation` / `system`) — not a private two-value enum — and lands as the first *application* of this mechanism, sequenced **after** D2/D3 exist. + +**Rejected**: keep trusted-by-default and rely on review vigilance; big-bang context threading (#2849 option b); per-surface bespoke fixes; DB-native SECURITY DEFINER roles. See Alternatives. + +--- + +## Context + +### The failure class, precisely + +The SecurityPlugin's find/write middleware contains this seam (`plugin-security/src/security-plugin.ts`, the empty-principal skip): + +```ts +// Skip security checks if no positions AND no explicit permission sets +// AND no userId (anonymous/unauthenticated). The auth middleware +// should handle authentication separately. +if (positions.length === 0 && explicitPermissionSets.length === 0 && !opCtx.context?.userId) { + return next(); +} +``` + +The comment states the original intent: *authentication* is the auth layer's job, so an identity-less call is presumed to be internal plumbing. That presumption made sense when the kernel had two callers (REST with auth, boot with `isSystem`). It stops being safe the moment the platform multiplies **metadata-driven execution surfaces** — places where *authored artifacts* (not platform code) cause engine calls: + +| Surface | Identity story today | How we learned | +|:---|:---|:---| +| REST / MCP object CRUD | caller's `ExecutionContext`, threaded | by design | +| Business-action `script`/`body` | **none** — facade drops context → fall-open | #2849 (adversarial review) | +| Flow (`runAs:'user'`, user-less trigger) | none → fall-open, warned | #2308 (adversarial review) | +| Flow (`runAs:'system'`) | explicit elevation | ADR-0049/#1888, by design | +| Record-change / schedule triggers | threaded (after #1888 fixes) | bug class, then fixed | +| Boot / seeding / internal jobs | `{ isSystem: true }` literals, scattered | by design, unaudited | + +The pattern is exact: **every row that says "adversarial review" is a place where an engineer forgot an optional parameter and the type system, the tests, and the runtime all stayed silent.** The two rows found so far were found by *humans doing security reviews*. Nothing structural prevents the next surface — an import pipeline, a webhook handler, the next AI tool family — from re-adding the same hole, because: + +1. The engine accepts context-lessness (optional parameter — nothing to forget *loudly*). +2. The security layer rewards context-lessness with full authority (fall-open). +3. No registry knows which surfaces exist, so no test can enumerate what to check. + +ADR-0049's principle ("spec must not declare security properties the runtime does not enforce") has a runtime sibling that this ADR names: **the runtime must not grant authority that no one explicitly requested.** The #2845 claim ("a `data:read` agent's action writes are blocked at the write") was exactly an ADR-0049 violation born from this seam — a security property everyone *believed* held, that nothing enforced. + +### Why the fall-open is load-bearing (and why we can't just delete it) + +Boot, seeding, migrations, and several internal services legitimately run without a human principal. Today some use explicit `{ isSystem: true }`; an unknown number ride the fall-open instead (that unknown-ness is itself the problem). Deleting the skip outright would fail-closed legitimate system operations at boot — the platform wouldn't start. So the inversion must be **staged**: first make explicit elevation *available and cheap* (D2), then make implicit context *impossible to forget* (D3), then measure what still hits the seam (audit, landed in #2964 for actions), and only then flip the seam itself (D5). + +### Relationship to ADR-0073 + +ADR-0073 answered the **identity** question for user-less *automation* runs: there is no anonymous run; `runAs` declares posture (`user` / `automation` / `system`); attribution is always concrete. It deliberately built almost nothing (M2 gated on the first real consumer). This ADR is the **engine-boundary** complement: 0073 says *what identity a run has*; 0096 says *the engine refuses to move without one*. The two compose: once D2/D3 exist, ADR-0073's postures become the values that flow through the required parameter, and action-level `runAs` (D6) becomes a candidate "first real consumer" that un-gates 0073's M2 — that call is made in 0073's terms when D6 lands, not pre-empted here. + +--- + +## Decisions + +### D1 — The invariant: identity or explicit grant, never absence + +Every invocation of the data engine (`IDataEngine` / ObjectQL find, insert, update, delete, executeAction, and any future verb) MUST carry exactly one of: + +- a **principal-bearing `ExecutionContext`** — a human, an agent (with the ADR-0090 D10 intersection), or the environment's automation principal (ADR-0073); or +- an **explicit `SystemGrant`** — a value that can only be produced by the D2 constructor, carrying a mandatory reason. + +A call carrying neither is a **defect**. The runtime's job is to make that defect: (a) impossible to write in first-party TypeScript (D3), (b) visible when it happens anyway — dynamic call sites, JS consumers, legacy paths (audit + D5 strict mode), and (c) eventually fatal (D5 default flip). + +This is Prime Directive #10 ("never declare what the platform does not deliver") read in the mirror: **never deliver what nobody declared.** + +### D2 — One elevation door: `systemContext(reason)` + +A single typed constructor in the engine's contract package: + +```ts +// The ONLY sanctioned way to run elevated. `reason` is mandatory, +// machine-greppable, and lands in the audit stream on every use. +function systemContext(reason: SystemReason): SystemGrant; +// SystemReason examples: 'boot:seed-permission-sets', 'action:convert_lead:trusted-body', +// 'migration:2026-07-backfill', 'trigger:schedule:runAs-system' +``` + +Rules: + +1. **`SystemGrant` is unforgeable in practice**: a branded type whose constructor lives in one module. Ad-hoc `{ isSystem: true }` object literals at call sites are retired by lint + migration; the security layer keeps honoring the wire-level `isSystem` flag during transition but the *authoring* path is the constructor. +2. **`reason` is not optional and not free-form-empty.** A namespaced string (`area:detail`) so audit queries can aggregate ("show me every elevation by the action subsystem this week"). +3. **Every construction is audit-logged** at use (extending the #2964 audit events from the action facades to all elevation). +4. **Consequence for review**: "list all elevation points" becomes `grep systemContext(` — the property ADR-0056 D10 gave the permission model ("a CHECKED artifact, not a one-time scan"), extended to elevation. + +### D3 — The compiler enforces threading: required identity parameter + +The engine's public entry points change from *context-optional* to *identity-required*: + +```ts +// Before (today) — forgetting `options.context` compiles, runs, and falls open: +find(object: string, options?: QueryOptions): Promise; + +// After — the identity is a first-class required argument: +find(object: string, identity: CallIdentity, options?: QueryOptions): Promise; +// CallIdentity = ExecutionContext | SystemGrant +``` + +Staging (this is a wide but mechanical change): + +- **Additive first**: introduce the identity-required signatures alongside the existing ones (new names or overloads), migrate first-party call sites package-by-package, with the D5 strict-mode CI proving each package clean as it converts. +- **Lint bridge**: until the old signatures are removed, an ESLint rule flags context-less calls to the legacy entry points (same idiom as the #2308 `flow-schedule-runas-unscoped` lint). +- **Major**: remove the legacy signatures. From then on the #2849 facade bug is *unwritable* — `buildActionEngineFacade` could not have compiled without deciding, visibly, whose authority it was using. + +D3 is what turns D1 from a policy into a property. Reviews catch instances; types remove the class. + +### D4 — Execution-surface admission test (the ADR-0088 idiom, one level up) + +ADR-0088 defined an admission test for metadata *kinds*; ADR-0085 §2 for authored *keys*. This ADR defines one for **execution surfaces** — any code path through which an authored artifact or external caller causes data-engine calls (today: REST/MCP CRUD, business actions, flows, triggers, scheduled jobs; tomorrow: import pipelines, webhooks, new AI tool families). + +A surface earns admission only if ALL three hold: + +1. **Declared posture.** The surface documents which ADR-0073 postures it can run under (`user` / `automation` / `system`), what selects the posture (author metadata like `runAs`, caller identity, or fixed), and what its *default* is. "Trusted, always" is an admissible answer — but it must be *written down and gated* (as #2964 did for actions: trusted body, therefore `ai.exposed` + capability gate at invoke time). +2. **Conformance registration.** One row per surface in the authz conformance matrix (`dogfood/test/authz-conformance.matrix.ts`, extending the ADR-0056 D10 structure with a `surface` category): its enforcement site, its posture declaration, and — for surfaces reachable by agents or external callers — a dogfood proof exercising the boundary. +3. **Strict-mode clean.** The surface's test suite passes with D5 strict mode ON: every engine call it makes carries identity or an explicit grant. + +**The meta-test**: a conformance test enumerates engine-reaching surfaces (mechanically: the modules invoking the identity-required entry points from D3, plus a maintained allowlist during transition) and **fails CI if a surface is not registered**. This is the piece that protects the *future*: the next engineer adding a surface gets a red build with a checklist, not a silent fall-open and an adversarial-review finding two months later. + +### D5 — Strict mode: fail-closed, staged by evidence + +A kernel-level mode (`security.identityStrict`): + +- **OFF (today's behavior)**: empty-principal calls skip checks; the #2964 audit events record them. +- **ON**: a non-`SystemGrant`, principal-less engine call is **denied** (`PermissionDeniedError`), same fail-closed posture as the permission-resolution-failure path that already exists in the security plugin. + +Rollout is gated on evidence, not dates (ADR-0049/0073 idiom): + +1. **Now**: ON in the dogfood suite and CI for packages already migrated to D3 signatures. Every red test is a real finding — either a missing thread (fix: pass the caller) or a legitimate elevation (fix: `systemContext(reason)`). +2. **Telemetry gate**: the audit stream (landed for actions in #2964, extended by D2 to all elevation) answers "how many distinct call sites still hit the fall-open in real deployments, and which reasons dominate?" — the same question #2849's Phase-3 plan needed answered before flipping action defaults. +3. **Major**: default ON. The empty-principal skip in `security-plugin.ts` keeps exactly one behavior: honoring `SystemGrant`/`isSystem`. Anonymous-but-authenticated-elsewhere flows must arrive with a principal by then (they already should — REST's `enforceAuth` is upstream). + +### D6 — Action-level `runAs` is an application of this mechanism, in ADR-0073's vocabulary + +#2849's Phase 2/3 (give business actions a `runAs`, eventually default bounded) is **confirmed as direction but re-sequenced and re-based**: + +1. **Vocabulary**: actions adopt ADR-0073's posture enum — `runAs?: 'user' | 'automation' | 'system'` — not a private two-value flavor. A trusted action body is `runAs:'system'`; the safe middle (`automation`: RLS-enforced against the automation principal's grants) becomes available to actions the moment ADR-0073 M2 seeds the principal. +2. **Sequencing**: D6 lands **after** D2/D3 reach the action facades, because "run the body as the caller" *is* "thread `CallIdentity` through the facade" — building it before the identity-required signatures exist would hand-roll exactly the plumbing D3 makes universal. +3. **Transitional default**: `'system'` (today's behavior, now explicit), with the authoring lint from #2849 Phase 2 (`ai.exposed:true` + `runAs:'system'` ⇒ warn: you are handing AI a trusted body). The default flip to a bounded posture is a candidate for the same major as D5's flip, decided on the same telemetry. +4. **Agent semantics**: when an action runs `runAs:'user'` and the caller is an agent, the threaded context is the ADR-0090 D10 **ceiling∩user** intersection — the honest version of the #2845 claim, delivered by construction instead of asserted by documentation. + +--- + +## What lands now (the non-speculative slice) + +Per ADR-0049's staging discipline, v1 of this ADR builds only what has a consumer today: + +1. **This decision record** — the invariant, the admission test, the staging plan. +2. **`systemContext(reason)` + `SystemGrant`** in the engine contract package, with audit emission; migrate the *known* elevation literals (boot/seeding, the action facades' trusted dispatch, report scheduler's `SYSTEM_CTX`) as the proof-of-idiom. +3. **Matrix rows** for today's surfaces (REST/MCP CRUD, actions, flows, triggers) in their *honest current states* — actions register as `surface: trusted-body, gated at invoke (ai.exposed + capability)` per #2964 — plus the meta-test skeleton with the transition allowlist. +4. **The authoring lint** from D6.3 (`ai.exposed` + trusted body ⇒ warn). + +Everything else — the D3 signature migration, strict-mode-ON-in-CI per package, the D5 default flip, D6's `runAs` field — is explicitly **M2+, gated on the prior step's evidence**, tracked as separate issues under #2849. + +--- + +## Alternatives considered + +**A1 — Keep trusted-by-default; rely on review vigilance + documentation.** This is the do-nothing baseline, and #2308/#2849 are its track record: two independent fall-opens found by *adversarial reviews*, not by any structural control. Review vigilance does not scale with surface count; the platform's core promise (metadata a business can trust AI to operate — see the repo's own description) makes "we document that it's unbounded" a non-answer. Rejected. + +**A2 — Big-bang context threading (#2849 option b).** Thread the caller's context through every facade now, no `runAs`, no staging. Correct end-state for *bounded* surfaces, but it breaks every action legitimately relying on trusted cross-object writes (convert-lead, cascade-close), conflates two decisions (the engine contract vs. each surface's default posture), and — without D2 — leaves no sanctioned way to elevate, so trusted logic would reinvent `{ isSystem: true }` scatter. Rejected as a single step; its substance arrives as D3 + per-surface posture decisions. + +**A3 — Per-surface bespoke fixes.** Fix actions with an action-shaped patch (#2849 Phase 2/3 as originally scoped), flows with a flow-shaped lint (#2308), the next surface with the next patch. Treats symptoms serially; the class remains open, and each fix hand-rolls its own identity plumbing. Rejected — this ADR exists precisely because the third instance of this bug should be a compile error, not a third review finding. + +**A4 — Database-native enforcement (Postgres RLS + SECURITY DEFINER-style per-surface roles).** Push the invariant into the DB so even a buggy engine can't fall open. Same shape as ADR-0095's rejected Postgres-native Layer-0 backstop, same verdict: the platform is DB-portable by contract, the engine boundary is where every driver converges, and a DB-level twin of the posture model would be a second implementation to keep in lock-step. Not scheduled; D5's strict mode delivers the fail-closed property at the portable boundary. + +--- + +## Consequences + +**Positive** +- The #2849/#2308 bug class becomes structurally unwritable (D3) instead of review-detectable; the next execution surface gets a red CI checklist (D4) instead of a silent escalation path. +- Elevation becomes enumerable (`grep systemContext(`), reasoned, and audited — security reviews audit a list, not a codebase. +- ADR-0073's posture model gets its engine-side enforcement and a concrete first consumer path (D6); the #2845-style gap between claimed and enforced security properties is closed at the layer that generates such gaps. +- #2849's Phase 2/3 stop being a bespoke action project and become the worked example of a platform mechanism. + +**Negative / costs** +- A wide (if mechanical) signature migration (D3) touching every engine call site, spread across a major cycle; transition period carries dual signatures + lint noise. +- The admission test adds real friction to shipping a new surface — one matrix row, one posture paragraph, one strict-mode-clean suite. This is the point, but it is a tax, and PRIORITIZATION review should treat matrix-row PRs as cheap to approve. +- The fall-open seam must keep working (for `SystemGrant`) forever; the invariant's honesty depends on the D2 constructor remaining the *only* producer — a lint + module-boundary discipline, not a cryptographic guarantee. + +**Explicitly unchanged** +- The trusted-action model itself: an author may still ship a `runAs:'system'` body; it is now explicit, gated (for AI, by `ai.exposed` per #2964), reasoned, and audited — not abolished. +- ADR-0073's M2 gating: this ADR does not force the automation principal into existence; it defines the contract that will carry it. +- The auth layer's ownership of *authentication*; this ADR governs *authorization at the engine boundary* only. + +--- + +## References + +- framework#2849 — the trigger instance (action bodies context-less); its comment thread contains the option analysis this ADR generalizes; Phases 2/3 re-based onto D6 +- framework#2964 — the landed invoke-time hardening (ai.exposed gate, flow context forwarding, trusted-dispatch audit) — v1's audit substrate +- framework#2308 / #1888 — the flow instance of the same class; the lint idiom D3/D6 reuse +- framework#2845 / #2843 — the agent action surface whose safety framing this ADR makes true by construction +- `packages/plugins/plugin-security/src/security-plugin.ts` — the empty-principal seam (D5's target) +- `packages/runtime/src/http-dispatcher.ts` — `buildActionEngineFacade` (both facades), `invokeBusinessAction` +- `packages/dogfood/test/authz-conformance.matrix.ts` — the ADR-0056 D10 matrix D4 extends From 1728a5a6a99630e0163a727cc81c929abc015b30 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Jul 2026 15:29:17 +0000 Subject: [PATCH 2/3] =?UTF-8?q?docs(adr):=20ADR-0096=20=E2=80=94=20add=20e?= =?UTF-8?q?numerated=20evidence=20+=20D4=20matrix=20seed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A four-axis sweep (context-less calls, elevation literals, fall-open seams, execution-surface identity) confirms #2849 is a populated class, not a lone bug. Records the findings as the ADR's evidence base: - E1: the empty-principal fall-open predicate replicated across getReadFilter, the tenant-scope path, guest routes, custom API endpoints, the authored-body sandbox, MCP resource reads, and messaging. - E2: three confirmed-exploitable trusted-implicit surfaces, spun out as #2980 (reports IDOR + scheduled-report RLS bypass), #2981 (knowledge/RAG retrieval fall-open), #2982 (bulk-write OWD gap) — fixed independently. - E3: structural middleware-bypass paths (executeAction, raw execute, driver getters). - E4: unknown seams (GraphQL, realtime, blob RLS, chatWithTools contract). - E5: the D2 migration sizing (~300 sites / ~50 constructors / ~25 files) and its existing prototype (resolveRunDataContext). Wires the seed rows into D4, the exploitable instances into 'what lands now', and the References/Trigger notes. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0155bjzJZxZaZTKSMSvg6Aum --- ...96-execution-surface-identity-admission.md | 60 ++++++++++++++++++- 1 file changed, 57 insertions(+), 3 deletions(-) diff --git a/docs/adr/0096-execution-surface-identity-admission.md b/docs/adr/0096-execution-surface-identity-admission.md index a17db9cafa..00f62f6ccc 100644 --- a/docs/adr/0096-execution-surface-identity-admission.md +++ b/docs/adr/0096-execution-surface-identity-admission.md @@ -8,7 +8,7 @@ **Premise**: pre-launch (ADR-0049 idiom) — specify the end-state invariant now, land only the non-speculative slice, and gate every runtime phase on evidence (audit telemetry), not on dates. -> **Trigger**: #2849 (fixed in #2964 at the invoke-time boundary) found that business-action bodies dispatch through an engine facade whose `insert/update/delete/find` carry **no `ExecutionContext`**. A context-less call hits the SecurityPlugin's empty-principal skip and runs with **ambient full authority** — no RLS, no FLS, no CRUD, no tenant scoping, no ADR-0090 D10 agent intersection. #2308 had already found the *same shape* on schedule-triggered flows (`runAs:'user'` with no user → UNSCOPED). Two independent surfaces, one root cause. This ADR is about the class, not the instances. +> **Trigger**: #2849 (fixed in #2964 at the invoke-time boundary) found that business-action bodies dispatch through an engine facade whose `insert/update/delete/find` carry **no `ExecutionContext`**. A context-less call hits the SecurityPlugin's empty-principal skip and runs with **ambient full authority** — no RLS, no FLS, no CRUD, no tenant scoping, no ADR-0090 D10 agent intersection. #2308 had already found the *same shape* on schedule-triggered flows (`runAs:'user'` with no user → UNSCOPED). Two independent surfaces, one root cause. A subsequent four-axis sweep (see **Evidence**) confirmed the class is broadly populated — a replicated fall-open predicate, several trusted-implicit surfaces (three confirmed exploitable: #2980, #2981, #2982), and structural middleware-bypass paths. This ADR is about the class, not the instances. --- @@ -140,6 +140,8 @@ A surface earns admission only if ALL three hold: **The meta-test**: a conformance test enumerates engine-reaching surfaces (mechanically: the modules invoking the identity-required entry points from D3, plus a maintained allowlist during transition) and **fails CI if a surface is not registered**. This is the piece that protects the *future*: the next engineer adding a surface gets a red build with a checklist, not a silent fall-open and an adversarial-review finding two months later. +The **Evidence** section below enumerates the surfaces found by the initial sweep — those become the matrix's seed rows in their honest current states (BOUNDED / TRUSTED-EXPLICIT / TRUSTED-IMPLICIT / UNKNOWN). + ### D5 — Strict mode: fail-closed, staged by evidence A kernel-level mode (`security.identityStrict`): @@ -164,15 +166,66 @@ Rollout is gated on evidence, not dates (ADR-0049/0073 idiom): --- +## Evidence: the class, enumerated + +The invariant above is not motivated by a single bug. A four-axis sweep of every data-engine-reaching path (context-less calls, elevation literals, fall-open seams, execution-surface identity) found the class is already populated — **two instances were previously found by adversarial review; the sweep found the rest sitting in the open.** This is the concrete case for a *mechanism* over per-surface patches, and it seeds the D4 matrix. Instances are grouped by how they relate to the invariant. + +### E1 — Missing-identity fall-open (the #2849 predicate, replicated) + +All trace to the same `positions==0 && permissions==0 && !userId → skip` predicate; fixing #2849 at one site does **not** close the others. + +| Site | Surface | Note | +|:---|:---|:---| +| `plugin-security/security-plugin.ts:626` | find/write middleware | the original seam (#2849) | +| `plugin-security/security-plugin.ts:1689` | `getReadFilter` (analytics / reports / raw-SQL RLS compile) | returns `undefined` = *no filter*; the analytics mirror of :626 | +| `objectql/engine.ts` Layer-0 + `driver-sql` `applyTenantScope` (opt-in on `tenantId`) | tenant scoping | Layer-0 is computed **after** the :626 skip, and the driver scope is opt-in → a principal-less context gets **no tenant filter at either layer** (cross-tenant read/write) | +| `rest/rest-server.ts:4317` (+ lookup `:4744`) | guest / public-form routes | bypass `enforceAuth`; fall open when `guest_portal` unregistered (partly mitigated by the `publicFormGrant`) | +| `runtime/http-dispatcher.ts:4231,4236,4240` | custom `object_operation` API endpoints | **accidental** — `callData` invoked with no `executionContext` (every sibling threads it) | +| `runtime/sandbox/body-runner.ts:155` | authored action/hook **body** interior facade | the inside of #2849 — only the *invoke* is gated (#2964); the body's `api.object().find/insert/...` run context-less | +| `mcp/mcp-server-runtime.ts:335` | MCP resource `…/records/{id}` | record read with no session identity | +| `service-messaging/messaging-service.ts` (+ recipient resolver) | notification read/write + recipient-role lookup | unscoped reads of `sys_notification*` and `sys_user`/`sys_member` | +| minor: `http-dispatcher.ts:1322` (env-member), `plugin-webhooks/auto-enqueuer.ts:175`, `objectql/engine.ts:722` (`visibleWhen`) | system plumbing | rely on the *implicit* fall-open; should carry an **explicit** grant so they survive the D5 flip | + +### E2 — Trusted-implicit surfaces (ignore the caller / fall back to system) — confirmed exploitable, spun out + +| Instance | Verdict | Issue | +|:---|:---|:---| +| Reports `getReport`/`deleteReport`/`listReports` discard the caller and query with `SYSTEM_CTX`; routes only `enforceAuth` → cross-user/cross-tenant IDOR (read+delete any report) | CONFIRMED exploitable | #2980 | +| Scheduled reports (`report-service.ts:425` `dispatchDue`) run `executeReport(..., {isSystem:true})` → a member-owned schedule emails the target object's **entire** table, RLS bypassed | CONFIRMED exploitable | #2980 | +| Knowledge/RAG `applyPermissionFilter` (`knowledge-service.ts:312`) returns **all** hits when `ctx` is missing/system; `chatWithTools`'s `ToolExecutionContext.actor` is optional with a system fallback → agent retrieval escapes the data ceiling | CONFIRMED (framework); exposure gated on cloud impl | #2981 | + +These are *not* the :626 fall-open (they use an unconditional `SYSTEM_CTX`), but they are exactly what a D4 conformance row (`caller-scoped?` proof) + the D2 audit would have flagged. Fixed independently of the mechanism, tracked as the mechanism's motivating evidence. + +### E3 — Structural: paths that never enter the security middleware (Class B) + +| Site | Note | +|:---|:---| +| `objectql/engine.ts:641` `executeAction` | handler invoked **directly**, not via `executeWithMiddleware` — an action touching the driver gets no RLS/FLS/CRUD/tenant | +| `objectql/engine.ts:2793` `engine.execute()` / `driver-sql:1371` `driver.execute()` | raw SQL, documented tenant-bypass; caller trusted to inline the filter | +| `objectql/engine.ts:2956/2991` `getDriverByName`/`getDriverForObject` | hand out the raw driver — middleware-free read/write | +| bulk `update({multi:true})`/`deleteMany` on pure-OWD `private` objects (`sharing-plugin.ts:391`, `security-plugin.ts:821`) | single-id owner check not applied to multi-writes → members modify peers' rows | (#2982) + +### E4 — Unknown seams (identity not visibly threaded — investigate, then register) + +`runtime/http-dispatcher.ts:1488` **GraphQL** (passes only `{request}`, not the resolved `ec`); `service-realtime` **websocket** delivery (no per-subscriber RLS re-check); attachment **blob-level** RLS; the `chatWithTools` contract's optional actor. Each becomes a D4 matrix row in state `experimental`/`UNKNOWN` with a follow-up. + +### E5 — The D2 migration is large but has a prototype + +~300 elevation call sites resolve to ~50 hand-rolled `SYSTEM_CTX`/`isSystem` constructors across ~25 files (heaviest: `plugin-approvals` ~64, `plugin-security` ~58, `plugin-sharing` ~52, `plugin-auth` ~44 with 13 in one file). **None are audited today.** The canonical shape already exists — `service-automation/runtime-identity.ts` `resolveRunDataContext` (the sole `runAs:'system'→context` mapper) — which `systemContext(reason)` generalizes. This sizes D2 honestly: a mechanical but wide migration, front-loaded on four packages. + +--- + ## What lands now (the non-speculative slice) Per ADR-0049's staging discipline, v1 of this ADR builds only what has a consumer today: 1. **This decision record** — the invariant, the admission test, the staging plan. 2. **`systemContext(reason)` + `SystemGrant`** in the engine contract package, with audit emission; migrate the *known* elevation literals (boot/seeding, the action facades' trusted dispatch, report scheduler's `SYSTEM_CTX`) as the proof-of-idiom. -3. **Matrix rows** for today's surfaces (REST/MCP CRUD, actions, flows, triggers) in their *honest current states* — actions register as `surface: trusted-body, gated at invoke (ai.exposed + capability)` per #2964 — plus the meta-test skeleton with the transition allowlist. +3. **Matrix rows** for the surfaces the Evidence section enumerated, in their *honest current states* — actions register as `trusted-body, gated at invoke (ai.exposed + capability)` per #2964 — plus the meta-test skeleton with the transition allowlist. 4. **The authoring lint** from D6.3 (`ai.exposed` + trusted body ⇒ warn). +The confirmed-exploitable instances the sweep surfaced (E2) are **not** waiting on this mechanism — they are fixed independently and tracked at #2980 (reports IDOR + scheduled-report RLS bypass), #2981 (knowledge/RAG retrieval fall-open), #2982 (bulk-write OWD gap). Their existence is this ADR's motivating evidence, not its deliverable. + Everything else — the D3 signature migration, strict-mode-ON-in-CI per package, the D5 default flip, D6's `runAs` field — is explicitly **M2+, gated on the prior step's evidence**, tracked as separate issues under #2849. --- @@ -215,6 +268,7 @@ Everything else — the D3 signature migration, strict-mode-ON-in-CI per package - framework#2964 — the landed invoke-time hardening (ai.exposed gate, flow context forwarding, trusted-dispatch audit) — v1's audit substrate - framework#2308 / #1888 — the flow instance of the same class; the lint idiom D3/D6 reuse - framework#2845 / #2843 — the agent action surface whose safety framing this ADR makes true by construction -- `packages/plugins/plugin-security/src/security-plugin.ts` — the empty-principal seam (D5's target) +- framework#2980 / #2981 / #2982 — the confirmed-exploitable instances the Evidence sweep surfaced (reports IDOR + scheduled-report RLS bypass; knowledge/RAG retrieval fall-open; bulk-write OWD gap) — fixed independently; this ADR's motivating evidence +- `packages/plugins/plugin-security/src/security-plugin.ts` — the empty-principal seam (`:626` middleware, `:1689` getReadFilter) (D5's target) - `packages/runtime/src/http-dispatcher.ts` — `buildActionEngineFacade` (both facades), `invokeBusinessAction` - `packages/dogfood/test/authz-conformance.matrix.ts` — the ADR-0056 D10 matrix D4 extends From 4beed26d0a768a36c7e27e7c6d97f5bec65202a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Jul 2026 16:05:35 +0000 Subject: [PATCH 3/3] fix(security): close reports IDOR, RAG retrieval fall-open, and bulk-write OWD gap (#2980, #2981, #2982) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three confirmed-exploitable execution-surface authz holes from the ADR-0096 class sweep, each fixed at its enforcement point; no change for correctly- scoped callers. - #2980 reports: getReport/deleteReport/saveReport/listReports were caller- identity-blind (read/wrote sys_saved_report with a system context) → any authenticated user could read/delete/overwrite/enumerate any report. Now owner-scoped (system read of the protection-locked metadata object, authz by owner match); create/overwrite can't spoof ownership. Scheduled dispatch no longer runs isSystem (which emailed the target object's whole table past the owner's RLS): resolves the owner via a new resolveOwnerContext seam and fails closed when unresolved (ADR-0073 user-less identity consumer). - #2981 knowledge/RAG: applyPermissionFilter returned all hits on missing OR system context. Missing identity now fails closed (drops object-backed hits, keeps ACL-less file/http); only an explicit system context passes through. - #2982 sharing: bulk update({multi:true})/deleteMany had no single id to canEdit-gate, skipping owner scoping on private/public_read objects. New SharingService.buildWriteFilter (edit-set analogue of buildReadFilter) is AND-ed into the multi-write AST, incl. the on-behalf-of delegator intersection. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0155bjzJZxZaZTKSMSvg6Aum --- .changeset/security-class-2980-2981-2982.md | 42 ++++++ .../plugin-reports/src/report-service.test.ts | 103 ++++++++++++- .../plugin-reports/src/report-service.ts | 135 +++++++++++++++--- .../plugin-reports/src/reports-plugin.ts | 7 + .../plugin-sharing/src/sharing-plugin.ts | 25 ++++ .../src/sharing-service.test.ts | 92 ++++++++++++ .../plugin-sharing/src/sharing-service.ts | 55 +++++++ .../src/__tests__/knowledge-service.test.ts | 20 ++- .../src/knowledge-service.ts | 22 ++- 9 files changed, 473 insertions(+), 28 deletions(-) create mode 100644 .changeset/security-class-2980-2981-2982.md diff --git a/.changeset/security-class-2980-2981-2982.md b/.changeset/security-class-2980-2981-2982.md new file mode 100644 index 0000000000..86571e4a78 --- /dev/null +++ b/.changeset/security-class-2980-2981-2982.md @@ -0,0 +1,42 @@ +--- +'@objectstack/plugin-reports': patch +'@objectstack/service-knowledge': patch +'@objectstack/plugin-sharing': patch +--- + +fix(security): close three execution-surface authz holes surfaced by the #2849 class sweep (#2980, #2981, #2982) + +Three independent, confirmed-exploitable defects where an execution surface +ignored the caller's identity or fell open on a missing one. Each is fixed at +its own enforcement point; none change behaviour for correctly-scoped callers. + +- **#2980 — reports IDOR + scheduled-report RLS bypass.** `ReportService` + discarded the caller's context and read/wrote `sys_saved_report` with a system + context, so any authenticated user could read, delete, or overwrite any saved + report by id (cross-owner / cross-tenant), and `listReports` enumerated all + owners. `getReport`/`deleteReport`/`saveReport`/`listReports` are now + owner-scoped (system read of the protection-locked metadata object, but + authorization enforced by owner match); create/overwrite can no longer spoof + ownership. Scheduled dispatch no longer runs `isSystem` (which emailed the + target object's entire table past the owner's RLS): it resolves the owner to a + real RLS-bearing context via a new `resolveOwnerContext` seam and **fails + closed** (skips + marks the schedule failed) when the owner can't be resolved, + rather than running elevated. Wiring that resolver is the reports-surface + consumer of ADR-0073's user-less identity resolution. + +- **#2981 — knowledge/RAG retrieval fall-open.** `applyPermissionFilter` returned + every hit when the context was missing *or* system. A missing identity is no + longer treated as a grant: object-backed hits fail closed (dropped, keeping + ACL-less file/http hits), and only an **explicit** system context passes + through. Closes the agent path where an omitted `ToolExecutionContext.actor` + yielded unfiltered semantic search over the whole corpus. + +- **#2982 — bulk-write OWD gap.** `update({multi:true})` / `deleteMany` had no + single id to `canEdit`-gate, so owner scoping was skipped on private (and + public_read) objects. A new `SharingService.buildWriteFilter` (the edit-set + analogue of `buildReadFilter`) is AND-ed into the write AST for multi writes, + constraining them to rows the caller may edit — including the on-behalf-of + delegator intersection. + +Tracked as the motivating evidence of ADR-0096 (execution-surface identity +admission); the mechanism that would prevent the class structurally is separate. diff --git a/packages/plugins/plugin-reports/src/report-service.test.ts b/packages/plugins/plugin-reports/src/report-service.test.ts index c17c071ce6..0006ce0869 100644 --- a/packages/plugins/plugin-reports/src/report-service.test.ts +++ b/packages/plugins/plugin-reports/src/report-service.test.ts @@ -119,6 +119,11 @@ describe('ReportService', () => { email, clock: { now: () => now }, maxRows: 5000, + // Scheduled runs execute under the owner's resolved context (#2980). + // The fake engine ignores context, so this just lets dispatch proceed + // under a non-elevated identity instead of failing closed. + resolveOwnerContext: async (ownerId: string) => + ownerId ? { userId: ownerId, tenantId: 't1', positions: [], permissions: [] } : null, }); // seed the underlying object the report will query. engine._tables['lead'] = [ @@ -170,9 +175,9 @@ describe('ReportService', () => { // Regression: the query sorted with the non-canonical `direction: 'desc'` // key, which SortNode strips — so it sorted ascending (oldest first). engine._tables['sys_saved_report'] = [ - { id: 'r_old', name: 'Old', object_name: 'lead', query_json: '{}', updated_at: '2026-01-01T00:00:00Z' }, - { id: 'r_new', name: 'New', object_name: 'lead', query_json: '{}', updated_at: '2026-03-01T00:00:00Z' }, - { id: 'r_mid', name: 'Mid', object_name: 'lead', query_json: '{}', updated_at: '2026-02-01T00:00:00Z' }, + { id: 'r_old', name: 'Old', object_name: 'lead', query_json: '{}', owner_id: 'u1', updated_at: '2026-01-01T00:00:00Z' }, + { id: 'r_new', name: 'New', object_name: 'lead', query_json: '{}', owner_id: 'u1', updated_at: '2026-03-01T00:00:00Z' }, + { id: 'r_mid', name: 'Mid', object_name: 'lead', query_json: '{}', owner_id: 'u1', updated_at: '2026-02-01T00:00:00Z' }, ]; const rows = await svc.listReports({ object: 'lead' }, CTX); expect(rows.map(r => r.id)).toEqual(['r_new', 'r_mid', 'r_old']); @@ -368,7 +373,11 @@ describe('ReportService', () => { }); it('dispatchDue: still runs (no mail) when email service absent', async () => { - const svcNoMail = new ReportService({ engine: engine as any, clock: { now: () => now } }); + const svcNoMail = new ReportService({ + engine: engine as any, + clock: { now: () => now }, + resolveOwnerContext: async (ownerId: string) => ({ userId: ownerId, positions: [], permissions: [] }), + }); const r = await svcNoMail.saveReport({ name: 'A', object: 'lead', query: {} }, CTX); await svcNoMail.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX); engine._tables['sys_report_schedule'][0].next_run_at = new Date(now.getTime() - 1).toISOString(); @@ -377,4 +386,90 @@ describe('ReportService', () => { expect(result.fired).toBe(1); expect(engine._tables['sys_report_schedule'][0].last_status).toBe('ok'); }); + + // ─── Authorization (#2980) ────────────────────────────────────── + describe('access control', () => { + const OTHER = { userId: 'u2', tenantId: 't1', positions: [], permissions: [] }; + + it('getReport: a non-owner cannot read another user\'s report (not-found)', async () => { + const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX); + expect(await svc.getReport(r.id, CTX)).not.toBeNull(); // owner sees it + expect(await svc.getReport(r.id, OTHER)).toBeNull(); // stranger cannot + }); + + it('deleteReport: a non-owner cannot delete another user\'s report', async () => { + const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX); + await expect(svc.deleteReport(r.id, OTHER)).rejects.toThrow(/REPORT_NOT_FOUND/); + expect(engine._tables['sys_saved_report'].length).toBe(1); // still there + await svc.deleteReport(r.id, CTX); // owner can + expect(engine._tables['sys_saved_report'].length).toBe(0); + }); + + it('saveReport: a non-owner cannot overwrite another user\'s report by id', async () => { + const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX); + await expect( + svc.saveReport({ id: r.id, name: 'Hijacked', object: 'lead', query: {} }, OTHER), + ).rejects.toThrow(/REPORT_NOT_FOUND/); + expect(engine._tables['sys_saved_report'][0].name).toBe('Mine'); + }); + + it('saveReport: a caller cannot assign ownership to someone else on create', async () => { + const r = await svc.saveReport( + { name: 'X', object: 'lead', query: {}, ownerId: 'victim' } as any, + CTX, + ); + expect(r.owner_id).toBe('u1'); + }); + + it('listReports: only the caller\'s own reports are returned', async () => { + await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX); // u1 + await svc.saveReport({ name: 'B', object: 'lead', query: {} }, OTHER); // u2 + const mine = await svc.listReports({}, CTX); + expect(mine.map(r => r.name)).toEqual(['A']); + const theirs = await svc.listReports({}, OTHER); + expect(theirs.map(r => r.name)).toEqual(['B']); + }); + + it('listReports: a caller-supplied ownerId cannot widen past the caller', async () => { + await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX); + await svc.saveReport({ name: 'B', object: 'lead', query: {} }, OTHER); + expect(await svc.listReports({ ownerId: 'u2' }, CTX)).toEqual([]); // u1 asking for u2 ⇒ nothing + }); + + it('system context sees all reports (scheduler / tooling path)', async () => { + await svc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX); + await svc.saveReport({ name: 'B', object: 'lead', query: {} }, OTHER); + const all = await svc.listReports({}, { isSystem: true } as any); + expect(all.length).toBe(2); + }); + + it('dispatchDue: fails closed (no RLS bypass) when no owner resolver is configured', async () => { + const noResolver = new ReportService({ engine: engine as any, email, clock: { now: () => now } }); + const r = await noResolver.saveReport({ name: 'A', object: 'lead', query: {} }, CTX); + await noResolver.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX); + engine._tables['sys_report_schedule'][0].next_run_at = new Date(now.getTime() - 1).toISOString(); + + const result = await noResolver.dispatchDue(); + expect(result.fired).toBe(0); + expect(result.failed).toBe(1); + expect(email._sent.length).toBe(0); // nothing emailed — no elevated run + expect(engine._tables['sys_report_schedule'][0].last_status).toBe('failed'); + expect(engine._tables['sys_report_schedule'][0].last_error).toMatch(/RLS bypassed/); + }); + + it('dispatchDue: runs under the owner context the resolver returns', async () => { + const seen: Array = []; + const spySvc = new ReportService({ + engine: engine as any, email, clock: { now: () => now }, + resolveOwnerContext: async (ownerId) => { seen.push(ownerId); return { userId: ownerId, positions: [], permissions: [] }; }, + }); + const r = await spySvc.saveReport({ name: 'A', object: 'lead', query: {} }, CTX); + await spySvc.scheduleReport({ reportId: r.id, recipients: ['x@t'], format: 'csv' }, CTX); + engine._tables['sys_report_schedule'][0].next_run_at = new Date(now.getTime() - 1).toISOString(); + + const result = await spySvc.dispatchDue(); + expect(result.fired).toBe(1); + expect(seen).toEqual(['u1']); // resolved the owner, not a system context + }); + }); }); diff --git a/packages/plugins/plugin-reports/src/report-service.ts b/packages/plugins/plugin-reports/src/report-service.ts index fb7ec6718d..4f1f8ccc10 100644 --- a/packages/plugins/plugin-reports/src/report-service.ts +++ b/packages/plugins/plugin-reports/src/report-service.ts @@ -170,6 +170,20 @@ function renderSubject(template: string | undefined, vars: Record Promise; + export interface ReportServiceOptions { engine: ReportEngine; email?: ReportEmail; @@ -177,6 +191,12 @@ export interface ReportServiceOptions { logger?: { info?: (msg: any, ...rest: any[]) => void; warn?: (msg: any, ...rest: any[]) => void; error?: (msg: any, ...rest: any[]) => void }; /** Cap rows per report to protect both DB and email size. */ maxRows?: number; + /** + * Resolves a report owner into an RLS-bearing context for scheduled runs + * (see {@link OwnerContextResolver}). When omitted, scheduled reports fail + * closed instead of running with RLS bypassed (#2980). + */ + resolveOwnerContext?: OwnerContextResolver; } export class ReportService implements IReportService { @@ -185,6 +205,7 @@ export class ReportService implements IReportService { private readonly clock: ReportClock; private readonly logger: NonNullable; private readonly maxRows: number; + private readonly resolveOwnerContext?: OwnerContextResolver; constructor(opts: ReportServiceOptions) { this.engine = opts.engine; @@ -192,6 +213,33 @@ export class ReportService implements IReportService { this.clock = opts.clock ?? { now: () => new Date() }; this.logger = opts.logger ?? {}; this.maxRows = Math.max(1, opts.maxRows ?? 5000); + this.resolveOwnerContext = opts.resolveOwnerContext; + } + + // ── Access control ───────────────────────────────────────────── + + /** + * Authorization for a saved-report row. `sys_saved_report` is a + * protection-locked system object, so its rows are *read* with + * `SYSTEM_CTX`; the caller's right to see/mutate a specific report is + * enforced HERE, by owner match, not by the metadata read's own RLS — + * otherwise any authenticated caller could read/delete/overwrite any + * report by id (#2980). An explicit elevated context (`isSystem`) — the + * scheduler / server tooling — sees everything. + */ + private canAccessReport(row: { owner_id?: unknown } | null | undefined, context: SharingExecutionContext | undefined): boolean { + if (!row) return false; + if (context?.isSystem) return true; + const userId = context?.userId; + return !!userId && row.owner_id === userId; + } + + /** Raw metadata read of a saved report by id (no authz — callers gate). */ + private async loadReportRow(reportId: string): Promise { + const rows = await this.engine.find('sys_saved_report', { + filter: { id: reportId }, limit: 1, context: SYSTEM_CTX, + }); + return Array.isArray(rows) && rows[0] ? rows[0] : null; } // ── Report CRUD ──────────────────────────────────────────────── @@ -202,23 +250,33 @@ export class ReportService implements IReportService { if (!input.query) throw new Error('VALIDATION_FAILED: query is required'); const now = this.clock.now().toISOString(); + // A non-system caller always owns what they create — a caller-supplied + // ownerId cannot assign the report to someone else (#2980). Only an + // explicit elevated context (server tooling / import) may set it. + const ownerId = context.isSystem ? (input.ownerId ?? context.userId ?? null) : (context.userId ?? null); const payload: any = { name: input.name, description: input.description ?? null, object_name: input.object, query_json: JSON.stringify(input.query ?? {}), format: input.format ?? DEFAULT_FORMAT, - owner_id: input.ownerId ?? context.userId ?? null, + owner_id: ownerId, updated_at: now, }; if (input.id) { - const existing = await this.engine.find('sys_saved_report', { - filter: { id: input.id }, limit: 1, context: SYSTEM_CTX, - }); - if (Array.isArray(existing) && existing[0]) { + const existing = await this.loadReportRow(input.id); + if (existing) { + // An update to an existing report is a mutation — a caller may only + // overwrite a report they own (#2980). Not-found for others so the + // response doesn't leak that the id exists. + if (!this.canAccessReport(existing, context)) { + throw new Error(`REPORT_NOT_FOUND: ${input.id}`); + } + // Never let a non-system caller reassign ownership away from the row. + if (!context.isSystem) payload.owner_id = existing.owner_id ?? payload.owner_id; await this.engine.update('sys_saved_report', { id: input.id, ...payload }, { context: SYSTEM_CTX }); - return rowFromSaved({ ...existing[0], ...payload, id: input.id }); + return rowFromSaved({ ...existing, ...payload, id: input.id }); } } @@ -230,26 +288,43 @@ export class ReportService implements IReportService { async listReports( filter: { object?: string; ownerId?: string } | undefined, - _context: SharingExecutionContext, + context: SharingExecutionContext, ): Promise { const f: any = {}; if (filter?.object) f.object_name = filter.object; - if (filter?.ownerId) f.owner_id = filter.ownerId; + // Owner scoping (#2980): a non-system caller sees ONLY their own reports — + // a caller-supplied ownerId can never widen past their own id. A caller + // with no identity sees nothing (fail closed). System/tooling sees all, + // honouring an explicit ownerId narrow. + if (context?.isSystem) { + if (filter?.ownerId) f.owner_id = filter.ownerId; + } else { + if (!context?.userId) return []; + if (filter?.ownerId && filter.ownerId !== context.userId) return []; + f.owner_id = context.userId; + } const rows = await this.engine.find('sys_saved_report', { filter: f, limit: 500, orderBy: [{ field: 'updated_at', order: 'desc' }], context: SYSTEM_CTX, }); return Array.isArray(rows) ? rows.map(rowFromSaved) : []; } - async getReport(reportId: string, _context: SharingExecutionContext): Promise { - const rows = await this.engine.find('sys_saved_report', { - filter: { id: reportId }, limit: 1, context: SYSTEM_CTX, - }); - return Array.isArray(rows) && rows[0] ? rowFromSaved(rows[0]) : null; + async getReport(reportId: string, context: SharingExecutionContext): Promise { + const row = await this.loadReportRow(reportId); + // Unauthorized reads are indistinguishable from a genuine miss (#2980). + if (!this.canAccessReport(row, context)) return null; + return rowFromSaved(row); } - async deleteReport(reportId: string, _context: SharingExecutionContext): Promise { + async deleteReport(reportId: string, context: SharingExecutionContext): Promise { if (!reportId) throw new Error('VALIDATION_FAILED: reportId is required'); + const row = await this.loadReportRow(reportId); + if (!row) return; // idempotent — nothing to drop + // A caller may only delete a report they own (#2980); others get a + // not-found so the delete neither fires nor reveals the report's existence. + if (!this.canAccessReport(row, context)) { + throw new Error(`REPORT_NOT_FOUND: ${reportId}`); + } // Cascade — drop attached schedules first. const schedules = await this.engine.find('sys_report_schedule', { filter: { report_id: reportId }, limit: 500, context: SYSTEM_CTX, @@ -410,8 +485,8 @@ export class ReportService implements IReportService { let fired = 0, failed = 0, skipped = 0; for (const schedule of list) { try { - const report = await this.getReport(schedule.report_id, { isSystem: true }); - if (!report) { + const row = await this.loadReportRow(schedule.report_id); + if (!row) { skipped++; await this.markSchedule(schedule.id, { last_status: 'skipped', @@ -419,10 +494,36 @@ export class ReportService implements IReportService { }); continue; } + const report = rowFromSaved(row); + + // Run the report under the OWNER's authority, not system (#2980). + // A scheduled run must not read rows the report's owner cannot see — + // that was a silent RLS bypass (a member's scheduled report emailed + // the target object's entire table). Resolve the owner to a real + // RLS-bearing context; if we can't (no resolver wired, or unknown/ + // disabled owner), FAIL CLOSED rather than run elevated. + const ownerId = report.owner_id; + const runContext = ownerId && this.resolveOwnerContext + ? await this.resolveOwnerContext(ownerId).catch((err) => { + this.logger.warn?.('ReportService.dispatchDue: owner context resolution failed', err); + return null; + }) + : null; + if (!runContext) { + failed++; + await this.markSchedule(schedule.id, { + last_status: 'failed', + last_error: ownerId + ? `owner '${ownerId}' context unavailable — refusing to run scheduled report with RLS bypassed (#2849/#2980)` + : 'report has no owner — refusing to run scheduled report with RLS bypassed (#2849/#2980)', + }); + continue; + } + // Force the schedule's own format so the recipient gets what // the admin configured (CSV attachment vs inline HTML table). const fmt: ReportFormat = (schedule.format ?? 'html_table') as ReportFormat; - const result = await this.executeReport({ ...report, format: fmt }, { isSystem: true }, false); + const result = await this.executeReport({ ...report, format: fmt }, runContext, false); const recipients = schedule.recipients.split(',').map(s => s.trim()).filter(Boolean); const subject = renderSubject(schedule.subject_template, { diff --git a/packages/plugins/plugin-reports/src/reports-plugin.ts b/packages/plugins/plugin-reports/src/reports-plugin.ts index 38e50f9f93..bcc63c769d 100644 --- a/packages/plugins/plugin-reports/src/reports-plugin.ts +++ b/packages/plugins/plugin-reports/src/reports-plugin.ts @@ -87,6 +87,13 @@ export class ReportsServicePlugin implements Plugin { email, logger: ctx.logger, maxRows: this.options.maxRows, + // Scheduled reports run under the owner's resolved RLS context, not a + // system bypass (#2980). No owner-context resolver is wired yet — that + // is the reports-surface consumer of ADR-0073's user-less identity + // resolution (M2) — so until it lands, scheduled runs FAIL CLOSED + // (skipped + marked failed) rather than exfiltrate. Interactive runs + // (run/runAdHoc) are unaffected: they carry the caller's context. + resolveOwnerContext: undefined, }); ctx.registerService('reports', this.service); diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index 7abf592a16..204cba4da8 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -408,6 +408,31 @@ export function buildSharingMiddleware(service: SharingService): EngineMiddlewar err.status = 403; throw err; } + return next(); + } + + // Bulk (multi) write — no single id to canEdit-gate (#2982). AND the + // editable-rows filter into the AST so the update/delete only touches + // rows the caller may edit, exactly as the read path scopes finds. The + // engine honours ast.where operation-agnostically (same seam the RLS + // write filter uses). Without this, a `multi:true` write on an + // owner-scoped object would hit every matching row, including peers'. + let writeFilter = await service.buildWriteFilter(ctx.object, exec ?? {}); + // [ADR-0090 D10] Intersect the delegator's editable set for on-behalf-of. + if (exec?.onBehalfOf?.userId) { + const delFilter = await service.buildWriteFilter(ctx.object, { + ...exec, + userId: exec.onBehalfOf.userId, + onBehalfOf: undefined, + __writeScope: exec.__delegatorWriteScope, + }); + writeFilter = composeAnd(writeFilter, delFilter); + } + if (writeFilter) { + const ast: any = ctx.ast ?? {}; + ast.where = composeAnd(ast.where, writeFilter); + ast.filter = composeAnd(ast.filter, writeFilter); + ctx.ast = ast; } return next(); } diff --git a/packages/plugins/plugin-sharing/src/sharing-service.test.ts b/packages/plugins/plugin-sharing/src/sharing-service.test.ts index d1a5569323..1fb0050a34 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.test.ts @@ -445,4 +445,96 @@ describe('buildSharingMiddleware (engine integration)', () => { await mw(ctx, async () => {}); expect(ctx.ast.where).toEqual({ $and: [{ name: 'Acme' }, { owner_id: 'alice' }] }); }); + + // ─── Bulk (multi) write owner scoping (#2982) ─────────────────── + it('bulk update: injects the editable-rows filter so a non-owner cannot touch peers', async () => { + const mw = buildSharingMiddleware(svc); + const ctx: any = { + object: 'account', + operation: 'update', + options: { multi: true, where: { name: 'X' } }, + ast: { where: { name: 'X' } }, + data: { status: 'archived' }, + context: { userId: 'bob' }, + }; + let nextCalled = false; + await mw(ctx, async () => { nextCalled = true; }); + expect(nextCalled).toBe(true); + // bob's bulk update is constrained to rows bob owns. + expect(ctx.ast.where).toEqual({ $and: [{ name: 'X' }, { owner_id: 'bob' }] }); + }); + + it('bulk delete: injects the owner filter (no single id to canEdit-gate)', async () => { + const mw = buildSharingMiddleware(svc); + const ctx: any = { + object: 'account', + operation: 'delete', + options: { multi: true, where: {} }, + ast: {}, + context: { userId: 'bob' }, + }; + await mw(ctx, async () => {}); + expect(ctx.ast.where).toEqual({ owner_id: 'bob' }); + }); + + it('bulk update: edit-shared records widen the editable set', async () => { + await svc.grant({ object: 'account', recordId: 'a1', recipientId: 'bob', accessLevel: 'edit' }, { userId: 'admin' }); + const mw = buildSharingMiddleware(svc); + const ctx: any = { + object: 'account', + operation: 'update', + options: { multi: true, where: {} }, + ast: {}, + data: { status: 'x' }, + context: { userId: 'bob' }, + }; + await mw(ctx, async () => {}); + expect(ctx.ast.where).toEqual({ $or: [{ owner_id: 'bob' }, { id: { $in: ['a1'] } }] }); + }); + + it('bulk update on a public_read (read) object is STILL owner-write-scoped', async () => { + // The crux of #2982: reads are open on a public_read object (buildReadFilter + // returns null), but writes remain owner-scoped — a distinction the old + // single-id-only gate missed for multi writes. + engine._tables.lead = [{ id: 'l1', owner_id: 'alice' }]; + const mw = buildSharingMiddleware(svc); + const ctx: any = { + object: 'lead', + operation: 'update', + options: { multi: true, where: {} }, + ast: {}, + data: { status: 'x' }, + context: { userId: 'bob' }, + }; + await mw(ctx, async () => {}); + expect(ctx.ast.where).toEqual({ owner_id: 'bob' }); + }); + + it('bulk update: system context is unrestricted', async () => { + const mw = buildSharingMiddleware(svc); + const ctx: any = { + object: 'account', + operation: 'update', + options: { multi: true, where: {} }, + ast: {}, + data: { status: 'x' }, + context: { isSystem: true }, + }; + await mw(ctx, async () => {}); + expect(ctx.ast.where).toBeUndefined(); + }); + + it('bulk update on a fully public object: no owner filter', async () => { + const mw = buildSharingMiddleware(svc); + const ctx: any = { + object: 'task', // EXPLICIT_PUBLIC_SCHEMA + operation: 'update', + options: { multi: true, where: {} }, + ast: {}, + data: { status: 'x' }, + context: { userId: 'bob' }, + }; + await mw(ctx, async () => {}); + expect(ctx.ast.where).toBeUndefined(); + }); }); diff --git a/packages/plugins/plugin-sharing/src/sharing-service.ts b/packages/plugins/plugin-sharing/src/sharing-service.ts index 36c8f5c0ba..8b9f733740 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.ts @@ -179,6 +179,61 @@ export class SharingService implements ISharingService { }; } + /** + * Build a `FilterCondition` restricting a **bulk** (multi-row) write to the + * records the caller may edit — the write analogue of {@link buildReadFilter}. + * Single-id writes are gated by {@link canEdit}; a `update({multi:true})` / + * `deleteMany` has no single id, so without this filter it would touch every + * matching row regardless of ownership (#2982). Returns `null` when no + * restriction applies (system/bypass, public objects, no owner field). + * + * Editable-set = owner-match (widened by write DEPTH) OR records shared to + * the caller at `edit`/`full`. Unlike reads, this applies to BOTH `private` + * and `read` (public_read) models — public_read is read-open but write-owned; + * only a fully `public` object is write-open. + */ + async buildWriteFilter( + object: string, + context: SharingExecutionContext, + ): Promise { + if (this.shouldBypass(object, context)) return null; + + const schema = this.engine.getSchema?.(object); + if (!schema) return null; + if (effectiveSharingModel(schema) === 'public') return null; + if (!hasOwnerField(schema)) return null; + if (!context.userId) { + // Authenticated but principal-less → edit nothing (fail closed), + // mirroring buildReadFilter's degenerate-context handling. + return { id: '__deny_all__' }; + } + + const writeScope = (context as any).__writeScope as ('own' | 'own_and_reports' | 'unit' | 'unit_and_below' | 'org' | undefined); + if (writeScope === 'org') return null; + const ownerIds = await this.resolveOwnerScopeIds(context, writeScope); + const ownerMatch: Record = ownerIds.length === 1 + ? { [OWNER_FIELD]: ownerIds[0] } + : { [OWNER_FIELD]: { $in: ownerIds } }; + + const grants = await this.engine.find('sys_record_share', { + where: { + object_name: object, + recipient_type: 'user', + recipient_id: context.userId, + access_level: { $in: ['edit', 'full'] }, + }, + fields: ['record_id'], + limit: 5000, + context: SYSTEM_CTX, + }); + const grantedIds: string[] = Array.isArray(grants) + ? grants.map((g: any) => String(g.record_id)).filter(Boolean) + : []; + + if (grantedIds.length === 0) return ownerMatch; + return { $or: [ownerMatch, { id: { $in: grantedIds } }] }; + } + /** * Return `true` if the caller may edit `(object, recordId)`. Always * `true` for system context, public objects, and objects without an diff --git a/packages/services/service-knowledge/src/__tests__/knowledge-service.test.ts b/packages/services/service-knowledge/src/__tests__/knowledge-service.test.ts index 77f7faac0a..84f54f8722 100644 --- a/packages/services/service-knowledge/src/__tests__/knowledge-service.test.ts +++ b/packages/services/service-knowledge/src/__tests__/knowledge-service.test.ts @@ -98,15 +98,29 @@ describe('KnowledgeService — permission-aware search', () => { return { svc, adapter, findSpy }; } - it('passes through every hit when no executionContext is provided', async () => { + it('fails closed on object-source hits when no executionContext is provided (#2981)', async () => { + // A missing identity is NOT a grant of authority: object-backed hits are + // dropped rather than returned unfiltered (the agent/RAG fall-open). const hits: KnowledgeHit[] = [ { chunkId: 'c1', documentId: 'd1', sourceId: 's1', sourceRecordId: 'rec_1', score: 0.9, snippet: '' }, { chunkId: 'c2', documentId: 'd2', sourceId: 's1', sourceRecordId: 'rec_2', score: 0.8, snippet: '' }, ]; const { svc, findSpy } = buildSetup(hits); const out = await svc.search('q'); - expect(out).toHaveLength(2); - expect(findSpy).not.toHaveBeenCalled(); + expect(out).toHaveLength(0); // object-backed hits withheld + expect(findSpy).not.toHaveBeenCalled(); // dropped without a lookup + }); + + it('keeps file/http hits but drops object hits when no executionContext (#2981)', async () => { + // The fail-closed drop is scoped to record-backed hits; ACL-less + // file/http hits still flow (their adapter owns any enforcement). + const hits: KnowledgeHit[] = [ + { chunkId: 'c1', documentId: 'd1', sourceId: 's1', sourceRecordId: 'rec_1', score: 0.9, snippet: '' }, + { chunkId: 'c2', documentId: 'd2', sourceId: 'http_src', score: 0.5, snippet: 'web' }, + ]; + const { svc } = buildSetup(hits); + const out = await svc.search('q'); + expect(out.map((h) => h.documentId)).toEqual(['d2']); }); it('passes through every hit when context is isSystem:true', async () => { diff --git a/packages/services/service-knowledge/src/knowledge-service.ts b/packages/services/service-knowledge/src/knowledge-service.ts index 498a2ca4d2..6609081175 100644 --- a/packages/services/service-knowledge/src/knowledge-service.ts +++ b/packages/services/service-knowledge/src/knowledge-service.ts @@ -301,15 +301,29 @@ export class KnowledgeService implements IKnowledgeService { * For hits without a `sourceRecordId` (file/http sources) we keep * them — adapter is responsible for any ACL enforcement there. * - * When the caller's context is `isSystem: true` or no context is - * supplied, every hit passes through — preserves today's behaviour - * for cron jobs / tests. + * Identity handling (#2981): an **explicit** `isSystem` context (indexing / + * trusted server jobs) passes every hit through — it is a deliberate, + * authored elevation. A **missing** context, however, is NOT a grant of + * authority: with no identity we cannot prove the caller may read the + * object-backed rows, so we FAIL CLOSED and drop object-source hits (the + * same conservative posture as a missing data engine). This closes the + * agent/RAG fall-open where an omitted `ToolExecutionContext.actor` used to + * return unfiltered semantic search over the whole corpus. */ private async applyPermissionFilter( hits: KnowledgeHit[], ctx: ExecutionContext | undefined, ): Promise { - if (!ctx || ctx.isSystem) return hits; + // Explicit elevation → pass through (indexing / trusted jobs). + if (ctx?.isSystem) return hits; + // No identity → fail closed on object-backed hits (keep file/http hits). + if (!ctx) { + this.options.logger?.warn?.( + '[knowledge] retrieval with no ExecutionContext — dropping object-source hits to stay safe (#2981). ' + + 'Pass the caller identity (or an explicit system context) to retrieve object-backed knowledge.', + ); + return hits.filter((h) => !h.sourceRecordId); + } if (!this.options.dataEngine) { this.options.logger?.warn?.( '[knowledge] no IDataEngine bound — dropping object-source hits to stay safe.',