Skip to content

fix(web): honor forwarded headers for Pico websocket URLs#2125

Closed
9ex wants to merge 1 commit intosipeed:mainfrom
9ex:main
Closed

fix(web): honor forwarded headers for Pico websocket URLs#2125
9ex wants to merge 1 commit intosipeed:mainfrom
9ex:main

Conversation

@9ex
Copy link
Copy Markdown

@9ex 9ex commented Mar 28, 2026

📝 Description

This PR fixes Pico WebUI websocket URL generation for reverse-proxied deployments.

Before this change, the backend could return a websocket URL that exposed the internal launcher port, for example wss://<domain>:18800/pico/ws. In reverse-proxy setups where the public WebUI is served through 443 or another forwarded port, the browser would try to connect to the internal port directly and fail.

This change:

  • builds Pico websocket URLs from the externally visible request authority instead of the internal launcher port
  • honors X-Forwarded-Host, X-Forwarded-Port, and X-Forwarded-Proto
  • keeps request host fallback behavior when no forwarded headers are present
  • adds regression coverage for default HTTPS and custom forwarded ports

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 📖 Documentation update
  • ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

  • 🤖 Fully AI-generated (100% AI, 0% Human)
  • 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
  • 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

Related to #1737

📚 Technical Context (Skip for Docs)

🧪 Test Environment

  • Hardware: Apple Silicon Mac
  • OS: macOS (darwin 25.3.0)
  • Model/Provider: N/A (not model/provider specific; verified on the Pico WebUI websocket bootstrap path)
  • Channels: Pico

📸 Evidence (Optional)

Click to view Logs/Screenshots

Validated locally with:

  • make check
  • go test ./web/backend/api

Regression coverage added for:

  • request host fallback when no forwarded host is present
  • X-Forwarded-Proto: https
  • TLS requests without forwarded headers
  • X-Forwarded-Host with X-Forwarded-Port: 443
  • X-Forwarded-Host with X-Forwarded-Port: 8443

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.
  • I have reviewed the documentation impact and confirmed no documentation update is needed for this bug fix.

Build Pico websocket URLs from the externally visible host and port so reverse-proxied WebUI deployments can connect through 443 without exposing the internal launcher port.
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 28, 2026

CLA assistant check
All committers have signed the CLA.

@sipeed-bot sipeed-bot bot added type: bug Something isn't working domain: channel labels Mar 28, 2026
@yinwm
Copy link
Copy Markdown
Collaborator

yinwm commented Mar 28, 2026

Heads up: this PR's changes to gateway_host.go / gateway_host_test.go / pico.go overlap significantly with #1953 (feat(web): protect launcher dashboard with token and SPA login).

#1953 refactors buildWsURL into picoWebUIAddr + requestHTTPScheme + forwardedHostFirst / forwardedPortFirst / forwardedRFC7239Host — achieving the same goal (honoring X-Forwarded-* headers) and going further:

  • Also covers SSE (/pico/events) and HTTP (/pico/send) URLs, not just WebSocket
  • The URL fix is a hard dependency for dashboard auth (cookie domain must match the request origin)

If #1953 merges first, this PR can likely be closed as superseded. If we want to ship the URL fix independently first, #1953 would need to rebase and drop the overlapping gateway_host changes afterward.

CC @zeed-w-beez (author of #1953)

@9ex
Copy link
Copy Markdown
Author

9ex commented Mar 28, 2026

Thanks for the heads-up! I see the overlap with #1953.

My intent with this PR was to provide a narrowly scoped fix for the reverse-proxy WebSocket URL issue reported in #1737. It strictly addresses the /api/pico/token URL generation, preserves the existing fallback behavior, and adds regression tests for forwarded host/port/proto handling.

I'm perfectly fine with whichever direction the maintainers prefer:

  • If you'd rather ship the broader launcher auth work first, this PR can be superseded.

  • If you'd like to unblock reverse-proxied Pico WebUI deployments now, this PR is isolated, all CI checks are green, and it should be safe to merge independently as a quick bugfix.

Happy to rebase, adjust, or close this out depending on what works best for the project.

@9ex
Copy link
Copy Markdown
Author

9ex commented Mar 31, 2026

superseded by #1953

@9ex 9ex closed this Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: channel type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants