diff --git a/.changeset/18066-meta-item-absent-404.md b/.changeset/18066-meta-item-absent-404.md new file mode 100644 index 0000000000..e1f55c0c6d --- /dev/null +++ b/.changeset/18066-meta-item-absent-404.md @@ -0,0 +1,22 @@ +--- +'@objectstack/rest': patch +--- + +`GET /api/v1/meta/:type/:name` answers `404 RESOURCE_NOT_FOUND` for a name with nothing behind it, instead of `200` carrying the declared envelope minus its `item` member (#18066). + +Measured on a real server (`examples/app-showcase`, API 17.4.0, four absent names, all identical): + +``` +GET /api/v1/meta/app/no_such_app_xyz +200 {"type":"app","name":"no_such_app_xyz","lock":"none","editable":true,"deletable":true,"resettable":false} +``` + +Two declarations in this repository already said otherwise, and this restores what they declare rather than deciding anything new. `GetMetaItemResponseSchema` — the route's own `responseSchema` — makes `item` a required member; parsing the body above against it fails `invalid_type` / `expected: 'nonoptional'` at `item`. And the **cached** arm of this same route has always answered this condition with `404 RESOURCE_NOT_FOUND`, because `getMetaItemCached` throws on a falsy `item`. Which arm a request took was deciding whether absence was an error at all — `app`, `dashboard`, `doc`, `book`, `?state=draft`, `?preview=draft`, `?package=` and every `enableCache: false` deployment are diverted around the cache. + +- **Every type is affected, not only `app`.** The fall-through sat in the shared tail of the uncached arm, below the per-type gates. The report measured `app` because that type bypasses the cache structurally; a `?state=draft` or `?package=` read of any type reached the same 200. +- ⚠️ **The break was at `JSON.stringify`, not in the producer.** `metadata-protocol`'s `getMetaItem` returns `{ type, name, item: undefined, lock, … }` for a miss — `item` is *present* holding `undefined`, which `z.unknown()` admits — so the returned object conforms and only the serialized body does not. A conformance probe written against the object rather than the wire bytes reports agreement. +- **The permission denial is unchanged.** `403 PERMISSION_DENIED` for an app that exists and whose `requiredPermissions` the session lacks answers exactly as before: the new check is ordered ahead of every gate, and those gates are reachable only by a document that exists, so an absent name can never be converted into a denial. Enumerating app names through the 403 stays impossible. +- **It also closes an enumeration hole in the other direction.** ADR-0045 §3 makes an unpublished app *externally unobservable*, and an unpublished app answered this 404 while a nonexistent name answered the 200 — so the pair of responses reported which app names exist-but-are-unpublished. Both absence answers now come from one emitter and are byte-identical. +- **An unreadable metadata store is still `503`, never this 404.** That distinction is a producer-side throw and never reaches the new check. + +⚠️ **For callers**: a probe that read "the call did not throw" as "this name resolves" now sees the 404 it should always have seen. A caller that read the item-less 200 as a create-vs-edit signal must read the status instead. The console side was already corrected independently (objectui#9262 reads both dialects as absence), so no first-party consumer depends on the old shape. diff --git a/packages/rest/src/meta-app-publish-gate.test.ts b/packages/rest/src/meta-app-publish-gate.test.ts index 1cd516d67b..396922d492 100644 --- a/packages/rest/src/meta-app-publish-gate.test.ts +++ b/packages/rest/src/meta-app-publish-gate.test.ts @@ -112,9 +112,24 @@ function setup(perms: string[], apps: any[] = ALL_APPS, serviceExists?: (name: s const t = String(type ?? ''); return t === 'app' || t === 'apps' ? JSON.parse(JSON.stringify(apps)) : []; }), + // [#18066] The MISS answers what the live producer answers, not + // `undefined`. `metadata-protocol`'s `getMetaItem` resolves the + // protection envelope AROUND a `item: undefined` for a name it cannot + // find — `lock` / `editable` / `deletable` / `resettable` come from + // `resolveLockState` unconditionally — so `undefined` was a shape no + // deployment produces. That mattered: the door read `envelope?.item`, + // which is `undefined` for both, and then fell through to `res.json` + // with the envelope, so the criterion-3 case below was passing over a + // stub that could not exhibit the defect the live provider had. The + // reject-path case further down keeps its own override, so both + // producer shapes reach this route from this file. getMetaItem: vi.fn(async ({ name }: any) => { const found = apps.find((a: any) => a.name === name); - return found ? { type: 'app', name, item: JSON.parse(JSON.stringify(found)) } : undefined; + return { + type: 'app', name, + item: found ? JSON.parse(JSON.stringify(found)) : undefined, + lock: 'none', editable: true, deletable: true, resettable: false, + }; }), findData: vi.fn().mockResolvedValue([]), }; @@ -447,15 +462,46 @@ describe('#8013 — by-name: a permission denial is REPORTED, absence still is n expect(missing.statusCode).not.toBe(403); expect(refusal(missing.body).code).not.toBe('PERMISSION_DENIED'); expect(JSON.stringify(missing.body ?? {})).not.toContain('PERMISSION_DENIED'); + + // [#18066] The POSITIVE half, which this case did not state and + // which is what let the route answer `200` with an item-less + // envelope for years while every assertion above stayed green: + // "not the denial" was satisfied by a SUCCESS just as well as by an + // absence. ADR-0112 — `status` and `code`. + expect(missing.statusCode).toBe(404); + expect(refusal(missing.body).code).toBe('RESOURCE_NOT_FOUND'); + expect(missing.body?.item).toBeUndefined(); + expect(missing.body?.lock).toBeUndefined(); } }); - 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. + it('criterion 3: …and it is the SAME answer the unpublished app gets, byte for byte', async () => { + // [#18066] ADR-0045 §3 makes an unpublished app externally + // unobservable, and this suite's partition note states the contract as + // absence and nonexistence being indistinguishable. Over the live + // producer shape they were not: `production_management` answered this + // 404 while `no_such_app` answered a 200 envelope, so the pair + // enumerated which app names exist-but-are-unpublished. Compared as + // whole bodies rather than field by field, because a single extra key + // on either side is the entire signal. + const unpublished = await getItem(setup(['manage_users'], GATED_APPS).rest, 'production_management'); + const absent = await getItem(setup(['manage_users'], GATED_APPS).rest, 'no_such_app'); + + expect(absent.statusCode).toBe(unpublished.statusCode); + expect(absent.body).toEqual(unpublished.body); + expect(absent.statusCode).toBe(404); + }); + + it('criterion 3: …and the REJECTING producer shape reaches the same status and code', async () => { + // The other producer shape this door must survive: a protocol + // implementation that REJECTS with a declared `RESOURCE_NOT_FOUND` / + // `status: 404` (`rest-meta-outage-vs-miss.test.ts` pins the rendering). + // ⚠️ Its body is the FLAT `{ error: '', code }` that + // `resolveErrorResponse`'s declared-status passthrough produces, not the + // nested ADR-0112 envelope the in-route refusals emit — so this case + // asserts `body.code`, and the case above asserts `body.error.code`, on + // purpose. Both reach this route, so the criterion is stated against + // both rather than against one stub's. const { rest, protocol } = setup([], GATED_APPS); protocol.getMetaItem = vi.fn().mockRejectedValue(Object.assign( new Error('Metadata item app/no_such_app not found'), diff --git a/packages/rest/src/meta-item-absent-404.test.ts b/packages/rest/src/meta-item-absent-404.test.ts new file mode 100644 index 0000000000..3df2d7a1fb --- /dev/null +++ b/packages/rest/src/meta-item-absent-404.test.ts @@ -0,0 +1,410 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#18066] `GET /meta/:type/:name` for a name with NOTHING behind it. +// +// Measured on a real server (`objectstack dev --seed-admin --fresh`, +// `examples/app-showcase`, API 17.4.0, four absent names, all identical): +// +// GET /api/v1/meta/app/no_such_app_xyz +// 200 {"type":"app","name":"no_such_app_xyz","lock":"none", +// "editable":true,"deletable":true,"resettable":false} +// +// — the declared envelope MINUS its `item` member, at 200. This file is the +// pin that the route now answers `404 RESOURCE_NOT_FOUND` instead, and that it +// answers it WITHOUT disturbing the #8013 partition beside it. +// +// ── Why this is execution and not a design question ───────────────────────── +// +// Three declarations in this repository already agreed with each other and +// against the live route; only the loudest of the three (the 200) was wrong. +// +// 1. `GetMetaItemResponseSchema` — the route's OWN `responseSchema` +// (`rest-route-ledger.ts`) — declares `item` as a required member, while +// every genuinely-optional key beside it is spelled `.optional()`. §4 below +// asserts that against the spec schema itself rather than restating it. +// 2. The CACHED arm of this very route already answers absence with `404 +// RESOURCE_NOT_FOUND`: `getMetaItemCached` throws `metadataItemNotFoundError` +// when `item` is falsy. §3 pins that the two arms now agree — which arm a +// request took had been deciding whether absence was an error at all. +// 3. `runtime/src/domains/meta.ts` refuses to treat the same item-less answer +// as a hit ("only treat the lookup as a hit when `item` is really there") +// and 404s. Cited, not re-pinned: that door is another package's. +// +// ── The trap this file exists to hold ────────────────────────────────────── +// +// The 403 `PERMISSION_DENIED` #8013 introduced lives inside the same `if +// (isAppType && visible)` the 404 did, so the obvious repair — make the gate +// reachable for a missing document — turns every withheld app into a probe for +// which app names exist. §2 is the ordering pin: the absent name and the +// withheld-but-existing app must keep answering DIFFERENT things (404 vs 403), +// and the absent name and the unpublished app must keep answering the SAME +// thing, byte for byte. + +import { describe, it, expect, vi } from 'vitest'; +import { GetMetaItemResponseSchema } from '@objectstack/spec/api'; +import { RestServer } from './rest-server'; + +const ANON_API = { api: { requireAuth: false } }; + +/** + * The document `metadata-protocol` resolves for a name that IS there, reduced + * to the two keys every assertion below reads. + */ +const CRM_APP = { name: 'crm', label: 'CRM', navigation: [{ id: 'nav_leads', type: 'object', objectName: 'lead' }] }; + +/** + * ⭐ THE FIXTURE THAT MATTERS: what `metadata-protocol`'s `getMetaItem` really + * resolves for a miss, key for key. + * + * Its three lookups (overlay row → MetadataService → SchemaRegistry) all leave + * `item` undefined, and the method then returns the protection envelope around + * it regardless — `lock` / `editable` / `deletable` / `resettable` are computed + * from `resolveLockState(undefined, false)` and are unconditional. So the + * producer's return is `{ type, name, item: undefined, lock, … }`. + * + * ⚠️ That object PARSES against `GetMetaItemResponseSchema`. `item` is present + * holding `undefined`, and `z.unknown()` admits an explicit `undefined`; it is + * `JSON.stringify` at `res.json` that drops the member and produces the wire + * body the schema rejects (§4 measures both halves). A double that answers + * `undefined` for a miss — the shape this suite's siblings used — never + * reproduces that, because it never had an envelope to lose a member from. + */ +function absentItemEnvelope(type: string, name: string) { + return { + type, name, + item: undefined, + lock: 'none', editable: true, deletable: true, resettable: false, + }; +} + +function mockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeRes() { + const res: any = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: any) => { res.body = b; return res; }); + res.header = vi.fn(() => res); + res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn(); res.send = vi.fn(); + return res; +} + +/** + * @param corpus the items this protocol double knows, keyed `type/name`. Any + * other address answers {@link absentItemEnvelope} — the live producer's miss, + * not `undefined`. + */ +function setup( + corpus: Record = {}, + opts: { perms?: string[]; config?: any; cached?: any } = {}, +) { + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ + version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([]), + getMetaItem: vi.fn(async ({ type, name }: any) => { + const hit = corpus[`${type}/${name}`]; + return hit === undefined + ? absentItemEnvelope(type, name) + : { type, name, item: JSON.parse(JSON.stringify(hit)), lock: 'none', editable: true, deletable: true, resettable: false }; + }), + findData: vi.fn().mockResolvedValue([]), + ...(opts.cached !== undefined ? { getMetaItemCached: opts.cached } : {}), + }; + const rest = new RestServer(mockServer() as any, protocol as any, (opts.config ?? ANON_API) as any); + (rest as any).resolveExecCtx = async () => ({ userId: 'u1', systemPermissions: opts.perms ?? [] }); + rest.registerRoutes(); + return { rest, protocol }; +} + +async function getItem(rest: any, type: string, name: string, query: Record = {}) { + const route = rest.getRoutes().find((r: any) => r.method === 'GET' && r.path === '/api/v1/meta/:type/:name'); + if (!route) throw new Error('GET /api/v1/meta/:type/:name route not registered'); + const res = makeRes(); + await route.handler({ method: 'GET', params: { type, name }, query, body: {}, headers: {} }, res); + return res; +} + +describe('[#18066] §1 — a name with nothing behind it is an ERROR, not an item-less 200', () => { + it('`app`: 404 + RESOURCE_NOT_FOUND, and no envelope rides along with it', async () => { + const res = await getItem(setup({ 'app/crm': CRM_APP }).rest, 'app', 'no_such_app_xyz'); + + // ADR-0112 — `status` AND `code`, never "something falsy came back". + expect(res.statusCode).toBe(404); + expect(res.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + + // The reported body, stated as the report stated it: the 200 carried + // `type` / `name` / `lock` / the three affordance verdicts and no + // `item`. A refusal carries none of them. + expect(res.body?.type).toBeUndefined(); + expect(res.body?.name).toBeUndefined(); + expect(res.body?.lock).toBeUndefined(); + expect(res.body?.editable).toBeUndefined(); + expect(res.body?.item).toBeUndefined(); + }); + + it('the name that IS there is untouched — the envelope, its document and its lock keys', async () => { + // The half a refusal change most easily breaks. Asserted on the same + // fixture as the case above, so the 404 is the miss firing rather than + // the double being broken. + const res = await getItem(setup({ 'app/crm': CRM_APP }).rest, 'app', 'crm'); + + expect(res.statusCode).toBe(200); + expect(res.body).toMatchObject({ type: 'app', name: 'crm', lock: 'none', editable: true }); + expect(res.body?.item?.label).toBe('CRM'); + expect(res.body?.error).toBeUndefined(); + }); + + it('it is NOT an `app` rule — every type the uncached arm serves answers the same', async () => { + // The card measured `app` alone and left "do other types behave the + // same way?" open. They did: the fall-through was in the shared tail of + // the uncached arm, below the per-type gates, so every type diverted + // around the cache reached it. `dashboard` is diverted structurally + // (#5881); `object` gets there whenever a deployment sets + // `enableCache: false`, and `?state=draft` / `?package=` divert any type + // at all. + const byType = setup({ 'app/crm': CRM_APP }); + for (const type of ['dashboard', 'view', 'object', 'flow', 'page']) { + const res = await getItem(byType.rest, type, 'no_such_thing'); + expect(`${type}:${res.statusCode}`).toBe(`${type}:404`); + expect(res.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + } + + // …and through the query flags that divert a type around the cache, + // where a deployment running the default `enableCache: true` meets it. + const withCache = setup({}, { cached: vi.fn() }); + for (const query of [{ state: 'draft' }, { preview: 'draft' }, { package: 'pkg_x' }]) { + const res = await getItem(withCache.rest, 'object', 'no_such_object', query); + expect(res.statusCode).toBe(404); + expect(res.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + } + expect(withCache.protocol.getMetaItemCached).not.toHaveBeenCalled(); + }); + + it('a protocol that resolves NOTHING at all is the same absence, not a crash', async () => { + // Not every implementation of the verb is `metadata-protocol`: a plugin + // protocol may answer a bare `undefined` for a miss rather than the + // envelope above. Both are "no document", and the door must not tell + // them apart — `envelope?.item` is `undefined` either way. + const { rest, protocol } = setup(); + protocol.getMetaItem = vi.fn().mockResolvedValue(undefined); + + const res = await getItem(rest, 'app', 'no_such_app_xyz'); + + expect(res.statusCode).toBe(404); + expect(res.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + }); +}); + +describe('[#18066] §2 — the #8013 partition survives, in both directions', () => { + /** + * An app that EXISTS and is withheld for a reason that is NOT the caller's + * permissions: ADR-0045 §3 says it is externally unobservable. + */ + const UNPUBLISHED_APP = { + name: 'production_management', label: '生产管理', _unpublished: true, + navigation: [{ id: 'nav_secret_lines', type: 'object', objectName: 'secret_production_line' }], + }; + /** An app that EXISTS and is withheld for exactly the caller's permissions. */ + const FINANCE_APP = { + name: 'finance', label: 'Finance', requiredPermissions: ['finance.access'], + navigation: [{ id: 'nav_invoices', type: 'object', objectName: 'invoice' }], + }; + const GATED = { 'app/production_management': UNPUBLISHED_APP, 'app/finance': FINANCE_APP }; + + it('THE security criterion: an absent name never becomes the 403', async () => { + // 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" — by a caller holding nothing. Asserted + // across the permission sets #8013 enumerates, because the 403's own + // gate is permission-shaped. + for (const perms of [[], ['manage_users'], ['finance.access'], ['studio.access']]) { + const res = await getItem(setup(GATED, { perms }).rest, 'app', 'no_such_app_xyz'); + + expect(res.statusCode).not.toBe(403); + expect(res.body?.error?.code).not.toBe('PERMISSION_DENIED'); + expect(JSON.stringify(res.body ?? {})).not.toContain('PERMISSION_DENIED'); + } + }); + + it('the withheld-but-EXISTING app still answers 403 PERMISSION_DENIED, unchanged', async () => { + // The other direction, and the one a careless repair breaks: making the + // gate reachable for a missing document would have converted this arm + // into an absence and deleted #8013's whole answer. + const res = await getItem(setup(GATED, { perms: ['manage_users'] }).rest, 'app', 'finance'); + + expect(res.statusCode).toBe(403); + expect(res.body?.error?.code).toBe('PERMISSION_DENIED'); + expect(res.body?.success).toBe(false); + expect(res.body?.error?.message).toContain('finance'); + expect(res.body?.item).toBeUndefined(); + expect(JSON.stringify(res.body ?? {})).not.toContain('invoice'); + + // …and the holder still gets it, so the 403 is the gate firing. + const held = await getItem(setup(GATED, { perms: ['finance.access'] }).rest, 'app', 'finance'); + expect(held.statusCode).toBe(200); + expect(held.body?.item?.name).toBe('finance'); + }); + + it('the same invariant on a NON-`app` type: a gated `book` that exists still answers 403', async () => { + // The ordering pin generalised, because the fix is not app-scoped and + // neither is the trap. ADR-0046 §6.7's audience gate is the other arm on + // this handler that converts a withheld-but-EXISTING document into a + // refusal — 401 anonymous, 403 for an authenticated non-holder — and it + // is guarded by `&& visible` exactly as the app gate is. A check placed + // after it, or one that fired on a document that exists, would turn a + // gated book into an absence and lose the distinction; a caller could + // then no longer tell "sign in / ask for the permission set" from "this + // book does not exist". + const GATED_BOOK = { name: 'admin_guide', label: 'Admin Guide', audience: { permissionSet: 'crm_admin' }, groups: [] }; + const { rest } = setup({ 'book/admin_guide': GATED_BOOK }); + + const withheld = await getItem(rest, 'book', 'admin_guide'); + expect(withheld.statusCode).toBe(403); + // ⚠️ MEASURED, not assumed: this arm emits through `sendDeclaredFault` + // -> `sendThrownError`, whose body is the FLAT `{ error: '', + // code }`, while the app gate's 403 one screen up emits + // `sendEnvelopeError`'s nested `{ success: false, error: { code, + // message } }`. So ONE handler answers `PERMISSION_DENIED` in two + // dialects depending on which gate fired, and `body.error.code` — the + // accessor #8013 settled on — reads `undefined` here. Asserted in the + // shape the route really sends rather than the shape it ought to; the + // divergence is reported as a finding, deliberately not converged in + // this change (see §3 for the same split on the 404). + expect(withheld.body?.code).toBe('PERMISSION_DENIED'); + expect(withheld.body?.item).toBeUndefined(); + + // …and the name with nothing behind it, on the same type and the same + // caller, is the absence instead. + const absent = await getItem(rest, 'book', 'no_such_book'); + expect(absent.statusCode).toBe(404); + expect(absent.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + }); + + it('⭐ absent and UNPUBLISHED are byte-identical — the enumeration hole this closes', async () => { + // The half the card did not name and the reason the fix is not merely a + // status-code correction. ADR-0045 §3 makes an unpublished app + // *externally unobservable*, and #8013 states the contract as absence + // and nonexistence being indistinguishable. They were not: the + // unpublished app answered this 404 while a nonexistent name answered + // 200, so the pair of responses enumerated exactly which app names + // exist-but-are-unpublished. One emitter, one body. + const unpublished = await getItem(setup(GATED, { perms: ['manage_users'] }).rest, 'app', 'production_management'); + const absent = await getItem(setup(GATED, { perms: ['manage_users'] }).rest, 'app', 'no_such_app_xyz'); + + expect(unpublished.statusCode).toBe(absent.statusCode); + expect(unpublished.body).toEqual(absent.body); + expect(unpublished.statusCode).toBe(404); + expect(unpublished.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + expect(JSON.stringify(unpublished.body ?? {})).not.toContain('secret_production_line'); + + // …and a builder, who MAY observe it, still gets the document — so the + // equality above is the gate withholding rather than the app missing. + const builder = await getItem(setup(GATED, { perms: ['studio.access'] }).rest, 'app', 'production_management'); + expect(builder.statusCode).toBe(200); + expect(builder.body?.item?.name).toBe('production_management'); + }); +}); + +describe('[#18066] §3 — the two arms of this route now answer absence alike', () => { + it('the CACHED arm already 404s, and the uncached arm now matches its status and code', async () => { + // The card left "does the cached arm agree?" unmeasured. It does not + // reach `res.json` at all: `getMetaItemCached` throws + // `metadataItemNotFoundError` on a falsy `item`, which the route's catch + // reports through `handleRouteError`. So the SAME request answered 404 + // or 200 depending on a server-side cache setting the caller cannot see. + const cachedMiss = Object.assign(new Error('Metadata item view/no_such_view not found'), { + code: 'RESOURCE_NOT_FOUND', status: 404, + }); + const cached = setup({}, { cached: vi.fn().mockRejectedValue(cachedMiss) }); + const viaCache = await getItem(cached.rest, 'view', 'no_such_view'); + expect(cached.protocol.getMetaItemCached).toHaveBeenCalled(); + expect(viaCache.statusCode).toBe(404); + + const viaUncached = await getItem( + setup({}, { config: { api: { requireAuth: false }, metadata: { enableCache: false } } }).rest, + 'view', 'no_such_view', + ); + expect(viaUncached.statusCode).toBe(viaCache.statusCode); + + // ⚠️ Status and CODE agree; the envelope DIALECT still differs and that + // is deliberately not touched here. The thrown arm is rendered by + // `resolveErrorResponse`'s declared-status passthrough, whose body is + // the flat `{ error: '', code }` pinned in + // `rest-meta-outage-vs-miss.test.ts`; the in-route arm emits ADR-0112's + // nested `{ error: { code, message } }`, which is what its own sibling + // refusals on this handler emit and what objectui#4252 reads. Matching + // the flat one HERE would have broken the §2 byte-identity, which is a + // security property; converging the two dialects is a separate change + // on the thrown side. Asserted rather than left implicit, so a future + // convergence is a deliberate edit to this line. + expect(viaCache.body?.code).toBe('RESOURCE_NOT_FOUND'); + expect(viaUncached.body?.error?.code).toBe('RESOURCE_NOT_FOUND'); + }); + + it('an unreadable metadata STORE is still a 503, never this 404', async () => { + // The distinction #5532 bought and this condition must not flatten. + // "The item is not there" and "we could not look" are different claims, + // and the producer throws rather than resolving item-less for the + // second — so it never reaches the new check at all. + const { rest, protocol } = setup(); + protocol.getMetaItem = vi.fn().mockRejectedValue(Object.assign( + new Error('The metadata store could not be read, so whether this item exists is unknown.'), + { code: 'SERVICE_UNAVAILABLE', status: 503 }, + )); + + const res = await getItem(rest, 'object', 'acct'); + + expect(res.statusCode).toBe(503); + expect(res.statusCode).not.toBe(404); + expect(JSON.stringify(res.body ?? {})).not.toContain('RESOURCE_NOT_FOUND'); + }); +}); + +describe('[#18066] §4 — against `packages/spec`, not against a restatement of it', () => { + it('the 200 this route used to send fails the route`s own declared responseSchema', async () => { + // `rest-route-ledger.ts` declares `GetMetaItemResponseSchema` as this + // route's `responseSchema`. The body is the one the real server sent, + // transcribed from the report. + const asSent = { + type: 'app', name: 'no_such_app_xyz', + lock: 'none', editable: true, deletable: true, resettable: false, + }; + const verdict = GetMetaItemResponseSchema.safeParse(asSent); + + expect(verdict.success).toBe(false); + expect(verdict.error?.issues?.map((i: any) => i.path.join('.'))).toContain('item'); + }); + + it('⚠️ the PRODUCER`s in-process return passes the same parse — the break is at JSON', () => { + // Why nobody caught this with a unit test on `getMetaItem`. `item` is + // PRESENT holding `undefined` and `z.unknown()` admits that, so the + // object conforms; `JSON.stringify` then drops the member on the way + // out. A conformance probe written against the returned object rather + // than the wire bytes reports agreement — which is the trap, not a + // detail. + const returned = absentItemEnvelope('app', 'no_such_app_xyz'); + + expect(GetMetaItemResponseSchema.safeParse(returned).success).toBe(true); + expect(JSON.parse(JSON.stringify(returned))).not.toHaveProperty('item'); + expect(GetMetaItemResponseSchema.safeParse(JSON.parse(JSON.stringify(returned))).success).toBe(false); + }); + + it('every 200 the route still serves conforms to that schema', async () => { + // The positive half: the fix must not have bought conformance by + // refusing things it should serve. + const { rest } = setup({ 'app/crm': CRM_APP }); + + for (const [type, name] of [['app', 'crm']] as const) { + const res = await getItem(rest, type, name); + expect(res.statusCode).toBe(200); + expect(GetMetaItemResponseSchema.safeParse(res.body).success).toBe(true); + } + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 10c408f908..a43dfeb1fa 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -1507,6 +1507,57 @@ function notImplementedRefusalAnswer( return { status: 501, body: { error: { code: 'NOT_IMPLEMENTED', message } } }; } +/** + * [#18066] THE one absence answer `GET /meta/:type/:name` gives — a single + * emitter, so the several conditions that mean "you get nothing" cannot answer + * several different bodies. + * + * ⭐ Byte-identity is the POINT here, not tidiness. #8013 partitioned this + * route's refusals deliberately: an app that EXISTS and whose + * `requiredPermissions` the caller lacks reports `403 PERMISSION_DENIED`, + * while an unpublished app (ADR-0045 §3, "externally unobservable"), an app + * gated by an absent optional service (ADR-0057 D10) and a name with nothing + * behind it must be INDISTINGUISHABLE — extending the denial to them "would + * make every app name on the platform enumerable", which is the unruled change + * that card fenced off. Two hand-built bodies for two of those three arms is + * that fence held by coincidence; one emitter makes it structural. + * + * The condition this closes was the LOUDEST of the three and the one that got + * away. The uncached arm reached its 404 only from INSIDE `if (isAppType && + * visible)`, where `visible` is the document — so for a name that resolves to + * nothing the gate was skipped whole and the envelope fell through to + * `res.json`, answering `200` with the declared envelope MINUS its `item` + * member. Two in-repo declarations already said otherwise, and this restores + * what they declare rather than deciding anything new: + * + * - `GetMetaItemResponseSchema` (the route's own `responseSchema`, see + * `rest-route-ledger.ts`) makes `item` a required member. Measured on this + * tree with the body a real server sent: `safeParse({ type: 'app', name: + * 'no_such_app_xyz', lock: 'none', editable: true, deletable: true, + * resettable: false })` fails `invalid_type` / `expected: 'nonoptional'` at + * `item`. ⚠️ The producer's in-process return passes that same parse — + * `item` is PRESENT holding `undefined`, and `z.unknown()` admits that — so + * the contract broke at `JSON.stringify`, which drops the member. A probe + * written against the object rather than the wire bytes sees nothing wrong. + * - The CACHED arm of this same route already answers this condition `404 + * RESOURCE_NOT_FOUND`: `getMetaItemCached` throws + * `metadataItemNotFoundError` on a falsy `item`. `app`, `dashboard`, `doc`, + * `book`, `?state=draft`, `?preview=draft`, `?package=` and every + * `enableCache: false` deployment are diverted around it, so which arm a + * request took decided whether absence was an error — the #5563 defect + * class, one member over. + * + * ⛔ Not `sendEnvelopeError`, which the 403 beside it uses: that builder adds + * `success: false`, and an absence answer that carries a key the unpublished + * app's answer does not is the enumeration signal all over again. The nested + * `error.code` accessor is the same one objectui#4252 reads on both. + */ +function sendMetaItemAbsent(res: any): void { + res.status(404).json({ + error: { code: 'RESOURCE_NOT_FOUND', message: 'Metadata item not found or access denied.' }, + }); +} + export class RestServer { private protocol: RestProtocol; private config: NormalizedRestServerConfig; @@ -6926,6 +6977,38 @@ export class RestServer { // envelope is rebuilt around the result at `res.json`. // Nothing downstream asks which shape it holds. let visible: any = envelope?.item; + + // [#18066] ABSENCE IS AN ERROR ON THIS ARM TOO. + // + // Ordered BEFORE every gate below, and that ordering + // is the security half of this change rather than a + // style choice. The three gates under it all read + // `&& visible`, so they are reached only by a + // document that EXISTS; a name that resolves to + // nothing can never enter the app gate and can + // therefore never be converted into the `403 + // PERMISSION_DENIED` #8013 reserves for an app the + // caller may not open. The withheld-but-existing + // app keeps answering exactly what it answered + // before — 403 for a permission denial, absence for + // the other two arms — because nothing on its path + // changed. + // + // `== null` rather than falsiness: the miss this + // catches is `undefined` (no overlay row, no + // MetadataService copy, no registry entry — see + // `metadata-protocol`'s `getMetaItem`, whose three + // lookups all leave `item` undefined) or a protocol + // implementation that resolved nothing at all. A + // document that is legitimately falsy-but-present is + // not a miss, and a metadata store that could not be + // READ never arrives here as a value at all — it + // throws 503 (#5532), which is the distinction this + // condition must not flatten. + if (visible == null) { + sendMetaItemAbsent(res); + return; + } // Same per-user RBAC filtering as the list endpoint: // for `app` items, drop entirely (404) when the user // lacks the app's `requiredPermissions`, and strip @@ -6982,9 +7065,14 @@ export class RestServer { ); return; } - res.status(404).json({ - error: { code: 'RESOURCE_NOT_FOUND', message: 'Metadata item not found or access denied.' }, - }); + // [#18066] Through the shared emitter, so + // this arm and the nothing-behind-the-name + // arm above it are byte-identical by + // construction — see + // {@link sendMetaItemAbsent} for why that + // is the ADR-0045 §3 property and not + // housekeeping. + sendMetaItemAbsent(res); return; } }