-
Notifications
You must be signed in to change notification settings - Fork 180
agent-server: allow WebSocket auth via headers #1786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d39c661
d6e2f05
78174be
3af6c71
c399dab
2be30ab
c3eeea1
6e4f7b1
52cb119
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,9 @@ | |
| WebSocket endpoints for OpenHands SDK. | ||
|
|
||
| These endpoints are separate from the main API routes to handle WebSocket-specific | ||
| authentication using query parameters instead of headers, since browsers cannot | ||
| send custom HTTP headers directly with WebSocket connections. | ||
| authentication. Browsers cannot send custom HTTP headers directly with WebSocket | ||
| connections, so we support the `session_api_key` query param. For non-browser | ||
| clients (e.g. Python/Node), we also support authenticating via headers. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π‘ Suggestion: Consider mentioning in the docstring or in documentation that header-based authentication is the recommended approach for security (avoids leaking secrets in URLs), while query param auth is maintained for backward compatibility with browser clients. This could help guide users toward the more secure option. |
||
| """ | ||
|
|
||
| import logging | ||
|
|
@@ -35,6 +36,42 @@ | |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _resolve_websocket_session_api_key( | ||
| websocket: WebSocket, | ||
| session_api_key: str | None, | ||
| ) -> str | None: | ||
enyst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| """Resolve the session API key from multiple sources. | ||
|
|
||
| Precedence order (highest to lowest): | ||
| 1. Query parameter (session_api_key) - for browser compatibility | ||
| 2. X-Session-API-Key header - for non-browser clients | ||
|
|
||
| Returns None if no valid key is found in any source. | ||
| """ | ||
| if session_api_key: | ||
enyst marked this conversation as resolved.
Show resolved
Hide resolved
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π’ Nit: Using truthiness check here means an empty string query param ( |
||
| return session_api_key | ||
|
|
||
| header_key = websocket.headers.get("x-session-api-key") | ||
enyst marked this conversation as resolved.
Show resolved
Hide resolved
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π΄ Critical: The PR description claims to support both Either:
If adding Bearer support, you could add after this line: # Support Authorization: Bearer <token> format
auth_header = websocket.headers.get("authorization")
if auth_header and auth_header.startswith("Bearer "):
return auth_header[7:] # Strip "Bearer " prefix |
||
| if header_key: | ||
| return header_key | ||
|
|
||
| return None | ||
enyst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| async def _accept_authenticated_websocket( | ||
| websocket: WebSocket, | ||
| session_api_key: str | None, | ||
| ) -> bool: | ||
| """Authenticate and accept the socket, or close with an auth error.""" | ||
| config = get_default_config() | ||
| resolved_key = _resolve_websocket_session_api_key(websocket, session_api_key) | ||
| if config.session_api_keys and resolved_key not in config.session_api_keys: | ||
| await websocket.close(code=4001, reason="Authentication failed") | ||
enyst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return False | ||
| await websocket.accept() | ||
| return True | ||
|
|
||
|
|
||
| @sockets_router.websocket("/events/{conversation_id}") | ||
| async def events_socket( | ||
| conversation_id: UUID, | ||
|
|
@@ -43,14 +80,9 @@ async def events_socket( | |
| resend_all: Annotated[bool, Query()] = False, | ||
| ): | ||
| """WebSocket endpoint for conversation events.""" | ||
| # Perform authentication check before accepting the WebSocket connection | ||
| config = get_default_config() | ||
| if config.session_api_keys and session_api_key not in config.session_api_keys: | ||
| # Close the WebSocket connection with an authentication error code | ||
| await websocket.close(code=4001, reason="Authentication failed") | ||
| if not await _accept_authenticated_websocket(websocket, session_api_key): | ||
| return | ||
|
|
||
| await websocket.accept() | ||
| logger.info(f"Event Websocket Connected: {conversation_id}") | ||
| event_service = await conversation_service.get_event_service(conversation_id) | ||
| if event_service is None: | ||
|
|
@@ -97,14 +129,9 @@ async def bash_events_socket( | |
| resend_all: Annotated[bool, Query()] = False, | ||
| ): | ||
| """WebSocket endpoint for bash events.""" | ||
| # Perform authentication check before accepting the WebSocket connection | ||
| config = get_default_config() | ||
| if config.session_api_keys and session_api_key not in config.session_api_keys: | ||
| # Close the WebSocket connection with an authentication error code | ||
| await websocket.close(code=4001, reason="Authentication failed") | ||
| if not await _accept_authenticated_websocket(websocket, session_api_key): | ||
| return | ||
|
|
||
| await websocket.accept() | ||
| logger.info("Bash Websocket Connected") | ||
| subscriber_id = await bash_event_service.subscribe_to_events( | ||
| _BashWebSocketSubscriber(websocket) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,3 +104,46 @@ async def test_agent_server_websocket_with_wsproto(agent_server): | |
| await ws.send( | ||
| json.dumps({"role": "user", "content": "Hello from wsproto test"}) | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_agent_server_websocket_with_wsproto_header_auth(agent_server): | ||
| port = agent_server["port"] | ||
| api_key = agent_server["api_key"] | ||
|
|
||
| response = requests.post( | ||
| f"http://127.0.0.1:{port}/api/conversations", | ||
| headers={"X-Session-API-Key": api_key}, | ||
| json={ | ||
| "agent": { | ||
| "llm": { | ||
| "usage_id": "test-llm", | ||
| "model": "test-provider/test-model", | ||
| "api_key": "test-key", | ||
| }, | ||
| "tools": [], | ||
| }, | ||
| "workspace": {"working_dir": "/tmp/test-workspace"}, | ||
| }, | ||
| ) | ||
| assert response.status_code in [200, 201] | ||
| conversation_id = response.json()["id"] | ||
|
|
||
| ws_url = f"ws://127.0.0.1:{port}/sockets/events/{conversation_id}?resend_all=true" | ||
|
|
||
| async with websockets.connect( | ||
| ws_url, | ||
| open_timeout=5, | ||
| additional_headers={"X-Session-API-Key": api_key}, | ||
| ) as ws: | ||
| try: | ||
| response = await asyncio.wait_for(ws.recv(), timeout=2) | ||
| assert response is not None | ||
| except TimeoutError: | ||
| pass | ||
|
|
||
| await ws.send( | ||
| json.dumps( | ||
| {"role": "user", "content": "Hello from wsproto header auth test"} | ||
| ) | ||
| ) | ||
enyst marked this conversation as resolved.
Show resolved
Hide resolved
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π‘ Suggestion: Consider adding negative test cases for header auth in the wsproto tests (e.g., connection fails without auth, connection fails with wrong header). While |
||
Uh oh!
There was an error while loading. Please reload this page.