-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Cmd click open finder #3807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cmd click open finder #3807
Conversation
| event.returnValue = app.getVersion(); | ||
| }); | ||
|
|
||
| ipcMain.handle('open-directory-in-explorer', async (_event, path: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is exposed as an IPC the path can be compromised to show any directory on the machine. Maybe we should add an allowlist (or blocklist) so only appropriate files are opened? This and anywhere else that we open directories or list files through an IPC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is possible in electron to have ipc only available to the renderer so if so, that isn't an issue (checking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep seems ok: Electron's ipcMain and ipcRenderer communicate through Chromium's internal IPC system
## Security Analysis of Electron IPC Setup
### ✅ **Secure Configuration Found:**
1. **Context Isolation is Enabled** (`contextIsolation: true`)
- This is the most critical security feature that prevents the renderer process from accessing Node.js APIs or Electron internals directly
- The renderer can only access what's explicitly exposed through the context bridge
2. **Node Integration is Disabled** (`nodeIntegration: false`)
- Prevents the renderer from having direct access to Node.js APIs
- This is a crucial security measure to prevent arbitrary code execution
3. **Web Security is Enabled** (`webSecurity: true`)
- Enforces same-origin policy and other web security features
- Prevents loading of insecure content
4. **Preload Script with Context Bridge**
- All IPC communication is channeled through a preload script (`preload.ts`)
- Uses `contextBridge.exposeInMainWorld()` to expose only specific, controlled APIs
- The exposed APIs are well-defined with type safety
5. **Controlled API Surface**
- Only specific methods are exposed to the renderer through two APIs:
- `window.electron`: Contains all IPC methods for file operations, settings, etc.
- `window.appConfig`: Contains configuration data
- Each exposed method is explicitly defined and typed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I'll add some tests to ensure this never changes down the road also 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zanesq oh yeah that would be stellar - as I am sure would be an easy mistake to make
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is exposed as an IPC the path can be compromised to show any directory on the machine
can you help me understand the line of attack here? even if an outside attacker would break into goose (at which point they have full control over the host machine anyway), if they would this trigger this end point, all it would do is start the finder in a particular directory. only the end user would be able to see that, no? not the attacker?
I'd love to understand security considerations with the electron app better as aI see some checks that I don't quite understand.
* main: (33 commits) fix: optimise reading large file content (#3767) fix: replace glob/grep tool with shell (#3834) docs: Add Youtube Link to dev.to tutorial (#3869) Changed app settings configuration form to match settings panels (#3829) Tell the user to hit compact (#3851) Pin @mcp-ui/client in package.json (#3860) blog for mcp-jupyter server (#3059) docs: Adding dev.to Tutorial & Update CLI Component (#3828) Detect client disconnects and cancel tool calls (#3782) Suppress ansi with pipes (#3775) Fix leaky env variable causing flaky test (#3761) Update gemini error msg (#3847) Generic retry and error parsing (#3558) Clear the current line on ctrl-c in line with other tools (#3764) chore: upgrade morph to use new model with instruction (#3745) add CODEOWNERS file with /documentation owners (#3840) Token counting in Auto-compact uses provider metadata (#3788) docs: Add YouTube link to Git MCP Tutorial (#3831) feat: more robust client initialization for the app (#3830) Build app bundles on release branches always (#3789) ...
michaelneale
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems reasonable (and IPC thing doesn't look like it is an issue - good to check of course).
* 'main' of github.com:block/goose: Make the window title reflect what we are doing (#3883) additional metrics + Ui implementation (#3871) feat: Add session description editing functionality (#3819) Update filename in contributing docs (#3866) Fix voice dictation provider selection bug (#3862) doc: Update supported container runtimes (#3874) feat: add OAuth provider abstraction for CLI configuration (#3157) Don't ignore lockfiles on linux/windows builds (#3859) Use RMCP for StreamableHTTP OAuth support (#3845) Try to keep key order for Databricks (#3876) Fix OpenAI Provider with GitHub Models (#3875) Cmd click open finder (#3807) fix: recipe parameter form max height and not scrolling (#3879) fix: optimise reading large file content (#3767) fix: replace glob/grep tool with shell (#3834) docs: Add Youtube Link to dev.to tutorial (#3869)
* 'main' of github.com:block/goose: Make the window title reflect what we are doing (#3883) additional metrics + Ui implementation (#3871) feat: Add session description editing functionality (#3819) Update filename in contributing docs (#3866) Fix voice dictation provider selection bug (#3862) doc: Update supported container runtimes (#3874) feat: add OAuth provider abstraction for CLI configuration (#3157) Don't ignore lockfiles on linux/windows builds (#3859) Use RMCP for StreamableHTTP OAuth support (#3845) Try to keep key order for Databricks (#3876) Fix OpenAI Provider with GitHub Models (#3875) Cmd click open finder (#3807) fix: recipe parameter form max height and not scrolling (#3879) fix: optimise reading large file content (#3767) fix: replace glob/grep tool with shell (#3834) docs: Add Youtube Link to dev.to tutorial (#3869)
* main: Upgrade to MCP-UI ~5.6.2 and handle internalized auto iframe resizing (#3889) docs: recipe updates (#3844) added notes about reducing context window by referencing external files (#3895) Make the window title reflect what we are doing (#3883) additional metrics + Ui implementation (#3871) feat: Add session description editing functionality (#3819) Update filename in contributing docs (#3866) Fix voice dictation provider selection bug (#3862) doc: Update supported container runtimes (#3874) feat: add OAuth provider abstraction for CLI configuration (#3157) Don't ignore lockfiles on linux/windows builds (#3859) Use RMCP for StreamableHTTP OAuth support (#3845) Try to keep key order for Databricks (#3876) Fix OpenAI Provider with GitHub Models (#3875) Cmd click open finder (#3807) fix: recipe parameter form max height and not scrolling (#3879)
This is a dumb hidden feature that some people want and Goose could do while I made a coffee, but it fixes #3158