Repository navigation
feat(ws): require a logged-in session for /ws when auth is enabled - #3324
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Existing sockets are authorized only at connection time, so enabling authentication or revoking a session does not remove already-connected listeners from WS_GROUP.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds session authentication to the /ws WebSocket endpoint when authentication is enabled, while preserving open access when disabled.
Changes:
- Wraps WebSocket routing with
AuthMiddlewareStack. - Rejects unauthenticated connections before joining the broadcast group.
- Adds end-to-end tests, typing stubs, and QA/developer documentation.
File summaries
| File | Description |
|---|---|
src/anthias_server/app/consumers.py |
Adds the WebSocket authorization gate. |
src/anthias_server/django_project/asgi.py |
Wires session authentication into ASGI routing. |
tests/test_consumers.py |
Tests authorization and real ASGI handshakes. |
website/content/docs/qa-checklist.md |
Documents WebSocket authentication QA. |
website/content/docs/developer-documentation.md |
Documents /ws authentication behavior. |
stubs/channels-stubs/generic/websocket.pyi |
Types the consumer scope. |
stubs/channels-stubs/auth.pyi |
Adds middleware typing. |
stubs/asgiref-stubs/testing.pyi |
Types ApplicationCommunicator. |
stubs/asgiref-stubs/py.typed |
Marks partial stubs. |
stubs/asgiref-stubs/__init__.pyi |
Adds the partial stub package marker. |
pyproject.toml |
Documents the added stubs. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
vpetersson-bot
force-pushed
the
siri-62-authenticate-ws
branch
from
September 15, 2026 15:17
c0c34b5 to
a5595ea
Compare
- Wrap the websocket router in AuthMiddlewareStack so the handshake resolves scope['user'] from the session cookie - Refuse the handshake in AssetConsumer.connect() before accept() and before the channel joins WS_GROUP, so an unauthenticated listener can no longer time the device's writes - Gate on settings['auth_backend'], matching @Authorized: with auth off the endpoint stays open, as every other surface does - Cover the gate with unit tests plus end-to-end tests that drive the real ASGI app with a genuine session cookie Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
connect() decided authorization once, so a socket opened while auth was off kept streaming after the operator turned auth on, and a socket opened for an operator outlived the credentials it was accepted under. - Re-check authorization per frame in asset_update(); stay silent rather than closing, since a close is itself an event the listener could time against the write being withheld - Add force_disconnect()/disconnect_all() and fan it out from both settings-save paths so sockets re-handshake against the new state - Have apply_auth_settings() report whether credentials in use were invalidated, so the HTML and DRF surfaces can't drift on when to reap sockets, and an unrelated save doesn't bounce them - Share the after-close race matcher between the send and close paths Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
vpetersson-bot
force-pushed
the
siri-62-authenticate-ws
branch
from
September 15, 2026 15:18
a5595ea to
e5bfc7f
Compare
|
vpetersson
approved these changes
Sep 15, 2026
There was a problem hiding this comment.
🟡 Changes recommended
The unresolved critical credential-revocation issue and two moderate disconnect-order issues must be fixed.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 16/17 changed files
- Comments generated: 3
- Review effort level: Lite
This was referenced Sep 15, 2026
vpetersson
pushed a commit
that referenced
this pull request
Oct 23, 2026
* fix(ws): make /ws credential revocation fail closed Follow-up Copilot review on PR #3324 found three ways a socket can still outlive the credentials it was accepted under. - Track an auth generation with each socket. A password or username rotation doesn't change the already-resolved scope['user'] — is_authenticated stays True for any real User row — so the per-frame re-check kept passing for a socket whose force_disconnect was lost (a transient Redis blip during the save is enough, since _broadcast deliberately swallows channel-layer failures). disconnect_all() now bumps a process-global counter before fanning the close out, and a socket whose recorded generation is stale goes silent. No DB hit per frame, and no dependence on the channel layer being healthy. - Skip that check while auth is disabled, so a socket that missed its close on an auth-off save isn't stranded on the 5s poll for good. - Reap sockets before publishing the viewer reload, on both settings-save surfaces. send_to_viewer() goes over Redis and can raise, which left the request exiting through its error handler with the new credentials persisted and every old socket still attached. Regression tests at both levels, each verified to fail without its fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * fix(ws): hook revocation to the User row, not the settings page Second Copilot review on this PR found the generation bump was reachable from too few places, and reachable too late. - Revoke from a User post_save/post_delete receiver. /admin is a routed URL and the stock UserAdmin ships a change-password form, so a rotation there — or from `manage.py changepassword`, or a shell — never reached disconnect_all(). Hooking the model's own save is the one place that sees every credential mutation, including ones added later. last_login is exempt (Django writes it on each login through update_fields); anything else revokes, so is_active/is_staff changes and a deleted operator are covered too. - Reap in a finally around the settings write. apply_auth_settings() has already persisted the rotated User row by then, so a conf write that fails on a full or read-only /data volume jumped to the error handler with the new password live and every old socket attached. The signal now covers the rotation case on its own; the finally is what also covers a bare auth_backend toggle, which touches no User. - Stop the docs and the disconnect_all() docstring claiming a surviving socket always goes quiet — while auth is being disabled it keeps working by design, which is the open-device contract. Regression tests for each, verified to fail with the fix backed out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * fix(ws): scope the revocation claim, and unmask the reap tests Third Copilot review on this PR. All five stand up. - Stop claiming out-of-process credential changes fail closed. The receiver runs in whichever process made the change, so `manage.py changepassword` or a shell bumps a counter nobody reads; only the best-effort Redis fan-out reaches uvicorn. What does fail closed is every surface served by uvicorn — the settings page, the v2 API and /admin — which is the gap that prompted the receiver. Narrowed the docstrings and the docs to say exactly that; making the rest fail closed would need the serving process re-reading credential state from the DB on a timer, which is the cost the counter exists to avoid. - Rewrite the settings-save reap tests. Adding the receiver hollowed them out: they enable auth by creating the operator, which fires the receiver, so `disconnect.called` stayed true with the view's reap deleted. They now toggle auth_backend against an operator row that already exists and send no new credentials, so no User is written, the receiver stays silent and the view is the only possible caller — and they assert the reap precedes the viewer publish rather than merely happening. Six tests, all verified to fail against the pre-fix arrangement; before this they could not. - Register the receiver through an explicit `signals.register()` call instead of a side-effect import needing `# noqa: F401`, which CLAUDE.md rules out where an idiom fixes the root cause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * fix(ws): split the auth-change signal, narrow the reap, defer to commit Fourth Copilot review on this PR — three findings, all of them fair. - Report backend and credential changes separately. apply_auth_settings returned one "something changed" bool, so the settings views reaped for a password rotation that the User receiver had already reaped, fanning a second force_disconnect over Redis and bumping the generation twice on every credential change. It now returns an AuthChange(backend_changed, credentials_rotated) and the views act only on the backend half — the half nothing else can see, since a toggle writes no User row. - Narrow the finally to settings.save(). Wrapping the whole mutation block meant an exception before the conf write also reaped; the write is the only thing there that can fail after apply_auth_settings() has committed its half. This also reverts the reindentation the wide version forced on ~8 unrelated lines. - Defer the receiver's revoke to transaction.on_commit. post_save fires inside the caller's transaction and Django's admin wraps its change form in one, so the write can still roll back — while the generation bump cannot, leaving a socket that also missed the close silent for good over a change that never happened. Outside a transaction (no ATOMIC_REQUESTS here) on_commit runs inline, so nothing is delayed on the paths that matter. Regression tests for each, plus a rolled-back admin-style change; all verified to fail with their fix backed out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * fix(ws): don't reap twice when one save both enables auth and sets credentials The view-level reap keys on backend_changed alone, but one save can do both: enabling auth *and* supplying the username/password returns AuthChange(backend_changed=True, credentials_rotated=True), which tests/test_auth.py already pins. apply_auth_settings() writes the User, the post_save receiver bumps the generation and reaps — and then this finally block fans a second force_disconnect over Redis and bumps the generation again, for one operator action. That duplicate is the thing splitting the signal was meant to prevent. Guarded on `and not auth_change.credentials_rotated` in both settings views, with the rationale extended rather than left half-true. The regression test drives the real view and asserts the view-level disconnect_all is not called for that save. Verified it catches the bug: with the guards reverted it fails with "Expected 'disconnect_all' to not have been called. Called 1 times." Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * fix(auth): write the initial operator once, not twice _create_initial_operator() called update_or_create() and then saved the same user again after hashing the password. The /ws revocation receiver is on post_save, so one operator action scheduled two disconnect_all() broadcasts — each paying the channel-layer timeout in full when Redis is down, and each sending healthy clients a redundant close. Build (or load) the row in memory, hash onto it, save once. The test pins the invariant by counting post_save: [True] now, [True, False] before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * fix(ws): stamp the auth generation before the session lookup AuthMiddlewareStack resolves scope['user'] and only then dispatches to the consumer, so a rotation committing inside that window was missed: the user came from a session that was valid when it was read, while the consumer — constructed afterwards — read the already-bumped counter, found it current, and kept the socket indefinitely. An ASGI middleware outside AuthMiddlewareStack records the generation on the way in; the consumer compares that instead. A handshake that straddles a rotation now carries the pre-rotation value and is refused. Also keeps the initial-operator upsert atomic: update_or_create() with a pre-hashed password is one write *and* one statement, where the read-then-save it briefly became would have raised IntegrityError for the loser of two racing first-time enables. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * fix(ws): serialize the auth-generation bump disconnect_all() can run on two threadpool workers at once (uvicorn runs sync views there), and `_auth_generation += 1` is load-add-store. A lost update would leave a socket stamped between the two changes matching the final generation — authorized after a revocation. Measured: no lost update in 480k racing increments at a 1 us switch interval, so the GIL hides this today and the test pins the invariant rather than demonstrating the race. The lock is for the build that doesn't have a GIL, and costs nothing uncontended. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * fix(ws): don't close a socket newer than the revocation A handshake completing between the generation bump and the force_disconnect publish joins the group already stamped with the new generation — it was accepted under the new credentials — and was then closed by the revocation it postdates, dropping the operator's reconnect for nothing. The event now carries the bump it belongs to and the process that made it; a socket stamped at or after that bump ignores it. Events from any other process still close unconditionally, since their counter says nothing about this one's, as does an event with neither field (an older build's). Also qualifies the developer docs: with authentication disabled a rotation closes sockets but a survivor keeps working, because _is_authorized() returns True before the generation check in that mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> --------- Co-authored-by: arm64-claude <307551610+vpetersson-bot@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> Co-authored-by: vpetersson-bot <vpetersson-bot@users.noreply.github.com>
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.
Issues Fixed
Follow-up from the review of PR 3308, accepted there as non-blocking.
/wswasunauthenticated —
AllowedHostsOriginValidatoronly — so anyone who could reachthe device's port could hold a socket open and watch it. That PR narrowed its own
payload to a
'*'sentinel, but the timing of each frame still leaked: ananonymous listener could tell when the screen rotated.
notify_asset_updatealsoemits real asset ids over the same socket on every write.
Description
/wsnow follows the same authentication switch as the rest of the app:django_project/asgi.pywraps the websocket router in Channels'AuthMiddlewareStack, so the handshake resolvesscope['user']from thesession cookie the browser already sends.
AllowedHostsOriginValidatorstaysoutermost — a cross-origin handshake is refused before we spend a session
lookup on it.
AssetConsumer.connect()refuses the handshake beforeaccept()and beforethe channel joins
WS_GROUP, so there is no frame left to time. Channelsanswers the upgrade with a 403.
settings['auth_backend'], mirroring@authorized: withauthentication disabled the endpoint stays open, exactly as every other surface
on the device does. Devices on the default config are unaffected.
Session cookie only, deliberately. Every page that opens
/wsextendsbase.htmland is itself behind@authorized, so an operator browser is theonly legitimate client; browsers can't set an
Authorizationheader on anew WebSocket()handshake anyway, so honouring the deprecated Basic path herewould only widen it to a surface that never had it.
Tests cover the gate directly and end-to-end through the real ASGI application
with a genuine session cookie — the latter is what pins that the middleware is
actually wired, not just that the consumer checks something.
Checklist
Hardware QA ran on the x86 testbed and a Pi 5, via the overlay method, 7/7 checks
passing on each: with auth off an anonymous socket opens and receives the
fan-out; with auth on an anonymous handshake is refused (403) while a real
login-form session opens the socket and still receives the fan-out; turning auth
back off reopens it. On the unpatched image the same probe confirmed the leak —
with auth on, an anonymous handshake was accepted (101) and received the
'*'frame. Both boards were restored to their pinned images and left with authoff and no leftover users or assets.
🤖 Generated with Claude Code