From 017201d47ae715e47a4cf553157a7c5be3e3a91f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 17:10:56 +0000 Subject: [PATCH] fix(rest): meta app by-name answers a permission-denied envelope, not absence `GET /api/v1/meta/app/` collapsed three different refusals into one 404-equivalent, so an app the session may never open and an app that does not exist were byte-identical on the wire. The console has nothing to branch on and renders its only copy for an absent app -- "it may still be publishing" -- over a permanent authorization denial. Measured cost on objectui#4252: two acceptance-test batches chasing a "platform defect" that was a missing permission-set binding. `filterAppForUser` now delegates to `filterAppForUserWithReason`, which reports WHICH gate fired. Exactly one of them converts: an app that EXISTS and whose `requiredPermissions` the session lacks answers 403 `PERMISSION_DENIED` (ADR-0112 standard catalog) in the declared envelope, written through the shared `sendError` from `@objectstack/types`. Everything else keeps answering absence, and the reason is taken from the branch that fired rather than inferred from `null` at the call site: - `_unpublished` -- ADR-0045 section 3 makes it externally unobservable, and a 403 confirms existence (#4829 pinned 404-over-403); - `requiresService` -- ADR-0057 D10 capability absence is a deployment fact, not a denial to this caller; - a nonexistent name -- converting it would make every app name on the platform enumerable, a different and unruled change; - the list route `GET /meta/apps` stays filtered exactly as-is, with no `authorized: false` leakage. All four acceptance criteria pinned in meta-app-publish-gate.test.ts on `status` AND `code`, plus the two partition cases. Five reversals confirm each pin goes red for its own reason; the naive "any null => 403" reddens four at once, including #4829's own unpublished-app pin. Fixes #8013 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V --- .../meta-app-by-name-denied-envelope.md | 29 +++ .../rest/src/meta-app-publish-gate.test.ts | 209 +++++++++++++++++- packages/rest/src/rest-server.ts | 116 +++++++++- 3 files changed, 341 insertions(+), 13 deletions(-) create mode 100644 .changeset/meta-app-by-name-denied-envelope.md diff --git a/.changeset/meta-app-by-name-denied-envelope.md b/.changeset/meta-app-by-name-denied-envelope.md new file mode 100644 index 0000000000..596c4af586 --- /dev/null +++ b/.changeset/meta-app-by-name-denied-envelope.md @@ -0,0 +1,29 @@ +--- +'@objectstack/rest': patch +--- + +`GET /api/v1/meta/app/`: report a permission denial instead of absence + +An app the session lacks the `requiredPermissions` for used to answer the same +404-equivalent as an app that does not exist, so the two were byte-identical on +the wire. A console has nothing to branch on and renders its only copy for an +absent app — "it may still be publishing" — over a permanent authorization +denial. + +The by-name route now answers `403` with the ADR-0112 standard catalog code +`PERMISSION_DENIED`, in the declared envelope +(`{ success: false, error: { code, message } }`), when the app EXISTS and the +session lacks its `requiredPermissions`. + +Deliberately unchanged, because the disclosure is licensed only for the case +above: + +- a **nonexistent** app name keeps answering absence — converting it too would + make every app name on the platform enumerable; +- an **unpublished** app keeps answering `404` (ADR-0045 §3 makes it externally + unobservable, and a 403 confirms existence); +- an app withheld by an absent optional service (ADR-0057 D10) keeps answering + `404` — nothing was denied to the caller; +- the **list** route `GET /meta/apps` stays filtered exactly as before, with no + `authorized: false` flag, so the enumeration surface is not widened past what a + direct by-name probe already implies. diff --git a/packages/rest/src/meta-app-publish-gate.test.ts b/packages/rest/src/meta-app-publish-gate.test.ts index 4140cc34ea..1cd516d67b 100644 --- a/packages/rest/src/meta-app-publish-gate.test.ts +++ b/packages/rest/src/meta-app-publish-gate.test.ts @@ -54,8 +54,35 @@ const CRM_APP = { navigation: [{ id: 'nav_leads', type: 'object', objectName: 'lead' }], }; +/** + * [#8013] A PUBLISHED app the session may only open with a capability it might + * not hold. Its `requiredPermissions` is a different layer from ADR-0045 §3's + * publish gate above, and the ONLY one this card converts into a denial. + */ +const FINANCE_APP = { + name: 'finance', + label: 'Finance', + requiredPermissions: ['finance.access'], + navigation: [{ id: 'nav_invoices', type: 'object', objectName: 'invoice' }], +}; + +/** + * [#8013] A published app gated by ADR-0057 D10 capability presence rather than + * by anything about the caller. Kept out of `ALL_APPS` for the same reason + * `FINANCE_APP` is: the #4829 / #7566 suites above pin exact name lists. + */ +const OPTIONAL_SERVICE_APP = { + name: 'telephony', + label: 'Telephony', + requiresService: 'voice', + navigation: [{ id: 'nav_calls', type: 'object', objectName: 'call_log' }], +}; + const ALL_APPS = [ACCOUNT_APP, UNPUBLISHED_APP, CRM_APP]; +/** [#8013] `ALL_APPS` plus the two extra gate shapes this card partitions. */ +const GATED_APPS = [ACCOUNT_APP, UNPUBLISHED_APP, CRM_APP, FINANCE_APP, OPTIONAL_SERVICE_APP]; + function createMockServer() { return { get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(), @@ -71,18 +98,22 @@ function makeRes() { return res; } -/** @param perms system permissions the caller holds */ -function setup(perms: string[]) { +/** + * @param perms system permissions the caller holds + * @param apps the app corpus this server serves. Defaults to `ALL_APPS` so the + * #4829 / #7566 suites keep their exact name lists; #8013 passes `GATED_APPS`. + */ +function setup(perms: string[], apps: any[] = ALL_APPS, serviceExists?: (name: string) => boolean) { const protocol: any = { getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' } }), getMetaTypes: vi.fn().mockResolvedValue([]), // Deep-clone per call: the filter must never mutate stored metadata. getMetaItems: vi.fn(async ({ type }: any) => { const t = String(type ?? ''); - return t === 'app' || t === 'apps' ? JSON.parse(JSON.stringify(ALL_APPS)) : []; + return t === 'app' || t === 'apps' ? JSON.parse(JSON.stringify(apps)) : []; }), getMetaItem: vi.fn(async ({ name }: any) => { - const found = ALL_APPS.find((a) => a.name === name); + const found = apps.find((a: any) => a.name === name); return found ? { type: 'app', name, item: JSON.parse(JSON.stringify(found)) } : undefined; }), findData: vi.fn().mockResolvedValue([]), @@ -91,6 +122,12 @@ function setup(perms: string[]) { // The RBAC filter only runs for a resolved caller; stubbing the context is // the established pattern in this package for exercising it by route. rest.resolveExecCtx = async () => ({ userId: 'u1', systemPermissions: perms }); + // [#8013] ADR-0057 D10's gate FAILS OPEN when it cannot be probed, and the + // stubbed context carries no `__kernel`, so without a provider a + // `requiresService` app is simply served and the `service` branch never + // runs. Assigned rather than threaded through the constructor's 16th + // positional parameter. + if (serviceExists) rest.serviceExistsProvider = serviceExists; rest.registerRoutes(); return { rest, protocol }; } @@ -328,3 +365,167 @@ describe('#7566 — `GET /meta/app?id=` narrows the list instead of being droppe expect(namesFrom(res.body)).toEqual(['all_leads', 'my_leads']); }); }); + +// ── #8013 ─────────────────────────────────────────────────────────────────── +// +// `GET /meta/app/` answered ONE shape for three different refusals, so an +// app the session may never open and an app that does not exist were byte- +// identical on the wire. The console has nothing to branch on, so it renders its +// only copy for an absent app — "it may still be publishing" — over a permanent +// authorization denial. Measured cost (objectui#4252): two acceptance-test +// batches spent chasing a "platform defect" that was a missing permission-set +// binding. +// +// The maintainer ruling (2026-08-12) converts exactly ONE of the three: an app +// that EXISTS and whose `requiredPermissions` the session lacks now answers +// `403 PERMISSION_DENIED`. The other two keep answering absence, and the cases +// below pin that partition rather than assuming it, because it is the security +// boundary of the change: +// +// - a DENIAL makes an app's existence observable to a caller who may not use +// it. The ruling accepts that for a by-name probe, which already implies the +// name. +// - extending it to a name that resolves to NOTHING would make every app name +// on the platform enumerable. That is a different, unruled change, and the +// nonexistent-name cases are what stand between the two. +// +// So every case here asserts `status` AND `code` (ADR-0112), never "an error +// came back" — the two answers under test are both errors, one apart. + +/** The declared refusal envelope, as the console will read it. */ +const refusal = (body: any) => ({ code: body?.error?.code, message: body?.error?.message }); + +describe('#8013 — by-name: a permission denial is REPORTED, absence still is not', () => { + it('criterion 1: a session WITHOUT the capability gets a named 403, not an absence', async () => { + const { rest } = setup(['manage_users'], GATED_APPS); + const denied = await getItem(rest, 'finance'); + + // The whole point of the card: `status` AND `code`, so objectui#4252 can + // branch. `PERMISSION_DENIED` is the ADR-0112 STANDARD catalog member for + // a generic authorization refusal (and what `standardErrorCodeForHttpStatus` + // answers for 403) rather than a bespoke synonym. + expect(denied.statusCode).toBe(403); + expect(refusal(denied.body).code).toBe('PERMISSION_DENIED'); + // The DECLARED envelope (`BaseResponseSchema`) — `code` and `message` + // nested INSIDE `error`, with the `success: false` flag. The console + // therefore reads `body.error.code` here and on the 404 below: one + // accessor for both answers, not a second dialect to special-case. + expect(denied.body?.success).toBe(false); + expect(typeof refusal(denied.body).message).toBe('string'); + expect(refusal(denied.body).message).toContain('finance'); + + // Refused means refused: the document did not ride along with the + // refusal, and neither did the objects it would have exposed. + expect(denied.body?.item).toBeUndefined(); + expect(denied.body?.data).toBeUndefined(); + expect(JSON.stringify(denied.body ?? {})).not.toContain('invoice'); + }); + + it('criterion 2: a session WITH the capability gets the app, unchanged', async () => { + const { rest } = setup(['finance.access'], GATED_APPS); + const allowed = await getItem(rest, 'finance'); + + expect(allowed.statusCode).toBe(200); + expect(allowed.body).toMatchObject({ type: 'app', name: 'finance' }); + expect(allowed.body?.item?.name).toBe('finance'); + // Including the navigation the denial withheld — the grant path is the + // half a refusal change most easily breaks. + expect(allowed.body?.item?.navigation?.map((n: any) => n.id)).toEqual(['nav_invoices']); + expect(allowed.body?.error).toBeUndefined(); + expect(allowed.body?.success).toBeUndefined(); + }); + + it('criterion 3: a NONEXISTENT app name keeps answering absence — never the denial', async () => { + // THE security criterion. If this ever reports `PERMISSION_DENIED`, the + // change has stopped being "an app you may not open says so" and become + // "every app name on the platform is enumerable" — including by a caller + // holding nothing. Asserted on the envelope, not on "still an error": + // both answers are errors. + for (const perms of [[], ['manage_users'], ['finance.access'], ['studio.access']]) { + const missing = await getItem(setup(perms, GATED_APPS).rest, 'no_such_app'); + + expect(missing.statusCode).not.toBe(403); + expect(refusal(missing.body).code).not.toBe('PERMISSION_DENIED'); + expect(JSON.stringify(missing.body ?? {})).not.toContain('PERMISSION_DENIED'); + } + }); + + it('criterion 3: …and the real producer miss is still the 404 it has always been', async () => { + // The fixture's `getMetaItem` answers `undefined` for an unknown name; + // `metadata-protocol` REJECTS with a declared `RESOURCE_NOT_FOUND` / + // `status: 404` (pinned in `rest-meta-outage-vs-miss.test.ts`). Both + // reach this route, so the criterion is stated against the production + // shape too rather than against the stub's alone. + const { rest, protocol } = setup([], GATED_APPS); + protocol.getMetaItem = vi.fn().mockRejectedValue(Object.assign( + new Error('Metadata item app/no_such_app not found'), + { code: 'RESOURCE_NOT_FOUND', status: 404 }, + )); + + const missing = await getItem(rest, 'no_such_app'); + + expect(missing.statusCode).toBe(404); + expect(missing.body?.code).toBe('RESOURCE_NOT_FOUND'); + expect(missing.statusCode).not.toBe(403); + expect(JSON.stringify(missing.body ?? {})).not.toContain('PERMISSION_DENIED'); + }); + + it('criterion 4: the LIST route is untouched — the app is absent, not flagged', async () => { + const { rest } = setup(['manage_users'], GATED_APPS); + const res = await getList(rest); + + // Read the list body and assert the ABSENCE, not "the endpoint still + // 200s" — a leak would be a 200 too. The ruling keeps this route + // filtered exactly as-is so the enumeration surface is not widened past + // what a direct by-name probe already implies. + expect(res.statusCode).toBe(200); + expect(namesFrom(res.body)).not.toContain('finance'); + expect(namesFrom(res.body).sort()).toEqual(['account', 'crm', 'telephony']); + + // No `authorized: false` (or any sibling spelling) leaked into the list, + // and no trace of what the withheld app would have exposed. + const wire = JSON.stringify(res.body); + expect(wire).not.toContain('authorized'); + expect(wire).not.toContain('PERMISSION_DENIED'); + expect(wire).not.toContain('invoice'); + expect(wire).not.toContain('finance'); + + // …and the holder still gets it, so the list is filtered rather than + // broken. + expect(namesFrom((await getList(setup(['finance.access'], GATED_APPS).rest)).body)) + .toContain('finance'); + }); + + it('the partition: an UNPUBLISHED app stays a 404 even when permissions are the reason too', async () => { + // ADR-0045 §3 says an unpublished app is *externally unobservable*, and a + // 403 confirms existence. The publish gate is judged FIRST, so an app + // that is both unpublished and permission-gated reports absence — the + // stricter contract wins over the new disclosure. + const bothGated = [{ ...UNPUBLISHED_APP, requiredPermissions: ['finance.access'] }]; + const denied = await getItem(setup(['manage_users'], bothGated).rest, 'production_management'); + + expect(denied.statusCode).toBe(404); + expect(refusal(denied.body).code).toBe('RESOURCE_NOT_FOUND'); + expect(refusal(denied.body).code).not.toBe('PERMISSION_DENIED'); + expect(JSON.stringify(denied.body ?? {})).not.toContain('secret_production_line'); + }); + + it('the partition: an app gated by an ABSENT SERVICE stays a 404 — nothing was denied to the caller', async () => { + // ADR-0057 D10 capability absence is a deployment fact about the + // platform, not a statement about this session: no permission of the + // caller's is missing, so there is no denial to report. It keeps + // answering absence. + const { rest } = setup(['manage_users'], GATED_APPS, () => false); + const denied = await getItem(rest, 'telephony'); + + expect(denied.statusCode).toBe(404); + expect(refusal(denied.body).code).toBe('RESOURCE_NOT_FOUND'); + expect(refusal(denied.body).code).not.toBe('PERMISSION_DENIED'); + + // …and it is served once the service is present, so the case above is + // the gate firing rather than the fixture being broken. + const ok = await getItem(setup(['manage_users'], GATED_APPS, (n: string) => n === 'voice').rest, 'telephony'); + expect(ok.statusCode).toBe(200); + expect(ok.body?.item?.name).toBe('telephony'); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 6438aa2303..883d7ab2d7 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -72,6 +72,16 @@ import { effectiveOperationsArray, DATA_ACTION_TO_API_OPERATION, } from '@objectstack/spec/data'; +// [#8013] The SHARED envelope writer (#3973), aliased: this module already has a +// module-scope `sendError` of its own — the sanitizing responder that maps a +// THROWN error onto a status — and the two are not interchangeable. This one +// emits the declared `{ success: false, error: { code, message } }` for a refusal +// the handler DECIDED, with `code` typed to the closed ADR-0112 vocabulary rather +// than `string`. Adding a call site here moves no `check:route-envelope` count: +// the body literal lives in `@objectstack/types` (the pinned `SHARED_BUILDER`), +// and this file is audited `dialectOnly` for the two non-conforming dialects it +// still emits — which this deliberately is not. +import { sendError as sendEnvelopeError } from '@objectstack/types'; /** * The protocol slice the REST layer actually consumes (ADR-0076 D9 / #2462 @@ -3211,7 +3221,59 @@ export class RestServer { * is why this used to sniff the shape. There is one shape now. */ private filterAppForUser(item: any, sysPerms: Set, serviceGate?: (name: string) => boolean): any | null { - if (!item || typeof item !== 'object') return item; + return this.filterAppForUserWithReason(item, sysPerms, serviceGate).app; + } + + /** + * {@link filterAppForUser}, plus WHICH gate withheld the app. + * + * The gate above collapses three different refusals into one `null`, and for + * the LIST route that is exactly right — every one of them means "not in + * your list". The by-name route is where they stop being the same answer + * (#8013). + * + * ## Why the caller needs the reason + * + * `GET /meta/app/` answered a 404-equivalent for all three, so an + * app the session may never use and an app that does not exist were + * BYTE-IDENTICAL on the wire. The console has nothing to branch on, so it + * renders its only copy for an absent app — "it may still be publishing" — + * over a permanent authorization denial. Measured cost (objectui#4252): two + * acceptance-test batches spent chasing a "platform defect" that was a + * missing permission-set binding. + * + * ## Only ONE of the three converts, and that is the whole design + * + * The maintainer ruling (2026-08-12) licenses an explicit denial for + * `permission` alone. The other two keep answering absence, for reasons + * that are not stylistic: + * + * - `unpublished` — ADR-0045 §3 says an unpublished app is *externally + * unobservable*, not merely unlisted. A 403 confirms existence, which is + * precisely what that contract withholds; `meta-app-publish-gate.test.ts` + * has pinned the 404-over-403 choice since #4829 and it stands. + * - `service` — an absent optional kernel service (ADR-0057 D10) is a + * deployment fact about the platform, not a statement about this caller. + * Nothing is denied TO the session, so there is no denial to report. + * + * That partition is the security boundary of #8013, and it cuts one way + * only: a denial for `permission` makes an app the caller may not use + * observable BY NAME, which the ruling accepts because a by-name probe + * already implies the name. Widening it to a name that resolves to nothing + * would make every app name on the platform enumerable — a different and + * unruled change. Hence `withheld` is set from the branch that fired, never + * inferred from `app == null` at the call site. + * + * Ordering is load-bearing for the same reason: `unpublished` is judged + * FIRST, so an app that is both unpublished and permission-gated reports + * `unpublished` and stays absent. ADR-0045 §3 wins over the disclosure. + */ + private filterAppForUserWithReason( + item: any, + sysPerms: Set, + serviceGate?: (name: string) => boolean, + ): { app: any | null; withheld?: 'unpublished' | 'permission' | 'service' } { + if (!item || typeof item !== 'object') return { app: item }; // ADR-0045 §3 (as revised 2026-08, #4829) — the publish gate. An // UNPUBLISHED app is externally unobservable, not merely unlisted: only // builders (studio/setup access) receive it at all, for direct-URL @@ -3229,20 +3291,20 @@ export class RestServer { // system. A hidden app is fully routable and permission-checked here; // only `_unpublished` withholds it. if (item._unpublished === true && !sysPerms.has('studio.access') && !sysPerms.has('setup.access')) { - return null; + return { app: null, withheld: 'unpublished' }; } const reqApp = Array.isArray(item.requiredPermissions) ? item.requiredPermissions : []; if (reqApp.length > 0 && !reqApp.every((p: string) => sysPerms.has(p))) { - return null; + return { app: null, withheld: 'permission' }; } // ADR-0057 D10 — capability gate: hide when the named kernel service is // absent. Fail-open when the gate can't be probed (serviceGate undefined). if (typeof item.requiresService === 'string' && serviceGate && serviceGate(item.requiresService) === false) { - return null; + return { app: null, withheld: 'service' }; } const nav = Array.isArray(item.navigation) ? item.navigation : null; const areas = Array.isArray(item.areas) ? item.areas : null; - if (!nav && !areas) return item; + if (!nav && !areas) return { app: item }; const filterNav = (entries: any[]): any[] => { const out: any[] = []; @@ -3347,9 +3409,11 @@ export class RestServer { }; return { - ...item, - ...(nav ? { navigation: filterNav(nav) } : {}), - ...(areas ? { areas: filterAreas(areas) } : {}), + app: { + ...item, + ...(nav ? { navigation: filterNav(nav) } : {}), + ...(areas ? { areas: filterAreas(areas) } : {}), + }, }; } @@ -5849,8 +5913,42 @@ export class RestServer { ); const registered = await this.resolveRegisteredServices((ctx as any).__kernel, [visible]); const serviceGate = registered ? (n: string) => registered.has(n) : undefined; - visible = this.filterAppForUser(visible, sysPerms, serviceGate); + const gated = this.filterAppForUserWithReason(visible, sysPerms, serviceGate); + visible = gated.app; if (visible == null) { + // [#8013] A PERMISSION denial is reported as + // one — everything else keeps answering + // absence. See + // {@link filterAppForUserWithReason} for why + // only this one of the three gates converts, + // and why the reason comes from the branch + // that fired rather than from `null`. + // + // The condition is generic, so it takes the + // ADR-0112 STANDARD catalog code rather than + // a bespoke synonym — 403 `PERMISSION_DENIED`, + // which is also what + // `standardErrorCodeForHttpStatus(403)` + // answers. objectui#4252 branches on exactly + // this `code`. + // + // Written through the shared `sendError` + // (`@objectstack/types`), aliased because this + // module has a local function of that name. + // That builder emits the DECLARED envelope + // `{ success: false, error: { code, message } }`, + // so the console reads `body.error.code` — the + // same accessor as the absence answer below, + // rather than a second dialect to special-case. + if (gated.withheld === 'permission') { + sendEnvelopeError( + res, + 403, + 'PERMISSION_DENIED', + `You do not have permission to open the '${req.params.name}' app.`, + ); + return; + } res.status(404).json({ error: { code: 'RESOURCE_NOT_FOUND', message: 'Metadata item not found or access denied.' }, });