Skip to content

fix: Use secure defaults for Pico channel setup and stop leaking the token in the URL#1563

Merged
wj-xiao merged 2 commits intosipeed:mainfrom
bittoby:fix/pico-setup-secure-defaults
Mar 16, 2026
Merged

fix: Use secure defaults for Pico channel setup and stop leaking the token in the URL#1563
wj-xiao merged 2 commits intosipeed:mainfrom
bittoby:fix/pico-setup-secure-defaults

Conversation

@bittoby
Copy link
Contributor

@bittoby bittoby commented Mar 14, 2026

📝 Description

Secure Pico channel setup defaults and stop leaking the token in the URL.

  • Don't auto-enable allow_token_query on setup
  • Use a localhost allowlist instead of wildcard * origins
  • Pass the token via WebSocket subprotocol header instead of query string

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)

🤖 AI Code Generation

  • 🛠️ Mostly AI-generated (AI draft, Human verified/modified)

🔗 Related Issue

Fixes #1530

📚 Technical Context (Skip for Docs)

  • Reasoning: Query-string tokens leak in logs, browser history, and proxies. Wildcard origins remove browser-side protection. Setup should only enable what's needed.

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.

@CLAassistant
Copy link

CLAassistant commented Mar 14, 2026

CLA assistant check
All committers have signed the CLA.

@bittoby
Copy link
Contributor Author

bittoby commented Mar 14, 2026

@alexhoshina Could you please review this PR? I'd appreciate any feedbacks.

@sipeed-bot sipeed-bot bot added type: bug Something isn't working domain: channel go Pull requests that update go code labels Mar 14, 2026
// local-dev scenarios (Vite on 5173, launcher on 18800) without opening the
// WebSocket to arbitrary cross-origin pages.
var defaultPicoOrigins = []string{
"http://localhost:5173",
Copy link
Collaborator

Choose a reason for hiding this comment

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

allow_origins should not be seeded with a fixed localhost port list here.

CheckOrigin does exact Origin matching in pkg/channels/pico/pico.go (line 69), while buildWsURL() intentionally returns the current request host in public/LAN launcher mode, for example ws://192.168.1.9:18790/pico/ws in web/backend/api/gateway_host_test.go (line 30).
After this change, /api/pico/setup writes only localhost/127.0.0.1 origins, so opening the launcher from http://192.168.1.9:18800 or any custom UI port will fail the WebSocket handshake even though setup reports success.

Could we derive the default allow_origins from the incoming request Origin (or the active launcher origin) instead of hard-coding 5173 and 18800? That would preserve the security improvement without breaking public/LAN or custom-port setups.
You can use picoclaw-launcher -public to test.

@alexhoshina
Copy link
Collaborator

hi @bittoby, I'm not very familiar with this part of the code, so I've asked other maintainers to review it🙏

@bittoby
Copy link
Contributor Author

bittoby commented Mar 14, 2026

Ok. @alexhoshina If possible, could you review this as soon as possible? I can update and respond right away. So feel free to reach out any time. I will solve soon.

@alexhoshina
Copy link
Collaborator

alexhoshina commented Mar 14, 2026

Ok. @alexhoshina If possible, could you review this as soon as possible? I can update and respond right away. So feel free to reach out any time. I will solve soon.

@bittoby this #1563 (comment)

@bittoby
Copy link
Contributor Author

bittoby commented Mar 14, 2026

Ok. @alexhoshina If possible, could you review this as soon as possible? I can update and respond right away. So feel free to reach out any time. I will solve soon.

@bittoby this #1563 (comment)

Yes. I am fixing that problem now

@bittoby
Copy link
Contributor Author

bittoby commented Mar 14, 2026

@wj-xiao Thanks for your feedback. Good points! I have fixed and added test code. It works well. please review again.

Cc: @alexhoshina

@bittoby bittoby requested a review from wj-xiao March 15, 2026 12:01
@bittoby
Copy link
Contributor Author

bittoby commented Mar 15, 2026

@alexhoshina @wj-xiao Could you please review once more?
thank you

@wj-xiao wj-xiao merged commit 71e2b63 into sipeed:main Mar 16, 2026
4 checks passed
@bittoby
Copy link
Contributor Author

bittoby commented Mar 16, 2026

Thanks!

@Orgmar
Copy link
Contributor

Orgmar commented Mar 16, 2026

@bittoby Good security hardening here. Switching the token from query string to WebSocket subprotocol header and locking down the CORS origins are both important fixes. Query string tokens leaking into logs and browser history is a real risk.

We have a PicoClaw Dev Group on Discord where contributors connect and collaborate on the project. If you'd like to join, send an email to support@sipeed.com with the subject [Join PicoClaw Dev Group] + your GitHub account and we'll send you the invite link!

aishannon pushed a commit to aishannon/picoclaw that referenced this pull request Mar 16, 2026
…token in the URL (sipeed#1563)

* fix: Use secure defaults for Pico channel setup and stop leaking the token in the URL

* fix: Derive default allow_origins from the setup request's Origin header instead of hardcoding localhost ports
alexhoshina pushed a commit to alexhoshina/picoclaw that referenced this pull request Mar 17, 2026
…token in the URL (sipeed#1563)

* fix: Use secure defaults for Pico channel setup and stop leaking the token in the URL

* fix: Derive default allow_origins from the setup request's Origin header instead of hardcoding localhost ports
j0904 pushed a commit to j0904/picoclaw that referenced this pull request Mar 22, 2026
…token in the URL (sipeed#1563)

* fix: Use secure defaults for Pico channel setup and stop leaking the token in the URL

* fix: Derive default allow_origins from the setup request's Origin header instead of hardcoding localhost ports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: channel go Pull requests that update go code type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: pico setup should not enable query-token auth and wildcard origins by default

5 participants