Skip to content

Commit a3014cb

Browse files
fix(rest)!: the approvals actions door answers 404 REQUEST_NOT_FOUND for an unknown or invisible request, as its parent door does (#27790)
<!-- claude-projects-attribution: {"github_login":"os-elon-musk"} --> _Requested by **Elon** · [project thread](https://claude.ai/code/project/chan_01WedDzV26nniwxv7jC88B9D?thread=cmsg_01WedDzV26nniwxv7jC88B9D2T3MYD69CaE3sEdpqrhNgW)_ Fixes #27451 Fixes #27493 Clause-②: no (narrowing) Metadata-protocol: none Skills-docs: none — no skill or doc teaches this door's answer for an unknown or invisible request id; `content/docs/api/client-sdk.mdx` names the `approvals.listActions(requestId)` call only ## 维护者速读 - 改了什么:`GET /api/v1/approvals/requests/:id/actions` 遇到不存在、或调用者无权看到的审批单,改为回 404 `REQUEST_NOT_FOUND`(与父接口 `GET …/requests/:id` 同一个查找、同一个响应体),不再回 200 空列表。 - 为什么改:「没有动作记录」与「没有这张审批单」以前无法区分;分诊裁定「404 `REQUEST_NOT_FOUND`, the same as the parent door.」。 - 风险与回滚:BREAKING(服务面收窄),只影响对不可读审批单读动作的调用方,SDK 的 `approvals.listActions` 会改为抛错;可见审批单的答复不变。回滚即 revert 本 PR 一个提交。 - 先看:`packages/rest/src/approvals-routes.ts`(`readableRequestOr404` 与 actions 处理器那一行),`packages/rest/src/rest-approvals-actions-unknown-request.test.ts`,`.changeset/27451-approvals-actions-unknown-request-404.md`。 - 两分钟验证:`pnpm --filter @objectstack/rest exec vitest run src/rest-approvals-actions-unknown-request.test.ts`,预期 3 passed。 - 需要你做的:无,席位落地。 **BREAKING** (a served-set narrowing of one REST door): reading the action trail of an approval request id that does not exist, or of a request the caller is not allowed to see, now answers `404 REQUEST_NOT_FOUND` instead of `200 {"data":[]}`. The changeset ships `patch` with the ADR-0087 disposition `not-required (no-migration-prescription)`: no metadata key changes, only the HTTP answer for a request the caller cannot read. ## What changes | request | before | after | |---|---|---| | `GET /api/v1/approvals/requests/UNKNOWN/actions` | `200 {"data":[]}` | `404 {"code":"REQUEST_NOT_FOUND","error":"Approval request 'UNKNOWN' not found"}` | | `GET …/requests/ID/actions`, ID exists but the caller cannot see it | `200 {"data":[]}` | the same `404` the parent door gives for ID | | `GET …/requests/ID/actions`, ID visible to the caller | `200 {"data":[...]}` | unchanged | The handler now resolves the parent request before listing. The lookup and the `404` envelope are the parent door's own: they move into one local helper, `readableRequestOr404`, inside `registerApprovalsEndpoints`, and both `GET …/requests/:id` and `GET …/requests/:id/actions` call it. There is no second copy of the rule. The parent door's answer is byte-for-byte what it was. `IApprovalService.getRequest` answers `null` both for an id it does not hold and for a request outside the caller's visibility (`ApprovalService.loadRequest`, `enforceVisibility: true`). So the two cases still answer alike, and the door reveals nothing about which requests exist (the no-leak point of #27493). Consumers: `ObjectStackClient.approvals.listActions` throws on any non-2xx answer, so it now rejects for such an id instead of resolving `[]`. In-repo UI callers: `RecordApprovalsPanel.tsx` already wraps each `listApprovalActions(id)` in `.catch(() => [])`; `approvalRequestsDataSource.ts`'s timeline read and `approvalsApi.ts` read the actions of a request they have already resolved. Each reads ids from the request list or the request read, which apply the same visibility rule. ## Tests - New: `packages/rest/src/rest-approvals-actions-unknown-request.test.ts`, the real `RestServer` routes over an approvals stub that models the service's rule (`getRequest` is `null` for an unknown id and for a non-participant; `listActions` is gated by `getRequest`, as `ApprovalService` gates it). - Unknown id: actions `404`, `code: REQUEST_NOT_FOUND`, body deep-equal to the parent door's body, and `listActions` is never called. - Existing request, caller cannot see it: actions `404`, body deep-equal to the parent door's body for the same id. Same status and code as an unknown id, and the message differs only by the id it names. `listActions` is never called. - Visible request: `200 {"data": ROWS}` with the rows unchanged, and `listActions` called once with the id and the caller's context. The parent door still answers `200` with the row. ## Acceptance notes - The fix adds one `getRequest` read before `listActions`, which runs its own `getRequest` as its tenant gate (`approval-service.ts`, `listActions`). That is two reads of the parent per actions request. The service's gate stays as defence in depth for non-REST callers. It is not removed here, because `packages/plugins/plugin-approvals` is outside this card's file surface. - The parent door's handler changed only in shape: its lookup and `404` now come from the shared helper. That was needed to reuse the rule without copying it. Its answer is unchanged. No test drove the parent door's `GET` before, so the new file pins it too: its `404` (unknown and invisible) and its `200` row (visible). ## Verification Final head `acf67294`, base `481ade61` (`origin/main`). The commit after `3e605bac` adds assertions to the new test file only. <details><summary>Pins before the fix (b21c97f, test only): 2 failed, 1 passed</summary> ``` × an unknown request id is 404 REQUEST_NOT_FOUND with the parent door's envelope, and no rows are listed × an existing request the caller cannot see answers the same 404 as the parent, indistinguishable from an unknown id AssertionError: expected 200 to be 404 // Object.is equality Tests 2 failed | 1 passed (3) ``` </details> <details><summary>Ablation at 3e605ba: parent lookup removed, 2 failed, 1 passed; restored blob == HEAD</summary> Through `scripts/ablation-replace.mjs` in wrap mode. The anchor `if (!(await readableRequestOr404(svc, req.params.id, context, res))) return;` went from 1 to 0 on disk, and the marker went from 0 to 1. The subject is imported from `src` by a relative path, so no `dist` leg applies. ``` × an unknown request id is 404 REQUEST_NOT_FOUND ... × an existing request the caller cannot see answers the same 404 as the parent ... AssertionError: expected 200 to be 404 // Object.is equality Tests 2 failed | 1 passed (3) ablation-replace: ok restored: blob == HEAD (6dc9976) and `git diff HEAD` is empty ``` </details> - **Changed-package tests**: `pnpm --filter @objectstack/rest test` at `3e605bac`: Test Files 343 passed (343), Tests 6695 passed, 326 skipped (7021), `VERDICT command-exit 0`. The new file again at `acf67294`: Tests 3 passed (3). - **Typecheck**: `pnpm --filter @objectstack/rest typecheck` (tsc plus `check:test-typecheck`, which compiles the test layer under `tsconfig.test.json`): exit 0. - **Lint**: `eslint --no-inline-config` on both touched source files: exit 0. The repo-wide `pnpm lint` is left to CI. - **Dogfood battery** at `acf67294` (owed by the `(narrowing)` arm), under `os-verify-lock` with CI's command `pnpm --filter @objectstack/dogfood test` (no shard, no path), after building its closure: `VERDICT command-exit 0`. Test Files 355 passed, 3 skipped (358). Tests 2867 passed, 20 skipped (2887). No failure. - **Derived gate set** at `acf67294` (`dispatch-gates --commands`, re-derived after the last commit, same 59 families as at `3e605bac`): reconciled with `--ran`, `59 derived, 57 run, 2 NOT-MEASURED, 0 UNRUN`. Every run family exited 0, including `check-adr-0087-registration` (one declared-breaking changeset, `not-required (no-migration-prescription)`), `check-changeset-no-major`, `check-empty-changeset`, `check:nul-bytes`, `check:doc-authoring`, `check:route-envelope` and `check:rest-log-spy-declared`. The two NOT MEASURED, each exit 3 (PREREQUISITE NOT MET): - `node scripts/check-plugin-teardown-shape.mjs --self-test`: its fixture is pinned to a commit that this shallow checkout cannot reach. CI runs it. - `pnpm check:dual-build-cjs-loads`: it needs every package's `dist/`. CI's `Lint & Repo Gates` runs it. --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016WV4d1Np3hU7RQnA13YZcL --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent f5dd057 commit a3014cb

3 files changed

Lines changed: 174 additions & 5 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@objectstack/rest': patch
3+
---
4+
5+
fix(rest): `GET /api/v1/approvals/requests/:id/actions` answers `404 REQUEST_NOT_FOUND` for a request that does not exist or that the caller cannot see, as its parent door `GET /api/v1/approvals/requests/:id` already did
6+
7+
Clause-②: no (narrowing)
8+
Metadata-protocol: none
9+
10+
<!-- adr-0087: not-required (no-migration-prescription) No key of any metadata schema is removed, renamed or re-shaped, so there is nothing for `objectstack migrate meta` to rewrite and no tombstone. What changes is the HTTP answer one REST door gives for an approval request the caller cannot read. -->
11+
12+
**BREAKING** (a served-set narrowing) for a caller that reads the action trail of an approval request id that does not exist, or of a request it is not allowed to see. That read used to answer `200 { "data": [] }`. It now answers `404 { "code": "REQUEST_NOT_FOUND", "error": "Approval request '<id>' not found" }`, the same status and body the parent door gives for the same id.
13+
14+
The actions door listed rows without resolving its parent request first, so "this request has no actions yet" and "there is no such request" got the same empty answer. It now asks the parent door's own lookup first, and answers the parent door's own `404`. Both doors share that one lookup. An unknown id and a request outside the caller's visibility still answer alike, so the door reveals nothing about which requests exist.
15+
16+
Unchanged: a request the caller can see keeps its answer, `200 { "data": [...] }` with the same rows in the same order.
17+
18+
**What changes for you.** `ObjectStackClient` already throws on any non-2xx answer, so `approvals.listActions(id)` now rejects with the not-found message for such an id, instead of resolving `[]`. Treat that rejection as "no such request". A caller that reads actions only for requests it got from the request list or the request read is not affected.

‎packages/rest/src/approvals-routes.ts‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,15 @@ export function registerApprovalsEndpoints(this: ApprovalsRouteHost, basePath: s
111111
code: 'NOT_IMPLEMENTED',
112112
message: 'Approvals service is not configured on this deployment',
113113
});
114+
// One request read as the caller, and one `404 REQUEST_NOT_FOUND`, for
115+
// every door that answers about a single request: `getRequest` answers
116+
// `null` alike for an id it does not hold and for a request outside the
117+
// caller's visibility, so the doors leak nothing about existence.
118+
const readableRequestOr404 = async (svc: any, id: string, context: any, res: any) => {
119+
const row = await svc.getRequest(id, context ?? {});
120+
if (!row) res.status(404).json({ code: 'REQUEST_NOT_FOUND', error: `Approval request '${id}' not found` });
121+
return row;
122+
};
114123
const handleApprovalError = (res: any, err: any): boolean => {
115124
const msg = String(err?.message ?? err ?? '');
116125
const mapping: Array<[RegExp, number, string]> = [
@@ -325,11 +334,8 @@ export function registerApprovalsEndpoints(this: ApprovalsRouteHost, basePath: s
325334
if (this.enforceAuth(req, res, context)) return;
326335
const svc = await resolveService(environmentId);
327336
if (!svc) return respond501(res);
328-
const row = await svc.getRequest(req.params.id, context ?? {});
329-
if (!row) {
330-
res.status(404).json({ code: 'REQUEST_NOT_FOUND', error: `Approval request '${req.params.id}' not found` });
331-
return;
332-
}
337+
const row = await readableRequestOr404(svc, req.params.id, context, res);
338+
if (!row) return;
333339
res.json(row);
334340
} catch (error: any) {
335341
logError('[REST] Get approval request error:', error);
@@ -581,6 +587,9 @@ export function registerApprovalsEndpoints(this: ApprovalsRouteHost, basePath: s
581587
if (this.enforceAuth(req, res, context)) return;
582588
const svc = await resolveService(environmentId);
583589
if (!svc) return respond501(res);
590+
// The parent door's lookup and 404 first: an unknown id and a
591+
// request the caller cannot see are not an empty audit trail.
592+
if (!(await readableRequestOr404(svc, req.params.id, context, res))) return;
584593
const rows = await svc.listActions(req.params.id, context ?? {});
585594
res.json({ data: rows });
586595
} catch (error: any) {
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* `GET /approvals/requests/:id/actions` answers a request the caller cannot
5+
* read the way its parent door `GET /approvals/requests/:id` does: `404
6+
* REQUEST_NOT_FOUND`, the same envelope, from the same lookup.
7+
*
8+
* Before, the sub-resource listed rows without resolving its parent, so an
9+
* unknown id answered `200 { data: [] }`, and so did an existing request the
10+
* caller may not see. "No actions yet" could not be told from "no such
11+
* request". An unknown id and an invisible id still answer alike, so the
12+
* sub-resource leaks no more about existence than the parent does.
13+
*
14+
* The stub models the service's own rule: `getRequest` answers `null` for an
15+
* id it does not hold AND for a request outside the caller's visibility, and
16+
* `listActions` is gated by `getRequest` exactly as `ApprovalService` gates it
17+
* (an empty list where the parent is null). That is what made the old answer
18+
* an empty 200 rather than a leak.
19+
*/
20+
21+
import { describe, it, expect, vi } from 'vitest';
22+
// `.js` on purpose — NodeNext resolution requires the extension (#7248).
23+
import { RestServer } from './rest-server.js';
24+
25+
const REQ = '/api/v1/approvals/requests/:id';
26+
27+
const REQUEST = { id: 'req_1', status: 'pending', object_name: 'deal', record_id: 'd1', participants: ['u1'] };
28+
const ACTIONS = [
29+
{ id: 'act_1', request_id: 'req_1', action: 'submit', actor_id: 'u1', actor_name: 'Ada' },
30+
{ id: 'act_2', request_id: 'req_1', action: 'comment', actor_id: 'u1', comment: 'ping' },
31+
];
32+
33+
function mockServer() {
34+
return {
35+
get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(),
36+
use: vi.fn(),
37+
listen: vi.fn().mockResolvedValue(undefined),
38+
close: vi.fn().mockResolvedValue(undefined),
39+
};
40+
}
41+
42+
function mockRes() {
43+
const res: any = {
44+
statusCode: 200,
45+
json: vi.fn(function (this: any, body: any) { this._body = body; return this; }),
46+
send: vi.fn(function (this: any) { return this; }),
47+
setHeader: vi.fn(function (this: any) { return this; }),
48+
status: vi.fn(function (this: any, code: number) { this.statusCode = code; return this; }),
49+
};
50+
return res;
51+
}
52+
53+
function approvalsStub() {
54+
const getRequest = vi.fn(async (id: string, ctx: any) =>
55+
id === REQUEST.id && REQUEST.participants.includes(ctx?.userId) ? { ...REQUEST } : null);
56+
const listActions = vi.fn(async (id: string, ctx: any) =>
57+
(await getRequest(id, ctx)) ? ACTIONS.map((a) => ({ ...a })) : []);
58+
return { getRequest, listActions };
59+
}
60+
61+
function boot(approvals: Record<string, any>, userId: string) {
62+
const protocol: any = {
63+
getDiscovery: vi.fn().mockResolvedValue({
64+
version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' },
65+
}),
66+
getMetaTypes: vi.fn().mockResolvedValue([]),
67+
getMetaItems: vi.fn().mockResolvedValue({ items: [] }),
68+
findData: vi.fn().mockResolvedValue({ records: [] }),
69+
};
70+
const rest = new RestServer(
71+
mockServer() as any,
72+
protocol as any,
73+
{ api: { requireAuth: false } } as any,
74+
undefined, // kernelManager
75+
undefined, // envRegistry
76+
undefined, // defaultEnvironmentIdProvider
77+
undefined, // authServiceProvider
78+
undefined, // objectQLProvider
79+
undefined, // emailServiceProvider
80+
undefined, // sharingServiceProvider
81+
undefined, // reportsServiceProvider
82+
async () => approvals, // approvalsServiceProvider
83+
);
84+
(rest as any).resolveExecCtx = async () => ({ userId });
85+
rest.registerRoutes();
86+
return rest;
87+
}
88+
89+
async function get(rest: any, path: string, id: string) {
90+
const found = rest.getRoutes().find((r: any) => r.method === 'GET' && r.path === path);
91+
if (!found) throw new Error(`route not registered: GET ${path}`);
92+
const res = mockRes();
93+
await found.handler({ method: 'GET', path, params: { id }, query: {}, headers: {}, body: {} } as any, res);
94+
return { status: res.statusCode, body: res.json.mock.calls.at(-1)?.[0] };
95+
}
96+
97+
describe('GET /approvals/requests/:id/actions answers an unreadable parent as the parent door does', () => {
98+
it('an unknown request id is 404 REQUEST_NOT_FOUND with the parent door\'s envelope, and no rows are listed', async () => {
99+
const svc = approvalsStub();
100+
const rest = boot(svc, 'u1');
101+
const parent = await get(rest, REQ, 'areq_does-not-exist');
102+
const actions = await get(rest, `${REQ}/actions`, 'areq_does-not-exist');
103+
104+
expect(parent.status).toBe(404);
105+
expect(parent.body?.code).toBe('REQUEST_NOT_FOUND');
106+
expect(actions.status).toBe(404);
107+
expect(actions.body?.code).toBe('REQUEST_NOT_FOUND');
108+
expect(actions.body).toEqual(parent.body);
109+
expect(svc.listActions).not.toHaveBeenCalled();
110+
});
111+
112+
it('an existing request the caller cannot see answers the same 404 as the parent, indistinguishable from an unknown id', async () => {
113+
const svc = approvalsStub();
114+
const rest = boot(svc, 'outsider');
115+
const parent = await get(rest, REQ, REQUEST.id);
116+
const actions = await get(rest, `${REQ}/actions`, REQUEST.id);
117+
const unknown = await get(rest, `${REQ}/actions`, 'areq_does-not-exist');
118+
119+
expect(parent.status).toBe(404);
120+
expect(actions.status).toBe(404);
121+
expect(actions.body?.code).toBe('REQUEST_NOT_FOUND');
122+
expect(actions.body).toEqual(parent.body);
123+
// No existence leak: the invisible id and the unknown id differ only in the id they name.
124+
expect({ status: unknown.status, code: unknown.body?.code }).toEqual({ status: actions.status, code: actions.body?.code });
125+
expect(unknown.body?.error).toBe(String(actions.body?.error).replace(REQUEST.id, 'areq_does-not-exist'));
126+
expect(svc.listActions).not.toHaveBeenCalled();
127+
});
128+
129+
it('a visible request with actions is still 200 with its rows unchanged, and the parent door still answers its row', async () => {
130+
const svc = approvalsStub();
131+
const rest = boot(svc, 'u1');
132+
const parent = await get(rest, REQ, REQUEST.id);
133+
const actions = await get(rest, `${REQ}/actions`, REQUEST.id);
134+
135+
expect(parent.status).toBe(200);
136+
expect(parent.body).toEqual(REQUEST);
137+
expect(actions.status).toBe(200);
138+
expect(actions.body).toEqual({ data: ACTIONS });
139+
expect(svc.listActions).toHaveBeenCalledTimes(1);
140+
expect(svc.listActions).toHaveBeenCalledWith(REQUEST.id, { userId: 'u1' });
141+
});
142+
});

0 commit comments

Comments
 (0)