fix(browser): only emit upgrade-insecure-requests over HTTPS#519
Conversation
On a plain-HTTP LAN deploy the proxied CSP's unconditional 'upgrade-insecure-requests' forced every rewritten subresource (http://host:proxy_port/...) to be upgraded to https:// — which the HTTP-only proxy origin can't serve, so all CSS, JS, images, and the injected copilot.js failed with ERR_SSL_PROTOCOL_ERROR. Real sites rendered unstyled with dead buttons (their JS never loaded). Emit the directive only when the proxy request is HTTPS (so HTTPS deploys keep the protection, HTTP deploys work). Factor scheme detection into _request_scheme(). Verified live: google.com went from 24 SSL errors / broken render to 0 errors and a styled, interactive page.
📝 WalkthroughWalkthroughThis PR makes the ChangesConditional upgrade-insecure-requests directive
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 `@tinyagentos/routes/desktop_browser/proxy.py`:
- Around line 355-358: The Copilot websocket URL currently uses
request.url.scheme causing ws:// to be emitted behind TLS termination; update
the code that builds ws_url to reuse the effective request scheme logic (the
same used by _request_scheme(request) and proxied_response_csp) so it emits
"wss" when _request_scheme(request) == "https" and "ws" otherwise. Locate where
ws_url is constructed and replace its scheme decision with this check so the
injected websocket URL matches the CSP/upgraded scheme and avoids mixed-content
blocking.
🪄 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: dc3a0731-851e-4c91-a446-43622673924b
📒 Files selected for processing (3)
tests/routes/desktop_browser/test_csp.pytinyagentos/routes/desktop_browser/csp.pytinyagentos/routes/desktop_browser/proxy.py
| out_headers["content-security-policy"] = proxied_response_csp( | ||
| _shell_origin(request), | ||
| upgrade_insecure=_request_scheme(request) == "https", | ||
| ) |
There was a problem hiding this comment.
Reuse the effective request scheme for the injected Copilot websocket.
This wiring fixes the CSP side, but ws_url still keys off request.url.scheme at Line 305. Behind TLS termination, that leaves an https://... page trying to open ws://..., which browsers block as mixed content, so Copilot stays non-interactive on HTTPS deployments.
Proposed fix
charset = _detect_charset(content_type, response.content)
+ request_scheme = _request_scheme(request)
rewritten = rewrite_html(
response.content, base_url=str(response.url), proxy=_proxy_url,
charset=charset,
)
- ws_scheme = "wss" if request.url.scheme == "https" else "ws"
+ ws_scheme = "wss" if request_scheme == "https" else "ws"
ws_url = (
f"{ws_scheme}://{request.url.netloc}/api/desktop/browser/copilot"
f"?profile_id={quote(profile_id, safe='')}"
)
@@
out_headers["content-security-policy"] = proxied_response_csp(
_shell_origin(request),
- upgrade_insecure=_request_scheme(request) == "https",
+ upgrade_insecure=request_scheme == "https",
)🤖 Prompt for 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.
In `@tinyagentos/routes/desktop_browser/proxy.py` around lines 355 - 358, The
Copilot websocket URL currently uses request.url.scheme causing ws:// to be
emitted behind TLS termination; update the code that builds ws_url to reuse the
effective request scheme logic (the same used by _request_scheme(request) and
proxied_response_csp) so it emits "wss" when _request_scheme(request) == "https"
and "ws" otherwise. Locate where ws_url is constructed and replace its scheme
decision with this check so the injected websocket URL matches the CSP/upgraded
scheme and avoids mixed-content blocking.
* fix(browser): copilot ws_url honours x-forwarded-proto scheme 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. * fix(browser): malformed x-forwarded-proto falls back to request scheme, 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.
Problem
On a plain-HTTP LAN deploy (the default), the proxied-response CSP unconditionally included
upgrade-insecure-requests. The rewriter points every subresource at the proxy origin (http://host:proxy_port/...), and that directive forces the browser to upgrade them tohttps://— which the HTTP-only proxy origin can't serve. Result: every stylesheet, script, image, and the injected copilot.js failed withERR_SSL_PROTOCOL_ERROR, so real sites rendered unstyled with dead buttons (their JS never loaded).Fix
Emit
upgrade-insecure-requestsonly when the proxy request is HTTPS (_request_scheme(request) == "https"). HTTPS deploys keep the mixed-content protection; HTTP deploys work. Scheme detection factored into_request_scheme()(also reused by_shell_origin).Verification (live on a Pi)
google.com: 24ERR_SSL_PROTOCOL_ERRORs → 0 console errors, page renders styled and interactive (Sign-in button, consent dialog) instead of faint/broken.Tests
Added: directive absent by default, present when
upgrade_insecure=True.Summary by CodeRabbit
upgrade-insecure-requestsdirective only for HTTPS-only proxy environments, improving security accuracy across different proxy configurations.