diff --git a/.changeset/approval-readback-loud-refusal.md b/.changeset/approval-readback-loud-refusal.md new file mode 100644 index 0000000000..a69554a1ef --- /dev/null +++ b/.changeset/approval-readback-loud-refusal.md @@ -0,0 +1,23 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(approvals): refuse loudly when a successful mutation's read-back is org-filtered out (#12769) + +Ten ApprovalService result sites (decide/decideNode, recall, sendBack, resubmit, +reassign, remind, requestInfo, comment) read the row they just mutated back +through the caller's organization narrowing and asserted the result non-null +(`fresh!`). For an org-less request row — produced by construction on every +schedule / time-relative / api trigger run (#10131; pinned rather than repaired +by #9132) — an org-scoped caller's read-back matches nothing, so a call that +SUCCEEDED shipped a well-formed success envelope whose declared-non-null +`request` was `null` (HTTP 200 with `"request": null` through the REST +pass-through), and a client dereferencing `request.status` crashed. + +The read-back now throws `READ_BACK_FAILED: …` instead: the write is recorded +and NOT rolled back; only the result echo is refused, loudly. Over REST the +error surfaces through each route's existing 500 arm (`APPROVAL_RECALL_FAILED` +and siblings — already-registered codes) with the `READ_BACK_FAILED:` message +in the body. No declared result type changed — the declared-non-null `request` +is now always true because a result that cannot be built is never returned. The +caller-org narrowing in `loadRequest` (tenancy wall) is deliberately untouched. diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 2aece88f1f..ebc706d637 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -2010,6 +2010,105 @@ describe('ApprovalService — admin override (#3424)', () => { }); }); +// #12769 — the read-back after a successful mutation narrows by the CALLER's +// org (`loadRequest`), so an org-less request row (every schedule / +// time-relative / api trigger run produces one — #10131, pinned rather than +// repaired by #9132) is invisible to an org-scoped caller. Ten result sites +// used to paper over that with a `fresh!` non-null assertion, shipping a +// well-formed success envelope whose declared-non-null `request` was `null`. +// The contract now: the write lands, and the call refuses LOUDLY instead of +// returning a malformed result. The org narrowing itself is a tenancy wall +// and is deliberately untouched by these cases. +describe('ApprovalService — org-filtered read-back refuses loudly (#12769)', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-02-01T10:00:00Z').getTime(); + + // No tenantId — the acting-context shape of a schedule / time-relative / + // api trigger run, which stamps `organization_id = null` on the row. + const ORGLESS_SUBMITTER = { userId: 'u1', positions: [], permissions: [] } as any; + // Org-scoped platform admin: admitted by the override gate on any request + // (#3424), but their read-back narrows to 't1' and misses an org-less row. + const PLATFORM_ADMIN = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any; + // Org-scoped ordinary slot holder — the same miss with no override involved. + const ORG_APPROVER = { userId: 'u9', tenantId: 't1', positions: [], permissions: [] } as any; + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } }); + }); + + const openOrgless = async (recordId = 'opp1', runId = 'run_1') => { + const req: any = await svc.openNodeRequest({ + object: 'opportunity', recordId, runId, nodeId: 'approve_step', + flowName: 'deal_approval', + config: { approvers: [{ type: 'user' as const, value: 'u9' }], behavior: 'first_response' as const }, + record: { id: recordId, amount: 100 }, + }, ORGLESS_SUBMITTER); + // Precondition pin: the row this family is about really is org-less. + expect(req.organization_id ?? null).toBeNull(); + return req; + }; + + it('recall: the row IS mutated and the call throws READ_BACK_FAILED — not a success with request:null', async () => { + const req = await openOrgless(); + await expect(svc.recall(req.id, { actorId: 'root' }, PLATFORM_ADMIN)) + .rejects.toThrow(/^READ_BACK_FAILED: /); + // The refusal is about the echo only — the recall itself landed. + const after = await svc.getRequest(req.id, SYS); + expect(after?.status).toBe('recalled'); + }); + + it('the refusal names the request and says the write was recorded, not rolled back', async () => { + const req = await openOrgless(); + const err: any = await svc.recall(req.id, { actorId: 'root' }, PLATFORM_ADMIN) + .then(() => null, (e: any) => e); + expect(err).toBeTruthy(); + expect(String(err.message)).toMatch(/^READ_BACK_FAILED: /); + expect(String(err.message)).toContain(`'${req.id}'`); + expect(String(err.message)).toContain('was recorded'); + expect(String(err.message)).toContain('NOT rolled back'); + }); + + it('decide: an ordinary org-scoped slot holder hits the same refusal — no override involved', async () => { + const req = await openOrgless(); + await expect(svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, ORG_APPROVER)) + .rejects.toThrow(/^READ_BACK_FAILED: /); + const after = await svc.getRequest(req.id, SYS); + expect(after?.status).toBe('approved'); // the decision landed + }); + + it('comment (thread interaction): same refusal shape', async () => { + const req = await openOrgless(); + await expect(svc.comment(req.id, { actorId: 'u9', comment: 'reviewing' }, ORG_APPROVER)) + .rejects.toThrow(/^READ_BACK_FAILED: /); + const acts = await svc.listActions(req.id, SYS); + // `[length - 1]`, not `.at(-1)`: this package's test layer type-checks under + // lib ES2021 in the TEST_DEBT re-measure, where `.at` is a TS2550 (#5278). + expect(acts[acts.length - 1]).toMatchObject({ action: 'comment', actor_id: 'u9' }); // the comment landed + }); + + it('control: an org-less caller (trigger/system context shape) reads back fine — no narrowing, no refusal', async () => { + const req = await openOrgless(); + const out = await svc.recall(req.id, { actorId: 'u1' }, ORGLESS_SUBMITTER); + expect(out.request).not.toBeNull(); + expect(out.request.status).toBe('recalled'); + }); + + it('control: an org-MATCHED caller on an org-stamped request reads back fine', async () => { + const req: any = await svc.openNodeRequest({ + object: 'opportunity', recordId: 'opp2', runId: 'run_2', nodeId: 'approve_step', + config: { approvers: [{ type: 'user' as const, value: 'u9' }], behavior: 'first_response' as const }, + record: { id: 'opp2' }, + }, CTX); // CTX carries tenantId 't1' → the row lands behind the 't1' wall + expect(req.organization_id).toBe('t1'); + const out = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, ORG_APPROVER); + expect(out.request.status).toBe('approved'); + }); +}); + describe('record-lock hook (node era)', () => { let engine: ReturnType; let svc: ApprovalService; diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 4905b96b8d..d1b7d684c7 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -2496,7 +2496,7 @@ export class ApprovalService implements IApprovalService { }, { context: SYSTEM_CTX }); await this.syncApproverIndex(requestId, stillPending, org, now); const fresh = await this.readBackRequest(requestId, context); - return { request: fresh!, runId, nodeId, finalized: false, decision: input.decision }; + return { request: fresh, runId, nodeId, finalized: false, decision: input.decision }; } } @@ -2522,7 +2522,7 @@ export class ApprovalService implements IApprovalService { ); } const fresh = await this.readBackRequest(requestId, context); - return { request: fresh!, runId, nodeId, finalized: true, decision: input.decision, outputs: mergedOutputs }; + return { request: fresh, runId, nodeId, finalized: true, decision: input.decision, outputs: mergedOutputs }; } /** @@ -2887,7 +2887,7 @@ export class ApprovalService implements IApprovalService { } const fresh = await this.readBackRequest(requestId, context); - return { request: fresh!, runId, resumed, ...(resumeError ? { resumeError } : {}) }; + return { request: fresh, runId, resumed, ...(resumeError ? { resumeError } : {}) }; } // ── Send back for revision / resubmit (ADR-0044) ───────────── @@ -2992,7 +2992,7 @@ export class ApprovalService implements IApprovalService { }); } const fresh = await this.readBackRequest(requestId, context); - return { request: fresh!, runId, resumed, autoRejected: true, ...(resumeError ? { resumeError } : {}) }; + return { request: fresh, runId, resumed, autoRejected: true, ...(resumeError ? { resumeError } : {}) }; } await this.engine.update('sys_approval_request', { @@ -3035,7 +3035,7 @@ export class ApprovalService implements IApprovalService { } const fresh = await this.readBackRequest(requestId, context); - return { request: fresh!, runId, resumed, ...(resumeError ? { resumeError } : {}) }; + return { request: fresh, runId, resumed, ...(resumeError ? { resumeError } : {}) }; } /** @@ -3110,7 +3110,7 @@ export class ApprovalService implements IApprovalService { } const fresh = await this.readBackRequest(requestId, context); - return { request: fresh!, runId, resumed, ...(resumeError ? { resumeError } : {}) }; + return { request: fresh, runId, resumed, ...(resumeError ? { resumeError } : {}) }; } /** @@ -3283,7 +3283,7 @@ export class ApprovalService implements IApprovalService { }); const fresh = await this.readBackRequest(requestId, context); - return { request: fresh! }; + return { request: fresh }; } /** @@ -3366,7 +3366,7 @@ export class ApprovalService implements IApprovalService { } const fresh = await this.readBackRequest(requestId, context); - return { request: fresh!, notified }; + return { request: fresh, notified }; } // ── Actionable links (ADR-0043) ────────────────────────────── @@ -3512,7 +3512,7 @@ export class ApprovalService implements IApprovalService { } const fresh = await this.readBackRequest(requestId, context); - return { request: fresh! }; + return { request: fresh }; } /** Free-form reply on the thread (submitter or any pending approver). */ @@ -3554,7 +3554,7 @@ export class ApprovalService implements IApprovalService { }); const fresh = await this.readBackRequest(requestId, context); - return { request: fresh! }; + return { request: fresh }; } // ── SLA escalation (ADR-0042) ───────────────────────────────── @@ -4757,12 +4757,33 @@ export class ApprovalService implements IApprovalService { * flow-driven resume, a service-to-service call), turning a successful write * into a `null` result. Gating belongs on the read API, not on an * operation's own return value. + * + * [#12769] The read still narrows by the CALLER's organization + * (`loadRequest` — a deliberate tenancy wall, untouched here), and an + * org-less request row is invisible inside an org-scoped caller's narrowing. + * Org-less rows arise by construction on every schedule / time-relative / + * api trigger run (#10131; #9132 pinned that behaviour rather than repairing + * it), so this is a live state, not an edge case: the write IS recorded and + * the echo cannot be built. It used to escape as `null` behind the callers' + * `fresh!` non-null assertions — a well-formed 200 whose declared-non-null + * `request` was `null` on the wire. Now it refuses loudly instead, which + * keeps every declared result type true: a result that cannot be built is + * never returned. Callers must NOT catch this to fall back to `null` — + * that would re-open the type-lie one level up. */ private async readBackRequest( requestId: string, context: ExecutionContext, - ): Promise { - return this.loadRequest(requestId, context, false); + ): Promise { + const fresh = await this.loadRequest(requestId, context, false); + if (!fresh) { + throw new Error( + `READ_BACK_FAILED: the write to approval request '${requestId}' was recorded, but the updated row is ` + + `not visible inside the caller's organization scope, so the result envelope cannot be built. The write ` + + `is NOT rolled back — read the request back with a system or matching-organization context (#12769).`, + ); + } + return fresh; } async getRequest(requestId: string, context: ExecutionContext): Promise {