fix(security): realtime dropped the org header and put the API key in the URL - #23
fix(security): realtime dropped the org header and put the API key in the URL#23yakimoto wants to merge 2 commits into
Conversation
… the URL Two defects in `wave/realtime.py`, both flagged against sdk-python. 1. Multi-tenant isolation bypass. `WaveClient._build_headers()` stamps `X-Organization-Id` when `organization_id` is configured, and every other SDK module goes through it. `RealtimeAPI` took only `client.api_key` and built its own header dict that omitted the org, so `publish()`, `presence()`, `history()` and the WebSocket upgrade all ran unscoped — the one surface where a channel subscription is exactly the thing that should be tenant-bounded. 2. API key in the WebSocket URL query string. The inline comment justified it as "Browser/SDK clients can't set headers on the WS upgrade". True of browsers; not true here. This is a Python client using websocket-client, whose `create_connection(url, header=[...])` sets arbitrary upgrade headers. The justification was imported from a constraint that does not bind this code path, and a credential in a URL is recorded by every hop that logs a request line. The key now travels as `Authorization: Bearer` on the upgrade. `token_in_query=True` re-enables the legacy parameter for a deployment that cannot read the header — off by default, documented as insecure, and it does not disable the header when on. 3. Found while reading: `channel` and `as_` were interpolated raw into the query string, and `channel` raw into the REST path. A channel containing `&` injected a query parameter; one containing `/` left its path segment. Now urlencoded (`urlencode`, and `quote(safe=":")` for the path, keeping WAVE's `stream:abc` shape literal). Verified: 9 new tests in tests/test_realtime_auth.py, all passing — org header present on REST and on the upgrade header list, api_key absent from the connect URL by default, legacy param opt-in, and a channel named `stream:abc&as=victim` cannot inject `as`. Full suite: 17 passed, 2 failed, 1 skipped. Both failures are pre-existing on origin/main and unrelated — test_sdk_exports asserts 33 APIs (there are 36) and version 2.0.0 (it is 2.1.0). Proved by running that file against a clean `git archive` of origin/main: same 2 failures, same reasons. Not established: `/v1/connect` does not appear anywhere in wave-realtime-edge@main, so I could not confirm server-side acceptance of the header form from code. `src/landing.ts` documents the edge's auth as `Authorization: Bearer <key>`, which is why header-first is the default rather than a guess — but a live handshake against realtime.wave.online has not been run. Filing that separately.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
![devin-ai-integration[bot]](https://cdn.github.com/in/811515?s=60&v=4){kind=small}
| def __init__(self, client: WaveClient, url: str = _DEFAULT_WS, token_in_query: bool = False): | ||
| 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 |
There was a problem hiding this comment.
🟡 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 (token_in_query at wave/realtime.py:142) without adding an entry to the Unreleased section of CHANGELOG.md, so users get no notice of the behaviour change.
Impact: Users upgrading the SDK will not see that realtime authentication changed or that a new opt-in option exists.
Repository rule requiring changelog updates
AGENTS.md states: "Conventional Commit titles; update CHANGELOG.md (Unreleased) for user-facing changes." The Unreleased section in CHANGELOG.md:7 is empty and untouched by this PR, while the PR changes realtime auth transport (header instead of query token), adds X-Organization-Id to realtime REST/WS traffic, and percent-encodes channel path segments — all user-visible.
Prompt for agents
AGENTS.md requires updating CHANGELOG.md's Unreleased section for user-facing changes. This PR changes realtime authentication (API key now sent in the Authorization header on the WS upgrade instead of the URL query), adds X-Organization-Id propagation to realtime REST and WS traffic, adds a new token_in_query opt-in on RealtimeAPI, and percent-encodes channel names in REST paths. Add appropriate Fixed/Added/Changed entries under ## [Unreleased] in CHANGELOG.md.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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) |
There was a problem hiding this comment.
🔍 Server must accept the Authorization upgrade header for realtime
Moving the credential out of the URL relies on the realtime gateway accepting Authorization: Bearer ... on the WebSocket upgrade. If the deployed gateway only reads ?access_token=, every existing user's connect() will start failing after upgrade unless they explicitly pass token_in_query=True. Worth confirming server-side support (or gating the change behind a version) before release.
Was this helpful? React with 👍 or 👎 to provide feedback.
Repo isort config groups `wave.*` with the first block and third-party after (matching the existing wave/realtime.py). Applied `ruff check --fix`; 9 tests still pass, `ruff check .` clean.
![cursor[bot]](https://cdn.github.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: high. Left a non-blocking comment: Cursor Bugbot passed with no findings, but this PR changes realtime auth and multi-tenant scoping so it exceeds the medium approval threshold and needs human review. No eligible non-author reviewers were available to assign.
Sent by Cursor Approval Agent: Pull Request Router and Approver
| 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.
🔍 Channel encoding in REST paths changes the wire format the server sees
_channel_path percent-encodes everything except : (wave/realtime.py:38-44), so channels containing characters like /, #, or spaces now reach the server as %2F/%23 path 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.
ApprovabilityVerdict: Needs human review Unable to check for correctness in 2f91999. This security fix changes how credentials are transmitted for realtime connections (from URL query to Authorization header), which is a security-sensitive authentication change that could break existing integrations if the server doesn't support header-based auth. Open review comment raises valid concern about deployment coordination. You can customize Macroscope's approvability policy. Learn more. |
The "not established" note in the description is now resolvedI verified the server side. The realtime endpoint this module targets is not implemented — it returns a So the auth question is settled in the best possible way: there is no server behaviour this change can break. Header-first is safe to merge now, and when the service is implemented it should be built against the header from the start — a credential in a URL is recorded by every hop that logs a request line.
Related: the module has a broader problem — the default host it points at doesn't work, and |
1 participant
Two security findings against
wave/realtime.py, both confirmed againstorigin/mainbefore touching anything.1 — Multi-tenant isolation bypass
WaveClient._build_headers()stamps the tenant on every request:RealtimeAPIdid not go through it. It tookclient.api_keyand built its own header dict:So
publish(),presence(),history()and the WebSocket upgrade all ran unscoped, while all 35 other API modules carried the org. Realtime is the surface where this matters most — a channel subscription is exactly the thing that should be tenant-bounded.2 — API key in the WebSocket URL query string
The comment is true of browsers. It is not true of this code path: this is a Python client using
websocket-client, whosecreate_connection(url, header=[...])sets arbitrary headers on the upgrade. The justification was imported from a constraint that does not bind here, and a credential in a URL is recorded by every hop that logs a request line — proxies, edge access logs,Referer, shell history.The key now travels as
Authorization: Beareron the upgrade.token_in_query=Truere-enables the legacy parameter for a deployment that cannot read the header — off by default, and it does not disable the header when switched on.3 — Query-parameter and path injection (found while reading)
channelandas_were interpolated raw. A channel namedstream:abc&as=victiminjected anasparameter on the upgrade; one containing/escaped its REST path segment. Nowurlencodefor the query andquote(safe=":")for the path, keeping WAVE'sstream:abcshape literal.Verification
The 9 new tests assert the org header on REST and in the upgrade header list, that
api_keyis absent from the connect URL by default, that the legacy param is opt-in, and thatstream:abc&as=victimcannot injectas.Both suite failures are pre-existing and unrelated.
test_sdk_exportsasserts 33 APIs (there are 36) and version2.0.0(it is2.1.0). Proved rather than assumed — running that file against a cleangit archiveoforigin/maingives the same two failures:Those two stale assertions are worth a separate fix; they are not this PR's to smuggle.
What is NOT established
/v1/connectdoes not appear anywhere inwave-realtime-edge@main, so I could not confirm server-side acceptance of the header form from code I can read.src/landing.tsdocuments that edge's auth asAuthorization: Bearer <key>("this edge makes zero auth decisions"), which is why header-first is the default rather than a coin flip — but no live handshake againstrealtime.wave.onlinehas been run. If the gateway turns out to read onlyaccess_token, the escape hatch isRealtimeAPI(client, token_in_query=True)and the correct follow-up is to fix the server, not to re-widen the SDK.Note
High Risk
Changes authentication and multi-tenant isolation for realtime WebSocket and REST; misconfiguration or server mismatch could break connects or leave operations unscoped until verified against production.
Overview
Realtime now aligns with the rest of the SDK on tenant scoping and credential transport.
RealtimeAPIcopiesorganization_idfromWaveClientand sendsX-Organization-Idon REST (publish,presence,history) and on the WebSocket upgrade. WebSocket auth defaults toAuthorization: Bearervia upgrade headers instead of?access_token=in the connect URL;token_in_query=Truekeeps the legacy query param as an opt-in without dropping the header.Connect and REST URLs use
urlencodefor query params and_channel_path(quotewith:safe) so channel/as_values cannot inject query parameters or break path segments.Adds
tests/test_realtime_auth.pywith stubbedwebsocket-clientto lock in org headers, header-based auth, legacy query opt-in, and encoding behavior.Reviewed by Cursor Bugbot for commit 7f2dd10. Configure here.
Summary by cubic
Secures the realtime client by scoping all calls to the organization and moving the API key from the URL into the Authorization header. Also encodes channel and query values to block injection.
Bug Fixes
X-Organization-Idon realtime REST calls and the WebSocket upgrade.Authorization: Beareron the WS upgrade (no key in the URL).token_in_query=Truekeeps the legacy?access_token=if needed.channelandas_(query and REST path) to prevent parameter injection and path escaping.ruff(no behavior change).Migration
access_tokenin the URL, initializeRealtimeAPI(client, token_in_query=True)until the server supports headers.Written for commit 2f91999. Summary will update on new commits.
Note
Fix realtime authentication to send API key in Authorization header instead of URL
RealtimeChannelnow sends the API key as aBearertoken in theAuthorizationheader and includesX-Organization-Idon WebSocket upgrades; the API key is no longer placed in the connect URL by default.token_in_queryflag onRealtimeAPIandRealtimeChannelrestores the legacy behavior of appendingaccess_tokento the query string for clients that require it.publish,presence,history) and WebSocket URLs now use percent-encoded channel names via the new_channel_pathhelper, preventing path/query injection.token_in_query=True.Macroscope summarized 2f91999.