From 72a700675ff813b16d72fbb9b7bbc925e8b9bf7a Mon Sep 17 00:00:00 2001 From: ObjectStack Agent Date: Sat, 8 Aug 2026 13:18:38 +0000 Subject: [PATCH] fix(runtime): /share-links denial keeps its own 403 through the domain catch (#6649) Route the domain's unified catch through the dispatcher's shared `errorFromThrown` mapper, which reads `status` OR `statusCode`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017uFVNMmTxLpmfQYiuKM1Yx --- .../share-links-denial-status-passthrough.md | 59 +++++ .../share-links-enforcement-context.test.ts | 202 +++++++++++++++++- packages/runtime/src/domains/share-links.ts | 37 +++- 3 files changed, 291 insertions(+), 7 deletions(-) create mode 100644 .changeset/share-links-denial-status-passthrough.md diff --git a/.changeset/share-links-denial-status-passthrough.md b/.changeset/share-links-denial-status-passthrough.md new file mode 100644 index 0000000000..7b9d620401 --- /dev/null +++ b/.changeset/share-links-denial-status-passthrough.md @@ -0,0 +1,59 @@ +--- +"@objectstack/runtime": minor +--- + +fix(runtime): a `/share-links` permission denial answers 403, not 500 (#6649) + +The dispatcher's `/share-links` domain ended in a hand-written catch that read +one status channel: + +``` +return sendErr(err?.status ?? 500, err?.code ?? 'INTERNAL', err?.message ?? '…'); +``` + +Every refusal `ShareLinkService` raises itself carries `status` (its `makeError` +sets `status` + `code`), which is why the 403 `FORBIDDEN` and 422 +`SHARING_NOT_ENABLED` answers were always correct. But the refusals that come +out of the **security middleware** do not come from that service. Creating a +link performs a visibility read — `svc.createLink` calls +`engine.find(object, { context })` — and when the caller's permission sets grant +no `allowRead` on the object, the CRUD gate throws +`PermissionDeniedError { code = 'PERMISSION_DENIED'; statusCode = 403 }`, a class +with **no `status` field at all** (`plugin-security/src/errors.ts`; runtime's own +mirror in `security/resolve-execution-context.ts` has the same shape). +`ShareLinkService` does not catch it, so it reached the domain catch, `err?.status` +was `undefined`, and a 403-class refusal left as **HTTP 500** while `error.code` +faithfully read `PERMISSION_DENIED`. + +That envelope contradicted itself, and the contradiction is load-bearing on the +client: 5xx is retryable to many SDKs and browser clients, so a permanent +authorization answer was being retried, and a caller branching on the status saw +"the server is broken" where the truth was "you may not read this record". It is +reproducible on either tenancy posture, and — because `registerShareLinkRoutes: +false` makes this domain the ONLY share-link surface on cloud's per-environment +kernels — it is the primary surface there, not a fallback one. + +The catch now exits through `deps.errorFromThrown`, the dispatcher's shared +thrown-error mapper that `/meta`, `/actions` and `/mcp` already use. It reads +`status` **or** `statusCode`, and it carries a thrown error's structured +`issues` / `fields` details through instead of collapsing them to a message. +Reaching for the shared mapper — rather than widening the hand-written chain to +`err?.status ?? err?.statusCode ?? 500` — is the part that stops this exit +re-diverging: a second hand-written copy is how the two drifted apart in the +first place. + +Two wire-visible consequences, both corrections: + +- A permission denial on `POST` / `GET` / `DELETE /share-links` answers **403 + `PERMISSION_DENIED`** where it answered 500 `PERMISSION_DENIED`. Clients + treating 5xx as retryable stop retrying a permanent refusal. +- A throw carrying neither status channel nor a code answers **500 + `INTERNAL_ERROR`** where it answered 500 `INTERNAL`. `'INTERNAL'` was never + registered for `@objectstack/runtime` in `ERROR_CODE_LEDGER` (only `rest`, + `service-storage`, `service-i18n` and `plugin-sharing` register it, and the + ledger's per-package rows are provenance) — so this domain was emitting a code + it had not registered, and the required field is now filled by the catalogued + derivation every other dispatcher exit uses (ADR-0112). + +Refusals that already carried `status` are untouched: the mapper reads that +channel on the same first branch the old chain did. diff --git a/packages/runtime/src/domains/share-links-enforcement-context.test.ts b/packages/runtime/src/domains/share-links-enforcement-context.test.ts index d67f85912c..460c1b3286 100644 --- a/packages/runtime/src/domains/share-links-enforcement-context.test.ts +++ b/packages/runtime/src/domains/share-links-enforcement-context.test.ts @@ -52,10 +52,12 @@ import { PermissionSetSchema } from '@objectstack/spec/security'; import type { PermissionSet } from '@objectstack/spec/security'; import type { ExecutionContext } from '@objectstack/spec/kernel'; import { SHARE_LINK_SERVICE } from '@objectstack/spec/contracts'; -import { SecurityPlugin } from '@objectstack/plugin-security'; +import { PermissionDeniedError, SecurityPlugin } from '@objectstack/plugin-security'; import { ShareLinkService } from '@objectstack/plugin-sharing'; +import { ApiErrorSchema, BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import { apiErrorResponse } from '../error-envelope.js'; import { handleShareLinksRequest } from './share-links.js'; +import { HttpDispatcher } from '../http-dispatcher.js'; import type { HttpProtocolContext } from '../http-dispatcher.js'; import type { DomainHandlerDeps } from '../domain-handler-registry.js'; @@ -121,6 +123,24 @@ const EAST_VIEWER: PermissionSet = PermissionSetSchema.parse({ ], }); +/** + * [#6649] The CRUD-gate denial's input: an object grant with NO `allowRead`. + * + * The baseline is ADDITIVE for every human principal (ADR-0090 D5 — the + * `fallbackPermissionSet` applies IN ADDITION to whatever else resolved), so a + * caller only reaches the CRUD gate's deny branch when the baseline itself + * withholds read. This set is therefore wired as BOTH the caller's explicit set + * and the fallback in the #6649 cases below: `allowCreate` alone, so + * `checkObjectPermission('find', 'crm_account', …)` is false and the security + * middleware throws `PermissionDeniedError` — the `statusCode`-only shape whose + * status the domain used to drop. + */ +const ACCT_NO_READ: PermissionSet = PermissionSetSchema.parse({ + name: 'acct_no_read', + label: 'Account create-only (no read grant)', + objects: { crm_account: { allowCreate: true } }, +}); + const PERMISSION_SETS = [ACCT_MEMBER, EAST_VIEWER]; function matches(row: any, filter: any): boolean { @@ -216,13 +236,21 @@ function makeEngine(tables: Record) { * posture arrives the production way — via the `tenancy` service (ADR-0093 * D4 / ADR-0105 D1). */ -async function bootSecurity(engine: any, posture: 'single' | 'group'): Promise { +async function bootSecurity( + engine: any, + posture: 'single' | 'group', + // [#6649] The permission-set world and its additive baseline are parameters + // now; the defaults are byte-for-byte what every #6551 case above booted + // with, so those verdicts are untouched. + sets: PermissionSet[] = PERMISSION_SETS, + fallback: string = 'acct_member', +): Promise { const services: Record = { manifest: { register: vi.fn() }, objectql: engine, metadata: { get: async (_type: string, name: string) => (name === OBJECT ? ACCOUNT_SCHEMA : null), - list: async () => PERMISSION_SETS, + list: async () => sets, }, tenancy: { posture }, }; @@ -236,11 +264,11 @@ async function bootSecurity(engine: any, posture: 'single' | 'group'): Promise { + const dispatcher: any = new HttpDispatcher({ context: { getService: () => null } } as any); + return (e: any, fallbackStatus?: number) => dispatcher.errorFromThrown(e, fallbackStatus); +})(); + function makeDeps(engine: any, svc: any): DomainHandlerDeps { const deps: any = { resolveService: async (_c: any, name: string) => @@ -284,6 +330,7 @@ function makeDeps(engine: any, svc: any): DomainHandlerDeps { // ADR-0112 shape, not a lookalike. error: (message: string, httpStatus = 500, details?: any) => apiErrorResponse({ message, httpStatus, details }), routeNotFound: (route: string) => apiErrorResponse({ message: `Route not found: ${route}`, httpStatus: 404 }), + errorFromThrown: realErrorFromThrown, }; return deps as DomainHandlerDeps; } @@ -295,6 +342,10 @@ interface MintOptions { posture: 'single' | 'group'; envelope: ExecutionContext | undefined; records: any[]; + /** [#6649] The permission-set world to boot; defaults to the #6551 one. */ + permissionSets?: PermissionSet[]; + /** [#6649] The additive baseline; defaults to the #6551 `acct_member`. */ + fallbackPermissionSet?: string; } /** @@ -305,7 +356,7 @@ interface MintOptions { async function mintOnDispatcher(opts: MintOptions): Promise<{ status: number; body: any }> { const tables: Record = { [OBJECT]: opts.records, sys_share_link: [], sys_permission_set: [] }; const engine = makeEngine(tables); - await bootSecurity(engine, opts.posture); + await bootSecurity(engine, opts.posture, opts.permissionSets, opts.fallbackPermissionSet); const svc = new ShareLinkService({ engine: engine as any }); const deps = makeDeps(engine, svc); const res = await handleShareLinksRequest( @@ -468,3 +519,142 @@ describe('[#6551] the dispatcher seam itself', () => { expect(svc.createLink).not.toHaveBeenCalled(); }); }); + +/** + * [#6649] The domain's unified catch and the two channels a thrown refusal + * carries its HTTP status on. + * + * The catch read `err?.status ?? 500` only. Every refusal `ShareLinkService` + * raises itself carries `status` (its `makeError` sets `status` + `code`), which + * is why the `403 FORBIDDEN` cases above were already correct and stayed correct + * — but the SECURITY middleware's refusals do not come from that service. The + * CRUD gate throws `PermissionDeniedError { code: 'PERMISSION_DENIED'; + * statusCode: 403 }` — no `status` field at all — straight out of + * `svc.createLink`'s visibility read, past a service that does not catch it, into + * this catch. `err?.status` was `undefined`, so the 403 left as a **500** while + * `code` still read `PERMISSION_DENIED`: an envelope that contradicts itself, and + * a status many clients treat as retryable when the answer is permanent. + * + * The fix routes the catch through `deps.errorFromThrown` — the dispatcher's + * shared mapper, which `/meta`, `/actions` and `/mcp` already exit through and + * which reads `status` OR `statusCode`. These cases assert `status` AND `code` + * together on purpose: a denial arriving as 403 under the wrong code would be + * just as wrong as the 500, and only the pair separates them. + */ + +/** The ADR-0112 envelope checks every case below shares. */ +function expectDeclaredEnvelope(res: { status: number; body: any }): any { + expect(BaseResponseSchema.safeParse(res.body).success).toBe(true); + expect(envelopeViolations(res.body), `not the declared envelope: ${JSON.stringify(res.body)}`).toEqual([]); + const parsed = ApiErrorSchema.safeParse(res.body.error); + // `ApiErrorSchema.code` validates against the CLOSED set (StandardErrorCode ∪ + // ERROR_CODE_LEDGER), so this is also what keeps the code out of the + // free-string space the old `'INTERNAL'` fallback sat in. + expect(parsed.error?.issues ?? []).toEqual([]); + expect(res.body.error.httpStatus).toBe(res.status); + return res.body.error; +} + +/** A `/share-links` call served by a service double that throws `thrown`. */ +async function refusalFromService( + thrown: unknown, + verb: 'POST' | 'GET' | 'DELETE' = 'POST', +): Promise<{ status: number; body: any }> { + const boom = async () => { throw thrown; }; + const svc = { + createLink: vi.fn(boom), + listLinks: vi.fn(boom), + revokeLink: vi.fn(boom), + resolveToken: vi.fn(async () => null), + }; + const deps = makeDeps(makeEngine({ [OBJECT]: [], sys_share_link: [] }), svc); + const res = await handleShareLinksRequest( + deps, + verb === 'DELETE' ? '/shl_x' : '', + verb, + verb === 'POST' ? { object: OBJECT, recordId: RECORD } : undefined, + {}, + httpContext(envelopeFor({ memberOf: [ORG_A], permissions: ['acct_member'] })), + ); + if (!res.handled || !res.response) throw new Error(`${verb} /share-links was not handled`); + return res.response as { status: number; body: any }; +} + +/** The caller of the repro: create-only grant, so the visibility read is denied. */ +const noReadCaller = (posture: 'single' | 'group') => ({ + posture, + envelope: envelopeFor({ memberOf: [ORG_A], permissions: ['acct_no_read'] }), + records: ownRecordInA(), + permissionSets: [ACCT_NO_READ], + fallbackPermissionSet: 'acct_no_read', +}); + +describe('[#6649] a security-middleware refusal keeps its own status through the domain catch', () => { + it('single posture: no allowRead on the object answers 403 PERMISSION_DENIED (was 500 + PERMISSION_DENIED)', async () => { + const res = await mintOnDispatcher(noReadCaller('single')); + + // The pair, together. Before the fix `code` was ALREADY + // `PERMISSION_DENIED` here — only the status was wrong — so a case + // asserting the code alone was green on the defect, and one asserting + // "not 500" alone could not tell a right refusal from a wrong one. + expect(res.status).toBe(403); + expect(expectDeclaredEnvelope(res).code).toBe('PERMISSION_DENIED'); + // Coverage, NOT a discriminator: the message does not move between the + // two directions of this fix (the pre-fix 500 exited through this + // harness's `error` double, which has no leak guard, and the security + // message trips no clause of `looksLikeInternalErrorLeak` anyway). It + // pins the refusal's own reason against a FUTURE widening of that + // heuristic swallowing an authorization answer. + expect(res.body.error.message).toContain('Access denied'); + }, 30_000); + + it('group posture: the same denial, the same envelope — the defect was never posture-specific', async () => { + const res = await mintOnDispatcher(noReadCaller('group')); + + expect(res.status).toBe(403); + expect(expectDeclaredEnvelope(res).code).toBe('PERMISSION_DENIED'); + }, 30_000); + + it('the catch is shared, so list and revoke answer the statusCode-only refusal identically', async () => { + // Driven with a service double raising the REAL `PermissionDeniedError` + // (the production class, `statusCode` and no `status`), because what is + // under test here is the CATCH, not a second trip through the middleware. + for (const verb of ['GET', 'DELETE'] as const) { + const res = await refusalFromService( + new PermissionDeniedError(`[Security] Access denied: operation on object '${OBJECT}'`), + verb, + ); + expect(res.status, `${verb} status`).toBe(403); + expect(expectDeclaredEnvelope(res).code, `${verb} code`).toBe('PERMISSION_DENIED'); + } + }); + + it('a throw carrying neither channel still answers 500 — under the CATALOGUED code, not the unregistered `INTERNAL`', async () => { + const res = await refusalFromService(new Error('share link storage exploded')); + + expect(res.status).toBe(500); + // `'INTERNAL'` is registered in `ERROR_CODE_LEDGER` for `rest` / + // `service-storage` / `service-i18n` / `plugin-sharing` — never for + // `@objectstack/runtime`. The union dedupes, so the schema stayed green + // while this domain emitted a code it had not registered; the shared + // mapper answers with the derived `INTERNAL_ERROR` instead. + expect(expectDeclaredEnvelope(res).code).toBe('INTERNAL_ERROR'); + }); + + it('a refusal that DOES carry `status` is untouched — the fix widens the channel, it does not switch it', async () => { + // Honest note: this case is green in BOTH directions of the fix, by + // construction — `ShareLinkService.makeError` sets `status`, which the + // old chain read first and the shared mapper reads first. It is not a + // regression pin for #6649 but for the NEXT change to this exit: it goes + // red if the `status` channel is ever dropped in favour of `statusCode`. + const res = await refusalFromService( + Object.assign(new Error('Sharing is not enabled for this object'), { + status: 422, + code: 'SHARING_NOT_ENABLED', + }), + ); + + expect(res.status).toBe(422); + expect(expectDeclaredEnvelope(res).code).toBe('SHARING_NOT_ENABLED'); + }); +}); diff --git a/packages/runtime/src/domains/share-links.ts b/packages/runtime/src/domains/share-links.ts index 73c65abeb2..55ee86fa8d 100644 --- a/packages/runtime/src/domains/share-links.ts +++ b/packages/runtime/src/domains/share-links.ts @@ -264,6 +264,41 @@ export async function handleShareLinksRequest( return { handled: true, response: deps.routeNotFound(`/share-links${subPath}`) }; } catch (err: any) { - return sendErr(err?.status ?? 500, err?.code ?? 'INTERNAL', err?.message ?? 'Share link request failed'); + // [#6649] The dispatcher's SHARED thrown-error mapper, not a hand-written + // status read. This catch used to be + // `sendErr(err?.status ?? 500, err?.code ?? 'INTERNAL', …)`, and the two + // channels it collapsed are the whole defect: + // + // 1. **Status.** The refusals that actually fly out of the enforcement + // paths below carry `statusCode`, not `status`: + // `PermissionDeniedError { code = 'PERMISSION_DENIED'; statusCode = 403 }` + // (`plugin-security/src/errors.ts`, mirrored by runtime's own + // `security/resolve-execution-context.ts`) is thrown by the security + // middleware's CRUD gate when the caller's permission sets grant no + // `allowRead` on the object — so `svc.createLink`'s visibility read + // `engine.find(object, { context })` throws it, `ShareLinkService` + // does not catch it, and it lands here. `err?.status` was `undefined` + // on it, so a 403-class refusal left as a **500** while `code` read + // `PERMISSION_DENIED` — an envelope that contradicts itself, and a + // status many SDK/browser clients treat as retryable when the answer + // is permanent. `errorFromThrown` reads `status` OR `statusCode`. + // 2. **Code.** The `'INTERNAL'` fallback is not registered for + // `@objectstack/runtime` in `ERROR_CODE_LEDGER` — only + // `service-storage` / `service-i18n` / `rest` / `plugin-sharing` + // register it, and the ledger's per-package rows are provenance, so + // the global union kept `ApiErrorSchema` green while this domain + // emitted a code it never registered. The shared mapper leaves the + // required field to `standardErrorCodeForHttpStatus` instead, which + // spells the catalogued `INTERNAL_ERROR` (ADR-0112) — the same + // derived code every other dispatcher exit already answers with. + // + // `ShareLinkService`'s own refusals are unaffected: its `makeError` sets + // `err.status` + `err.code`, which the mapper reads on the same first + // branch the old chain did (403 `FORBIDDEN`, 422 `SHARING_NOT_ENABLED`, + // …). What it adds on top is the structured `issues` / `fields` detail + // the `/meta` and `/actions` domains already carry through this exit — + // which is the point: a hand-written catch is exactly how this domain + // diverged from the shared mapper in the first place. + return { handled: true, response: deps.errorFromThrown(err, 500) }; } }