Load packaged Desktop from a dedicated goose-app://goose origin - #10188
lifeizhou-ap wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1961ce1cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
|
|
||
| protocol.handle(PACKAGED_RENDERER_PROTOCOL, async (request) => { |
There was a problem hiding this comment.
Electron's protocol handlers are session-scoped (see https://electronjs.org/docs/latest/api/protocol#using-protocol-with-a-custom-partition-or-session). This registers goose-app on the default session, but the packaged windows use partition: 'persist:goose' (for example the main window at ui/desktop/src/main.ts:1319), so in packaged builds those windows do not have a handler for goose-app://goose/index.html; loadURL will fail before the renderer starts. Register the handler on session.fromPartition('persist:goose').protocol before loading the windows.
Useful? React with 👍 / 👎.
| serverSecret, | ||
| dir: workingDir, | ||
| tls: true, | ||
| allowedOrigins: app.isPackaged ? [PACKAGED_RENDERER_ORIGIN] : undefined, |
There was a problem hiding this comment.
This only teaches the backend that Desktop launches about goose-app://goose; when externalGoosed/GOOSE_EXTERNAL_BACKEND is used, no flag is passed to that server and create_router defaults to loopback-only origins, while the renderer's WebSocket now sends Origin: goose-app://goose instead of the old rewritten http://localhost:5173. Existing external backends therefore pass the main-process health check but reject the renderer ACP WebSocket unless users know to restart them with --allowed-origin goose-app://goose.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fixed. Added the text on the Backend Server Setting page to let user know to start with --allowed-origin goose-app://goose
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e6710ced3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| registerPackagedRendererProtocol(); | ||
| return packagedRendererUrl(); |
There was a problem hiding this comment.
When upgrading from a packaged build that loaded file://.../index.html, this now moves the renderer to the new goose-app://goose origin, so existing file-origin localStorage is no longer visible. The preload lazy migration still reads keys like theme, use_system_theme, response_style, show_pricing, and seenAnnouncementIds only from the current origin, and recent message history is also stored there, so users who have not rewritten those values into the settings store will silently fall back to defaults after the upgrade. Add a migration path for the old file-origin storage before cutting over to the custom origin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
"theme, use_system_theme, response_style, show_pricing, etc" state is already outside renderer localStorage: settings/external backend config are in settings.json, and recipe trust is in recipe_hashes.
The remaining old file:// localStorage is only prompt recall (goose-chat-history) and sidebar expanded state, so the user impact without this migration is low. I’d skip it to keep this fix focused on moving packaged Desktop to a stable non-null origin.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82534c650f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| scheme: PACKAGED_RENDERER_PROTOCOL, | ||
| privileges: { | ||
| standard: true, | ||
| secure: true, |
There was a problem hiding this comment.
When packaged Desktop uses an http:// external backend, acpWebSocketUrlFromHttpBase converts it to ws://.../acp, and the settings UI still accepts plain HTTP unless a certificate fingerprint is set. Marking the new goose-app scheme as secure makes the renderer a secure context, so Chromium blocks ws:// connections to non-loopback hosts as mixed content; the main-process /status check can still pass, leaving the chat window unable to establish ACP. Either reject/plainly warn on insecure external backends for packaged builds or route those connections through a safe main-process path.
Useful? React with 👍 / 👎.
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
LGTM — nice cleanup replacing the Origin-header spoofing hack with a real goose-app://goose origin. The appProtocol.ts module is clean and the path-traversal tests are worth having.
One thing worth double-checking before/after merge: codex's latest note about the secure context blocking ws:// to non-loopback HTTP external backends as mixed content. If that turns out to be real it'd break ACP for insecure external backends even when /status passes — but that shouldn't block this.
|
Close it as the issue is fixed on the ACP server side #10194. This PR is the client side fix which is more secure, however, we still need to test it and is not user friendly. Can revisit it when it is required |
2 participants
Summary
Testing
Related Issues
Relates to #ISSUE_ID
Discussion: LINK (if any)
Screenshots/Demos (for UX changes)
Before:
After: