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
5 tasks
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