Skip to content

Commit 0b5ec6b

Browse files
committed
Merge remote-tracking branch 'origin/main' into claude/issue-24579-v18-sweep-accuracy-round2
2 parents d5dd7ed + 973917c commit 0b5ec6b

9 files changed

Lines changed: 606 additions & 111 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)