From aec121f692bbaa6842d21f0e63dd7547cdd03d22 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 11:31:35 +0000 Subject: [PATCH 1/4] =?UTF-8?q?fix(approvals):=20the=20#3424=20override=20?= =?UTF-8?q?reaches=20a=20pending=20request=20only=20=E2=80=94=20refuse=20o?= =?UTF-8?q?verride-recall=20of=20a=20returned=20request?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ApprovalService.recall`'s override short-circuit sat above ADR-0044's widened state check with no status test of its own, so a platform or tenant admin could recall a `returned` request — a reach no UI ever offered (`viewer.can_override` is ANDed with `status === 'pending'` where it is computed) and no pin ever held, while the `isOverrideActor` doc block and the viewer flag both said pending-only. Re-scope the short-circuit to `pending`, spelled exactly as the viewer flag, so the gate, the doc block and the flag agree at one point. On `returned` an override actor is judged as any other non-submitter and receives the existing refusal (`FORBIDDEN:` + the catalog sentence); the developer log line now names the status it refused on. The submitter's revise-window recall and the override's pending-request rescue are untouched. Pins in approval-revise.test.ts: the narrowing (both override postures, nothing moves — status, actions, lock, run), the reverse checks on `pending`, the submitter's ADR-0044 recall, and a control that the refusal shape is unchanged. Changeset declares the narrowing as an explicit behaviour change. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- .../approval-override-recall-pending-only.md | 47 ++++++ .../src/approval-revise.test.ts | 143 ++++++++++++++++++ .../plugin-approvals/src/approval-service.ts | 34 ++++- 3 files changed, 217 insertions(+), 7 deletions(-) create mode 100644 .changeset/approval-override-recall-pending-only.md diff --git a/.changeset/approval-override-recall-pending-only.md b/.changeset/approval-override-recall-pending-only.md new file mode 100644 index 0000000000..ae1c468ee7 --- /dev/null +++ b/.changeset/approval-override-recall-pending-only.md @@ -0,0 +1,47 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(approvals): the #3424 override reaches a `pending` request only — override-recall of a `returned` request is now refused (#12775) + +**Behaviour change, declared explicitly (maintainer ruling 2026-09-02) so that +one revert of this changeset's PR restores the previous behaviour.** + +`ApprovalService.recall` admits two callers: the submitter, and a platform or +tenant admin releasing a stuck request (the #3424 privileged override). Recall +is also valid on the LATEST `returned` request of a run — the ADR-0044 revise +window, where the submitter abandons the revision instead of resubmitting. +Those two rules met above the state check: the override short-circuit carried +no status test of its own, so an override actor could recall a `returned` +request too. Nothing else on the platform said so — `isOverrideActor`'s doc +block names a PENDING request, `attachViewers` computes +`viewer.can_override` as `status === 'pending' && isOverrideActor(...)`, and +the `approval_recall` action's override arm reads that flag — so the reach was +API-only, never offered by any UI, and pinned by nothing. + +What changes: + +- **Override-recall of a `returned` request is refused.** The override + short-circuit in `recall` now applies only while the request is `pending`, + spelled exactly as the viewer flag is computed. On `returned` an override + actor is judged exactly as any other non-submitter and receives the existing + refusal: `403 FORBIDDEN` over REST, with the operation catalog's + `approval_recall_not_submitter` sentence. No new error code, no new envelope. +- **Only `pending` requests are override-recallable** — the same scope as the + other three override levers (approve / reject / reassign are pending-only at + their endpoints), and the same scope the viewer flag has always declared. + +What does not change: + +- The submitter's own recall of a `returned` request (the ADR-0044 revise + window) is untouched; so is the submitter's recall of a `pending` one. +- The override actor's recall of a `pending` request — the #3424 rescue of a + request routed to an unstaffed position — is untouched, lock release and all. +- The refusal's message, wire code and developer log line keep their shape; the + log line now also names the request status it refused on. + +Why: the gate now agrees with the viewer flag and the documented contract at +one point instead of disagreeing with both. If a real operator workflow +depended on override-recalling a `returned` request, this is the change to +revert; the `returned` record lock is already released, so the stuck-record +rescue motive that justifies the override does not apply on that status. diff --git a/packages/plugins/plugin-approvals/src/approval-revise.test.ts b/packages/plugins/plugin-approvals/src/approval-revise.test.ts index 676c5310f4..783180039f 100644 --- a/packages/plugins/plugin-approvals/src/approval-revise.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-revise.test.ts @@ -16,6 +16,7 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { AutomationEngine, registerScreenNodes } from '@objectstack/service-automation'; import { APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation'; +import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; import { ApprovalService } from './approval-service.js'; import { registerApprovalNode } from './approval-node.js'; import { bindApprovalLockHook, APPROVALS_HOOK_PACKAGE } from './lifecycle-hooks.js'; @@ -370,6 +371,148 @@ describe('Send back for revision (ADR-0044)', () => { await expect(service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter'))).rejects.toThrow(/INVALID_STATE/); }); + describe('the #3424 override reaches `pending` only — override-recall of a `returned` request is refused (#12775)', () => { + // Maintainer ruling 2026-09-02 (via the director seat): the status-blind + // override short-circuit in `recall` is re-scoped to `pending`, spelled + // exactly as `attachViewers` computes `viewer.can_override`, so the gate, + // the `isOverrideActor` doc block and the viewer flag agree at one point. + // Before, the short-circuit sat above ADR-0044's widened state check and + // admitted an override actor on `returned` too — a reach no UI ever + // offered (the flag is ANDed with `status === 'pending'` where it is + // computed) and no pin ever held. The narrowing is deliberate and + // declared as a behaviour change in the package changeset; this block is + // the pin on it, with its reverse checks. + // + // Both #3424 override postures are exercised. This harness's requests + // carry no organization: a tenant admin qualifies as an override actor on + // them (`isOverrideActor`: a null-org request is global), and — as the + // `can_override` pin above notes — `loadRequest` narrows by the CALLER's + // org, so neither context carries a `tenantId` (the ADR-0095 `posture` is + // the tenant-admin signal here). Neither is the submitter, who is + // `submitter`. + const PLATFORM_ADMIN = { isSystem: false, userId: 'root', positions: [], permissions: ['admin_full_access'] } as any; + const TENANT_ADMIN = { isSystem: false, userId: 'org_owner', posture: 'TENANT_ADMIN', positions: [], permissions: [] } as any; + /** + * `@objectstack/rest`'s `handleApprovalError` derives 403 + the ADR-0112 + * wire code `FORBIDDEN` from this prefix and strips it off the body — the + * prefix IS the status/code contract at the service seam. + */ + const WIRE_CODE = /^FORBIDDEN:\s/; + const NOT_SUBMITTER = `FORBIDDEN: ${BUILTIN_OPERATION_MESSAGES.en.approval_recall_not_submitter}`; + const refusalOf = (p: Promise) => p.then(() => null, (e: any) => e); + + /** A pending request on `x1`, with the REAL record-lock hook bound. */ + async function pendingRequest() { + registerReviseFlow(); + const { runId, req } = await startFlow(); + let hook: ((ctx: any) => Promise) | undefined; + bindApprovalLockHook({ + registerHook: (_e: string, h: any) => { hook = h; }, + unregisterHooksByPackage: () => 0, + find: fake.find.bind(fake), + } as any, noopLogger); + expect(hook).toBeDefined(); + const editAttempt = () => hook!({ + object: 'fin_expense', + input: { id: 'x1', data: { amount: 1200 } }, + session: { isSystem: false, positions: [] }, + }); + return { runId, req, editAttempt }; + } + + /** The same request, sent back by its approver: `returned`, unlocked (ADR-0044). */ + async function returnedRequest() { + const opened = await pendingRequest(); + await service.sendBack(opened.req.id, { actorId: 'u1' }, asUser('u1')); + const row = await service.getRequest(opened.req.id, SYSTEM_CTX); + expect(row!.status).toBe('returned'); + return opened; + } + + const runStatus = async () => (await automation.listRuns('expense_approval'))[0].status; + + it('the narrowing: an override actor is refused on `returned` with the non-submitter refusal, and nothing moves', async () => { + const { req, editAttempt } = await returnedRequest(); + await expect(editAttempt()).resolves.toBeUndefined(); // returned → unlocked (ADR-0044) + const runBefore = await runStatus(); + + for (const [label, actor, actorId] of [['platform admin', PLATFORM_ADMIN, 'root'], ['tenant admin', TENANT_ADMIN, 'org_owner']] as const) { + // The viewer flag the gate now agrees with, read by the same actor. + const seen = await service.getRequest(req.id, actor); + expect(seen!.status, label).toBe('returned'); + expect(seen!.viewer!.can_override, label).toBe(false); + + const err = await refusalOf(service.recall(req.id, { actorId, comment: 'rescue' }, actor)); + expect(err, label).toBeInstanceOf(Error); + // The EXISTING non-submitter refusal, envelope and sentence: the + // `FORBIDDEN:` prefix is what REST maps to 403 + code `FORBIDDEN`; + // the sentence is the operation catalog's (#11993). No new code. + expect(err.message, label).toMatch(WIRE_CODE); + expect(err.message, label).toBe(NOT_SUBMITTER); + } + + // Nothing moved: status, the action ledger, the record lock, the run. + const after = await service.getRequest(req.id, SYSTEM_CTX); + expect(after!.status).toBe('returned'); + expect(await actionsOf(req.id)).not.toContain('recall'); + await expect(editAttempt()).resolves.toBeUndefined(); // still unlocked — the refusal touched no lock + expect(await runStatus()).toBe(runBefore); + expect(await runStatus()).not.toBe('cancelled'); + // The window is still open for its owner. + await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); + const round2 = await pendingReq(); + expect(round2.id).not.toBe(req.id); + expect((await service.getRequest(round2.id, SYSTEM_CTX))?.round).toBe(2); + }); + + it('reverse check: the same override actor still recalls the same request while `pending`, and the flag says so', async () => { + const { req, editAttempt } = await pendingRequest(); + await expect(editAttempt()).rejects.toThrow(/RECORD_LOCKED/); // pending → locked + const seen = await service.getRequest(req.id, PLATFORM_ADMIN); + expect(seen!.status).toBe('pending'); + expect(seen!.viewer!.can_override).toBe(true); + + const out = await service.recall(req.id, { actorId: 'root', comment: 'unstaffed role' }, PLATFORM_ADMIN); + expect(out.request.status).toBe('recalled'); + expect(out.resumed).toBe(true); // resumed down the reject branch + expect(await actionsOf(req.id)).toContain('recall'); + await expect(editAttempt()).resolves.toBeUndefined(); // the #3424 release still happens + }); + + it('reverse check (tenant admin): admitted on `pending` too — the narrowing is about status, not posture', async () => { + const { req } = await pendingRequest(); + const out = await service.recall(req.id, { actorId: 'org_owner' }, TENANT_ADMIN); + expect(out.request.status).toBe('recalled'); + expect(await actionsOf(req.id)).toContain('recall'); + }); + + it("the submitter's own revise-window recall is untouched (ADR-0044)", async () => { + const { runId, req } = await returnedRequest(); + const out = await service.recall(req.id, { actorId: 'submitter', comment: 'never mind' }, asUser('submitter')); + expect(out.request.status).toBe('recalled'); + expect(out.resumed).toBe(false); // terminally cancelled, not resumed + expect(out.runId).toBe(runId); + expect(await runStatus()).toBe('cancelled'); + expect(await actionsOf(req.id)).toContain('recall'); + }); + + it('control: a non-submitter without the override is refused on `returned` exactly as before — same envelope, same sentence', async () => { + const { req } = await returnedRequest(); + // The approver who sent it back: not the submitter, holds no override. + const plain = await refusalOf(service.recall(req.id, { actorId: 'u1' }, asUser('u1'))); + expect(plain).toBeInstanceOf(Error); + expect(plain.message).toMatch(WIRE_CODE); + expect(plain.message).toBe(NOT_SUBMITTER); + // The override actor's refusal on `returned` is byte-identical to it: + // the narrowing added no refusal shape, it routed one more caller + // through the existing one. + const admin = await refusalOf(service.recall(req.id, { actorId: 'root' }, PLATFORM_ADMIN)); + expect(admin.message).toBe(plain.message); + expect((await service.getRequest(req.id, SYSTEM_CTX))!.status).toBe('returned'); + expect(await actionsOf(req.id)).not.toContain('recall'); + }); + }); + it('refuses resubmit while another pending request collides on the record (run stays resumable)', async () => { registerReviseFlow(); const { runId, req } = await startFlow(); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index fd8a406788..fed51d8147 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -2876,6 +2876,13 @@ export class ApprovalService implements IApprovalService { * is then paused at the revise-window node (no reject edge), so it is * terminally cancelled via {@link ApprovalResumeSurface.cancelRun} rather * than resumed. + * + * The #3424 privileged override reaches a PENDING request only (#12775, + * maintainer ruling 2026-09-02). On `returned` an override actor is refused + * exactly as any other non-submitter: the gate is spelled as `attachViewers` + * computes `viewer.can_override`, so the gate, the + * {@link ApprovalService.isOverrideActor} doc block and the viewer flag agree + * at one point. */ async recall( requestId: string, @@ -2894,21 +2901,34 @@ export class ApprovalService implements IApprovalService { if (raw.status !== 'pending' && !inReviseWindow) { throw new Error(`INVALID_STATE: request is ${raw.status}`); } - // The submitter withdraws their own request; a privileged admin may recall - // any pending request to release a stuck record (#3424). + // The submitter withdraws their own request — while it is `pending`, or + // while it is `returned` (ADR-0044: abandoning the revision instead of + // resubmitting). A privileged admin may recall a PENDING request to + // release a stuck record (#3424) — and only a pending one. This + // short-circuit used to sit above ADR-0044's widened state check with no + // status test of its own, so it also admitted the override caller on + // `returned`: a reach no UI ever offered (`viewer.can_override` is ANDed + // with `status === 'pending'` where it is computed, in `attachViewers`) + // and no pin ever held. #12775 (maintainer ruling 2026-09-02) re-scoped it + // to `pending`, spelled exactly as the viewer flag, so the gate, the + // `isOverrideActor` doc block and the flag agree at one point. On + // `returned` the override caller is judged exactly as any other + // non-submitter: the branch below, its catalog sentence, its `FORBIDDEN` + // wire code. // - // [#11993] The GATE is untouched — who may recall an approval is exactly - // what it was. Only the refusal's user-facing half changed: it used to be + // [#11993] Only the refusal's user-facing half changed there: it used to be // one hardcoded English sentence that Console rendered verbatim in a // toast. See {@link ApprovalService.userFacingRefusal}. - if (!this.isOverrideActor(context, raw.organization_id ?? null) - && raw.submitter_id && String(raw.submitter_id) !== String(actorId)) { + const overrideAdmits = raw.status === 'pending' + && this.isOverrideActor(context, raw.organization_id ?? null); + if (!overrideAdmits && raw.submitter_id && String(raw.submitter_id) !== String(actorId)) { // The developer's half: the ids the catalog sentence deliberately does // not name (the throw site knows the submitter only as an opaque user // id), kept where a developer reads them and a user never does. const developerMessage = `[approvals] recall refused: actor '${actorId}' is not the submitter of request ` - + `'${requestId}' (submitter '${String(raw.submitter_id)}') and holds no #3424 override`; + + `'${requestId}' (submitter '${String(raw.submitter_id)}') and holds no #3424 override ` + + `for a '${raw.status}' request (the override reaches pending requests only, #12775)`; this.logger?.warn?.(developerMessage, { request: requestId, actor: actorId, From 971c8cabfeead17c9858db5a2aec510e64578779 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 11:32:21 +0000 Subject: [PATCH 2/4] docs(permissions): re-anchor the system-context census after the recall gate edit Pure line rot: the recall-gate hunk in approval-service.ts moved the six `context.isSystem` read sites below it by +20 lines. Rewritten by the gate's own `--fix` (`pnpm gen:system-context-census`); no row content changed. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- content/docs/permissions/system-context.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 682fd4c272..6ff23f4f6c 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -145,7 +145,7 @@ The largest single consumer — **20 of the 109 sites**. |:--|:---|:---|:---|:---| | 40 | **Approval record lock released** — a locked record is writable | plugin-approvals | Get: engine self-writes (the status mirror) pass. Lose: the lock that stops edits while an approval is live. Note there is deliberately **no admin exemption** here — only `isSystem` | `lifecycle-hooks.ts:347` | | 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:570` | -| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:950`, `:1059`, `:3163`, `:3309`, `:3476`, `:3547`, `:3736`, `:3776` | +| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:950`, `:1059`, `:3183`, `:3329`, `:3496`, `:3567`, `:3756`, `:3796` | | 43 | Saved-report ownership is **assignable**, and an update may reassign it | plugin-reports | Get: `ownerId` from input is honoured. A non-system caller always owns what it creates and can never reassign | `plugin-reports/src/report-service.ts:404`, `:425` | | 44 | Saved-report access / export / mutation gates bypassed | plugin-reports | Get: read, bulk-export and overwrite any report | `plugin-reports/src/report-service.ts:343`, `:372`, `:447`, `:684` | | 45 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `attachment-access-hooks.ts:300`, `:349`, `:448`, `:524` | From 9f08922f6a385fe7f5df5ea8df9b992751523baf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 11:58:09 +0000 Subject: [PATCH 3/4] fix(approvals): keep the tracker id out of the recall refusal's runtime log string `check:doc-authoring` (maintainer ruling 2026-08-12) refuses a new internal issue id inside sibling-package string prose; the anchor lives in the adjacent comment, where the reader who can resolve it reads it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- packages/plugins/plugin-approvals/src/approval-service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index fed51d8147..e2ead9bbbe 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -2928,7 +2928,7 @@ export class ApprovalService implements IApprovalService { const developerMessage = `[approvals] recall refused: actor '${actorId}' is not the submitter of request ` + `'${requestId}' (submitter '${String(raw.submitter_id)}') and holds no #3424 override ` - + `for a '${raw.status}' request (the override reaches pending requests only, #12775)`; + + `for a '${raw.status}' request (the override reaches pending requests only)`; this.logger?.warn?.(developerMessage, { request: requestId, actor: actorId, From 9da369ffe1d114b4d1684977fe4ca579336365f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 12:23:28 +0000 Subject: [PATCH 4/4] docs(permissions): regenerate the system-context census on the merged tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regeneration commit after merging origin/main (the merge driver deferred content/docs/permissions/system-context.mdx). Both sides had re-anchored row 42 for approval-service.ts line shifts — this branch's recall-gate hunk and #14542's JSDoc hunk — so the anchors are re-derived from the merged code by the gate's own --fix; no row content changed. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8 --- content/docs/permissions/system-context.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 6ff23f4f6c..b488327b6b 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -145,7 +145,7 @@ The largest single consumer — **20 of the 109 sites**. |:--|:---|:---|:---|:---| | 40 | **Approval record lock released** — a locked record is writable | plugin-approvals | Get: engine self-writes (the status mirror) pass. Lose: the lock that stops edits while an approval is live. Note there is deliberately **no admin exemption** here — only `isSystem` | `lifecycle-hooks.ts:347` | | 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:570` | -| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:950`, `:1059`, `:3183`, `:3329`, `:3496`, `:3567`, `:3756`, `:3796` | +| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3196`, `:3342`, `:3509`, `:3580`, `:3769`, `:3809` | | 43 | Saved-report ownership is **assignable**, and an update may reassign it | plugin-reports | Get: `ownerId` from input is honoured. A non-system caller always owns what it creates and can never reassign | `plugin-reports/src/report-service.ts:404`, `:425` | | 44 | Saved-report access / export / mutation gates bypassed | plugin-reports | Get: read, bulk-export and overwrite any report | `plugin-reports/src/report-service.ts:343`, `:372`, `:447`, `:684` | | 45 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `attachment-access-hooks.ts:300`, `:349`, `:448`, `:524` |