-
Notifications
You must be signed in to change notification settings - Fork 0
fix(security): realtime dropped the org header and put the API key in the URL #23
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
base: main
Are you sure you want to change the base?
Changes from all commits
7f2dd10
2f91999
f1136a0
5176381
a73e522
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 |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| """Realtime auth-transport tests. | ||
|
|
||
| Covers two defects that were live on main: | ||
|
|
||
| 1. ``RealtimeAPI`` built its own header dict and omitted ``X-Organization-Id``, so every realtime | ||
| operation ran unscoped while the rest of the SDK carried the tenant. | ||
| 2. The API key travelled in the WebSocket URL query string, where every hop that logs a request | ||
| line records it. | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import sys | ||
| import types | ||
|
|
||
| import pytest | ||
|
|
||
| from wave_sdk.client import WaveClient | ||
| from wave_sdk.realtime import RealtimeAPI | ||
|
|
||
|
|
||
| class _FakeSocket: | ||
| def __init__(self, url: str, header: list[str] | None = None, **_: object) -> None: | ||
| self.url = url | ||
| self.header = header or [] | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def captured_ws(monkeypatch): | ||
| """Stub the optional ``websocket-client`` dep and capture the upgrade it would perform.""" | ||
| calls: list[_FakeSocket] = [] | ||
|
|
||
| def create_connection(url: str, header: list[str] | None = None, **kwargs: object) -> _FakeSocket: | ||
| sock = _FakeSocket(url, header, **kwargs) | ||
| calls.append(sock) | ||
| return sock | ||
|
|
||
| module = types.ModuleType("websocket") | ||
| module.create_connection = create_connection # type: ignore[attr-defined] | ||
| monkeypatch.setitem(sys.modules, "websocket", module) | ||
| return calls | ||
|
|
||
|
|
||
| def _api(**client_kwargs) -> RealtimeAPI: | ||
| client = WaveClient(api_key="sk-test-key", **client_kwargs) | ||
| return RealtimeAPI(client) | ||
|
|
||
|
|
||
| # --- Finding 1: multi-tenant isolation ------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_rest_headers_carry_the_organization(): | ||
| headers = _api(organization_id="org_123")._headers() | ||
| assert headers["X-Organization-Id"] == "org_123" | ||
| assert headers["Authorization"] == "Bearer sk-test-key" | ||
|
|
||
|
|
||
| def test_rest_headers_omit_the_organization_when_unset(): | ||
| assert "X-Organization-Id" not in _api()._headers() | ||
|
|
||
|
|
||
| def test_ws_upgrade_carries_the_organization(captured_ws): | ||
| _api(organization_id="org_123").connect("stream:abc") | ||
| assert "X-Organization-Id: org_123" in captured_ws[0].header | ||
|
|
||
|
|
||
| def test_rest_path_encodes_the_channel(): | ||
| from wave_sdk.realtime import _channel_path | ||
|
|
||
| assert _channel_path("stream:abc") == "stream:abc" | ||
| assert _channel_path("a/../b") == "a%2F..%2Fb" | ||
|
|
||
|
|
||
| # --- Finding 2: credential in the URL -------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_api_key_is_not_in_the_connect_url(captured_ws): | ||
| _api(organization_id="org_123").connect("stream:abc") | ||
| assert "sk-test-key" not in captured_ws[0].url | ||
| assert "access_token" not in captured_ws[0].url | ||
|
|
||
|
|
||
| def test_api_key_travels_in_the_upgrade_header(captured_ws): | ||
| _api().connect("stream:abc") | ||
| assert "Authorization: Bearer sk-test-key" in captured_ws[0].header | ||
|
|
||
|
|
||
| def test_legacy_query_token_is_opt_in(captured_ws): | ||
| client = WaveClient(api_key="sk-test-key") | ||
| RealtimeAPI(client, token_in_query=True).connect("stream:abc") | ||
| assert "access_token=sk-test-key" in captured_ws[0].url | ||
| # The header is still sent — opting into the legacy param does not disable the correct path. | ||
| assert "Authorization: Bearer sk-test-key" in captured_ws[0].header | ||
|
|
||
|
|
||
| # --- Finding 3: query-parameter injection ---------------------------------------------------- | ||
|
|
||
|
|
||
| def test_channel_cannot_inject_a_query_parameter(captured_ws): | ||
| _api().connect("stream:abc&as=victim") | ||
| url = captured_ws[0].url | ||
| assert "&as=victim" not in url | ||
| assert "as%3Dvictim" in url | ||
|
|
||
|
|
||
| def test_as_parameter_is_encoded(captured_ws): | ||
| _api().connect("stream:abc", as_="user&admin=1") | ||
| url = captured_ws[0].url | ||
| assert "&admin=1" not in url |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,19 @@ | |
| WebSocket support uses the optional ``websocket-client`` package: ``pip install 'wave-sdk[realtime]'``. | ||
| Auth, scope, entitlement, and metering are enforced server-side (the gateway, via realtime's /v1/verify | ||
| federation) — the SDK only forwards your API key. | ||
|
|
||
| Credentials travel in the ``Authorization`` header on both the REST calls and the WebSocket upgrade. | ||
| ``websocket-client`` sets arbitrary upgrade headers, so the browser constraint that forces a | ||
| ``?access_token=`` query parameter does not apply to this client. See ``token_in_query`` on | ||
| :class:`RealtimeAPI` for the legacy escape hatch. | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import contextlib | ||
| import json | ||
| from collections.abc import Iterator | ||
| from typing import Any, Callable | ||
| from urllib.parse import quote, urlencode | ||
|
|
||
| import httpx | ||
|
|
||
|
|
@@ -30,6 +36,15 @@ def _http_origin(ws_url: str) -> str: | |
| return base | ||
|
|
||
|
|
||
| def _channel_path(channel: str) -> str: | ||
| """Percent-encode a channel for use as a single REST path segment. | ||
|
|
||
| ``:`` stays literal because WAVE channel names are ``stream:abc`` shaped; everything else that | ||
| could leave the segment (``/``, ``?``, ``#``, ``&``) is encoded. | ||
| """ | ||
| return quote(channel, safe=":") | ||
|
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. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: sed -n '1,90p' wave_sdk/realtime.py
sed -n '55,80p' tests/test_realtime_auth.pyRepository: wave-av/sdk-python Length of output: 4348 Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') Exploitability: Difficult Reject dot-only channel names.
🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| class RealtimeChannel: | ||
| """One subscribed channel over a WebSocket. | ||
|
|
||
|
|
@@ -40,19 +55,40 @@ class RealtimeChannel: | |
| ch.run() # blocks, dispatching frames | ||
| """ | ||
|
|
||
| def __init__(self, channel: str, api_key: str, ws_base: str = _DEFAULT_WS, as_: str | None = None): | ||
| def __init__( | ||
| self, | ||
| channel: str, | ||
| api_key: str, | ||
| ws_base: str = _DEFAULT_WS, | ||
| as_: str | None = None, | ||
| organization_id: str | None = None, | ||
| token_in_query: bool = False, | ||
| ): | ||
| try: | ||
| import websocket # websocket-client (optional dep) | ||
| except ImportError as e: # pragma: no cover - import guard | ||
| raise ImportError( | ||
| "WAVE realtime requires the 'websocket-client' package: pip install 'wave-sdk[realtime]'" | ||
| ) from e | ||
| self.channel = channel | ||
| # Browser/SDK clients can't set headers on the WS upgrade → key travels as a query param (wss). | ||
| url = f"{ws_base.rstrip('/')}/v1/connect?channel={channel}&access_token={api_key}" | ||
|
|
||
| # Every value is urlencoded: a channel containing '&' or '#' would otherwise inject or | ||
| # truncate query parameters on the upgrade. | ||
| params: dict[str, str] = {"channel": channel} | ||
| if as_: | ||
| url += f"&as={as_}" | ||
| self._ws = websocket.create_connection(url) | ||
| params["as"] = as_ | ||
|
|
||
| headers = [f"Authorization: Bearer {api_key}"] | ||
| if organization_id: | ||
| headers.append(f"X-Organization-Id: {organization_id}") | ||
|
|
||
| if token_in_query: | ||
| # Legacy form for deployments that cannot read the upgrade header. The key lands in | ||
| # proxy logs, edge access logs, and shell history — opt in deliberately or not at all. | ||
| params["access_token"] = api_key | ||
|
|
||
| url = f"{ws_base.rstrip('/')}/v1/connect?{urlencode(params)}" | ||
| self._ws = websocket.create_connection(url, header=headers) | ||
|
Comment on lines
+81
to
+91
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. 🔍 Server must accept the Authorization upgrade header for realtime Moving the credential out of the URL relies on the realtime gateway accepting Was this helpful? React with 👍 or 👎 to provide feedback. 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. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: In the websocket-client library (including v1.0.0 and later versions), the connection process, including handling redirects and custom headers, is managed within the websocket._core module, specifically through the connect method [1][2][3]. When you use create_connection or WebSocket.connect, you can pass a header argument (as a dictionary or list) to include custom HTTP headers in the initial handshake request [4][5]. Regarding redirects: 1. The library handles redirects automatically during the handshake process [1][2][3]. 2. The core logic iterates through a redirect limit (defaulting to 3, configurable via the redirect_limit option) [1][2][3]. 3. If the server returns a supported redirect status code, the library closes the current socket, connects to the new URL provided in the location header, and performs a new handshake [1][2][3]. Important Note on Custom Headers and Redirects: In many versions of the library, the custom options (including the header dictionary/list) passed to the initial connect call are passed into the handshake function for each subsequent connection attempt during a redirect [1][2][3]. However, developers should be aware that custom headers intended for the original target might be sent to the redirected target as well, as they are persisted in the options object throughout the redirect loop [1][2][3]. If you encounter issues where specific headers should not be sent to the redirected host, you may need to implement manual redirect handling by setting redirect_limit=0 and performing the connection logic manually in your application code [1][2][3]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- dependency declarations ---'
rg -n -i 'websocket-client|websocket' pyproject.toml setup.py setup.cfg requirements\*.txt requirements.txt poetry.lock uv.lock 2>/dev/null || true
printf '%s\n' '--- realtime implementation ---'
sed -n '1,115p' wave_sdk/realtime.py
printf '%s\n' '--- repository guidance ---'Repository: wave-av/sdk-python Length of output: 4728 🌐 Web query:
💡 Result: In
Citations: 🤖 get_repo_knowledge executed:
Length of output: 550 🏁 Script executed: #!/bin/bash
set -eu
sed -n '55,82p' pyproject.toml
rg -n 'redirect_limit|create_connection\(' wave_sdk tests pyproject.tomlRepository: wave-av/sdk-python Length of output: 1222 Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor Reachability: External · Exploitability: Moderate Do not forward bearer credentials across redirects.
🤖 Prompt for AI Agents |
||
| self._handlers: dict[str, list[Callable[[Any], None]]] = {} | ||
|
|
||
| def __iter__(self) -> Iterator[dict]: | ||
|
|
@@ -97,34 +133,56 @@ def close(self) -> None: | |
|
|
||
| class RealtimeAPI: | ||
| """Realtime entry point. ``wave.realtime.connect('stream:abc')`` for WS; ``publish/presence/history`` | ||
| are one-shot REST calls for producers that don't hold a socket.""" | ||
| are one-shot REST calls for producers that don't hold a socket. | ||
|
|
||
| ``token_in_query`` re-enables the legacy ``?access_token=`` upgrade parameter for a deployment | ||
| that cannot read the ``Authorization`` header. It is off by default because a credential in a URL | ||
| is recorded by every hop that logs the request line. | ||
| """ | ||
|
|
||
| def __init__(self, client: WaveClient, url: str = _DEFAULT_WS): | ||
| def __init__(self, client: WaveClient, url: str = _DEFAULT_WS, token_in_query: bool = False): | ||
|
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. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: sed -n '1,220p' wave_sdk/realtime.py
printf '\n--- dependency declarations ---\n'
rg -n -C 2 'websocket-client|httpx|RealtimeAPI|_DEFAULT_WS|_http_origin|create_connection' pyproject.toml setup.py setup.cfg requirements*.txt wave_sdk tests 2>/dev/nullRepository: wave-av/sdk-python Length of output: 21661 Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information Exploitability: Moderate Reject non- When Proposed fix-from urllib.parse import quote, urlencode
+from urllib.parse import quote, urlencode, urlsplit
def __init__(self, client: WaveClient, url: str = _DEFAULT_WS, token_in_query: bool = False):
+ parsed_url = urlsplit(url)
+ if parsed_url.scheme != "wss" or not parsed_url.hostname:
+ raise ValueError("Realtime URL must use wss://")
self._api_key = client.api_key🤖 Prompt for AI Agents |
||
| self._api_key = client.api_key | ||
| # Multi-tenant isolation: WaveClient stamps X-Organization-Id on every other surface, so | ||
| # realtime carries it too — on the REST calls and on the WS upgrade. | ||
| self._organization_id = client.organization_id | ||
| self._ws_base = url.rstrip("/") | ||
| self._http_base = _http_origin(self._ws_base) | ||
| self._token_in_query = token_in_query | ||
|
Comment on lines
+143
to
+150
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. 🟡 Release notes not updated for a user-facing behaviour change The change alters how credentials and tenant scoping are sent for realtime connections and adds a new opt-in setting ( Repository rule requiring changelog updates
Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| def connect(self, channel: str, as_: str | None = None) -> RealtimeChannel: | ||
| return RealtimeChannel(channel, self._api_key, self._ws_base, as_) | ||
| return RealtimeChannel( | ||
| channel, | ||
| self._api_key, | ||
| self._ws_base, | ||
| as_, | ||
| organization_id=self._organization_id, | ||
| token_in_query=self._token_in_query, | ||
| ) | ||
|
|
||
| def _headers(self) -> dict[str, str]: | ||
| return {"Authorization": f"Bearer {self._api_key}", "content-type": "application/json"} | ||
| headers = {"Authorization": f"Bearer {self._api_key}", "content-type": "application/json"} | ||
| if self._organization_id: | ||
| headers["X-Organization-Id"] = self._organization_id | ||
| return headers | ||
|
|
||
| def publish(self, channel: str, event: str, data: Any = None) -> dict: | ||
| r = httpx.post( | ||
| f"{self._http_base}/v1/channels/{channel}/publish", | ||
| f"{self._http_base}/v1/channels/{_channel_path(channel)}/publish", | ||
| headers=self._headers(), | ||
| json={"event": event, "data": data}, | ||
| ) | ||
|
Comment on lines
169
to
173
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: These calls bypass Assessment: 🟠 Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** wave_sdk/realtime.py
**Line:** 169:173
**Comment:**
*Api Mismatch: These calls bypass `WaveClient._request`, so configured timeouts, retries, rate-limit handling, and `WaveError` conversion do not apply to realtime REST operations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| return r.json() | ||
|
|
||
| def presence(self, channel: str) -> dict: | ||
| r = httpx.get(f"{self._http_base}/v1/channels/{channel}/presence", headers=self._headers()) | ||
| r = httpx.get( | ||
| f"{self._http_base}/v1/channels/{_channel_path(channel)}/presence", | ||
| headers=self._headers(), | ||
| ) | ||
| return r.json() | ||
|
|
||
| def history(self, channel: str, limit: int = 50) -> dict: | ||
| r = httpx.get( | ||
| f"{self._http_base}/v1/channels/{channel}/history", | ||
| f"{self._http_base}/v1/channels/{_channel_path(channel)}/history", | ||
| headers=self._headers(), | ||
| params={"limit": limit}, | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔍 Channel encoding in REST paths changes the wire format the server sees
_channel_pathpercent-encodes everything except:(wave/realtime.py:38-44), so channels containing characters like/,#, or spaces now reach the server as%2F/%23path segments where they previously produced multiple path segments or truncated URLs. Worth confirming the realtime service decodes the path segment before matching channel names, otherwise previously-working channels with unusual characters would start resolving differently.Was this helpful? React with 👍 or 👎 to provide feedback.