FIX: Stream Auth through protocol bug fixed#1463
Conversation
|
Someone is attempting to deploy a commit to the motia Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe WebSocket handler now inspects all Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/workers/stream/stream.rs`:
- Around line 77-81: The code that sets has_authorization_protocol currently
uses headers.get(...) which only returns the first Sec-WebSocket-Protocol header
instance; change the logic to iterate over
headers.get_all("sec-websocket-protocol"), for each header value try
to_str().ok(), split by ',' and trim each token, and set
has_authorization_protocol = true if any token equals "Authorization"
(short-circuiting once found). Update the usage around the
has_authorization_protocol variable to rely on this aggregated check so repeated
header instances or comma-separated lists are both handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75443384-1c52-4a49-958a-228ef7a00a3f
📒 Files selected for processing (1)
engine/src/workers/stream/stream.rs
…d not terminate the stream automatically (by return the Authorization protocol)
Atlas1225
force-pushed
the
slu/streamAuth
branch
from
3177d0c to
47fc53c
Compare
|
@ytallo So I implemented in my frontend app a Stream authentication with oauth following the documentation: https://www.motia.dev/docs/development-guide/react-stream-client#motiastreamprovider but it seems that it is not working properly. The problem is that motia is not answering with the same header so the websocket is terminated automatically on browser side. So my fix is about this issue. |
|
@ytallo Also I want to ask if I use previous versions (before motia 1.0.0) it should be more stable. I am doing some projects that needs to pass from SBX to production in 3 months aprox and I want to know if I can rely on this new framework. |
|
@rohitg00 Hi, maybe you can help me clear my doubts as well :D |
|
Thanks for submitting! We have been heads down but @ytallo will take a look soon. This PR touches engine code. For us to accept it, we need your explicit confirmation in this thread that you license your changes to the engine portion of the codebase under Apache 2.0. Please reply with: |
|
Hey @Atlas1225 add at least one integration test? |
|
I agree that my contributions in this PR to the engine code are licensed under Apache 2.0.
|
Okay, Sorry for that |
|
@Atlas1225 Please edit your PR message (the one you wrote to open this PR) to add this string at the bottom:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine/src/workers/stream/stream.rs (1)
2042-2054: Nit: thedGhlIHNhbXBsZSBub25jZQ==value will likely keep tripping secret scanners.This is the RFC 6455 sample nonce ("the sample nonce") and is not a real secret, but the
Betterleaksgeneric-api-key check already flagged it. If it becomes noisy, consider either adding a baseline/allow entry in the scanner config or generating a random 16-byte base64 nonce per call to avoid the pattern match. No functional concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/workers/stream/stream.rs` around lines 2042 - 2054, The hard-coded RFC6455 sample nonce "dGhlIHNhbXBsZSBub25jZQ==" in ws_upgrade_request is triggering secret scanners; replace it with a per-call random 16-byte nonce encoded in base64 instead of the literal string. Update the ws_upgrade_request function to generate 16 random bytes (e.g., via rand::Rng or getrandom), base64-encode them (e.g., base64::engine::general_purpose::STANDARD.encode) and use that encoded string for the "sec-websocket-key" header so each call produces a non-static value and avoids scanner matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@engine/src/workers/stream/stream.rs`:
- Around line 2042-2054: The hard-coded RFC6455 sample nonce
"dGhlIHNhbXBsZSBub25jZQ==" in ws_upgrade_request is triggering secret scanners;
replace it with a per-call random 16-byte nonce encoded in base64 instead of the
literal string. Update the ws_upgrade_request function to generate 16 random
bytes (e.g., via rand::Rng or getrandom), base64-encode them (e.g.,
base64::engine::general_purpose::STANDARD.encode) and use that encoded string
for the "sec-websocket-key" header so each call produces a non-static value and
avoids scanner matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ff1928ce-d17e-41b1-88f9-6fd428f2b271
📒 Files selected for processing (1)
engine/src/workers/stream/stream.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/workers/stream/stream.rs`:
- Around line 2132-2140: The test
test_ws_authorization_protocol_in_second_header_connection_succeeds currently
only asserts the 101 status but should also assert that the server echoes the
"Authorization" protocol; update the test (which uses raw_ws_handshake and the
ws.protocols selection behavior) to capture the response headers/body from
raw_ws_handshake and add an assertion verifying that the Sec-WebSocket-Protocol
in the 101 response equals "Authorization" (matching the comma-separated variant
and the ws.protocols(["Authorization"]) behavior).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 067e15f9-8015-4434-8aaf-1f9d09954b78
📒 Files selected for processing (1)
engine/src/workers/stream/stream.rs
| // axum reads only the first Sec-WebSocket-Protocol header for protocol selection, so the | ||
| // 101 response won't echo Authorization when it's in the second header — but the connection | ||
| // is established, which is what matters (the browser won't close the socket). | ||
| #[tokio::test] | ||
| async fn test_ws_authorization_protocol_in_second_header_connection_succeeds() { | ||
| let port = start_ws_test_server().await; | ||
| let (status, _) = raw_ws_handshake(port, &["graphql-ws", "Authorization"]).await; | ||
| assert_eq!(status, 101); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd engine && find . -name "stream.rs" -type f | head -5Repository: iii-hq/iii
Length of output: 84
🏁 Script executed:
# Get line count to understand file size
wc -l engine/src/workers/stream/stream.rsRepository: iii-hq/iii
Length of output: 94
🏁 Script executed:
# Read the test function and surrounding context (lines 2120-2160)
sed -n '2120,2160p' engine/src/workers/stream/stream.rsRepository: iii-hq/iii
Length of output: 1025
🏁 Script executed:
# Read the detection logic at lines 77-82
sed -n '77,82p' engine/src/workers/stream/stream.rsRepository: iii-hq/iii
Length of output: 281
🏁 Script executed:
# Find raw_ws_handshake function definition
rg -n "fn raw_ws_handshake" engine/src/workers/stream/stream.rs -A 15Repository: iii-hq/iii
Length of output: 764
🏁 Script executed:
# Check what raw_ws_handshake returns
rg -n "raw_ws_handshake" engine/src/workers/stream/stream.rs | head -20Repository: iii-hq/iii
Length of output: 575
🏁 Script executed:
# Read more context around the detection logic - what function/scope is it in?
sed -n '70,95p' engine/src/workers/stream/stream.rsRepository: iii-hq/iii
Length of output: 864
🏁 Script executed:
# Read the full raw_ws_handshake function to understand protocol extraction
sed -n '2052,2090p' engine/src/workers/stream/stream.rsRepository: iii-hq/iii
Length of output: 1502
🏁 Script executed:
# Search for ws.protocols calls to understand how protocols are set
rg -n "ws\.protocols" engine/src/workers/stream/stream.rs -B 2 -A 2Repository: iii-hq/iii
Length of output: 178
🏁 Script executed:
# Read the context around where ws.protocols is called
sed -n '80,110p' engine/src/workers/stream/stream.rsRepository: iii-hq/iii
Length of output: 1325
Add assertion to verify Authorization protocol is echoed in the 101 response.
The detection logic at lines 77–82 correctly finds Authorization in the second header via get_all() and flat_map(), which triggers ws.protocols(["Authorization"]) at line 85. This means the server will echo the protocol in the 101 response, matching the behavior of the comma-separated test (test_ws_authorization_protocol_comma_separated_last_is_echoed).
The inline comment suggesting the echo won't happen is misleading — the code already handles this case. The test should assert the echo to align with the comma-separated variant and lock in the correct behavior:
Suggested assertion
- let (status, _) = raw_ws_handshake(port, &["graphql-ws", "Authorization"]).await;
+ let (status, proto) = raw_ws_handshake(port, &["graphql-ws", "Authorization"]).await;
assert_eq!(status, 101);
+ assert_eq!(proto.as_deref(), Some("Authorization"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/workers/stream/stream.rs` around lines 2132 - 2140, The test
test_ws_authorization_protocol_in_second_header_connection_succeeds currently
only asserts the 101 status but should also assert that the server echoes the
"Authorization" protocol; update the test (which uses raw_ws_handshake and the
ws.protocols selection behavior) to capture the response headers/body from
raw_ws_handshake and add an assertion verifying that the Sec-WebSocket-Protocol
in the 101 response equals "Authorization" (matching the comma-separated variant
and the ws.protocols(["Authorization"]) behavior).
|
nit on the multi-header detection at the top is correct — per RFC 6455 §4.2.2 and the HTML living spec WebSocket-handshake step 4, if the client requested a subprotocol and the server's response doesn't echo one of the requested values, the browser MUST the current test bakes this gap in (assert-only-status; no protocol assertion), so split-header browser clients would still hit the original "ws terminated automatically" symptom this PR set out to fix. two ways to close it:
happy to send a follow-up PR with (1) and a strengthened 5th test ( |
5 participants
Per RFC 6455, if a WebSocket client sends a Sec-WebSocket-Protocol header during the handshake, the server must include a matching Sec-WebSocket-Protocol in its response — or the browser will immediately close the connection with code 1006.
The iii-stream worker was not echoing back the requested subprotocol, which made it impossible for browser clients to use the Sec-WebSocket-Protocol header as an authentication transport. The connection was silently dropped on every attempt.
FIX:
Based on your documentation the motia client in front should pass the token in websocket with this format
HEADER: Sec-WebSocket-Protocol: Authorization, <jwt>.
The server must echo: HEADER Sec-WebSocket-Protocol: Authorization. Without that, the ws is terminated automatically.
The server extract the token from the header value in the authenticateStream function for verification.
Summary by CodeRabbit
Bug Fixes
Tests