Skip to content

Publish MQTT last wills for non-super users - #3302

Merged
kriszyp merged 3 commits into
mainfrom
fix/mqtt-will-authorization-1874
Oct 20, 2026
Merged

kriszyp merged 3 commits into
mainfrom
fix/mqtt-will-authorization-1874

Conversation

@kriszyp

@kriszyp kriszyp commented Oct 19, 2026 •

Copy link
Copy Markdown
Member
A mailroom clerk at a red-string corkboard while undelivered letters pile up

⊙ Problem

An MQTT last will was never published for anyone but a super user. On disconnect, the server read the will's row in system.hdb_session_will as the client, with authorization on. Only super users may read that system table. The will was dropped, its row leaked, and each disconnect logged Error publishing MQTT will. A clean DISCONNECT leaked the row too.

🤔 Is the requirement right as specified?

Yes. The will is the client's own message, so it should be authorized like its other publishes. The row is server bookkeeping and should not depend on the client's rights. v5.3 has the same code, so this goes to main with milestone v5.3; no separate v5.3 PR is needed.

💡 Solution

  • The will row is read and deleted with a context of its own, never the client's.
  • Only the publish runs as the client (or a scoped token's own role), checked against the will's topic.
  • The row is deleted after any outcome, a refused publish included.
  • A session with no will still never touches the table.

⚖️ Alternatives

  • Exempt system tables from the one-shot authorization check — changes it for every caller; hdb_user must stay super-user-only.
  • Move only the read out of the client's context — a refused publish still rolls back the delete, and a clean DISCONNECT's delete is still checked.
  • Grant every role access to hdb_session_will — every user could read every client's will.

🤔 Should a will row survive a transient publish failure?

No, by decision. The only retry is worker 0's sweep at the next restart, which has no liveness check (#3034), so it would announce a reconnected client as gone. A will that fails with a 503 is lost. Keeping the row is easy once #3034 lands.

🤔 Should the will re-read the user's permissions at disconnect?

Not in this PR. The will is authorized as the user captured at CONNECT, like every PUBLISH on that connection. A permission revoked mid-connection still counts. Re-checking it is #3262.

🔧 Changes

  • server/DurableSubscriptionsSession.ts — disconnect hands the will to settleWill, which reads the row with its own context, publishes on a fresh client context (scoped wills use their own role), and deletes the row in finally with its own context. ⚠️ Look hardest: the explicit {} context on the read and delete is what keeps them off the client's authorization, including under an ambient request context.
  • server/DESIGN.md — the invariant: only the publish is authorized, the row is always deleted, and the publish-before-delete crash window.

✅ Verification

  • npx mocha unitTests/server/sessionWill.test.js (new): 7 passing on RocksDB and on LMDB. On base sources 6 fail; the no-will case already held. The scoped case fails if the will's own role stops deciding.
  • npm run test:integration -- integrationTests/mqtt/will-authorization.test.ts (new, fixture config and schema): 4 passing. On a base build 3 fail: no will delivered, and the row kept after a refused publish and after a clean DISCONNECT.
  • The commits cherry-pick cleanly onto v5.3, and both test files pass there.
  • Gates: test:unit:main 7286 passing, test:unit:resources 4356 passing, test:integration:all 2539 passing and 0 failing. Its 6 cancelled tests are the Ollama suite, which cannot load on this machine.
  • PR QA: skipped @ 7f4aa87 — dispatch unavailable (no worker token in the agent's home)
Test cases

Unit (getSession + disconnect, non-super users with real role permissions):

  • a will its user may publish is published, and its row deleted
  • a will its user may not publish is not published, and its row is still deleted
  • a clean DISCONNECT does not publish or read the will, and deletes the row
  • a second disconnect settles nothing more (one read, one delete)
  • a scoped token's will publishes under its own role, not the session's user; an expired one and one whose role may not publish are dropped; all rows deleted
  • a disconnect under an ambient context that requests authorization settles the row the same
  • a session with no will makes no call on the will table

Integration (broker over WebSocket, users from add_role/add_user, mqtt.requireAuthentication: false):

  • a non-super user's will reaches a separate subscriber after stream.destroy()
  • a will its user may not publish is not delivered, its row is deleted, and the refusal is logged
  • an anonymous client with no will logs no will error and has no will row
  • a clean DISCONNECT suppresses the will and deletes its row

Refs #1874

— Claude Opus 5.5

🤖 Generated with Claude Code

https://claude.ai/code/session_013DQSK8EpQXBf1a5PuaTfLD

🤖 Generated by Claude Code (Claude Opus 5.5); posted via @kriszyp.

Release target: v5.3 — fixes the disconnect will code that #3100 backported to v5.3; the commits cherry-pick cleanly
Related PRs: #2946 overlaps (this rewrites its disconnect will block), #3100 overlaps (its v5.3 backport of that block), 16 others independent
Complexity: medium

Deferred findings

  • 54230fd1488d60f7 major correctness — server/DurableSubscriptionsSession.ts — boot will sweep publishes and deletes will rows of live connections — NOT FILED
  • 4d70083e9aadbda9 minor reliability — server/DurableSubscriptionsSession.ts — any GET/publish failure, retryable 503 included, deletes the only will row — NOT FILED
  • 9134211e84a8d7d6 nit test-coverage — unitTests/server/sessionWill.test.js — spy patches shared will-table statics — NOT FILED
  • 01a2efb9947201f0 none concurrency — server/DurableSubscriptionsSession.ts — settleWill uses three separate transactions — NOT FILED
Origin — the dispatch brief this PR was written from

MQTT Last Will: authorize only the publish, not the internal hdb_session_will read; carry the willId guard to v5.3

Fix harper#1874 as widened in its latest comment (#1874 (comment)): MQTT Last Will never publishes for any non-super user.

Mechanism (server/DurableSubscriptionsSession.ts, SubscriptionsSession.disconnect, ~:607-630 on main): disconnect runs transaction(this.createContext(), ...); createContext sets authorize: true and the session's user. The first Resource call inside is getLastWill().get(willId) on system.hdb_session_will. In resources/Resource.ts authorizeActionOnResource, if (context.authorize) consumes the one-shot flag and checks the session user's read permission on that system table, which fails for everyone but super users (AccessViolation, 401 "Must login" for anonymous, 403 otherwise). publishMessage is never reached. The authorization intended for the will's publish is spent on an internal bookkeeping read.

Fix on main:

  1. Read and delete the hdb_session_will record as internal bookkeeping, outside the session's authorized context (no user, authorize false), so it never fails on the session's permissions.
  2. Then call publishMessage with the session's user (or the scoped will's embedded user, as today via isWillFromExpiredScopedToken / will.user._scopedToken) and authorize: true, so only the publish is authorized against the will topic.
  3. Keep the existing ordering guarantees: will keyed by [sessionId, incarnation], delete in finally, the scoped-token expiry check.

v5.3 line: v5.3 still runs the will lookup on EVERY disconnect that is not a clean DISCONNECT, even when no will was registered, producing one failing transaction and one "Error publishing MQTT will" warn per abnormal disconnect. main fixed that in harper#2946 (commit ce32b18) by tracking session.willId and only touching the table when it is set, but #2946 is a large PR that is not on v5.3. Set the main PR's milestone to v5.3; if the cherry-pick to v5.3 cannot apply (v5.3 lacks willId), open a second minimal PR targeting the v5.3 branch that ports only the willId guard plus this authorization fix. Do not port the rest of #2946.

Acceptance

  • Integration test (integrationTests/mqtt): a non-super user with publish permission on topic T connects with a will on T; its socket is destroyed without DISCONNECT; a separate subscriber on T receives the will payload. Fails on origin/main sources before the fix (prove it with a forced rebuild).
  • Control: a user WITHOUT publish permission on T sets a will; after an abrupt close the will is not published, and the hdb_session_will row is still deleted.
  • Anonymous client (requireAuthentication: false) with no will, abrupt close: no "Error publishing MQTT will" log and no hdb_session_will access.
  • Clean DISCONNECT still suppresses the will and deletes the row.
  • main PR milestone v5.3; if a v5.3-targeted companion PR is needed, it carries the same tests adapted to v5.3.

Dispatch: task harper-1874-will-auth · queued by kris-session · ran by claude/opus/xhigh · worker kzyp-xps-1

Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse,muse; rounds=3; full=1 @ 7f4aa87

Review-Attention: study ~9m (hot: DurableSubscriptionsSession.ts; decisions: unconditional-row-delete, separate-transactions-per-step, bookkeeping-isolation-scope, publish-under-connect-time-user; raised: open major) @ 7f4aa87

kriszyp and others added 3 commits October 19, 2026 16:04
SubscriptionsSession.disconnect ran the will lookup and delete inside the
session's authorize:true context, so the first call, a read of
system.hdb_session_will, spent the one-shot check on the session user's
permission for that system table. Only super users passed: every other
user's will never published, and the aborted transaction rolled back the
delete, leaking the row. A clean DISCONNECT leaked it the same way.

The will row is now read and deleted on contexts of its own, and only the
publish runs on a fresh session context, as the connection's user or a
scoped will's embedded role. The row is deleted whether the publish was
allowed or refused.

Refs #1874

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013DQSK8EpQXBf1a5PuaTfLD
Dispatch-Task: harper-1874-will-auth
The scoped-token case now gives each session a user opposite to its will's
embedded role, so it fails if the will's role stops deciding the publish.
The denied-will integration case drops a redundant fixed wait: its refusal
log line already shows the publish was attempted and refused.

Refs #1874

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013DQSK8EpQXBf1a5PuaTfLD
Dispatch-Task: harper-1874-will-auth
Refs #1874

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013DQSK8EpQXBf1a5PuaTfLD
Dispatch-Task: harper-1874-will-auth
@kriszyp kriszyp added this to the v5.3 milestone Oct 19, 2026
@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Release cherry-pick v5.3: merged

Cherry-picked onto v5.3.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses issue harper#1874 by ensuring that only the publishing of an MQTT last will is authorized, while the session bookkeeping row in system.hdb_session_will is read and deleted on its own context. This prevents non-super users' wills from leaking or failing to publish due to lack of permissions on system tables. The changes refactor the will-settling logic into a dedicated settleWill method in DurableSubscriptionsSession.ts, update the design documentation in DESIGN.md, and introduce comprehensive integration and unit tests to verify the authorization and deletion behavior under various scenarios (such as clean disconnects, expired scoped tokens, and unauthorized publishers). No review comments were provided, so there is no feedback to address.

@kriszyp
kriszyp marked this pull request as ready for review October 20, 2026 00:44
@kriszyp
kriszyp merged commit 4b3e0f6 into main Oct 20, 2026
58 checks passed
@kriszyp
kriszyp deleted the fix/mqtt-will-authorization-1874 branch October 20, 2026 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant