Skip to content

Commit 65a8eeb

Browse files
fix(plugin-approvals): a reminder's notification payload carries no one-tap action token (#24620)
Fixes #24564 Clause-②: no — the change narrows what the platform persists; it is a runtime security-boundary change, so it sits on the human floor and its PR stays draft until the maintainer approves ## What changes - **`remind()` mints no action token and attaches no link.** Each concrete pending approver still gets their own `approval.reminder` notification, with the same title, body, deep link (`/system/approvals?request=ID`) and dedup key. The payload handed to `messaging` no longer carries an `actions` list, and `remind()` no longer calls `issueActionTokens`. - **The approval notification payload type no longer accepts links.** `ApprovalNotificationPayload` drops its optional `actions` member, so a call site that hands a link to `notify()` does not compile. Its doc says why: messaging keeps this payload, verbatim on the `sys_notification` event row and snapshotted onto each `sys_notification_delivery` row, and ADR-0043's storage axis is that the database holds no usable link. - **`issueActionTokens` states the caller's half of that guarantee.** A raw token, or a URL built from one, is never written to a persisted row (a notification payload, an audit row, a log line). It travels only in a message that is sent and not kept. - **Untouched:** the token store, the action page (`GET` and `POST /api/v1/approvals/act`), peek and redeem. A redeemed token still decides as the approver it binds. ## How the links were delivered before, and why the fix is at the producer Read in this repository at the merge base `de43524`: - The reminder's emit names no channel, so messaging falls back to the in-app `inbox` channel (`MessagingService.emit`). Delivery preferences only remove channels; they never add one. - The inbox materialization (`sys_inbox_message`) keeps the title, the body and the action URL, never `actions`. The console bell reads `sys_inbox_message`. - No channel in this repository (inbox, email, SMS) reads `payload.actions`, and no `approval.reminder` template exists. So no recipient received the links through any channel, and the stored payload rows were the only copies. Carrying the links only on a synchronous send has no send to ride on, and minting them at delivery time would be new plumbing for a channel that renders no links. Neither is needed to make the contract hold, so the producer stops putting them in. Nothing a recipient sees changes. ## Tokens stored before this fix A reminder sent by an earlier build stored its links. Each such token is dead at its 72-hour expiry (`ACTION_TOKEN_TTL_MS`), or earlier: once it is used, once its request leaves `pending`, or once its slot is reassigned. No revocation migration ships. 18.0 is a fresh-install release, so no 18.0 deployment holds such rows, and on a development database they lapse within 72 hours. The rows themselves age out under the notification tables' retention, 90 days at most. ## Tests Run at head `63262a2` (the branch with `origin/main` at `5a871e4` merged in), unless a line says otherwise. - **New: `reminder-action-link-at-rest.integration.test.ts`.** A real `ObjectQL` over `SqlDriver` (better-sqlite3, in-memory), the real plugin and the real `sys_notification` object. Live tokens are minted and held in memory, so every scan can fail. - No emit input, and no row of any table in the database, holds an action-page URL or a string whose SHA-256 is a live token's digest. - The data door's read of `sys_notification` serves the reminder event and nothing redeemable in it. - The reminder mints no token and reaches its approver with the deep link. - Positive control: the action page decides once as the bound approver (`actor_id` and `acted_as` are that approver, `via_override` is false) and refuses a replay. - **Updated:** `approval-service.test.ts` (the remind pin asserts per-approver reminders with no link and no mint), `approval-notification-body.integration.test.ts` (the declared payload fields no longer include `actions`), `sys-approval-token-generic-door.integration.test.ts` (mints with `issueActionTokens` instead of reading a link off a reminder). - `pnpm --filter @objectstack/plugin-approvals test`: 82 files, 1116 tests passed. `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0. - **Ablation** (one-time, not committed). The old mint-and-attach was restored in `remind()` with `scripts/ablation-replace.mjs` (anchor hit once, file restored to the HEAD blob, `git diff HEAD` empty) and the four test files above were run: 7 tests went red in three of them, namely the three at-rest pins (the positive control stayed green), the remind unit pin and three body-battery pins. The token-door file stayed green, as it mints directly. The at-rest pin's failure lists the action URL both in the emit input and in the stored `sys_notification` row. - **Live, on a fresh showcase boot of this branch at `466a4fd`** (before the `origin/main` merge, which touched neither package). After a reminder, the stored `sys_notification`, `sys_notification_delivery` and `sys_inbox_message` rows for it carry no action link: one reminder row in each table, zero hits in each. ## Gates - Local: the derived set for this diff (`node scripts/pm/dispatch-gates.mjs --commands`, 69 commands) run at `63262a2`, every one exit 0; `dispatch-gates.mjs --ran` reconciles 69 derived, 69 run, 0 NOT-MEASURED, 0 UNRUN. That includes the `check:type-check-debt` re-measure, the `check:query-options-erasure` ratchet and `check:test-source-alias`. - Not run locally, left to CI: the repo-wide families (`pnpm lint` and the rest of the lint workflow), the `test:repo` vitest project, and the cross-package consumers of `@objectstack/plugin-approvals`, whose public surface is unchanged. - CI: in progress when this PR opened. ## Acceptance notes - **ADR-0043 now says more than the code does in one place.** Its Mechanics bullet says `remind()` gives each concrete approver "their own approve/reject links", and its Non-goals say "the links ride the existing messaging payloads". After this PR no notification carries a link. `docs/adr/**` is a governed surface outside this card's file surface, so it is not edited here; an amendment status line is the maintainer's call. - **The platform checklist item `approvals.email-action-token-door`** (`docs/qa/platform-checklist/areas/approvals.json`) captures live tokens from the stored reminder payload. That capture no longer exists, and after this change no REST door mints a token, so the item needs a revision. It is outside this card's file surface. - `actionLinkUrl` and the plugin's `publicBaseUrl` option keep their contract but have no first-party caller now. They stay, together with `issueActionTokens` and the action page. - Not measured: whether any channel outside this repository reads `payload.actions`. This session has no access to that code. - `remind()` counts a slot that a per-group slate lists twice as two in `notified`, while messaging dedups the second emit. This is unchanged here and was seen on the live boot. - The end-to-end pin through the real messaging service and the REST data door belongs to the dogfood layer (`packages/qa/dogfood`), outside this card's file surface. The in-package pin covers the producer boundary and every stored row of a real engine. --- _Generated by [Claude Code](https://claude.ai/code/session_01CfRDy7SuTfUqaTu2AEH2Hi)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8fbbd50 commit 65a8eeb

