[oh-tab-h3g] Security: drop session_api_key from RemoteConversation WS URL#873
[oh-tab-h3g] Security: drop session_api_key from RemoteConversation WS URL#873
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR removes legacy WebSocket query-parameter authentication (session_api_key) and enforces header-based WebSocket authentication (X-Session-API-Key or Bearer). Tests and docs were updated to validate header-only WS auth and to stop retrying with session_api_key on 403 errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as RemoteConversation
participant WS as WebSocket
participant Server as Agent Server
rect rgba(200,200,255,0.5)
Client->>WS: Open WS connection (no session_api_key in URL)\nInclude header: X-Session-API-Key or Authorization: Bearer
WS->>Server: WS handshake with headers
Server-->>WS: 101 / 403 (handshake)
note right of WS: On 403 -> propagate error to Client\n(no retry with session_api_key-in-URL)
WS-->>Client: handshake result / error event
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello @enyst, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a critical security improvement by refactoring how the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses a security vulnerability by removing the session_api_key from the WebSocket URL's query string and moving it to the X-Session-API-Key header. The changes in RemoteConversation.ts correctly remove the logic that added the key to the URL. The existing getAuthHeaders method already handles adding the key to headers, which is now leveraged by the WebSocket connection logic. The pull request also includes excellent test coverage: the MockWS class in remoteConversation.test.ts is updated to allow inspection of connection options (like headers), and a new test case is added to assert that the session_api_key is absent from the URL and correctly placed in the request headers. The changes are clean, targeted, and effectively mitigate the risk of leaking secrets through URLs. The implementation is solid.
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
✅ PinkCat Review: LGTMTaste Rating: 🟢 Good taste - Critical security fix This PR addresses bead oh-tab-h3g - the secret-in-URL security issue. Security Impact:
This eliminates URL logging, browser history, and referrer leakage vectors. Ready to merge (pending upstream #1786 for production use). |
|
@OpenHands Look at this PR diff, understand that we renamed the agent-sdk package, and re-apply the fixes in this PR on clean main so we can review and merge this PR Mark it ready for review when you verify that it has all we wanted; monitor it then for some 5 mins then use gh to check for AI reviews that will be posted; read both top and inline review threads and address them as you see fit. They are AI so maybe they're wrong, you need to validate first and think if it needs fixed. |
|
I'm on it! enyst can track my progress at all-hands.dev |
e38540b to
b8d7d5a
Compare
Port the original security fix onto current develop after the agent-sdk package rename. Keep runtime session keys in WebSocket headers only, remove the legacy query-string fallback, and tighten the RemoteConversation tests around URL-secret leakage. Co-authored-by: openhands <openhands@all-hands.dev>
b8d7d5a to
a21260b
Compare
|
Since my last summary, there have been no further code changes or pushes. Current final state:
Checklist:
Remaining blocker:
So: the requested PR-porting/review-readiness work is complete, the diff is concise, and the only unresolved item is the newly surfaced CI incompatibility. |
🔧 VSCode Extension Built Successfully• File: openhands-tab-0.9.0.vsix (545 KB) To install:
Built with Node 22. Commit 0249bd4. |
Fixes oh-tab-h3g.
session_api_keyfrom the RemoteConversation WebSocket URL query string (prevents URL secret leakage).session_api_keyinto the WS URL.session_api_keyor key material.Notes
X-Session-API-KeyorAuthorization: Bearer ...).Summary by CodeRabbit
Refactor
Tests
Documentation