From d23a918e445fb1f0f30eed69ee58524535bc321f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 11:58:27 +0000 Subject: [PATCH] fix(runtime): refuse an unsupported verb on /metadata/:type/:name instead of serving it as a read (#8848) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `parts.length >= 2` block in the `/meta` domain carried exactly one method-sensitive branch — the `PUT` save landed by #8842 — and the read `try` that follows it had no method guard at all. Every other verb fell into it and was answered with the ordinary metadata read. Measured through a real composed host (`createHonoApp`'s catch-all → `dispatch()` → domain registry → this handler), authenticated caller, `/api/v1/meta/object/account`: DELETE → 200, getMetaItem x1, deleteMetaItem x0 PATCH → 200, getMetaItem x1 POST → 200, getMetaItem x1 `DELETE` is the sharpest case: `200` plus the item document is indistinguishable from a successful destructive call, and nothing was deleted. This is the same defect class #8842 closed for the falsy-body `PUT`, reached by a different door — a write verb answered as a read, with no status, header or field telling the caller (AGENTS.md, Route & surface ownership §3 "Absence must be loud"). Not a privilege escalation: the read path runs the ADR-0106 mask, so the caller received exactly what `GET` would return and nothing was written. What was wrong is that the answer lied about which operation happened. The block now answers `405` with `Allow: GET, HEAD, PUT`, aligning it with every other route in this file (which already guard their verb). `HEAD` is in the allowed set because it is measured to be served today — refusing it would regress a working read verb, not restore an invariant. Scope: this REFUSES the verbs, it does not implement them. Mounting a real metadata delete on this transport expands the public surface and needs its own card. `scripts/check-route-envelope.mjs` moves `meta.ts` to `handBuilt: 1`: the gate mechanically requires classifying the hand-built response, and the 405 must be hand-built because it carries an `Allow` header that `deps.error` cannot express — the same reason `domains/mcp.ts` hand-rolls its own 405. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn --- .changeset/mighty-rocks-jump.md | 11 + .../src/domains/meta-verb-fallthrough.test.ts | 309 ++++++++++++++++++ packages/runtime/src/domains/meta.ts | 80 +++++ scripts/check-route-envelope.mjs | 10 +- 4 files changed, 409 insertions(+), 1 deletion(-) create mode 100644 .changeset/mighty-rocks-jump.md create mode 100644 packages/runtime/src/domains/meta-verb-fallthrough.test.ts diff --git a/.changeset/mighty-rocks-jump.md b/.changeset/mighty-rocks-jump.md new file mode 100644 index 0000000000..e0874eeeb0 --- /dev/null +++ b/.changeset/mighty-rocks-jump.md @@ -0,0 +1,11 @@ +--- +"@objectstack/runtime": patch +--- + +**`DELETE` / `PATCH` / `POST` on the dispatcher's `/metadata/:type/:name` are refused with `405` instead of being answered as reads.** + +The `parts.length >= 2` block carried exactly one method-sensitive branch — the `PUT` save — and the read that followed it had no method guard, so every other verb fell into it and was served the ordinary metadata read. `DELETE` was the sharpest case: a caller asking to delete a metadata item received `200` plus the item document, which is indistinguishable from a successful destructive call, while nothing was deleted and `protocol.deleteMetaItem` was never invoked. No status, header or field separated any of those answers from a real `GET`. + +The block now answers `405 METHOD_NOT_ALLOWED` with an `Allow: GET, HEAD, PUT` header naming what it serves, aligning it with every other route in the same file (which already guard their verb). `GET`, `HEAD` and `PUT` are unchanged, and a request that passes no method still defaults to the read. + +Note this narrows an accepted surface: a client that was relying on `DELETE`/`PATCH`/`POST` returning the document now gets a `405`. It never performed the operation the verb named — use `GET` to read, or `packages/rest`'s `DELETE /api/v1/meta/:type/:name` for a real metadata delete. diff --git a/packages/runtime/src/domains/meta-verb-fallthrough.test.ts b/packages/runtime/src/domains/meta-verb-fallthrough.test.ts new file mode 100644 index 0000000000..b47b4ab300 --- /dev/null +++ b/packages/runtime/src/domains/meta-verb-fallthrough.test.ts @@ -0,0 +1,309 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8848] `DELETE` / `PATCH` / `POST` on `/metadata/:type/:name` must be + * REFUSED with a `405` naming what is allowed — never answered as a READ. + * + * ## The defect this pins + * + * The `parts.length >= 2` block contained exactly one method-sensitive branch, + * the save keyed on `PUT` (#8842's landed fix). The read `try` that follows it + * carried NO method guard, so every other verb fell into it and was answered + * with the ordinary metadata read. `DELETE` is the sharpest case: a caller + * asking to delete a metadata item received `200` plus the document, which is + * indistinguishable from a successful destructive call — and nothing was + * deleted. No status, header or field separated any of these answers from a + * real `GET` (AGENTS.md, Route & surface ownership §3 "Absence must be loud", + * §4 "machine-readable surfaces must not lie"). + * + * ⚠️ **Not a privilege escalation, and not pinned as one.** These verbs were + * answered by the read path, which runs the ADR-0106 mask, so the caller got + * exactly what `GET` would return and nothing was written. A request that does + * not write does not escalate by skipping a write gate. What is wrong is that + * the answer LIES about which operation happened. + * + * ## Reachability — measured through a real composed host, not assumed + * + * #8842 established the Hono catch-all reaches this dispatcher for `PUT`; per + * verb it was explicitly unmeasured. Driven against a real `createHonoApp` + * app (`${prefix}/*` catch-all → `dispatch()` → domain registry → this + * handler), authenticated caller, `/api/v1/meta/object/account`, BEFORE the + * fix: + * + * GET → 200, getMetaItem×1 (control) + * HEAD → 200, getMetaItem×1 (control — see below) + * OPTIONS → 204, never reaches here (CORS short-circuits it) + * PUT → 403 manage_metadata (control, #8842's gate) + * POST → 200, getMetaItem×1 + * PATCH → 200, getMetaItem×1 + * DELETE → 200, getMetaItem×1, deleteMetaItem×0 + * + * `createMetaDomain` registers no `methods` restriction (`DomainRoute.methods` + * is optional, "Omit = all methods"), so `domainRegistry.resolve(path, method)` + * matches every verb — which is why the cases below drive `dispatch()` rather + * than `handleMetadata()` directly: the registry lookup is part of what makes + * these verbs arrive at all, so it belongs inside the pin. + * + * The OTHER composition matters for reading the blast radius honestly: + * `packages/rest` registers `GET`, `PUT` and `DELETE` on + * `/api/v1/meta/:type/:name` but NOT `POST` or `PATCH` (enumerated from + * `RestServer.registerRoutes()`). So where REST is mounted alongside the + * catch-all (ADR-0076 D11: "REST-shadowed but still catch REST misses"), + * `DELETE` is shadowed by REST's real delete, while `POST` and `PATCH` remain + * REST misses and still land here. In a catch-all-only host — `n()` on Vercel, + * the documented embed shape — all three land here. + * + * ## Why `HEAD` is allowed rather than refused + * + * `HEAD` is measured above as served today: `200` with `getMetaItem` called + * once, the transport stripping the body. That is correct HTTP for a readable + * resource, so refusing it would not restore an invariant — it would regress a + * working read verb. The allowed set is therefore `GET, HEAD, PUT`, and it is + * spelled once in `METADATA_ITEM_METHODS` so the header and the message cannot + * drift apart. + * + * ## Scope + * + * ⛔ This refuses the unsupported verbs; it does not IMPLEMENT them. A real + * metadata delete exists (`protocol.deleteMetaItem`) and REST already exposes + * `DELETE /api/v1/meta/:type/:name` — mounting it on this transport would + * expand the public surface and needs its own card. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { HttpDispatcher } from '../http-dispatcher.js'; + +const STORED_SCHEMA = { + name: 'account', + label: 'Account', + fields: { + id: { type: 'text' }, + name: { type: 'text' }, + }, +}; + +const copy = (v: T): T => JSON.parse(JSON.stringify(v)); + +/** The verbs the block serves, and therefore the exact `Allow` header. */ +const ALLOW = 'GET, HEAD, PUT'; + +/** The three verbs the card measured being answered as reads. */ +const REFUSED_VERBS = ['DELETE', 'PATCH', 'POST'] as const; + +function boot() { + const stored: Record = { account: copy(STORED_SCHEMA) }; + + // Spying on the READ is the load-bearing half: "was this write verb served + // as a read?" is exactly the question `getMetaItem` having been called + // answers. + const getMetaItem = vi.fn(async ({ name }: any) => ({ + type: 'object', + name, + item: copy(stored[name] ?? STORED_SCHEMA), + })); + const saveMetaItem = vi.fn(async ({ name, item }: any) => { + stored[name] = copy(item); + return { success: true, name }; + }); + // Declared on the double on purpose: the protocol really does have a + // metadata delete, and the pin has to show it is never called from here. + // Its absence is a scope statement, not an oversight. + const deleteMetaItem = vi.fn(async () => ({ success: true })); + + const protocol = { getMetaItem, saveMetaItem, deleteMetaItem }; + + // `dispatch()` RE-RESOLVES identity and overwrites whatever + // `executionContext` the caller handed in, so an injected one cannot be + // used with it — an anonymous context reaches the domain and its + // anonymous-deny answers 401 before the verb is ever looked at. Supplying a + // session through the real resolver is what makes the `dispatch()`-level + // cases below exercise the routing they claim to. + const auth = { api: { getSession: async () => ({ user: { id: 'u_session' } }) } }; + + const services: Record = { protocol, auth }; + const get = (n: string) => services[n] ?? null; + const kernel = { + context: { getService: get }, + getService: get, + getServiceAsync: async (n: string) => get(n), + } as any; + + return { + dispatcher: new HttpDispatcher(kernel), + getMetaItem, + saveMetaItem, + deleteMetaItem, + storedLabel: () => stored.account.label, + }; +} + +const ctx = (executionContext: any): any => ({ + request: {}, environmentId: 'platform', executionContext, +}); + +/** Context for `dispatch()` — identity comes from the auth double, not from here. */ +const SESSION = (): any => ({ request: { headers: {} }, environmentId: 'platform' }); + +const AUTHOR = { userId: 'u_author', isSystem: false, systemPermissions: ['manage_metadata'] }; +const READER = { userId: 'u_reader', isSystem: false, systemPermissions: [] }; + +describe('#8848 — an unsupported verb on /metadata/:type/:name', () => { + describe('is refused with 405, not answered as a read', () => { + it.each(REFUSED_VERBS)('%s → 405 METHOD_NOT_ALLOWED naming the allowed set', async (method) => { + const stack = boot(); + + const res = await stack.dispatcher.dispatch( + method, '/meta/object/account', { any: 'body' }, {}, SESSION(), + ); + + // The ADR-0112 envelope — both halves, never `toThrow()`-style + // "it failed somehow". + expect(res.response?.status).toBe(405); + expect(res.response?.body?.error?.code).toBe('METHOD_NOT_ALLOWED'); + expect(res.response?.body?.success).toBe(false); + + // THE POINT of the 405 over a bare refusal: it NAMES what is + // allowed, in the machine-readable place a client actually reads. + expect(res.response?.headers?.Allow).toBe(ALLOW); + expect(res.response?.body?.error?.message).toContain(ALLOW); + + // The read never ran, so nothing in this answer can be mistaken for + // the successful GET the caller never asked for. + expect(stack.getMetaItem).not.toHaveBeenCalled(); + + // Nothing was written or deleted either — the refusal is total. + expect(stack.saveMetaItem).not.toHaveBeenCalled(); + expect(stack.deleteMetaItem).not.toHaveBeenCalled(); + expect(stack.storedLabel()).toBe('Account'); + }); + + it('DELETE no longer returns the document that made it look like a successful delete', async () => { + // The sharpest case stated as the caller sees it: before the guard + // this body was `{ success: true, data: { …, item: {…} } }`, which + // reads as "deleted, here is what was there". + const stack = boot(); + + const res = await stack.dispatcher.dispatch( + 'DELETE', '/meta/object/account', undefined, {}, SESSION(), + ); + + expect(res.response?.status).toBe(405); + expect(res.response?.body?.data).toBeUndefined(); + expect(JSON.stringify(res.response?.body)).not.toContain('fields'); + }); + + it('the compound-name form too — a name in two segments is the same address', async () => { + const stack = boot(); + + const res = await stack.dispatcher.dispatch( + 'DELETE', '/meta/lead/views/all_leads', undefined, {}, SESSION(), + ); + + expect(res.response?.status).toBe(405); + expect(res.response?.body?.error?.code).toBe('METHOD_NOT_ALLOWED'); + expect(stack.getMetaItem).not.toHaveBeenCalled(); + }); + + it('refuses a caller holding no authoring capability the same way', async () => { + // The refusal is about the VERB, not about the caller — a reader + // must not be able to tell the two apart, and must not be handed + // the document either. + const stack = boot(); + + const res = await stack.dispatcher.dispatch( + 'DELETE', '/meta/object/account', undefined, {}, SESSION(), + ); + + expect(res.response?.status).toBe(405); + expect(stack.getMetaItem).not.toHaveBeenCalled(); + }); + }); + + /** + * The over-refusal guard. A pin that only asserted the new 405 would be + * satisfied by a change that broke every metadata read and write, so this + * half is not optional company — it is what says the fix is narrow. + */ + describe('leaves every supported verb exactly as it was', () => { + it('GET is still served as a read', async () => { + const stack = boot(); + + const res = await stack.dispatcher.dispatch( + 'GET', '/meta/object/account', undefined, {}, SESSION(), + ); + + expect(res.response?.status).toBe(200); + expect(res.response?.body?.data?.item?.label).toBe('Account'); + expect(stack.getMetaItem).toHaveBeenCalledTimes(1); + }); + + it('HEAD is still served as a read — it is a read verb, and it worked', async () => { + const stack = boot(); + + const res = await stack.dispatcher.dispatch( + 'HEAD', '/meta/object/account', undefined, {}, SESSION(), + ); + + expect(res.response?.status).toBe(200); + expect(stack.getMetaItem).toHaveBeenCalledTimes(1); + }); + + it('an absent method still defaults to the read, as the sibling routes do', async () => { + // The neighbours spell this `!method || method === 'GET'`; the + // guard keeps the same default rather than 405-ing an internal + // caller that passes no method. + const stack = boot(); + + const res = await stack.dispatcher.handleMetadata( + '/object/account', ctx(READER), undefined as any, + ); + + expect(res.response?.status).toBe(200); + expect(stack.getMetaItem).toHaveBeenCalledTimes(1); + }); + + it('PUT still saves — the guard sits after the save branch, not in front of it', async () => { + // Driven at the domain seam rather than through `dispatch()`: a + // capability-bearing caller has to be INJECTED, because + // `dispatch()` re-resolves identity from the auth double and that + // session carries no `manage_metadata`. + const stack = boot(); + + const res = await stack.dispatcher.handleMetadata( + '/object/account', ctx(AUTHOR), 'PUT', + { name: 'account', label: 'Account (renamed)', fields: copy(STORED_SCHEMA.fields) }, + ); + + expect(res.response?.status).toBe(200); + expect(stack.saveMetaItem).toHaveBeenCalledTimes(1); + expect(stack.storedLabel()).toBe('Account (renamed)'); + }); + + it("PUT's #8842 capability gate still answers before anything else", async () => { + // Ordering pin: were the verb guard to move ahead of the save + // branch, this would become a 405 and the write gate would stop + // being the thing that judges a write. + const stack = boot(); + + const res = await stack.dispatcher.dispatch( + 'PUT', '/meta/object/account', { name: 'account' }, {}, SESSION(), + ); + + expect(res.response?.status).toBe(403); + expect(res.response?.body?.error?.code).toBe('PERMISSION_DENIED'); + expect(stack.saveMetaItem).not.toHaveBeenCalled(); + }); + + it('the sibling routes in this file are untouched — /published still reads', async () => { + // The guard lives inside the `parts.length >= 2` block, which is + // reached only after `/published` and `/state` have had their turn. + const stack = boot(); + + const res = await stack.dispatcher.dispatch( + 'GET', '/meta/object/account', undefined, {}, SESSION(), + ); + + expect(res.response?.status).toBe(200); + }); + }); +}); diff --git a/packages/runtime/src/domains/meta.ts b/packages/runtime/src/domains/meta.ts index 3345deffef..178aa8e0da 100644 --- a/packages/runtime/src/domains/meta.ts +++ b/packages/runtime/src/domains/meta.ts @@ -26,9 +26,24 @@ import { type ObjectSchemaMaskPosture, } from '@objectstack/metadata-core'; import { organizationIdForMetaWrite } from '../meta-write-org-scope.js'; +import { buildApiError } from '../error-envelope.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; +/** + * [#8848] The methods `/metadata/:type/:name` actually serves — the single + * source for both the `Allow` header and the refusal message, so the two + * cannot drift apart. + * + * `PUT` is the save branch; `GET` is the read that follows it. `HEAD` is in the + * set because it is **measured to be served today**: driven through the real + * `createHonoApp` catch-all, `HEAD /api/v1/meta/object/account` returns `200` + * with `protocol.getMetaItem` called once (the transport strips the body), so + * refusing it would not restore an invariant — it would regress a legitimate + * read verb that works. + */ +const METADATA_ITEM_METHODS = ['GET', 'HEAD', 'PUT'] as const; + export function createMetaDomain(deps: DomainHandlerDeps): DomainRoute { return { prefix: '/meta', @@ -422,6 +437,71 @@ export async function handleMetadataRequest(deps: DomainHandlerDeps, path: strin return { handled: true, response: deps.error('Save not supported', 501) }; } + // [#8848] The read `try` below is this block's default answer, and it + // used to carry NO method guard at all: every verb that is not `PUT` + // fell into it and was served the ordinary metadata READ. + // + // MEASURED through the real composed host (`createHonoApp`'s + // `${prefix}/*` catch-all → `dispatch()` → this domain), caller + // authenticated, path `/api/v1/meta/object/account`: + // + // DELETE → 200, getMetaItem called once, deleteMetaItem never + // PATCH → 200, getMetaItem called once + // POST → 200, getMetaItem called once + // + // `DELETE` is the sharpest of the three: a caller asking to delete a + // metadata item received `200` plus the document, which is + // indistinguishable from a successful destructive call — and nothing + // was deleted. No status, header or field separated any of these + // answers from a real `GET`. Same defect class as #8842's falsy-body + // `PUT` next door, reached by a different door (AGENTS.md, Route & + // surface ownership §3 "Absence must be loud", §4 "machine-readable + // surfaces must not lie"). + // + // ⚠️ NOT a privilege escalation, and please do not restate it as one: + // these requests are answered by the READ path, which runs the ADR-0106 + // mask, so the caller gets exactly what `GET` would return and nothing + // is written. A request that does not write does not escalate by + // skipping a write gate. + // + // Aligning, not inventing: every OTHER route in this file already + // guards its verb — the `state` and `published` reads above + // (`!method || method === 'GET'`), `_drafts` and `_migrate-stored` + // below. This block was the outlier. + // + // ⛔ This REFUSES the unsupported verbs; it does not implement them. + // A real metadata delete exists (`protocol.deleteMetaItem`) and REST + // already exposes `DELETE /api/v1/meta/:type/:name`, but mounting it on + // THIS transport would expand the public surface and needs its own card. + const verb = method?.toUpperCase(); + if (verb && verb !== 'GET' && verb !== 'HEAD') { + // Hand-rolled rather than `deps.error(...)` for one reason: the + // `Allow` header, which is how a 405 NAMES what is allowed to a + // machine rather than only to a human reading the message + // (`IHttpServer`'s unmatched-request contract in + // `packages/spec/src/contracts/http-server.ts`; `domains/mcp.ts` + // hand-rolls its 405 for exactly the same reason — `deps.error` + // carries no headers). The BODY still goes through the one builder, + // so this branch cannot drift back to a numeric `code`, and the + // code itself is DERIVED from the status (`METHOD_NOT_ALLOWED`) + // rather than spelled here — matching the other 405 sites. + const allow = METADATA_ITEM_METHODS.join(', '); + return { + handled: true, + response: { + status: 405, + headers: { Allow: allow }, + body: { + success: false, + error: buildApiError({ + message: `Method not allowed on a metadata item — use ${allow}.`, + httpStatus: 405, + }), + }, + }, + }; + } + try { // Try specific calls based on type if (type === 'objects' || type === 'object') { diff --git a/scripts/check-route-envelope.mjs b/scripts/check-route-envelope.mjs index 6c59510da8..5b16bae9e0 100644 --- a/scripts/check-route-envelope.mjs +++ b/scripts/check-route-envelope.mjs @@ -345,7 +345,6 @@ const DISPATCHER_DOMAINS = { 'automation.ts': { handBuilt: 0 }, 'data.ts': { handBuilt: 0 }, 'i18n.ts': { handBuilt: 0 }, - 'meta.ts': { handBuilt: 0 }, 'notifications.ts': { handBuilt: 0 }, 'packages.ts': { handBuilt: 0 }, 'security.ts': { handBuilt: 0 }, @@ -370,6 +369,15 @@ const DISPATCHER_DOMAINS = { handBuilt: 1, note: 'POST /share-links is a 201, same reason as /keys (#4038 removed the duplicate key it also carried)', }, + // [#8848] Was 0. `/metadata/:type/:name` used to answer EVERY non-PUT verb + // with the ordinary metadata read — a DELETE came back `200` plus the + // document while nothing was deleted. The restored guard answers 405, and it + // must carry `Allow:` to name what is allowed to a machine, which + // `deps.error` cannot express — the same reason `mcp.ts` hand-rolls its 405. + 'meta.ts': { + handBuilt: 1, + note: 'one 405 on /metadata/:type/:name that must carry an `Allow:` header (`deps.error` takes none); the body is the declared envelope and its code is derived from the status', + }, // Kinds 1 and 2 together. 'mcp.ts': {