6 files changed

Lines changed: 411 additions & 80 deletions
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
fix(plugin-approvals): a reminder's notification no longer carries one-tap approve / reject links, so no stored notification row holds a raw action token
6+
7+
Clause-②: no
8+
9+
- **What changed.** `remind()` still sends each concrete pending approver their own `approval.reminder` notification, with the same title, body and deep link to the request in the Approvals Inbox. It no longer mints ADR-0043 action tokens for that notification, and the payload it hands to `messaging` has no `actions` list. Messaging stores that payload, on the `sys_notification` event row and on each `sys_notification_delivery` row, so the links were kept in the database as usable links. ADR-0043's storage rule (`sys_approval_token` keeps SHA-256 digests only) exists so that the database holds none.
10+
- **What a recipient sees.** Nothing changes. A reminder names no channel, so it is delivered to the in-app inbox, and the inbox row never carried the links.
11+
- **Unchanged.** The action page (`GET` and `POST /api/v1/approvals/act`), `issueActionTokens`, `peekActionToken` and `redeemActionToken` behave exactly as before, and a redeemed token still decides as the approver it binds. The approval notification payload type no longer accepts links, and `issueActionTokens` now states that a raw token may travel only in a message that is sent and not kept.
12+
- **Rows written before this fix.** A reminder sent by an earlier build stored its links. Each of those tokens stops working at its 72-hour expiry, or earlier: once it is used, once its request is decided or recalled, or once its slot is reassigned. The rows themselves age out under the notification tables' retention, 90 days at most.

‎packages/plugins/plugin-approvals/src/approval-notification-body.integration.test.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,11 @@ const ORG_A = 'org_battery_a';
111111
const ORG_B = 'org_battery_b';
112112
const orgOfDeal = (key: string): string => (key.startsWith('sla_') ? ORG_B : ORG_A);
113113

114-
/** The fields `notify()` declares for a notification's payload. */
115-
const PAYLOAD_FIELDS = new Set(['title', 'body', 'actionUrl', 'actions']);
114+
/**
115+
* The fields `notify()` declares for a notification's payload. [#24564] No
116+
* `actions`: an ADR-0043 action link never rides a payload messaging stores.
117+
*/
118+
const PAYLOAD_FIELDS = new Set(['title', 'body', 'actionUrl']);
116119

