Skip to content

fix(browser): only emit upgrade-insecure-requests over HTTPS#519

Merged
jaylfc merged 1 commit into
masterfrom
fix/proxy-upgrade-insecure-http
Jun 2, 2026
Merged

fix(browser): only emit upgrade-insecure-requests over HTTPS#519
jaylfc merged 1 commit into
masterfrom
fix/proxy-upgrade-insecure-http

Conversation

@jaylfc
Copy link
Copy Markdown
Owner

@jaylfc jaylfc commented Jun 2, 2026

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 to https:// — which the HTTP-only proxy origin can't serve. Result: every stylesheet, script, image, and the injected copilot.js failed with ERR_SSL_PROTOCOL_ERROR, so real sites rendered unstyled with dead buttons (their JS never loaded).

Fix

Emit upgrade-insecure-requests only 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: 24 ERR_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

  • Improvements
    • Refined Content Security Policy configuration to conditionally apply the upgrade-insecure-requests directive only for HTTPS-only proxy environments, improving security accuracy across different proxy configurations.

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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 2, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR makes the upgrade-insecure-requests CSP directive conditional rather than always present. The proxied_response_csp function now accepts an upgrade_insecure parameter; a new request scheme detection helper determines when to enable it based on HTTPS; test coverage validates both the default absence and conditional inclusion of the directive.

Changes

Conditional upgrade-insecure-requests directive

Layer / File(s) Summary
CSP parameter and conditional directive
tinyagentos/routes/desktop_browser/csp.py
Function signature adds keyword-only upgrade_insecure: bool = False parameter; directive construction conditionally appends upgrade-insecure-requests only when enabled.
Request scheme detection and CSP wiring
tinyagentos/routes/desktop_browser/proxy.py
New _request_scheme() helper extracts and clamps the effective request scheme from x-forwarded-proto; CSP header generation passes the HTTPS detection result as the upgrade_insecure flag to proxied_response_csp.
Test coverage for upgrade-insecure behavior
tests/routes/desktop_browser/test_csp.py
Two new test methods verify that upgrade-insecure-requests is absent by default and present only when upgrade_insecure=True.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • jaylfc/tinyagentos#301: Both PRs modify the proxied response CSP generation in tinyagentos/routes/desktop_browser/csp.py with changes to the upgrade-insecure-requests directive.
  • jaylfc/tinyagentos#518: Both PRs update the same desktop-browser CSP pipeline (proxied_response_csp and its use in routes/desktop_browser/proxy.py), with the related PR handling frame-ancestors/shell_origin computation.

Poem

🐰 A directive now skips the rush,
When upgrade_insecure reigns supreme—
Scheme detection guides the push,
HTTPS-safe, a CSP dream!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: conditionally emitting the upgrade-insecure-requests CSP directive based on HTTPS availability, which is the core problem being solved.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/proxy-upgrade-insecure-http

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3cb14b8 and 9049c84.

📒 Files selected for processing (3)
  • tests/routes/desktop_browser/test_csp.py
  • tinyagentos/routes/desktop_browser/csp.py
  • tinyagentos/routes/desktop_browser/proxy.py

Comment on lines +355 to +358
out_headers["content-security-policy"] = proxied_response_csp(
_shell_origin(request),
upgrade_insecure=_request_scheme(request) == "https",
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

@jaylfc jaylfc merged commit 72f70c3 into master Jun 2, 2026
8 checks passed
@jaylfc jaylfc deleted the fix/proxy-upgrade-insecure-http branch June 2, 2026 19:29
@github-project-automation github-project-automation Bot moved this from Todo to Done in TinyAgentOS Roadmap Jun 2, 2026
jaylfc added a commit that referenced this pull request Jun 2, 2026
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant