diff --git a/.changeset/24023-keys-actions-405-allow.md b/.changeset/24023-keys-actions-405-allow.md new file mode 100644 index 00000000000..d3b5c054fe0 --- /dev/null +++ b/.changeset/24023-keys-actions-405-allow.md @@ -0,0 +1,16 @@ +--- +'@objectstack/runtime': patch +--- + +fix(runtime): the `keys` and `actions` dispatcher domains answer their `405 METHOD_NOT_ALLOWED` with an `Allow: POST` header + +Clause-②: no + +RFC 9110 requires a `405` to carry an `Allow` header naming the methods the target resource supports. The `mcp`, `meta` and `ai` dispatcher domains already send one. The `keys` and `actions` domains answered their `405` without it, so a client could see that its verb was refused but not which verb the route takes. + +Both domains serve one method, `POST`, on every path they claim, and their `405` now carries `Allow: POST`: + +- **`/api/v1/keys`.** A signed-in `GET`, `PUT`, `PATCH` or `DELETE` is answered `405` with `Allow: POST`, on the standalone server (`os dev`, `os serve`) and on a host built with `@objectstack/hono`'s `createHonoApp`. +- **`/api/v1/actions/...`.** A signed-in non-`POST` request that reaches the `actions` domain is answered `405` with `Allow: POST`. That is every such request on a `createHonoApp` host. The standalone server mounts these paths for `POST` only, so its HTTP adapter answers other verbs itself, and that answer already carried `Allow: POST`. + +The status, the body and its `error.code` are unchanged: `{ success: false, error: { code: 'METHOD_NOT_ALLOWED', message: 'Method not allowed', httpStatus: 405 } }`. Each domain still decides an anonymous caller before it looks at the method, so an anonymous `GET` (or any other verb the anonymous decision refuses) that reaches either domain is answered `401 UNAUTHENTICATED`, with no `Allow` header. diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index c989d1e62c4..e15f8b9fff1 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -72,6 +72,7 @@ import { isCodeArtifactBody, isTenantAuthored } from '@objectstack/metadata-core import * as actionExec from '../action-execution.js'; import { actorUserFromExecutionContext, resolveActorDisplayName } from '../security/actor-user.js'; import { validationFailureDetails } from '../validation-failure.js'; +import { buildApiError } from '../error-envelope.js'; // [ADR-0126 §5] The shared activation write-authority gates — the same two // tiers `/automation`'s toggle door passes, one implementation. import { @@ -108,6 +109,12 @@ import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry. */ const ACTION_ACTIVATION_SEGMENT = '_activation'; +/** + * The one method this domain serves, on every path it claims — invocation and + * the activation door alike: the `Allow` value of its 405. + */ +const ACTIONS_ALLOWED_METHODS = 'POST'; + function isActionActivationWrite(parts: string[], method: string): boolean { return method === 'POST' && parts[0] === ACTION_ACTIVATION_SEGMENT; } @@ -443,7 +450,25 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string } } if (method.toUpperCase() !== 'POST') { - return { handled: true, response: deps.error('Method not allowed', 405) }; + // RFC 9110 §15.5.6: a 405 MUST carry an `Allow` header naming the + // target resource's supported methods. Hand-rolled rather than + // `deps.error(...)` only for that header, which `deps.error` cannot + // carry — the shape `domains/mcp.ts`, `domains/meta.ts` and + // `domains/ai.ts` already answer with. The BODY still goes through the + // one builder, so its code stays DERIVED from the status + // (`METHOD_NOT_ALLOWED`) and is not spelled here. Below the anonymous + // baseline above, which stays this handler's first statement. + return { + handled: true, + response: { + status: 405, + headers: { Allow: ACTIONS_ALLOWED_METHODS }, + body: { + success: false, + error: buildApiError({ message: 'Method not allowed', httpStatus: 405 }), + }, + }, + }; } const parts = path.split('/').filter(Boolean); if (parts.length < 1) { diff --git a/packages/runtime/src/domains/keys-actions-method-not-allowed-allow.test.ts b/packages/runtime/src/domains/keys-actions-method-not-allowed-allow.test.ts new file mode 100644 index 00000000000..9bcbfb7d1aa --- /dev/null +++ b/packages/runtime/src/domains/keys-actions-method-not-allowed-allow.test.ts @@ -0,0 +1,186 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The `keys` and `actions` domains' 405 names what each serves, in `Allow`. + * + * RFC 9110 §15.5.6: "The origin server MUST generate an Allow header field in + * a 405 (Method Not Allowed) response containing a list of the target + * resource's currently supported methods." The `mcp`, `meta` and `ai` domains + * answer their 405 that way. These two answered theirs through `deps.error`, + * which builds `{ status, body }` and carries no headers, so the status was + * right and the header was missing — on every host that reaches the domain. + * + * Each domain serves exactly one method on every path it claims: `POST` (the + * key mint; an action invocation or the activation door). So `Allow: POST`. + * + * ## Two legs, because "on both hosts" is a question about the wire + * + * 1. `dispatch()` — the call both hosts make: `@objectstack/hono`'s + * `createHonoApp` catch-all hands every `${prefix}/*` request to it and + * writes `result.response.headers` onto the wire, and the standalone + * server's explicit mounts call it too. Driving `dispatch()` rather than + * `handleKeys()` / `handleActions()` keeps the domain-registry lookup + * inside the pin. + * 2. A real Hono socket (`HonoServerPlugin` + `createDispatcherPlugin`, the + * standalone `os dev` / `os serve` composition), read through `fetch`. + * `/api/v1/keys` is mounted for every verb, so a signed-in `GET` reaches + * the domain and its 405 is the one on the wire. `/actions` is mounted for + * `POST` alone there, so a `GET` never reaches the domain: the adapter's + * own unmatched-method 405 answers it, and that one already names `POST`. + * The row is kept so "both hosts" is a statement about the wire for both + * domains, not about one handler. + * + * ## The anonymous baseline stays first (ADR-0138 D2) + * + * Both handlers decide an anonymous caller as their first statement, so an + * anonymous non-`POST` is answered `401` and learns nothing about the route's + * shape — no `Allow` either. Pinned here beside the new header so the header + * cannot be won by moving the 405 above that decision. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { LiteKernel, Plugin, PluginContext } from '@objectstack/core'; +import { HonoServerPlugin } from '@objectstack/plugin-hono-server'; +import { ApiErrorSchema, BaseResponseSchema } from '@objectstack/spec/api'; +import type { IHttpServer } from '@objectstack/spec/contracts'; + +import { HttpDispatcher } from '../http-dispatcher.js'; +import { createDispatcherPlugin } from '../dispatcher-plugin.js'; + +/** Both domains serve `POST` and nothing else, on every path they claim. */ +const ALLOW = 'POST'; + +const SESSION_HEADER = 'x-test-session'; + +/** The `auth` slot answers a session only when the request carries the header. */ +const authService = () => ({ + api: { + getSession: async ({ headers }: any) => + (headers?.get?.(SESSION_HEADER) ? { user: { id: 'u_allow_header' } } : undefined), + }, +}); + +/** One non-allowed verb and one path per domain. */ +const DOMAINS = [ + { domain: 'keys', path: '/keys', verb: 'GET' }, + { domain: 'actions', path: '/actions/task/close', verb: 'GET' }, +] as const; + +/** The declared dispatcher envelope, parsed against the spec's own schemas. */ +function expectDeclaredEnvelope(body: any): void { + expect(BaseResponseSchema.safeParse(body).success).toBe(true); + expect(ApiErrorSchema.safeParse(body.error).success).toBe(true); +} + +describe('the keys and actions domains answer a 405 with `Allow` (dispatch(), the call both hosts make)', () => { + function makeDispatcher(): HttpDispatcher { + const services: Record = { auth: authService() }; + const get = (name: string) => services[name] ?? null; + const kernel = { + context: { getService: get }, + getService: get, + getServiceAsync: async (name: string) => get(name), + } as any; + // No ObjectQL is composed here, and both answers under test come before + // either domain touches storage; the environment-membership gate would + // answer that missing engine as an outage (503), so it is switched off + // the way the dispatcher option documents for tests that seed none. + return new HttpDispatcher(kernel, undefined, { enforceProjectMembership: false }); + } + + const callerContext = (signedIn: boolean): any => ({ + request: { headers: signedIn ? { [SESSION_HEADER]: '1' } : {} }, + }); + + for (const { domain, path, verb } of DOMAINS) { + it(`signed-in ${verb} ${path} → 405 METHOD_NOT_ALLOWED with \`Allow: ${ALLOW}\``, async () => { + const result = await makeDispatcher().dispatch(verb, path, undefined, {}, callerContext(true)); + + expect(result.handled).toBe(true); + expect(result.response?.status).toBe(405); + expect(result.response?.headers?.Allow).toBe(ALLOW); + expect(result.response?.body?.success).toBe(false); + expect(result.response?.body?.error?.code).toBe('METHOD_NOT_ALLOWED'); + expect(result.response?.body?.error?.httpStatus).toBe(405); + expectDeclaredEnvelope(result.response?.body); + }); + + it(`anonymous ${verb} ${path} → the ${domain} domain's 401 first, naming no method`, async () => { + const result = await makeDispatcher().dispatch(verb, path, undefined, {}, callerContext(false)); + + expect(result.response?.status).toBe(401); + expect(result.response?.body?.error?.code).toBe('UNAUTHENTICATED'); + expect(result.response?.headers?.Allow).toBeUndefined(); + }); + } +}); + +describe('the keys and actions 405s carry `Allow` on the wire of the standalone server (real Hono socket)', () => { + function fakeAuthPlugin(): Plugin { + return { + name: 'com.objectstack.test.fake-auth-keys-actions-allow', + version: '1.0.0', + init: async (ctx: PluginContext) => { + ctx.registerService('auth', authService()); + }, + }; + } + + let kernel: LiteKernel; + let baseUrl: string; + + beforeAll(async () => { + kernel = new LiteKernel(); + kernel.use(fakeAuthPlugin()); + // port 0 → an OS-assigned free port, read back through getPort(). + kernel.use(new HonoServerPlugin({ port: 0, cors: false })); + kernel.use(createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false })); + await kernel.bootstrap(); + const httpServer = kernel.getService('http.server'); + baseUrl = `http://127.0.0.1:${httpServer.getPort!()}`; + }, 30_000); + + afterAll(async () => { + if (kernel) { + await Promise.race([ + kernel.shutdown(), + new Promise((resolve) => setTimeout(resolve, 10_000)), + ]); + } + }, 30_000); + + async function call(method: string, path: string, signedIn: boolean) { + const res = await fetch(`${baseUrl}/api/v1${path}`, { + method, + headers: signedIn ? { [SESSION_HEADER]: '1' } : {}, + }); + return { res, body: (await res.json()) as Record }; + } + + it(`signed-in GET /api/v1/keys → the keys domain's 405, with \`Allow: ${ALLOW}\` on the wire`, async () => { + const { res, body } = await call('GET', '/keys', true); + + expect(res.status).toBe(405); + expect(res.headers.get('allow')).toBe(ALLOW); + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + // The door: the domain fills `httpStatus`; the adapter fills `details`. + expect(body.error.httpStatus).toBe(405); + expect(body.error).not.toHaveProperty('details'); + }); + + it('anonymous GET /api/v1/keys → 401, and no `Allow` on the wire', async () => { + const { res, body } = await call('GET', '/keys', false); + + expect(res.status).toBe(401); + expect(body.error.code).toBe('UNAUTHENTICATED'); + expect(res.headers.get('allow')).toBeNull(); + }); + + it(`signed-in GET /api/v1/actions/task/close → 405 with \`Allow: ${ALLOW}\` (the adapter's: GET is not mounted)`, async () => { + const { res, body } = await call('GET', '/actions/task/close', true); + + expect(res.status).toBe(405); + expect(res.headers.get('allow')).toBe(ALLOW); + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + }); +}); diff --git a/packages/runtime/src/domains/keys.ts b/packages/runtime/src/domains/keys.ts index a3c2710ed41..f6d9a554769 100644 --- a/packages/runtime/src/domains/keys.ts +++ b/packages/runtime/src/domains/keys.ts @@ -48,6 +48,7 @@ import type { ApiKeyLifecyclePolicy } from '@objectstack/spec/contracts'; import { postureEnforcesWall } from '@objectstack/spec/security'; import { generateApiKey } from '../security/api-key.js'; +import { buildApiError } from '../error-envelope.js'; import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js'; import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js'; @@ -64,6 +65,12 @@ const EXPIRY_CEILING_SKEW_MS = 60_000; /** [#23367] The ceiling refusal, as its wire pair (ADR-0112 ledger, `@objectstack/runtime`). */ const API_KEY_EXPIRY_TOO_LONG = { code: 'API_KEY_EXPIRY_TOO_LONG', status: 400 } as const; +/** + * The one method this domain serves, on every path it claims (the handler + * never reads the path): the `Allow` value of its 405. + */ +const KEYS_ALLOWED_METHODS = 'POST'; + /** * The legacy branch matched `=== '/keys' || startsWith('/keys/') || * startsWith('/keys?')` — a segment match PLUS the query-string form some @@ -109,7 +116,25 @@ export async function handleKeysRequest( } if (method !== 'POST') { - return { handled: true, response: deps.error('Method not allowed', 405) }; + // RFC 9110 §15.5.6: a 405 MUST carry an `Allow` header naming the + // target resource's supported methods. Hand-rolled rather than + // `deps.error(...)` only for that header, which `deps.error` cannot + // carry — the shape `domains/mcp.ts`, `domains/meta.ts` and + // `domains/ai.ts` already answer with. The BODY still goes through the + // one builder, so its code stays DERIVED from the status + // (`METHOD_NOT_ALLOWED`) and is not spelled here. Below the anonymous + // baseline above, so an anonymous caller never reaches this answer. + return { + handled: true, + response: { + status: 405, + headers: { Allow: KEYS_ALLOWED_METHODS }, + body: { + success: false, + error: buildApiError({ message: 'Method not allowed', httpStatus: 405 }), + }, + }, + }; } // ── Whitelist the body. Only `name` and optional `expires_at`. ── diff --git a/packages/runtime/src/domains/mcp.ts b/packages/runtime/src/domains/mcp.ts index 23a671e51d5..66f4902f6f4 100644 --- a/packages/runtime/src/domains/mcp.ts +++ b/packages/runtime/src/domains/mcp.ts @@ -173,10 +173,10 @@ export async function handleMcpSkillRequest(deps: DomainHandlerDeps, method: str // Hand-rolled rather than `deps.error(...)` only because it carries an // `Allow` header; the BODY still goes through the one builder (#3842), // so this branch cannot drift back to a numeric `code`. The code is - // DERIVED (`method_not_allowed`), not spelled here — the other two 405 - // sites (`domains/actions.ts`, `domains/keys.ts`) go through - // `deps.error` and derive theirs, and one dispatcher answering the same - // status two ways is the drift this issue is closing. + // DERIVED (`METHOD_NOT_ALLOWED`), not spelled here. Every 405 this + // dispatcher's domains answer (`actions.ts`, `ai.ts`, `keys.ts`, + // `meta.ts`, this one) is built this way: an `Allow` header, and the + // body through the one builder. return { handled: true, response: { diff --git a/scripts/check-route-envelope.mjs b/scripts/check-route-envelope.mjs index efb01c92cc4..8d5b00a18cf 100644 --- a/scripts/check-route-envelope.mjs +++ b/scripts/check-route-envelope.mjs @@ -823,7 +823,13 @@ const SHARED_BUILDER = { const DISPATCHER_DOMAIN_DIR = 'packages/runtime/src/domains'; const DISPATCHER_DOMAINS = { - 'actions.ts': { handBuilt: 0 }, + // Was 0. Kind 1: the handler's one 405 (every verb but POST) must carry + // `Allow:` — RFC 9110 requires it — and `deps.error` takes no headers, the + // same reason `mcp.ts`, `meta.ts` and `ai.ts` hand-roll theirs. + 'actions.ts': { + handBuilt: 1, + note: 'one 405 for every verb but POST, which must carry an `Allow:` header (`deps.error` takes none); the body is the declared envelope, the code derived from the status', + }, // [#12160] Not a domain either — the ADR-0126 §5 activation write-authority // gates, shared by the `/automation` toggle door, the `/actions` activation // door and the `/security` catalog activation door so one security policy @@ -875,9 +881,12 @@ const DISPATCHER_DOMAINS = { 'webhooks.ts': { handBuilt: 0 }, // Kind 1 — enveloped, hand-built only because the helper cannot say it. + // Was 1. The 405 for every verb but POST must carry `Allow:` (RFC 9110), and + // `deps.error` takes no headers — the same reason `actions.ts` above and + // `mcp.ts`, `meta.ts` and `ai.ts` below hand-roll theirs. 'keys.ts': { - handBuilt: 1, - note: 'POST /keys is a 201 and `deps.success` hardcodes 200; the body is the declared envelope', + handBuilt: 2, + note: 'POST /keys is a 201 and `deps.success` hardcodes 200; one 405 for every other verb, which must carry an `Allow:` header (`deps.error` takes none); both bodies are the declared envelope, the 405 code derived from the status', }, 'share-links.ts': { handBuilt: 1,