diff --git a/.changeset/api-action-missing-record-id.md b/.changeset/api-action-missing-record-id.md new file mode 100644 index 00000000000..444550f80f7 --- /dev/null +++ b/.changeset/api-action-missing-record-id.md @@ -0,0 +1,12 @@ +--- +'@objectstack/runtime': patch +--- + +fix(runtime): an `api` action whose `target` names its record is refused `400` naming `recordId` when called with no record id, instead of being forwarded to a path that reaches no route + +Clause-②: no +Metadata-protocol: none + +A same-origin `type: 'api'` action whose `target` carries a record token (`{id}`, `${ctx.recordId}`, or a `{field}` read from the record, for example `approval_approve` → `/api/v1/approvals/requests/{id}/approve`) used to be forwarded even when the caller gave no `recordId`, or an empty one: the token was blanked or left as written, the forwarded path matched no route, and the action door answered `404 ENDPOINT_NOT_FOUND`, telling the caller the endpoint did not exist. + +Both action doors (REST `POST /api/v1/actions/:object/:action` and the MCP `run_action` tool) now refuse that call `400 VALIDATION_ERROR` before any forward, naming the missing input: `recordId` when no record was named, otherwise the field the record has no value for. A call with a real id, and a call with an id the target does not know, are forwarded and answered by the target as before. diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index ca5ef1c5104..12811782c09 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -1331,6 +1331,31 @@ export function foldsApiDecisionOutputs(action: any): boolean { return !isDeclarativeUpdateAction(action) && action?.type === 'api' && apiBodyWrap(action) === undefined; } +/** + * Which record input does this partly built forward `path` still lack, by its + * `{field}` tokens? `recordId` when the caller named no record, otherwise the + * field the record has no value for; `undefined` when every token resolved. + * Read after `${…}` and `{{…}}` are refused, so a brace pair left here is a + * `{field}` token. + */ +function unresolvedApiTargetRecordInput(path: string, routedRecordId: unknown): string | undefined { + const match = /\{([a-z_][a-z0-9_]*)\}/i.exec(path); + if (!match) return undefined; + return routedRecordId === undefined ? 'recordId' : match[1]; +} + +/** The refusal for a `target` record token the dispatch inputs left unresolved: nothing is forwarded. */ +function unresolvedApiTargetRecordRefusal(name: string, target: string, missing: string): ApiActionRequestRefusal { + return { + status: 400, + message: missing === 'recordId' + ? `Action '${name}' has a \`target\` ('${target}') that names its record, and no \`recordId\` was given. ` + + `Pass the id of the record to run it on as \`recordId\`. The request was not sent.` + : `Action '${name}' has a \`target\` ('${target}') that reads \`${missing}\` from its record, and this ` + + `record has no value for it. The request was not sent.`, + }; +} + /** * Assemble the request a same-origin `type: 'api'` action makes — method, * path and JSON body — from its DECLARATION and the dispatch inputs, by the @@ -1341,6 +1366,10 @@ export function foldsApiDecisionOutputs(action: any): boolean { * the caller (`ctx.recordId`, `ctx.user.id`, `ctx.org.id`; `ctx.origin` / * `ctx.apiBase` are `''` on the server, as they are for a same-origin * console), values percent-encoded; then `{field}` from the subject row. + * A record token neither can fill — no `recordId` given (an empty one is + * none), or a row with no value for the field — is REFUSED naming the + * missing input, never blanked or forwarded as written: that path reaches + * no route, and its 404 would say the endpoint does not exist. * 2. The body: the params bag, flat or under `bodyShape.wrap`; the `outputs.*` * fold (#3447); `recordIdParam` seeded from the subject row's * `recordIdField` (default `id`) — or REFUSED when the row cannot supply it @@ -1373,23 +1402,37 @@ export function buildApiActionRequest( : undefined; // ── 1. the path ── + // An empty id is no id: `${ctx.recordId}` and `{id}` stay unresolved, and + // are refused below by name, rather than blanked into a path naming no record. + const routedRecordId: unknown = input.recordId || (input.record as any)?.id || undefined; let path = substituteApiTargetTokens(target, { param: input.params, ctx: { origin: '', apiBase: '', - recordId: input.recordId ?? (input.record as any)?.id, + recordId: routedRecordId, user: { id: ec.userId, email: ec.email }, org: { id: organizationId }, }, }); - if (input.record && /\{[a-z_][a-z0-9_]*\}/i.test(path)) { - const row = input.record; - path = path.replace(/\{([a-z_][a-z0-9_]*)\}/gi, (_m, k: string) => { - const v = row[k]; - return v == null ? '' : encodeURIComponent(String(v)); + if (/\{[a-z_][a-z0-9_]*\}/i.test(path)) { + // Without a row only `{id}` has a source: the routed `recordId`, as + // for `recordIdParam` below. A field with no value is LEFT IN PLACE, + // never blanked: the refusal below names it, and nothing is forwarded. + const row: Record = input.record && typeof input.record === 'object' ? input.record : {}; + path = path.replace(/\{([a-z_][a-z0-9_]*)\}/gi, (m, k: string) => { + const v = row[k] ?? (k === 'id' ? routedRecordId : undefined); + return v == null || v === '' ? m : encodeURIComponent(String(v)); }); } + // A record token left unresolved — the caller named no record, or the + // record has no value for the field the path reads — is refused BEFORE the + // forward, naming the missing input: a forward with a hole in its path + // reaches no route, and its 404 would tell the caller the endpoint does + // not exist instead of what they did not send. `${ctx.recordId}` is read + // here, ahead of the browser-only refusal below, which would otherwise + // claim it; a `{field}` token after it, so a `{{page.x}}` keeps that one. + if (path.includes('${ctx.recordId}')) return unresolvedApiTargetRecordRefusal(name, target, 'recordId'); if (/\$\{[\w.]+\}|\{\{/.test(path)) { return { status: 400, @@ -1400,6 +1443,8 @@ export function buildApiActionRequest( `exists only in the browser). Pass the value as a param, or call the target from the console.`, }; } + const missingRecordInput = unresolvedApiTargetRecordInput(path, routedRecordId); + if (missingRecordInput) return unresolvedApiTargetRecordRefusal(name, target, missingRecordInput); if (!isSameOriginApiTarget(path)) { return { status: 400, message: `Action '${name}' interpolated its \`target\` to '${path}', which is not a same-origin path.` }; } diff --git a/packages/runtime/src/domains/api-action-forward.test.ts b/packages/runtime/src/domains/api-action-forward.test.ts index 5f309e576b6..fec7e6da54e 100644 --- a/packages/runtime/src/domains/api-action-forward.test.ts +++ b/packages/runtime/src/domains/api-action-forward.test.ts @@ -99,6 +99,12 @@ const FLAG_BY_EMAIL = { ai: { exposed: true, description: 'Flag the lead by email.' }, }; +/** An api action whose `target` carries no record token. */ +const PING_LOCAL = { + name: 'ping_local', label: 'Ping', objectName: OBJECT, type: 'api', method: 'POST', + target: '/api/v1/apps/showcase/ping', ai: { exposed: true, description: 'Ping the app.' }, +}; + type Injected = { url: string; init: RequestInit }; function boot(opts: { @@ -122,7 +128,7 @@ 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] }; + const objectDef = { name: OBJECT, actions: [BAN_LEAD, REMOTE_PING, DELETE_LEAD, RECALC_LEAD, APPROVE_LEAD, NOTE_OWNER, FLAG_BY_EMAIL, PING_LOCAL] }; const ql: any = { executeAction, getSchema: (name: string) => (name === OBJECT ? objectDef : undefined), @@ -1342,3 +1348,104 @@ describe('[#28657] a denied subject load forwards an api action whose request ne expect(apiForwardSubjectRecord('u_target', delivered)).toBe(delivered.record); }); }); + +// An api action whose `target` names its record (`{id}`, `${ctx.recordId}`, +// `{field}`) called with no record id, or an empty one, used to be forwarded +// with the token blanked or left as written: the path reached no route and the +// door answered the router's 404 ENDPOINT_NOT_FOUND, telling the caller the +// endpoint does not exist. It is refused 400 VALIDATION_ERROR naming the +// missing input, before any forward. The real-stack half, on the real +// approvals routes, is `packages/verify/src/action-door-api-forward-denied-subject.test.ts`. +describe('an api action whose target names its record, called with no record id, is refused naming `recordId`', () => { + const runViaMcp = async (h: ReturnType, name: string, input: any = {}) => { + 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, input, { + driver: h.ql, + ec: { userId: 'u_agent', tenantId: 'org_1', systemPermissions: [] }, + getMeta: () => h.metadata, + callData: async (_a: string, p: any) => ({ object: p?.object, id: p?.id, record: { id: p?.id, email: 'target@example.test' } }), + }); + }; + + it('REST: the approve shape with no `recordId`, or an empty one, answers 400 VALIDATION_ERROR naming it, and nothing is forwarded', async () => { + for (const body of [{ params: {} }, { recordId: '', params: {} }]) { + const { dispatcher, request } = boot(); + + const res = await dispatcher.handleActions(`/${OBJECT}/${APPROVE_LEAD.name}`, 'POST', body, CTX()); + + expect(statusOf(res), JSON.stringify(body)).toBe(400); + expect(codeOf(res), JSON.stringify(body)).toBe(standardErrorCodeForHttpStatus(400)); + expect(codeOf(res)).toBe('VALIDATION_ERROR'); + expect(messageOf(res)).toContain('`recordId`'); + expect(request, JSON.stringify(body)).not.toHaveBeenCalled(); + } + }); + + it('REST: a `${ctx.recordId}` target and a `{field}` target are refused the same way with no record id', async () => { + for (const action of [DELETE_LEAD, NOTE_OWNER]) { + for (const body of [{}, { recordId: '' }]) { + const { dispatcher, request } = boot(); + + const res = await dispatcher.handleActions(`/${OBJECT}/${action.name}`, 'POST', body, CTX()); + + expect(statusOf(res), `${action.name} ${JSON.stringify(body)}`).toBe(400); + expect(codeOf(res)).toBe('VALIDATION_ERROR'); + expect(messageOf(res)).toContain('`recordId`'); + expect(request).not.toHaveBeenCalled(); + } + } + }); + + it('REST, the controls: a real id is forwarded (200), an unknown id gets the target\'s 404 REQUEST_NOT_FOUND, a tokenless target is forwarded with no id', async () => { + const real = boot(); + const ok = await real.dispatcher.handleActions(`/${OBJECT}/${APPROVE_LEAD.name}`, 'POST', { recordId: 'u_target', params: {} }, CTX()); + expect(statusOf(ok), JSON.stringify(ok?.response?.body)).toBe(200); + expect(real.injected[0].url).toBe('http://app.example.test/api/v1/approvals/requests/u_target/approve'); + + const unknown = boot({ answer: { status: 404, body: { code: 'REQUEST_NOT_FOUND', error: "request 'areq_none' not found" } } }); + const missing = await unknown.dispatcher.handleActions(`/${OBJECT}/${APPROVE_LEAD.name}`, 'POST', { recordId: 'areq_none', params: {} }, CTX()); + expect(statusOf(missing)).toBe(404); + expect(codeOf(missing)).toBe('REQUEST_NOT_FOUND'); + expect(unknown.injected[0].url).toBe('http://app.example.test/api/v1/approvals/requests/areq_none/approve'); + + const ping = boot(); + const pinged = await ping.dispatcher.handleActions(`/${OBJECT}/${PING_LOCAL.name}`, 'POST', { params: {} }, CTX()); + expect(statusOf(pinged), JSON.stringify(pinged?.response?.body)).toBe(200); + expect(ping.injected[0].url).toBe('http://app.example.test/api/v1/apps/showcase/ping'); + }); + + it('MCP `run_action`: the approve shape with no `recordId`, or an empty one, is thrown 400 VALIDATION_ERROR naming it, and nothing is forwarded', async () => { + for (const input of [{ params: {} }, { recordId: '', params: {} }]) { + const h = boot(); + + const thrown = await runViaMcp(h, APPROVE_LEAD.name, input).catch((e) => e); + + expect((thrown as any).status, JSON.stringify(input)).toBe(400); + expect((thrown as any).code).toBe('VALIDATION_ERROR'); + expect(String((thrown as any).message)).toContain('`recordId`'); + expect(h.request).not.toHaveBeenCalled(); + } + }); + + it('buildApiActionRequest: a row with no value for a `{field}` token is refused naming that field, never blanked', () => { + const ec = { userId: 'u1', tenantId: 'org_9' }; + for (const record of [{ id: 'r1' }, { id: 'r1', email: null }, { id: 'r1', email: '' }]) { + const refused = buildApiActionRequest(NOTE_OWNER, { recordId: 'r1', record, params: {}, ec }); + expect((refused as any).status, JSON.stringify(record)).toBe(400); + expect((refused as any).message).toContain('`email`'); + } + // No record at all: the caller named none, so the missing input is `recordId`. + const none = buildApiActionRequest(NOTE_OWNER, { record: {}, params: {}, ec }); + expect((none as any).message).toContain('`recordId`'); + // `{id}` without a row is filled from the routed id, as `recordIdParam` is. + const rowless = buildApiActionRequest(APPROVE_LEAD, { recordId: 'areq 1', params: {}, ec }); + expect((rowless as any).path).toBe('/api/v1/approvals/requests/areq%201/approve'); + }); +}); diff --git a/packages/verify/src/action-door-api-forward-denied-subject.test.ts b/packages/verify/src/action-door-api-forward-denied-subject.test.ts index 6bfe0a75b8a..7881c283c24 100644 --- a/packages/verify/src/action-door-api-forward-denied-subject.test.ts +++ b/packages/verify/src/action-door-api-forward-denied-subject.test.ts @@ -219,4 +219,29 @@ describe('a pending approver decides through the action door — the forward aut expect(await approveRows(requestId)).toEqual([]); expect(pendingOf(await requestRow(requestId))).toEqual([FIRST, SECOND]); }); + + it('Approve with no record id, or an empty one, is refused 400 naming `recordId` — never forwarded to a path with a hole in it', async () => { + const requestId = await openRequest('INV-3'); + + for (const body of [{ params: {} }, { recordId: '', params: {} }]) { + const answer = await call(first, 'POST', '/actions/sys_approval_request/approval_approve', body); + + // The route's 404 ENDPOINT_NOT_FOUND would say the endpoint does not exist. + expect(answer.status, answer.text).toBe(400); + expect(answer.json?.error?.code, answer.text).toBe('VALIDATION_ERROR'); + expect(answer.json?.error?.message, answer.text).toContain('`recordId`'); + } + // Nothing reached the decision route. + expect(await approveRows(requestId)).toEqual([]); + expect(pendingOf(await requestRow(requestId))).toEqual([FIRST, SECOND]); + }); + + it('the control: an id that names no request is still forwarded, and the decision route\'s 404 REQUEST_NOT_FOUND is the answer', async () => { + const answer = await call(first, 'POST', '/actions/sys_approval_request/approval_approve', { + recordId: 'areq_does_not_exist', params: {}, + }); + + expect(answer.status, answer.text).toBe(404); + expect(answer.json?.error?.code, answer.text).toBe('REQUEST_NOT_FOUND'); + }); });