Skip to content

fix(plugin-approvals): a reminder's notification payload carries no one-tap action token - #24620

Merged
os-zhuang merged 8 commits into
mainfrom
claude/funny-albattani-eue8pk
Oct 19, 2026
Merged

os-zhuang merged 8 commits into
mainfrom
claude/funny-albattani-eue8pk

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

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

…ction-link token

The reminder fan-out no longer mints ADR-0043 action tokens into the
payload it hands to messaging. That payload is stored, so a raw token in it
was a usable link at rest. ApprovalNotificationPayload is now closed to
links, and issueActionTokens states where a raw token may travel.

Claude-Session: https://claude.ai/code/session_01CfRDy7SuTfUqaTu2AEH2Hi
Co-authored-by: Claude <noreply@anthropic.com>
…in one assertion

Claude-Session: https://claude.ai/code/session_01CfRDy7SuTfUqaTu2AEH2Hi
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Oct 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-approvals, touching 5 documentable anchor(s).

5 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/automation/approvals.mdx (via sys_approval_request (literal, a string literal in remind))
  • content/docs/automation/flows.mdx (via ApprovalService (symbol, a top-level class), sys_approval_request (literal, a string literal in remind))
  • content/docs/automation/workflows.mdx (via sys_approval_request (literal, a string literal in remind))
  • content/docs/plugins/packages.mdx (via sys_approval_request (literal, a string literal in remind))
  • content/docs/ui/translations.mdx (via sys_approval_request (literal, a string literal in remind))

⛔ 2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via sys_approval_request (literal, a string literal in remind))
  • content/docs/releases/v17/17-6.mdx (via ApprovalService (symbol, a top-level class), sys_approval_request (literal, a string literal in remind))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 54 of 206 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 97 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 6 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 5a871e4e41b110d5d4b0cdcc325a75723740bb31 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from ab866b8d769fa52dddafc1c6158a836095cb090c — the merge of head 63262a2561c62a6894be03e42b6a6edac3d10e9d into base 5a871e4e41b110d5d4b0cdcc325a75723740bb31, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin ab866b8d769fa52dddafc1c6158a836095cb090c && git checkout ab866b8d769fa52dddafc1c6158a836095cb090c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 5a871e4e41b110d5d4b0cdcc325a75723740bb31 63262a2561c62a6894be03e42b6a6edac3d10e9d && git checkout -B drift-repro 5a871e4e41b110d5d4b0cdcc325a75723740bb31 && git merge --no-ff 63262a2561c62a6894be03e42b6a6edac3d10e9d

node scripts/docs-audit/affected-docs.mjs --json 5a871e4e41b110d5d4b0cdcc325a75723740bb31

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 5a871e4e41b110d5d4b0cdcc325a75723740bb31 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

approvals: authorization defect in approval action links (18.0 QA sweep) — detail withheld pending maintainer

3 participants