Repository navigation
Publish MQTT last wills for non-super users - #3302
Conversation
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
Release cherry-pick
|
There was a problem hiding this comment.
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.
⊙ 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_willas the client, with authorization on. Only super users may read that system table. The will was dropped, its row leaked, and each disconnect loggedError 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
⚖️ Alternatives
hdb_usermust stay super-user-only.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—disconnecthands the will tosettleWill, which reads the row with its own context, publishes on a fresh client context (scoped wills use their own role), and deletes the row infinallywith its own context.{}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.v5.3, and both test files pass there.test:unit:main7286 passing,test:unit:resources4356 passing,test:integration:all2539 passing and 0 failing. Its 6 cancelled tests are the Ollama suite, which cannot load on this machine.Test cases
Unit (
getSession+disconnect, non-super users with real role permissions):disconnectsettles nothing more (one read, one delete)disconnectunder an ambient context that requests authorization settles the row the sameIntegration (broker over WebSocket, users from
add_role/add_user,mqtt.requireAuthentication: false):stream.destroy()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
54230fd1488d60f7major correctness —server/DurableSubscriptionsSession.ts— boot will sweep publishes and deletes will rows of live connections — NOT FILED4d70083e9aadbda9minor reliability —server/DurableSubscriptionsSession.ts— any GET/publish failure, retryable 503 included, deletes the only will row — NOT FILED9134211e84a8d7d6nit test-coverage —unitTests/server/sessionWill.test.js— spy patches shared will-table statics — NOT FILED01a2efb9947201f0none concurrency —server/DurableSubscriptionsSession.ts— settleWill uses three separate transactions — NOT FILEDOrigin — 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 setsauthorize: trueand the session's user. The first Resource call inside isgetLastWill().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:
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
Dispatch: task
harper-1874-will-auth· queued by kris-session · ran by claude/opus/xhigh · worker kzyp-xps-1Review-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