fix(browser): copilot ws_url honours x-forwarded-proto scheme#521
Conversation
The injected copilot websocket URL derived its scheme from request.url.scheme directly, so behind a TLS terminator it would inject ws:// instead of wss:// and the socket would fail. Route it through _request_scheme() (x-forwarded-proto aware), matching the CSP line. Addresses CodeRabbit on #519. Adds _request_scheme unit tests.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds tests for ChangesHTTP Scheme Forwarding for WebSocket Injection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/routes/desktop_browser/test_proxy_shell.py`:
- Around line 180-182: The test and implementation currently force an invalid
x-forwarded-proto to "http" which downgrades HTTPS; update the logic in
_request_scheme to use request.url.scheme as the fallback when the
X-Forwarded-Proto header is malformed (instead of hard-coding "http"), and
change the test test_bogus_forwarded_proto_clamped_to_http to assert that
_request_scheme(self._req(xff="javascript", scheme="http" or scheme="https"))
returns request.url.scheme (i.e., the original request.url.scheme), covering
both HTTPS and HTTP cases so malformed headers do not force a downgrade.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f453d26-2669-4698-884d-2a593232a5d2
📒 Files selected for processing (2)
tests/routes/desktop_browser/test_proxy_shell.pytinyagentos/routes/desktop_browser/proxy.py
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by nemotron-3-super-120b-a12b-20230311:free · 391,460 tokens |
…e, not http A junk forwarded-proto header on a genuinely HTTPS request was being clamped to http, downgrading the injected Copilot URL to ws:// and disabling the HTTPS CSP path. Fall back to request.url.scheme instead.
Addresses CodeRabbit's Major finding on #519.
The injected copilot websocket URL derived its scheme from
request.url.schemedirectly, while the CSP (from #519) uses thex-forwarded-proto-aware_request_scheme(). So behind a TLS terminator the page would injectws://instead ofwss://and the copilot socket would fail (mixed-content / wrong scheme).Fix: route
ws_schemethrough_request_scheme(request), matching the CSP line. Adds unit tests for_request_scheme(plain http, direct https, x-forwarded-proto override, comma-list first-value, bogus-value clamp).Only affects reverse-proxied HTTPS deploys; plain LAN http was already correct.
Summary by CodeRabbit
Bug Fixes
Tests