fix(events): re-validate live socket keys against the database - #1628
Merged
Merged
Conversation
A WebSocket socket carries the API key as it stood at connect and never refreshes it, while the rooms it joined are never revisited. Only the node processing a revoke, delete or narrowing tore its sockets down, so on any other node the key kept streaming events it had just lost until the client resubscribed or disconnected. The same snapshot gates the pairing QR. A socket that connected in the instant its key was being revoked kept a stale authorization even on a single node: the eviction found nothing to evict, the registration landed after it. The gateway's expiry sweep becomes an authorization sweep on the same 60 s timer. The expiry each socket already carries is settled first and without the database, so a table that is unreachable or locked cannot keep an expired key connected; then one batched read of the key ids currently holding sockets, and eviction with the reason that applies (row gone, inactive, expired, or role/allowedIps/allowedSessions/expiry no longer matching the snapshot). Comparison runs over an authorization fingerprint shared with the immediate eviction path, so the usage tracker's windowed lastUsedAt and usageCount write cannot disconnect every client once a minute, and a rename or a reordered allowlist still evicts nobody. The connect-time snapshot is deliberately not refreshed on subscribe: that would launder a narrowed key while its earlier rooms stay joined. A subscribe under a key that no longer matches that snapshot is recorded instead, because what it grants outlives the row that granted it: a widening put back before the next tick would otherwise leave the socket holding every session's rooms with nothing left to compare. A subscribe whose socket is evicted mid-flight now returns without joining, instead of registering rooms in the adapter after the close.
# Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A WebSocket socket carries the API key as it stood when it connected and never refreshes that snapshot, while the rooms it joined are never revisited. Only the node that processes a revoke, delete or narrowing tears the key's sockets down, so on any other node the key keeps receiving events it is no longer authorized for until the client resubscribes or disconnects. The
session.qrgate rests on the same snapshot, refreshed only at subscribe time. Even on a single node the snapshot can go stale for good: a socket whose key was revoked between its validation and its registration in the gateway is not there when the eviction runs, and nothing looks at it again.EventsGateway.sweepExpiredApiKeysbecomessweepApiKeyAuthorizationon the same 60 s timer: the expiry each socket already carries is settled first and without the database, so an unreachable or locked table cannot keep an expired key streaming; then one batched read (AuthService.findAuthorizationStates) of the distinct key ids currently holding sockets, and eviction through the existing path with the reason that applies, so the client-facing frame is unchanged in shape and names the cause: row gone (deleted), inactive (revoked), past its expiry (expired), or role /allowedIps/allowedSessions/ expiry no longer matching the socket's snapshot (authorization_changed).src/modules/auth/api-key-authorization.tsholds the authorization fingerprint (role, both allowlists normalized and sorted, expiry) plus the scope-list normalizer moved out ofAuthService. The immediate eviction inAuthService.updatenow compares the same fingerprint, so the two paths cannot disagree, and the columns the usage tracker rewrites (lastUsedAt,usageCount,updatedAt) are outside it by construction.handleSubscribereturns without joining any room when the socket was evicted while its re-validation was in flight, instead of registering rooms in the adapter after the disconnect. The connect-time snapshot is still deliberately not refreshed there, with the reason stated in the code; what the subscribe does record is that the fresh key no longer matched that snapshot, because the rooms (and the QR gate) it grants outlive the row that granted them. A widening put back before the next tick would otherwise leave the socket holding every session's rooms with nothing left for the sweep to compare.Behaviour: an operator-driven key change still evicts immediately and synchronously in the same request, with no new dependency, no Redis requirement and no new environment variable. What changes is that a client can now also be disconnected with an
UNAUTHORIZEDframe up to a minute after a change this process never saw, and should reconnect and resubscribe. Redis propagation of evictions is deliberately out of scope here; the timer is the convergence mechanism.Verified with a spec that drives the real
AuthService,ApiKeyUsageTrackerand a real better-sqlite3api_keystable, since a stub cannot prove the fingerprint does not evict every socket on each tick: usage-only writes and a rename or reordered allowlist evict nobody; deletion, revocation, expiry and narrowing each evict with their own message; the connect-while-revoking race is evicted; a resubscribe under a narrowed key does not launder the snapshot; a socket that subscribed under a widening is evicted after the row is put back; an expired key's sockets are evicted with the table unreadable; an eviction during a subscribe joins no room; the operator path still evicts synchronously. Each behaviour was mutation-checked by reverting the line and confirming the failure. The CI gate set was run locally.Fixes #1625