diff --git a/.changeset/28943-api-key-revoke-mark.md b/.changeset/28943-api-key-revoke-mark.md new file mode 100644 index 00000000000..8f248139f6e --- /dev/null +++ b/.changeset/28943-api-key-revoke-mark.md @@ -0,0 +1,10 @@ +--- +'@objectstack/runtime': patch +--- + +fix(runtime): `list_actions` no longer offers `revoke_api_key` as runnable to a caller signed in with an API key. The action is marked `unavailable` with the API-key management guard's own `PERMISSION_DENIED` and sentence, the refusal `run_action` gives (#28943) + +Clause-②: no +Metadata-protocol: none + +The forward's credential marker (`apiForwardCredentialRefusal`) only asked whether the target's door reads the caller's credential. The data door reads an API key, so `sys_api_key`'s Revoke was listed as runnable to an `osk_` caller. The row's per-object gate (`@objectstack/core` `apiKeyManagementCredentialRefusal`) then refused the write with `403 PERMISSION_DENIED`. The marker now also asks that guard about the data-door request a forward makes (the object, and the operation its method and path name). The guard is the one source: the listing's `unavailable` entry carries the guard's code and sentence, and the REST action door and MCP `run_action` answer that same refusal before forwarding instead of after. The guard's refusal is unchanged, and so are reads of `sys_api_key`, every other object, and the session caller's listing and run. diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index eba998277c1..433ded2f66e 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -1651,14 +1651,19 @@ export function apiForwardCredentialKind(requestContext: HttpProtocolContext | u * A forward refusal: the status and standard code, and the sentence that says * why. `401` when the target's door cannot read the caller's credential (the * status the target would have answered); `403` when the scope-family rule - * refuses an agent the consent did not grant the forward's family. + * refuses an agent the consent did not grant the forward's family, or when the + * target's door accepts the credential and its per-object gate refuses the + * request for it ({@link apiForwardDataDoorGuardRefusal}, that gate's own code + * and sentence). */ export type ApiForwardCredentialRefusal = { status: 401 | 403; code: string; message: string }; /** * The refusal for running `action` by forward with this caller's credential — - * `null` when the target's door accepts it ({@link API_FORWARD_DOOR_ACCEPTS}), - * or when the action is not a same-origin `type: 'api'` action at all. + * `null` when the target's door accepts it ({@link API_FORWARD_DOOR_ACCEPTS}) + * and the target's per-object gate would not refuse the request for it + * ({@link apiForwardDataDoorGuardRefusal}), or when the action is not a + * same-origin `type: 'api'` action at all. * * `target` is the forward's path, which decides the door; the listing passes * the declared `target`, whose door prefix is the same before its tokens are @@ -1678,7 +1683,7 @@ export function apiForwardCredentialRefusal( const kind = apiForwardCredentialKind(requestContext); if (!kind) return null; const door = apiForwardTargetDoor(target); - if (API_FORWARD_DOOR_ACCEPTS[door].has(kind)) return null; + if (API_FORWARD_DOOR_ACCEPTS[door].has(kind)) return apiForwardDataDoorGuardRefusal(kind, action, target); const path = (typeof action?.target === 'string' ? action.target : target).split('?')[0]!; // [ADR-0090 D10 rule 5] The two flipped cells: an agent principal is carried // by a forward credential, bounded by the scope-family rule. @@ -1692,6 +1697,62 @@ export function apiForwardCredentialRefusal( return { status: 401, code: standardErrorCodeForHttpStatus(401), message }; } +/** + * [#28943] The data-door operation a forward to `/api/v1/data/[/…]` + * asks, by the door's own operation names — `undefined` for a path the data + * door does not serve. A safe method (`GET`, `HEAD`) and the door's two reads + * by `POST` (`/:object/query`, `/:object/:id/page`) are reads; `PATCH` and + * `PUT` are `update`, `DELETE` is `delete`, `POST` to the collection is + * `create`, and every other `POST` (`batch`, `createMany`, `import`, `clone`, + * …) is a write the door names on its own route. Only the read-or-write + * reading is load-bearing: the per-object rule this feeds refuses every + * operation it does not list as a read. + */ +function apiForwardDataDoorOperation(method: string, path: string): { object: string; operation: string } | undefined { + const m = /^\/api\/v1(?:\/environments\/[^/]+)?\/data\/([^/?#]+)(?:\/([^?#]*))?/.exec(path); + if (!m) return undefined; + const object = m[1]!; + const rest = (m[2] ?? '').replace(/\/+$/, '').split('/').filter(Boolean); + const verb = method.toUpperCase(); + if (verb === 'GET' || verb === 'HEAD') return { object, operation: rest.length ? 'get' : 'list' }; + if (verb === 'PATCH' || verb === 'PUT') return { object, operation: 'update' }; + if (verb === 'DELETE') return { object, operation: 'delete' }; + if (rest.length === 0) return { object, operation: 'create' }; + if (rest.length === 1 && rest[0] === 'query') return { object, operation: 'list' }; + if (rest.length === 2 && rest[1] === 'page') return { object, operation: 'get' }; + return { object, operation: rest[rest.length - 1]! }; +} + +/** + * [#28943] A forward the target's door ACCEPTS the caller's credential for, + * which the target's per-object gate then refuses for that credential: today + * one rule, a write to `sys_api_key` from a principal an API key admitted + * (`@objectstack/core` `apiKeyManagementCredentialRefusal`, the predicate + * every data door's gate asks, read here and never restated). The refusal is + * the rule's own: its `403`, its `PERMISSION_DENIED`, its sentence. So + * `list_actions` marks `revoke_api_key` unavailable to an `osk_` caller with + * the answer the target gives, and the run doors answer that same refusal + * before forwarding instead of after. A session caller, and a caller whose + * door recorded no credential, are never refused here (the rule is positive + * on `api_key`). + * + * `method` is the forward's (`action.method`, default `POST`, as + * {@link buildApiActionRequest} reads it), so the listing (the declared + * `target`, tokens unfilled) and the run door (the filled path) judge the same + * request: the object segment is never a token. + */ +function apiForwardDataDoorGuardRefusal( + kind: ApiForwardCredentialKind, + action: any, + target: string, +): ApiForwardCredentialRefusal | null { + if (kind !== 'api_key') return null; + const request = apiForwardDataDoorOperation(String(action?.method || 'POST'), target); + if (!request) return null; + const refusal = apiKeyManagementCredentialRefusal({ ...request, credential: kind }); + return refusal ? { status: refusal.status, code: refusal.code, message: refusal.message } : null; +} + /** * [ADR-0090 D10 rule 5, the scope-family rule] Whether an agent's consented * scopes cover a forward outside MCP — `null` when they do, else the refusal, diff --git a/packages/runtime/src/domains/api-action-forward.test.ts b/packages/runtime/src/domains/api-action-forward.test.ts index fec7e6da54e..d69a66a3b1f 100644 --- a/packages/runtime/src/domains/api-action-forward.test.ts +++ b/packages/runtime/src/domains/api-action-forward.test.ts @@ -48,6 +48,7 @@ import { import { FORWARD_CREDENTIAL_HEADER, redeemForwardCredential } from '@objectstack/rest'; import { standardErrorCodeForHttpStatus } from '@objectstack/spec/api'; import { ActionActivationProjection } from '@objectstack/objectql'; +import { apiKeyManagementCredentialRefusal, API_KEY_MANAGEMENT_SESSION_REQUIRED_MESSAGE } from '@objectstack/core'; const OBJECT = 'crm_lead'; const BAN_LEAD = { @@ -112,6 +113,8 @@ function boot(opts: { /** What the target answers: status, JSON body, extra response headers. */ answer?: { status?: number; body?: unknown; text?: string; headers?: Array<[string, string]> }; noServer?: boolean; + /** [#28943] Objects served beside `crm_lead` (by name), e.g. a `sys_api_key` carrying its revoke action. */ + extraObjects?: any[]; } = {}) { const disabled = new Set(opts.disabled ?? []); const injected: Injected[] = []; @@ -129,14 +132,15 @@ function boot(opts: { const server = { getRawApp: () => ({ request }) }; const executeAction = vi.fn(async () => ({ ran: 'script' })); const objectDef = { name: OBJECT, actions: [BAN_LEAD, REMOTE_PING, DELETE_LEAD, RECALC_LEAD, APPROVE_LEAD, NOTE_OWNER, FLAG_BY_EMAIL, PING_LOCAL] }; + const served: Record = Object.fromEntries([objectDef, ...(opts.extraObjects ?? [])].map((o) => [o.name, o])); const ql: any = { executeAction, - getSchema: (name: string) => (name === OBJECT ? objectDef : undefined), - registry: { getObject: (name: string) => (name === OBJECT ? objectDef : undefined), getItem: () => undefined }, + getSchema: (name: string) => served[name], + registry: { getObject: (name: string) => served[name], getItem: () => undefined }, // The subject rows the doors load under the caller's scope. find: vi.fn(async (_object: string, q: any) => { const id = q?.where?.id; - return id === 'u_target' || id === 'lead 7' ? [{ id, email: 'target@example.test' }] : []; + return id === 'u_target' || id === 'lead 7' || id === 'key_1' ? [{ id, email: 'target@example.test' }] : []; }), insert: vi.fn(), update: vi.fn(), delete: vi.fn(), isActionEnabled: (name: string) => !disabled.has(name), @@ -147,8 +151,8 @@ function boot(opts: { load: vi.fn(async () => null), loadDiagnosed: vi.fn(async () => ({ data: null, degraded: false, errors: [] })), loadMany: vi.fn(async () => []), - listObjects: vi.fn(async () => [objectDef]), - getObject: vi.fn(async () => objectDef), + listObjects: vi.fn(async () => Object.values(served)), + getObject: vi.fn(async (name?: string) => (name && served[name]) || objectDef), }; const resolve = (n: string) => n === 'objectql' || n === 'data' ? ql @@ -1000,6 +1004,161 @@ describe('[#28178] the credential × target-door table: an OAuth caller with no }); }); +// [#28943] The 18.3.0 RC's one FAIL: `list_actions` offered `revoke_api_key` +// to an API-key caller as runnable, because the table above only asks whether +// the target's DOOR reads the credential, and the data door reads an API key. +// The `sys_api_key` row's per-object gate then refuses every write from a +// key-admitted principal (`@objectstack/core` `apiKeyManagementCredentialRefusal`, +// the signed-in-session rule `/keys` also keeps), so `run_action` answered +// `403 PERMISSION_DENIED`. The marker now reads that rule itself: the listing +// carries the rule's own code and sentence, the run doors answer that refusal +// before forwarding, and the session caller's listing and run are unchanged. +describe('[#28943] an API-key caller is never offered a `sys_api_key` write as runnable: the mark is the guard\'s own refusal', () => { + /** `@objectstack/platform-objects` `sys_api_key`'s Revoke, as declared. */ + const REVOKE_API_KEY = { + name: 'revoke_api_key', label: 'Revoke API Key', objectName: 'sys_api_key', type: 'api', method: 'PATCH', + target: '/api/v1/data/sys_api_key/{id}', bodyExtra: { revoked: true }, + ai: { exposed: true, description: 'Revoke an API key.', requiresConfirmation: true }, + }; + const API_KEY_OBJECT = { name: 'sys_api_key', actions: [REVOKE_API_KEY] }; + /** The same shape on an object no per-object rule guards: the control. */ + const CLOSE_CASE = { + name: 'close_case', label: 'Close Case', objectName: 'crm_case', type: 'api', method: 'PATCH', + target: '/api/v1/data/crm_case/{id}', bodyExtra: { status: 'closed' }, + ai: { exposed: true, description: 'Close the case.' }, + }; + const CASE_OBJECT = { name: 'crm_case', actions: [CLOSE_CASE] }; + const bootWithKeys = (opts: Parameters[0] = {}) => boot({ ...opts, extraObjects: [API_KEY_OBJECT, CASE_OBJECT] }); + + const KEY_CTX = (): HttpProtocolContext => { + const ctx: any = CTX({ 'x-api-key': 'osk_member_key' }); + delete ctx.request.headers.authorization; + delete ctx.request.headers.cookie; + ctx.admission = { credential: 'api_key', authGate: undefined }; + return ctx as HttpProtocolContext; + }; + const SESSION_CTX = (): HttpProtocolContext => + ({ ...CTX(), admission: { credential: 'session', authGate: undefined } } as unknown as HttpProtocolContext); + const runViaMcp = async (h: ReturnType, ctx: HttpProtocolContext, objectName: string, name: string, recordId: string) => { + const deps: any = { + resolveService: async (_ctx: unknown, service: string) => + service === 'metadata' ? h.metadata + : service === 'automation' ? null + : service === 'http.server' ? { getRawApp: () => ({ request: h.request }) } + : h.ql, + getObjectQL: async () => h.ql, + }; + return invokeBusinessAction(deps, ctx as any, name, { objectName, recordId, confirm: true }, { + driver: h.ql, + ec: { userId: 'u_member', tenantId: 'org_1', systemPermissions: [] }, + getMeta: () => h.metadata, + callData: async (_a: string, p: any) => ({ object: p?.object, id: p?.id, record: { id: p?.id } }), + }); + }; + const listFor = async (h: ReturnType, ctx: HttpProtocolContext): Promise> => + Object.fromEntries(((await (h.dispatcher as any).buildMcpBridge(ctx).listActions()) as any[]).map((a) => [a.name, a])); + + /** What the guard itself answers for this request: the one source of the code and the sentence. */ + const GUARD = apiKeyManagementCredentialRefusal({ object: 'sys_api_key', operation: 'update', credential: 'api_key' })!; + + it('`list_actions` under an API key marks `revoke_api_key` with the guard\'s own code and sentence', async () => { + const listed = await listFor(bootWithKeys(), KEY_CTX()); + + expect(listed[REVOKE_API_KEY.name]).toBeDefined(); + expect(listed[REVOKE_API_KEY.name].unavailable).toEqual({ code: GUARD.code, reason: GUARD.message }); + expect(listed[REVOKE_API_KEY.name].unavailable).toEqual({ + code: 'PERMISSION_DENIED', reason: API_KEY_MANAGEMENT_SESSION_REQUIRED_MESSAGE, + }); + }); + + it('MCP `run_action` and the REST action door under an API key answer that same refusal, and nothing is forwarded', async () => { + const h = bootWithKeys(); + const listed = await listFor(h, KEY_CTX()); + + const thrown = await runViaMcp(h, KEY_CTX(), 'sys_api_key', REVOKE_API_KEY.name, 'key_1').catch((e) => e); + expect(thrown).toBeInstanceOf(Error); + expect((thrown as any).status).toBe(403); + expect((thrown as any).code).toBe(listed[REVOKE_API_KEY.name].unavailable.code); + expect(String(thrown.message)).toBe(listed[REVOKE_API_KEY.name].unavailable.reason); + + const res = await h.dispatcher.handleActions(`/sys_api_key/${REVOKE_API_KEY.name}`, 'POST', { recordId: 'key_1' }, KEY_CTX()); + expect(statusOf(res)).toBe(403); + expect(codeOf(res)).toBe('PERMISSION_DENIED'); + expect(messageOf(res)).toBe(API_KEY_MANAGEMENT_SESSION_REQUIRED_MESSAGE); + + expect(h.request).not.toHaveBeenCalled(); + }); + + it('control: under a signed-in session `revoke_api_key` is listed with no mark, and `run_action` revokes', async () => { + const h = bootWithKeys({ answer: { body: { success: true, data: { id: 'key_1', revoked: true } } } }); + + const listed = await listFor(h, SESSION_CTX()); + expect(listed[REVOKE_API_KEY.name]).toBeDefined(); + expect(listed[REVOKE_API_KEY.name].unavailable).toBeUndefined(); + + const result: any = await runViaMcp(h, SESSION_CTX(), 'sys_api_key', REVOKE_API_KEY.name, 'key_1'); + expect(result.ok).toBe(true); + expect(h.injected).toHaveLength(1); + expect(h.injected[0].url).toBe('http://app.example.test/api/v1/data/sys_api_key/key_1'); + expect(h.injected[0].init.method).toBe('PATCH'); + expect(JSON.parse(String(h.injected[0].init.body))).toMatchObject({ revoked: true }); + }); + + it('control: the same data-door write on an object no rule guards stays runnable under an API key', async () => { + const h = bootWithKeys({ answer: { body: { success: true, data: { id: 'key_1', status: 'closed' } } } }); + + const listed = await listFor(h, KEY_CTX()); + expect(listed[CLOSE_CASE.name]).toBeDefined(); + expect(listed[CLOSE_CASE.name].unavailable).toBeUndefined(); + + const result: any = await runViaMcp(h, KEY_CTX(), 'crm_case', CLOSE_CASE.name, 'key_1'); + expect(result.ok).toBe(true); + expect(h.injected[0].url).toBe('http://app.example.test/api/v1/data/crm_case/key_1'); + expect(headersOf(h.injected[0].init)['x-api-key']).toBe('osk_member_key'); + }); + + it('the reading, request by request: every `sys_api_key` write the guard refuses is refused, its reads are not, and only for a key', () => { + const api = (method: string, target: string) => ({ name: 'probe', type: 'api', method, target }); + const refused = (ctx: HttpProtocolContext, method: string, target: string) => { + const r = apiForwardCredentialRefusal(ctx, api(method, target)); + if (r === null) return 'ok'; + expect(r).toEqual({ status: 403, code: GUARD.code, message: GUARD.message }); + return 'refused'; + }; + const requests: Array<[string, string]> = [ + ['POST', '/api/v1/data/sys_api_key'], + ['PATCH', '/api/v1/data/sys_api_key/{id}'], + ['PUT', '/api/v1/data/sys_api_key/k1'], + ['DELETE', '/api/v1/data/sys_api_key/k1'], + ['POST', '/api/v1/data/sys_api_key/batch'], + ['POST', '/api/v1/data/sys_api_key/k1/clone'], + ['PATCH', '/api/v1/environments/env_1/data/sys_api_key/k1?x=1'], + ['GET', '/api/v1/data/sys_api_key'], + ['GET', '/api/v1/data/sys_api_key/k1'], + ['POST', '/api/v1/data/sys_api_key/query'], + ['POST', '/api/v1/data/sys_api_key/k1/page'], + ['PATCH', '/api/v1/data/sys_api_key_audit/k1'], + ]; + const verdicts = (ctx: () => HttpProtocolContext) => requests.map(([m, t]) => `${m} ${t} ${refused(ctx(), m, t)}`); + expect(verdicts(KEY_CTX)).toEqual([ + 'POST /api/v1/data/sys_api_key refused', + 'PATCH /api/v1/data/sys_api_key/{id} refused', + 'PUT /api/v1/data/sys_api_key/k1 refused', + 'DELETE /api/v1/data/sys_api_key/k1 refused', + 'POST /api/v1/data/sys_api_key/batch refused', + 'POST /api/v1/data/sys_api_key/k1/clone refused', + 'PATCH /api/v1/environments/env_1/data/sys_api_key/k1?x=1 refused', + 'GET /api/v1/data/sys_api_key ok', + 'GET /api/v1/data/sys_api_key/k1 ok', + 'POST /api/v1/data/sys_api_key/query ok', + 'POST /api/v1/data/sys_api_key/k1/page ok', + 'PATCH /api/v1/data/sys_api_key_audit/k1 ok', + ]); + // A session caller, and a caller whose door recorded no credential, keep today's answer everywhere. + for (const ctx of [SESSION_CTX, CTX]) expect(verdicts(ctx).filter((v) => v.endsWith('refused'))).toEqual([]); + }); +}); + describe('[ADR-0090 D10 rule 5, #28367] the two flipped cells: an OAuth agent is forwarded with a forward credential, under the scope-family rule', () => { /** The agent principal the MCP door assembles from a verified OAuth access token. */ const AGENT_CTX = (scopes: string[] = ['actions:execute', 'data:read', 'data:write'], over: Record = {}): HttpProtocolContext => { diff --git a/packages/runtime/src/domains/mcp.ts b/packages/runtime/src/domains/mcp.ts index 8a0a241295f..a33cfc02b9a 100644 --- a/packages/runtime/src/domains/mcp.ts +++ b/packages/runtime/src/domains/mcp.ts @@ -888,6 +888,9 @@ export function buildMcpBridge(deps: DomainHandlerDeps, context: HttpProtocolCon // an API key) is LISTED, marked with the refusal `run_action` // would answer — the same table, the same sentence — so an // agent can explain the gap instead of discovering it. + // [#28943] …and one the target's per-object gate refuses for + // that credential (a `sys_api_key` write under an API key), + // marked with that gate's own code and sentence. const refusal = actionExec.apiForwardCredentialRefusal(context, action); out.push({ ...actionExec.summarizeAction(deps, action, obj, objectName, flow ?? undefined),