Repository navigation
fix(ws): take WebSocket identity from the session cookie - #607
Open
prince-shakyaa wants to merge 1 commit into
Open
prince-shakyaa wants to merge 1 commit into
prince-shakyaa wants to merge 1 commit into
Conversation
/ws/connect trusted namespace and user_id from the query string, and subscribe accepted any topic, so anyone could listen to another player's live events. The handshake now resolves the user from the session cookie (read-only, no rotation) and only allows subscribing to the user's own activity topic. Fixes GenAI-Security-Project#606
Author
|
This PR resolves #606 where Let me know if you need any changes. |
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.
Fixes #606
What was wrong
/ws/connectused whatevernamespaceanduser_idthe client put in the URL, andsubscribeaccepted any topic. Anyone with another player's user_id could open a socket as them and receive their activity feed and challenge-completion events, including the snippet of the prompt they used to solve the challenge.What changed
finbot/core/websocket/routes.py: the handshake reads thefinbot_sessioncookie and getsnamespace/user_idfrom the session. The query params are gone. If there's no valid session, the socket is closed before it's accepted (1008), and clients see that as an HTTP 403 on the handshake.subscribeonly accepts the connection's ownactivity:{ns}:{user_id}topic and sends an error event for anything else.finbot/core/auth/session.py: addedSessionManager.get_session_identity(). It checks that the session exists, isn't expired or too old, and has a valid signature, then returns(namespace, user_id).get_session()because it has side effects. If the session is due for rotation, it creates a new session ID and expires the old one in 60s, but a WebSocket handshake has no response to send the new cookie on, so the player would get logged out a minute later. It can also delete a temporary session on a fingerprint mismatch.finbot/static/js/vendor/ctf_sidecar.js: connects to/ws/connectwith no params and drops the explicit subscribe, since the server already auto-subscribes the connection to its own topic.window.CTF_USER_ID, so before this they connected asanonymous.Tests
New file
tests/unit/auth/test_websocket_auth.py(9 tests):get_session_identity()doesn't rotate or modify a session that is due for rotationget_session_identity()returns None for a missing or empty cookieAgainst the old route, 7 of the 9 fail (the other 2 only test the new helper). All 9 pass with the fix.
The rest of the suite:
tests/unit/agents/test_specialized_agents.pyand fail the same way onmainwithout this change.Manual testing
I ran the app with uvicorn and Redis and used a real WebSocket client (the
websocketslibrary). The cookie came from loading/ctf/, just like in a browser.