From 67bb0b6d5225e89f80bf32f31c3de5cb5750a1b6 Mon Sep 17 00:00:00 2001 From: brettchien Date: Fri, 14 Aug 2026 15:40:34 +0800 Subject: [PATCH] fix(remote): auth /acp dial via Authorization header, offer only acp.v1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remote reverse-MCP dial failed at the WS handshake with `SubProtocol error: Server sent an invalid subprotocol`, even though the gateway is healthy (verified live: a compliant client sending the exact `openab.bearer., acp.v1` offer connects and negotiates `acp.v1`; a wrong bearer gets HTTP 401). Root cause is client-side, in tokio-tungstenite 0.23. It parses the request's `Sec-WebSocket-Protocol` with `split(",")` and does NOT trim (handshake/client.rs:189), so our single combined offer `openab.bearer., acp.v1` is stored as [`openab.bearer.`, ` acp.v1`] — the second element keeps a leading space. When the server echoes `acp.v1`, the response check (client.rs:270-274) does `accepted_subprotocols.contains("acp.v1")`, which is false (` acp.v1` != `acp.v1`), so it errors with `InvalidSubProtocol`. Browser/Python clients maintain their own offer list and trim, so they were unaffected. Fix: Studio is a native (non-browser) client, so send the bearer in the `Authorization` header (the gateway reads `ws_bearer_token` from it) and offer only `acp.v1` as the sub-protocol — one clean token that matches the server's echo. This also stops the secret bearer from riding `Sec-WebSocket-Protocol`, which is prone to being logged by proxies (Cloudflare sits in the path). `bearer_subprotocol()` is retained, documented as the browser-only form, and guarded by a regression test. Verified live end-to-end against wss://orca-acp.brettchien.cc/acp (Authorization header + subprotocol acp.v1 connects). acp-tunnel unit tests pass; studio-desktop compilation is left to CI (separate GUI workspace). Co-Authored-By: Claude Opus 4.8 --- crates/acp-tunnel/src/lib.rs | 15 +++++++++++++++ src-tauri/src/remote.rs | 22 ++++++++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/crates/acp-tunnel/src/lib.rs b/crates/acp-tunnel/src/lib.rs index 52e89ba..139edb8 100644 --- a/crates/acp-tunnel/src/lib.rs +++ b/crates/acp-tunnel/src/lib.rs @@ -33,6 +33,13 @@ pub const ACP_SUBPROTOCOL: &str = "acp.v1"; /// `openab.bearer., acp.v1` (tunnel contract §1). The server echoes only /// `acp.v1`. The token is a secret — this returns it for the transport to place /// in the handshake header; never log the result. +/// +/// ⚠️ Browser transport only. A native tokio-tungstenite (0.23) client must **not** +/// feed this as a single `Sec-WebSocket-Protocol` value: that transport parses the +/// offer with `split(",")` without trimming, so the ` acp.v1` element keeps a +/// leading space and never matches the server's echoed `acp.v1`. Native clients +/// send the bearer in the `Authorization` header and offer only [`ACP_SUBPROTOCOL`] +/// (see `src-tauri/src/remote.rs`). pub fn bearer_subprotocol(token: &str) -> String { format!("openab.bearer.{token}, {ACP_SUBPROTOCOL}") } @@ -343,6 +350,14 @@ mod tests { ); } + #[test] + fn acp_subprotocol_is_a_single_clean_token() { + // Native transports offer this verbatim as one `Sec-WebSocket-Protocol` + // value; tokio-tungstenite 0.23 splits on "," without trimming, so any + // comma or whitespace here would break the match against the server's echo. + assert!(!ACP_SUBPROTOCOL.contains([',', ' '])); + } + #[test] fn server_decl_wire_form_is_type_acp() { let d = oab_server("conn-uuid"); diff --git a/src-tauri/src/remote.rs b/src-tauri/src/remote.rs index b520c0e..a59faab 100644 --- a/src-tauri/src/remote.rs +++ b/src-tauri/src/remote.rs @@ -135,10 +135,24 @@ async fn run_once( .as_str() .into_client_request() .map_err(|e| format!("bad url: {e}"))?; - // Bearer rides the WS sub-protocol offer (contract §1). Never logged. - let proto = HeaderValue::from_str(&acp::bearer_subprotocol(&cfg.token)) - .map_err(|_| "invalid token in sub-protocol header".to_string())?; - req.headers_mut().insert("Sec-WebSocket-Protocol", proto); + // Native (non-browser) client: the bearer rides the `Authorization` header + // (the gateway reads `ws_bearer_token` from it), and the WS sub-protocol + // offers only `acp.v1`. This split matters: tokio-tungstenite 0.23 parses the + // request's `Sec-WebSocket-Protocol` with `split(",")` and does *not* trim, so + // a combined `openab.bearer., acp.v1` offer is stored as + // [`openab.bearer.`, ` acp.v1`] (note the leading space) and never + // matches the server's echoed `acp.v1` — `Server sent an invalid subprotocol`. + // `bearer_subprotocol()` remains the browser form (browsers can't set + // `Authorization` on a WS upgrade). Both values carry secrets — never logged. + req.headers_mut().insert( + "Authorization", + HeaderValue::from_str(&format!("Bearer {}", cfg.token)) + .map_err(|_| "invalid token in Authorization header".to_string())?, + ); + req.headers_mut().insert( + "Sec-WebSocket-Protocol", + HeaderValue::from_static(acp::ACP_SUBPROTOCOL), + ); let (ws, _resp) = tokio_tungstenite::connect_async(req) .await