117120
const sysUser = {
118121
name: 'sys_user',
@@ -230,7 +233,7 @@ describe('every approval notification carries its text in body', () => {
230233
expect(thread.pending_approvers).toEqual(['position:legal', 'u_alice']);
231234
await svc.comment(ids.thread, { actorId: 'u_sub', comment: COMMENT }, asUser('u_sub'));
232235
await svc.requestInfo(ids.thread, { actorId: 'u_alice', comment: QUESTION }, asUser('u_alice'));
233-
// A remind reaches the person with their own action links.
236+
// A remind reaches the person — with no action link (#24564).
234237
await svc.remind(ids.thread, { actorId: 'u_sub' }, asUser('u_sub'));
235238
await svc.reassign(ids.thread, { actorId: 'u_alice', to: 'u_bob' }, asUser('u_alice'));
236239

@@ -298,8 +301,8 @@ describe('every approval notification carries its text in body', () => {
298301
'approval.returned@revise',
299302
'approval.returned@rework',
300303
'approval.reassigned@thread',
301-
'approval.reminder@thread+links',
302-
'approval.reminder@bare+links',
304+
'approval.reminder@thread',
305+
'approval.reminder@bare',
303306
'approval.request_info@thread',
304307
'approval.comment@thread',
305308
'approval.escalated@sla_reassign',
@@ -328,7 +331,7 @@ describe('every approval notification carries its text in body', () => {
328331
// name in each body, not the rest of the sentence around it.
329332
const NAMED: Record<string, string> = {
330333
'approval.requested@thread': 'Deal D_thread',
331-
'approval.reminder@thread+links': 'Deal D_thread',
334+
'approval.reminder@thread': 'Deal D_thread',
332335
'approval.sla_breached@sla_notify': 'Deal D_sla_notify',
333336
'approval.sla_breached@sla_notify>submitter': 'Deal D_sla_notify',
334337
'approval.escalated@sla_reassign': 'Deal D_sla_reassign',
@@ -339,7 +342,7 @@ describe('every approval notification carries its text in body', () => {
339342
'approval.ooo_skipped@ooo': 'Deal D_ooo',
340343
// The control: the object's schema has no label, so the record is named
341344
// by the object's API name and the id.
342-
'approval.reminder@bare+links': 'crm_memo D_bare',
345+
'approval.reminder@bare': 'crm_memo D_bare',
343346
};
344347
const offenders = Object.entries(NAMED).flatMap(([site, name]) => {
345348
const bodies = emits.filter((e) => siteOf(e) === site).map((e) => String(e.payload?.body));

‎packages/plugins/plugin-approvals/src/approval-service.test.ts‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1686,20 +1686,22 @@ describe('ApprovalService (node era)', () => {
16861686
expect(await svc.redeemActionToken(forU7.reject)).toMatchObject({ ok: false, reason: 'not_pending' });
16871687
});
16881688

1689-
it('remind: each concrete approver gets their own action links', async () => {
1689+
it('remind: each concrete approver gets their own reminder, and none carries an action link (#24564)', async () => {
16901690
const emitted: any[] = [];
16911691
svc.attachMessaging({ async emit(input) { emitted.push(input); } });
1692-
const req = await svc.openNodeRequest(openInput(['u9', 'ada@example.com']), CTX);
1693-
await svc.remind(req.id, { actorId: 'u1' }, CTX);
1692+
const reqId = (await svc.openNodeRequest(openInput(['u9', 'ada@example.com']), CTX)).id;
1693+
const minted = (engine._tables['sys_approval_token'] ?? []).length;
1694+
await svc.remind(reqId, { actorId: 'u1' }, CTX);
16941695
const reminders = emitted.filter(e => e.topic === 'approval.reminder');
1695-
expect(reminders).toHaveLength(2);
1696+
expect(reminders.map(r => r.audience)).toEqual([['u9'], ['ada@example.com']]);
16961697
for (const r of reminders) {
1697-
expect(r.audience).toHaveLength(1);
1698-
expect(r.payload.actions).toHaveLength(2);
1699-
expect(r.payload.actions[0].url).toContain('/api/v1/approvals/act?token=');
1698+
// Messaging stores the payload it is handed: no link, no token, in any field.
1699+
expect(Object.keys(r.payload).sort()).toEqual(['actionUrl', 'body', 'title']);
1700+
expect(JSON.stringify(r)).not.toMatch(/token=|\/approvals\/act/);
1701+
expect(r.payload.actionUrl).toBe(`/system/approvals?request=${reqId}`);
17001702
}
1701-
const urls = reminders.flatMap(r => r.payload.actions.map((a: any) => a.url));
1702-
expect(new Set(urls).size).toBe(4); // every link is personal + per-action
1703+
// A reminder mints nothing: a token minted here could only travel in the stored payload.
1704+
expect((engine._tables['sys_approval_token'] ?? []).length).toBe(minted);
17031705
});
17041706

17051707
// ── pagination + search pushdown (#1745) ────────────────────────

‎packages/plugins/plugin-approvals/src/approval-service.ts‎

Lines changed: 48 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -331,14 +331,23 @@ export interface ApprovalMessagingSurface {
331331

332332
/**
333333
* What an approval notification hands to the messaging service: the
334-
* `EmitInput` payload fields the delivered notification is built from, plus a
335-
* reminder's one-tap decision links.
334+
* `EmitInput` payload fields the delivered notification is built from.
336335
*
337336
* `body` is the text the recipient reads under the title. Messaging builds the
338337
* inbox row's `body_md`, and each channel's fallback rendering, from
339338
* `payload.body` and no other field, so the text is declared here rather than
340339
* left to a free-form bag: a call site that spells it any other way does not
341340
* compile. Spelled `message`, it once reached every recipient as an empty body.
341+
*
342+
* ⛔ [#24564] No field carries a bearer credential: no ADR-0043 action link and
343+
* no raw action token, in any spelling. Messaging KEEPS this payload — verbatim
344+
* on the `sys_notification` event row, and snapshotted onto every
345+
* `sys_notification_delivery` row it enqueues — so anything in it is in the
346+
* database for as long as those rows live. ADR-0043's storage axis is that the
347+
* database holds no usable link (`sys_approval_token` stores digests only),
348+
* and a raw link here is a usable link at rest. The shape is closed so that a
349+
* call site handing one over does not compile; {@link ApprovalService.issueActionTokens}
350+
* says where a raw token may travel instead.
342351
*/
343352
type ApprovalNotificationPayload = {
344353
/** The headline. */
@@ -347,8 +356,6 @@ type ApprovalNotificationPayload = {
347356
readonly body: string;
348357
/** Where opening it lands; `notify()` narrows the approvals inbox to the request. */
349358
readonly actionUrl: string;
350-
/** Approve / Reject links on a reminder to a concrete approver (ADR-0043). */
351-
readonly actions?: ReadonlyArray<{ readonly label: string; readonly url: string }>;
352359
};
353360

354361
/** Minimum time between submitter reminders on one request. */
@@ -3719,8 +3726,8 @@ export class ApprovalService implements IApprovalService {
37193726
// `onEmptyApprovers` decides that case, not this fan-out;
37203727
// - not an out-of-office DELEGATE, who was already told above by
37213728
// `approval.ooo_substituted` — one message per approver, not two.
3722-
// No one-tap decision links: ADR-0043 mints those on `remind()` only, and
3723-
// minting bearer tokens on every opening is a decision this does not make.
3729+
// No one-tap decision links: a notification payload is stored, and a raw
3730+
// ADR-0043 token in it is a usable link at rest (`ApprovalNotificationPayload`).
37243731
//
37253732
// [#23556] The body names the record by its object's label and its id —
37263733
// `Deal D_123`, never `crm_deal/D_123` — so a grouped bell entry tells its
@@ -5204,39 +5211,37 @@ export class ApprovalService implements IApprovalService {
52045211
actor_id: recordedActor(context), comment: input.comment ?? null, created_at: nowIso,
52055212
}), { context: SYSTEM_CTX });
52065213

5207-
// Per-approver fan-out: concrete identities (user ids / emails) each get
5208-
// their OWN one-tap approve/reject links (ADR-0043); `position:*`-style
5209-
// literals can't carry a personal token and fall back to a plain nudge.
5214+
// Per-approver fan-out, the opening's shape (`approval.requested`): each
5215+
// concrete identity (a user id / an email) gets its own reminder;
5216+
// `position:*`-style literals go to the shared nudge below, which
5217+
// `notify()` drops — a slot address names no one.
52105218
// [#23978] One name for every recipient, the form `approval.requested` uses.
5219+
//
5220+
// ⛔ [#24564] No one-tap decision link (ADR-0043) rides a reminder.
5221+
// Messaging stores the payload it is handed (`ApprovalNotificationPayload`),
5222+
// so a raw token there is a usable link in the database — the one thing
5223+
// ADR-0043's storage axis rules out. Nor would a link here reach the
5224+
// approver: a reminder names no channel, messaging's default materializes
5225+
// it in the in-app inbox, and the inbox row carries the title, the body and
5226+
// the deep link below, never links. The deep link is the approver's way in.
52115227
const subject = this.nameSubjectRecord(raw.object_name, raw.record_id);
52125228
let notified = 0;
52135229
const concrete = pending.filter(a => a && !a.includes(':'));
52145230
const literals = pending.filter(a => a && a.includes(':'));
52155231
for (const approver of concrete) {
5216-
try {
5217-
const tokens = await this.issueActionTokens(requestId, approver);
5218-
notified += await this.notify({
5219-
topic: 'approval.reminder',
5220-
organizationId: raw.organization_id,
5221-
audience: [approver],
5222-
actorContext: context,
5223-
source: { object: 'sys_approval_request', id: requestId },
5224-
dedupKey: `approval-remind-${requestId}-${nowIso}-${approver}`,
5225-
payload: {
5226-
title: 'Approval reminder',
5227-
body: `A decision on ${subject} is still waiting on you.`,
5228-
actionUrl: '/system/approvals',
5229-
actions: [
5230-
{ label: 'Approve', url: this.actionLinkUrl(tokens.approve) },
5231-
{ label: 'Reject', url: this.actionLinkUrl(tokens.reject) },
5232-
],
5233-
},
5234-
});
5235-
} catch (err: any) {
5236-
this.logger?.warn?.('[approvals] reminder with action links failed', {
5237-
request: requestId, approver, error: err?.message ?? String(err),
5238-
});
5239-
}
5232+
notified += await this.notify({
5233+
topic: 'approval.reminder',
5234+
organizationId: raw.organization_id,
5235+
audience: [approver],
5236+
actorContext: context,
5237+
source: { object: 'sys_approval_request', id: requestId },
5238+
dedupKey: `approval-remind-${requestId}-${nowIso}-${approver}`,
5239+
payload: {
5240+
title: 'Approval reminder',
5241+
body: `A decision on ${subject} is still waiting on you.`,
5242+
actionUrl: '/system/approvals',
5243+
},
5244+
});
52405245
}
52415246
if (literals.length) {
52425247
notified += await this.notify({
@@ -5269,6 +5274,13 @@ export class ApprovalService implements IApprovalService {
52695274
* Issue one-tap approve/reject tokens for one approver on one pending
52705275
* request. Raw tokens are returned ONCE; only SHA-256 hashes are stored
52715276
* (`sys_approval_token`), so a DB leak yields no usable links.
5277+
*
5278+
* ⛔ [#24564] That guarantee holds only as far as the CALLER keeps it: a raw
5279+
* token — or an {@link actionLinkUrl} built from one — is never written to a
5280+
* persisted row. Not a notification payload (messaging stores those, see
5281+
* `ApprovalNotificationPayload`), not an audit row, not a log line. It may
5282+
* travel only in a message that is sent and not kept. No notification this
5283+
* service sends carries one, so no first-party path calls this today.
52725284
*/
52735285
async issueActionTokens(
52745286
requestId: string,
@@ -5423,15 +5435,15 @@ export class ApprovalService implements IApprovalService {
54235435
* vouch for on the decision's context — or `null` when no person can be
54245436
* named without guessing.
54255437
*
5426-
* Tokens are minted for concrete slots only (`remind`'s fan-out), so the
5427-
* slot is a user id or an email:
5438+
* A token binds the slot `issueActionTokens` minted it for, normally a
5439+
* concrete one — a user id or an email:
54285440
*
54295441
* - a user id IS the person;
54305442
* - an email names the ONE account carrying exactly that email — the same
54315443
* proof `resolveActor` takes for a named email, and the recipient
54325444
* resolver's own rule for delivering to it. No account, or more than one,
54335445
* names nobody;
5434-
* - a `type:value` literal names no person (no first-party path mints one).
5446+
* - a `type:value` literal names no person.
54355447
*
54365448
* Before this the slot itself went on the context, so an email-bound link
54375449
* recorded the email as the acting user — on the action row and in the

0 commit comments

Comments
 (0)