From 55aa096e74f69e51f346c334ca7cb2d46098bb67 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 02:50:28 +0000 Subject: [PATCH] feat(spec): share-link enforcement takes the full ExecutionContext (#6430) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `IShareLinkService.createLink` / `revokeLink` / `listLinks` now declare their context parameter as the complete `ExecutionContext` envelope instead of the five-field `ShareLinkExecutionContext`. All three adjudicate access, so each needs the whole `resolveAuthzContext` result — `accessible_org_ids`, `org_user_ids`, `systemPermissions`, `posture`, `tabPermissions` included. `ShareLinkExecutionContext` is retained, unchanged in shape, as the route's own 401 vocabulary; its TSDoc now states the boundary and why tsc cannot enforce it (structural subtyping accepts a narrow object wherever the wide type is expected). Implements the maintainer ruling on #6206 (option A, 2026-08-07). Contract half only — the `@objectstack/plugin-sharing` consumer is the follow-up. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011M7UwH25Unfi73UHim7ajY --- .../share-link-enforcement-full-envelope.md | 35 ++++ .../src/contracts/share-link-service.test.ts | 195 ++++++++++++++++++ .../spec/src/contracts/share-link-service.ts | 108 +++++++++- 3 files changed, 328 insertions(+), 10 deletions(-) create mode 100644 .changeset/share-link-enforcement-full-envelope.md create mode 100644 packages/spec/src/contracts/share-link-service.test.ts diff --git a/.changeset/share-link-enforcement-full-envelope.md b/.changeset/share-link-enforcement-full-envelope.md new file mode 100644 index 0000000000..47613de451 --- /dev/null +++ b/.changeset/share-link-enforcement-full-envelope.md @@ -0,0 +1,35 @@ +--- +"@objectstack/spec": minor +--- + +feat(spec): share-link enforcement takes the full `ExecutionContext`; the narrow context is route-401 only (#6430, #6206 ruling A) + +`IShareLinkService.createLink` / `revokeLink` / `listLinks` now declare their +context parameter as the complete `ExecutionContext` envelope instead of the +five-field `ShareLinkExecutionContext`. All three ADJUDICATE access — the +[Finding-2] visibility re-read on create, the ADR-0111 D8 share-manager probe +on revoke, the context-scoped listing — so each needs the whole +`resolveAuthzContext` result, `accessible_org_ids` / `org_user_ids` / +`systemPermissions` / `posture` / `tabPermissions` included. + +The measured failure behind the ruling: the share-link route assembled exactly +those five fields and handed the result straight to `engine.find` as the +enforcement context. Under the `group` tenancy posture `accessible_org_ids` IS +the Layer 0 wall (ADR-0105 D2) and an absent set denies, so link creation +returned a blanket 403 on a posture that ships. Fail-closed, not a leak — but a +trimmed envelope feeding enforcement is a bypass-shaped pattern, and ADR-0095 +D2 already rules that posture is resolved once and carried, never re-derived at +the enforcement site. This was the third assembly site of that family (#5997, +#6071), so the contract converges on the whole envelope rather than keeping a +per-site subset. + +`ShareLinkExecutionContext` is retained and unchanged in shape — it is the +route's own "authenticated or 401?" vocabulary — with TSDoc that now states the +boundary and why TypeScript cannot enforce it (structural subtyping accepts a +narrow object wherever the wide type is expected, so the declared parameter +type plus the caller's obligation are what hold the line). + +Contract-only, no runtime behaviour change here: existing implementations keep +compiling (method parameters are bivariant), and the `@objectstack/plugin-sharing` +consumer that actually threads the envelope through is the follow-up half +tracked on #6206. diff --git a/packages/spec/src/contracts/share-link-service.test.ts b/packages/spec/src/contracts/share-link-service.test.ts new file mode 100644 index 0000000000..bfe7ee86f3 --- /dev/null +++ b/packages/spec/src/contracts/share-link-service.test.ts @@ -0,0 +1,195 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#6430 / #6206 ruling A] Share-link contract pins — the enforcement/401 line. +// +// ## What the ruling decided +// +// The share-link visibility check must run on the FULL `ExecutionContext` (the +// complete `resolveAuthzContext` envelope: `accessible_org_ids`, +// `org_user_ids`, `systemPermissions`, `posture`, `tabPermissions`, …). +// `ShareLinkExecutionContext` — the five-field shape the route assembled — may +// keep serving the route's own 401 decision, but must never reach an +// enforcement path. ADR-0095 D2 (posture resolved once, carried, never +// re-derived at enforcement) and ADR-0105 D2 (`accessible_org_ids` IS the +// `group`-posture Layer 0 wall, absent ⇒ deny) are the anchors; #5997 and +// #6071 were the two prior assembly sites of the same family. +// +// ## What is pinned here, and what deliberately is NOT +// +// PINNED: (1) each `IShareLinkService` method that adjudicates access declares +// its context parameter as `ExecutionContext` — by TYPE IDENTITY, so +// re-narrowing it to anything (including back to `ShareLinkExecutionContext`) +// goes red; (2) a call site may state the whole envelope, which under the old +// signature was a TS2353 excess-property error on every one of the five +// dropped keys — that is this file's before-red direction; (3) the narrow type +// survives, unchanged in shape, as the route's 401 vocabulary. +// +// NOT PINNED, on purpose: there is no `@ts-expect-error` asserting that a +// `ShareLinkExecutionContext` is REJECTED by an enforcement parameter, because +// it is not. Structural subtyping makes the narrow type assignable to +// `ExecutionContext` — its five fields all exist there with compatible types, +// and nothing in `ExecutionContext` is required — so such a directive would be +// unsatisfied and fail the build. TypeScript cannot express "this optional +// field had better have been populated". A pin shaped to look like compiler +// enforcement, where only documentation exists, would read as verified and be +// worse than the honest statement, so the last case below pins the assignment +// as the legal-but-unwanted fact it is and names what actually holds the line: +// the declared parameter type, which makes any narrowing visible at the call +// site, plus the caller's obligation to pass the resolved envelope through +// whole. + +import { describe, expect, it } from 'vitest'; + +import type { ExecutionContext } from '../kernel/execution-context.zod'; +import type { + CreateShareLinkInput, + IShareLinkService, + ListShareLinksFilter, + ShareLink, + ShareLinkExecutionContext, +} from './share-link-service'; + +/** Type-level identity: true iff A and B are the same type. */ +type Eq = (() => T extends A ? 1 : 2) extends (() => T extends B ? 1 : 2) + ? true + : false; +/** Compile error when the argument is not `true`. */ +type Assert = T; +/** Compile error when the argument is not `false`. */ +type Refute = T; + +type CreateCtx = Parameters[1]; +type RevokeCtx = Parameters[1]; +type ListCtx = Parameters[1]; + +// Every adjudicating method takes the full envelope, by identity. +export type EnforcementTakesFullEnvelope0 = Assert>; +export type EnforcementTakesFullEnvelope1 = Assert>; +export type EnforcementTakesFullEnvelope2 = Assert>; + +// …and none of them takes the route's narrow 401 shape. +export type EnforcementIsNotNarrow0 = Refute>; +export type EnforcementIsNotNarrow1 = Refute>; +export type EnforcementIsNotNarrow2 = Refute>; + +// The narrow type keeps exactly the five route-401 fields — it was not widened +// as a shortcut around the ruling. Widening it would recreate the subset this +// card removed, one field at a time. +export type NarrowTypeKeepsRouteShape = Assert< + Eq +>; + +/** Records what the service actually received, so no assertion is vacuous. */ +function makeRecordingService(): { + service: IShareLinkService; + seen: ExecutionContext[]; +} { + const seen: ExecutionContext[] = []; + const link: ShareLink = { + id: 'shl_pin', + token: 'tok_0123456789012345678901', + object_name: 'contract', + record_id: 'rec_1', + permission: 'view', + audience: 'link_only', + }; + const service: IShareLinkService = { + async createLink(_input: CreateShareLinkInput, context: ExecutionContext) { + seen.push(context); + return link; + }, + async revokeLink(_idOrToken: string, context: ExecutionContext) { + seen.push(context); + }, + async listLinks(_filter: ListShareLinksFilter, context: ExecutionContext) { + seen.push(context); + return [link]; + }, + async resolveToken() { + return null; + }, + }; + return { service, seen }; +} + +describe('share-link contract — enforcement takes the full ExecutionContext (#6430)', () => { + it('accepts the whole resolveAuthzContext envelope at an enforcement call site', async () => { + const { service, seen } = makeRecordingService(); + + // The before-red direction of this file. Written as an object LITERAL on + // purpose: excess-property checking applies to literals, so under the old + // `ShareLinkExecutionContext` parameter every key below the first five was + // a TS2353 error ("does not exist in type ShareLinkExecutionContext") — + // which is precisely why the route hand-assembled a subset instead of + // passing what it had already resolved. Restore the narrow parameter type + // and this call stops compiling. + await service.createLink( + { object: 'contract', recordId: 'rec_1' }, + { + userId: 'usr_1', + tenantId: 'org_plant_a', + positions: ['sales'], + permissions: ['standard_user'], + // The five dimensions the trimmed envelope dropped (#6206): + accessible_org_ids: ['org_plant_a', 'org_plant_b'], + org_user_ids: ['usr_1', 'usr_2'], + systemPermissions: ['manage_sharing'], + posture: 'MEMBER', + tabPermissions: { crm: 'visible' }, + }, + ); + + const received = seen[0]!; + // ADR-0105 D2: this set IS the `group`-posture Layer 0 wall. Its absence + // was the blanket 403 — so its arrival is the fact worth observing. + expect(received.accessible_org_ids).toEqual(['org_plant_a', 'org_plant_b']); + // ADR-0095 D2: resolved once upstream, carried — never re-derived here. + expect(received.posture).toBe('MEMBER'); + expect(received.org_user_ids).toEqual(['usr_1', 'usr_2']); + expect(received.systemPermissions).toEqual(['manage_sharing']); + expect(received.tabPermissions).toEqual({ crm: 'visible' }); + }); + + it('carries the envelope through revokeLink and listLinks too', async () => { + const { service, seen } = makeRecordingService(); + const envelope: ExecutionContext = { + userId: 'usr_1', + accessible_org_ids: ['org_plant_a'], + posture: 'TENANT_ADMIN', + }; + + // Both are adjudicating paths: revoke authority is creator ∪ record + // share-manager (ADR-0111 D8, probed with `context`), and the listing is + // read under `context`. + await service.revokeLink('shl_pin', envelope); + await service.listLinks({ object: 'contract' }, envelope); + + expect(seen).toHaveLength(2); + for (const received of seen) { + expect(received.accessible_org_ids).toEqual(['org_plant_a']); + expect(received.posture).toBe('TENANT_ADMIN'); + } + }); + + it('keeps the narrow type for the route 401 — and states why no compiler pin exists', () => { + // Still exported, still the route's vocabulary: `userId` present ⇒ the + // request is authenticated, absent ⇒ 401. That decision reads no + // authorization dimension, so it needs no authorization envelope. + const anonymous: ShareLinkExecutionContext = {}; + const authenticated: ShareLinkExecutionContext = { userId: 'usr_1', positions: ['sales'] }; + expect(anonymous.userId).toBeUndefined(); + expect(authenticated.userId).toBe('usr_1'); + + // The honest half. This assignment is LEGAL and compiles — five optional + // fields, all present in the wider type. So the enforcement boundary is + // held by the declared parameter type and the caller's obligation, not by + // tsc; an `@ts-expect-error` here would be unsatisfied and fail the build. + // What the contract change buys is that the narrowing is now visible where + // it happens — the call site names `ExecutionContext` and a reviewer can + // see a five-field object being handed to it — instead of being hidden + // behind a parameter type that looked purpose-built for the job. + const widened: ExecutionContext = authenticated; + expect(widened.userId).toBe('usr_1'); + expect(widened.accessible_org_ids).toBeUndefined(); + }); +}); diff --git a/packages/spec/src/contracts/share-link-service.ts b/packages/spec/src/contracts/share-link-service.ts index ce458b8684..53ae37ee73 100644 --- a/packages/spec/src/contracts/share-link-service.ts +++ b/packages/spec/src/contracts/share-link-service.ts @@ -30,8 +30,16 @@ * access only to its single `(object, recordId)` tuple at the * declared `permission` level. This keeps audit trails clean and * prevents lateral movement. + * + * 5. **Enforcement runs on the FULL envelope.** Every method that + * adjudicates access takes a complete {@link ExecutionContext} — the + * whole `resolveAuthzContext` envelope, not a per-site subset. See + * {@link ShareLinkExecutionContext} for the boundary this draws and + * why (#6206 / #6430). */ +import type { ExecutionContext } from '../kernel/execution-context.zod.js'; + /** Levels selectable when issuing a link. */ export type ShareLinkPermission = 'view' | 'comment' | 'edit'; @@ -96,16 +104,65 @@ export interface ResolveShareLinkResult { redactFields: string[]; } -/** Minimal context interface — kept compatible with `SharingExecutionContext`. */ +/** + * ROUTE-LOCAL identity shape — what the share-link HTTP routes need to answer + * **"is this request authenticated at all?"** (their own 401), and nothing + * more. + * + * ## ⛔ Never an enforcement context (#6206, maintainer ruling 2026-08-07) + * + * This type must never reach a path that ADJUDICATES access — no + * `engine.find` / `engine.update` `context`, no visibility probe, no + * capability gate. Those take a full {@link ExecutionContext}; see + * {@link IShareLinkService}, whose every context parameter is now that type. + * + * The reason is a measured failure, not a style preference. The share-link + * route used to assemble exactly these five fields out of the + * `resolveAuthzContext` envelope and hand the result straight to `engine.find` + * as the [Finding-2] visibility check's context. Dropped on the floor: + * `accessible_org_ids`, `org_user_ids`, `systemPermissions`, `posture`, + * `tabPermissions`. Under the `group` tenancy posture `accessible_org_ids` IS + * the Layer 0 wall (ADR-0105 D2) and an absent set denies — so the check + * failed closed for every caller and share-link creation returned a blanket + * 403 on a posture that ships. A trimmed envelope feeding enforcement is a + * bypass-SHAPED pattern even when today's instance happens to fail closed; + * ADR-0095 D2 already rules that posture is resolved once and flows with the + * context, never re-derived (or re-assembled) at the enforcement site. This + * was the third assembly site of that family (#5997, #6071), which is why the + * governance default converged on the whole envelope instead of yet another + * per-site subset. + * + * ## What it is still legitimately for + * + * A route handler that only has to decide *authenticated vs anonymous* before + * dispatching — `userId` present ⇒ proceed, absent ⇒ 401. That decision reads + * no authorization dimension, so it needs no authorization envelope. Anything + * past that gate hands the request's **complete** resolved context down. + * + * ## A note for implementors and reviewers + * + * TypeScript cannot police this boundary for you. Structural subtyping makes a + * value of this type assignable to {@link ExecutionContext} — every field here + * exists there with a compatible type, and the ones that matter are optional — + * so a narrowed context passed to an enforcement path still COMPILES. What the + * contract can do, and now does, is state the enforcement parameter type as + * the full envelope so the narrowing is visible at the call site instead of + * hidden behind a type that looked like it was designed for the job. Producing + * that envelope is the caller's obligation: pass the `resolveAuthzContext` + * result through whole. + */ export interface ShareLinkExecutionContext { userId?: string; tenantId?: string; isSystem?: boolean; /** * [Finding-2] The caller's resolved positions / permissions. Populated by the - * verified route wiring so RLS-aware checks (e.g. "can this caller actually - * read the record being shared?") evaluate against the real principal rather - * than a spoofable header identity. + * verified route wiring so the route's own principal checks read the real + * identity rather than a spoofable header. + * + * These are NOT sufficient for an RLS-aware visibility check — that check + * lives on {@link IShareLinkService} and takes the full + * {@link ExecutionContext}. */ positions?: string[]; permissions?: string[]; @@ -117,16 +174,47 @@ export interface ShareLinkExecutionContext { * Implementations MUST treat `context.isSystem === true` as a bypass * (skip the per-object opt-in check) so platform bootstrappers can seed * demo links. + * + * ## The context every method takes (#6206 ruling, #6430) + * + * `createLink` / `revokeLink` / `listLinks` all ADJUDICATE access, so each + * takes a full {@link ExecutionContext} — the complete `resolveAuthzContext` + * envelope, threaded through unchanged. Callers MUST NOT rebuild a subset of + * it: `accessible_org_ids` (the `group`-posture Layer 0 wall, ADR-0105 D2), + * `org_user_ids`, `systemPermissions`, `posture` (ADR-0095 D2: resolved once, + * carried, never re-derived at enforcement) and `tabPermissions` are all read + * downstream of these calls, and a caller cannot know which of them the + * deployment's posture makes load-bearing. + * + * {@link ShareLinkExecutionContext} is the route's own 401 shape and is + * deliberately NOT accepted here — its doc comment carries the full rationale. */ export interface IShareLinkService { - /** Mint a new link. Throws when the object is not opt-in or limits are exceeded. */ - createLink(input: CreateShareLinkInput, context: ShareLinkExecutionContext): Promise; + /** + * Mint a new link. Throws when the object is not opt-in or limits are exceeded. + * + * ENFORCEMENT PATH: implementations re-read the target record under + * `context` ([Finding-2] — you may only link-share a record you can + * yourself see), so `context` must be the caller's complete resolved + * envelope. A trimmed one silently changes the verdict of that read. + */ + createLink(input: CreateShareLinkInput, context: ExecutionContext): Promise; - /** Mark a link as revoked. No-op when already revoked or not found. */ - revokeLink(idOrToken: string, context: ShareLinkExecutionContext): Promise; + /** + * Mark a link as revoked. No-op when already revoked or not found. + * + * ENFORCEMENT PATH: revoke authority is creator ∪ record share-manager + * (ADR-0111 D8), and the share-manager probe evaluates `context`. + */ + revokeLink(idOrToken: string, context: ExecutionContext): Promise; - /** List links for a record, an object, or a creator. */ - listLinks(filter: ListShareLinksFilter, context: ShareLinkExecutionContext): Promise; + /** + * List links for a record, an object, or a creator. + * + * ENFORCEMENT PATH: the listing is read under `context`, so row visibility + * is decided by it. + */ + listLinks(filter: ListShareLinksFilter, context: ExecutionContext): Promise; /** * Resolve a token at request-handling time. Returns null when the