fix(rest)!: the approvals actions door answers 404 REQUEST_NOT_FOUND for an unknown or invisible request, as its parent door does

.changeset/27451-approvals-actions-unknown-request-404.md

@@ -0,0 +1,18 @@
+---
+'@objectstack/rest': patch
+---
+
+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
+
+Clause-②: no (narrowing)
+Metadata-protocol: none
+
+<!-- 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. -->
+
+**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.
+
+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.
+
+Unchanged: a request the caller can see keeps its answer, `200 { "data": [...] }` with the same rows in the same order.
+
+**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

@@ -111,6 +111,15 @@ export function registerApprovalsEndpoints(this: ApprovalsRouteHost, basePath: s
         code: 'NOT_IMPLEMENTED',
         message: 'Approvals service is not configured on this deployment',
     });
+    // One request read as the caller, and one `404 REQUEST_NOT_FOUND`, for
+    // every door that answers about a single request: `getRequest` answers
+    // `null` alike for an id it does not hold and for a request outside the
+    // caller's visibility, so the doors leak nothing about existence.
+    const readableRequestOr404 = async (svc: any, id: string, context: any, res: any) => {
+        const row = await svc.getRequest(id, context ?? {});
+        if (!row) res.status(404).json({ code: 'REQUEST_NOT_FOUND', error: `Approval request '${id}' not found` });
+        return row;
+    };
     const handleApprovalError = (res: any, err: any): boolean => {
         const msg = String(err?.message ?? err ?? '');
         const mapping: Array<[RegExp, number, string]> = [
@@ -325,11 +334,8 @@ export function registerApprovalsEndpoints(this: ApprovalsRouteHost, basePath: s
                 if (this.enforceAuth(req, res, context)) return;
                 const svc = await resolveService(environmentId);
                 if (!svc) return respond501(res);
-                const row = await svc.getRequest(req.params.id, context ?? {});
-                if (!row) {
-                    res.status(404).json({ code: 'REQUEST_NOT_FOUND', error: `Approval request '${req.params.id}' not found` });
-                    return;
-                }
+                const row = await readableRequestOr404(svc, req.params.id, context, res);
+                if (!row) return;
                 res.json(row);
             } catch (error: any) {
                 logError('[REST] Get approval request error:', error);
@@ -581,6 +587,9 @@ export function registerApprovalsEndpoints(this: ApprovalsRouteHost, basePath: s
                 if (this.enforceAuth(req, res, context)) return;
                 const svc = await resolveService(environmentId);
                 if (!svc) return respond501(res);
+                // The parent door's lookup and 404 first: an unknown id and a
+                // request the caller cannot see are not an empty audit trail.
+                if (!(await readableRequestOr404(svc, req.params.id, context, res))) return;
                 const rows = await svc.listActions(req.params.id, context ?? {});
                 res.json({ data: rows });
             } catch (error: any) {

packages/rest/src/rest-approvals-actions-unknown-request.test.ts

@@ -0,0 +1,142 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * `GET /approvals/requests/:id/actions` answers a request the caller cannot
+ * read the way its parent door `GET /approvals/requests/:id` does: `404
+ * REQUEST_NOT_FOUND`, the same envelope, from the same lookup.
+ *
+ * Before, the sub-resource listed rows without resolving its parent, so an
+ * unknown id answered `200 { data: [] }`, and so did an existing request the
+ * caller may not see. "No actions yet" could not be told from "no such
+ * request". An unknown id and an invisible id still answer alike, so the
+ * sub-resource leaks no more about existence than the parent does.
+ *
+ * The stub models the service's own rule: `getRequest` answers `null` for an
+ * id it does not hold AND for a request outside the caller's visibility, and
+ * `listActions` is gated by `getRequest` exactly as `ApprovalService` gates it
+ * (an empty list where the parent is null). That is what made the old answer
+ * an empty 200 rather than a leak.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+// `.js` on purpose — NodeNext resolution requires the extension (#7248).
+import { RestServer } from './rest-server.js';
+
+const REQ = '/api/v1/approvals/requests/:id';
+
+const REQUEST = { id: 'req_1', status: 'pending', object_name: 'deal', record_id: 'd1', participants: ['u1'] };
+const ACTIONS = [
+    { id: 'act_1', request_id: 'req_1', action: 'submit', actor_id: 'u1', actor_name: 'Ada' },
+    { id: 'act_2', request_id: 'req_1', action: 'comment', actor_id: 'u1', comment: 'ping' },
+];
+
+function mockServer() {
+    return {
+        get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(),
+        use: vi.fn(),
+        listen: vi.fn().mockResolvedValue(undefined),
+        close: vi.fn().mockResolvedValue(undefined),
+    };
+}
+
+function mockRes() {
+    const res: any = {
+        statusCode: 200,
+        json: vi.fn(function (this: any, body: any) { this._body = body; return this; }),
+        send: vi.fn(function (this: any) { return this; }),
+        setHeader: vi.fn(function (this: any) { return this; }),
+        status: vi.fn(function (this: any, code: number) { this.statusCode = code; return this; }),
+    };
+    return res;
+}
+
+function approvalsStub() {
+    const getRequest = vi.fn(async (id: string, ctx: any) =>
+        id === REQUEST.id && REQUEST.participants.includes(ctx?.userId) ? { ...REQUEST } : null);
+    const listActions = vi.fn(async (id: string, ctx: any) =>
+        (await getRequest(id, ctx)) ? ACTIONS.map((a) => ({ ...a })) : []);
+    return { getRequest, listActions };
+}
+
+function boot(approvals: Record<string, any>, userId: string) {
+    const protocol: any = {
+        getDiscovery: vi.fn().mockResolvedValue({
+            version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' },
+        }),
+        getMetaTypes: vi.fn().mockResolvedValue([]),
+        getMetaItems: vi.fn().mockResolvedValue({ items: [] }),
+        findData: vi.fn().mockResolvedValue({ records: [] }),
+    };
+    const rest = new RestServer(
+        mockServer() as any,
+        protocol as any,
+        { api: { requireAuth: false } } as any,
+        undefined, // kernelManager
+        undefined, // envRegistry
+        undefined, // defaultEnvironmentIdProvider
+        undefined, // authServiceProvider
+        undefined, // objectQLProvider
+        undefined, // emailServiceProvider
+        undefined, // sharingServiceProvider
+        undefined, // reportsServiceProvider
+        async () => approvals, // approvalsServiceProvider
+    );
+    (rest as any).resolveExecCtx = async () => ({ userId });
+    rest.registerRoutes();
+    return rest;
+}
+
+async function get(rest: any, path: string, id: string) {
+    const found = rest.getRoutes().find((r: any) => r.method === 'GET' && r.path === path);
+    if (!found) throw new Error(`route not registered: GET ${path}`);
+    const res = mockRes();
+    await found.handler({ method: 'GET', path, params: { id }, query: {}, headers: {}, body: {} } as any, res);
+    return { status: res.statusCode, body: res.json.mock.calls.at(-1)?.[0] };
+}
+
+describe('GET /approvals/requests/:id/actions answers an unreadable parent as the parent door does', () => {
+    it('an unknown request id is 404 REQUEST_NOT_FOUND with the parent door\'s envelope, and no rows are listed', async () => {
+        const svc = approvalsStub();
+        const rest = boot(svc, 'u1');
+        const parent = await get(rest, REQ, 'areq_does-not-exist');
+        const actions = await get(rest, `${REQ}/actions`, 'areq_does-not-exist');
+
+        expect(parent.status).toBe(404);
+        expect(parent.body?.code).toBe('REQUEST_NOT_FOUND');
+        expect(actions.status).toBe(404);
+        expect(actions.body?.code).toBe('REQUEST_NOT_FOUND');
+        expect(actions.body).toEqual(parent.body);
+        expect(svc.listActions).not.toHaveBeenCalled();
+    });
+
+    it('an existing request the caller cannot see answers the same 404 as the parent, indistinguishable from an unknown id', async () => {
+        const svc = approvalsStub();
+        const rest = boot(svc, 'outsider');
+        const parent = await get(rest, REQ, REQUEST.id);
+        const actions = await get(rest, `${REQ}/actions`, REQUEST.id);
+        const unknown = await get(rest, `${REQ}/actions`, 'areq_does-not-exist');
+
+        expect(parent.status).toBe(404);
+        expect(actions.status).toBe(404);
+        expect(actions.body?.code).toBe('REQUEST_NOT_FOUND');
+        expect(actions.body).toEqual(parent.body);
+        // No existence leak: the invisible id and the unknown id differ only in the id they name.
+        expect({ status: unknown.status, code: unknown.body?.code }).toEqual({ status: actions.status, code: actions.body?.code });
+        expect(unknown.body?.error).toBe(String(actions.body?.error).replace(REQUEST.id, 'areq_does-not-exist'));
+        expect(svc.listActions).not.toHaveBeenCalled();
+    });
+
+    it('a visible request with actions is still 200 with its rows unchanged, and the parent door still answers its row', async () => {
+        const svc = approvalsStub();
+        const rest = boot(svc, 'u1');
+        const parent = await get(rest, REQ, REQUEST.id);
+        const actions = await get(rest, `${REQ}/actions`, REQUEST.id);
+
+        expect(parent.status).toBe(200);
+        expect(parent.body).toEqual(REQUEST);
+        expect(actions.status).toBe(200);
+        expect(actions.body).toEqual({ data: ACTIONS });
+        expect(svc.listActions).toHaveBeenCalledTimes(1);
+        expect(svc.listActions).toHaveBeenCalledWith(REQUEST.id, { userId: 'u1' });
+    });
+});