Skip to content

Conversation

@michaelneale
Copy link
Collaborator

fixes: #1822

this will load/cache a remote allowlist to prevent accidental deeplink activation of unwanted extensions

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements an optional allowlist for extensions by fetching and caching a remote YAML file to prevent accidental activation of unwanted extensions. Key changes include:

  • Adding allowlist validation for extension commands in the request handler.
  • Implementing fetching, caching, and checking logic for allowed extensions.
  • Adding tests for allowlist functionality and updating Cargo.toml with a new reqwest dependency.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
crates/goose-server/src/routes/extension.rs Added allowlist fetching, caching, and command validation features.
crates/goose-server/Cargo.toml Introduced reqwest dependency for remote allowlist fetching.
Comments suppressed due to low confidence (1)

crates/goose-server/src/routes/extension.rs:330

  • Using 'contains' for matching commands may lead to unintended partial matches. Consider replacing it with an equality check (==) to ensure only exact command matches are allowed or document the intended behavior explicitly.
extensions.extensions.iter().any(|entry| cmd_base.contains(&entry.command))

## How It Works

1. The allowlist is fetched from a URL specified by the `GOOSE_ALLOWLIST` environment variable.
2. The allowlist is a YAML file that contains a list of allowed extension commands.
Copy link
Contributor

Choose a reason for hiding this comment

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

Pairs of extension IDs + Commands?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it is technically that (although id doesn't carry any intrinsic meaning)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we may want to validate both. In a world where IDs are unique I could see this being better long term validation (as you rightly point out no short term impact)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think for now, just extension command until we know what id's are for!


## Security Considerations

1. **HTTPS**: Always use HTTPS URLs for your allowlist to prevent man-in-the-middle attacks.
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably validate this in the code too right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes


/// Implementation of command allowlist checking that takes an explicit allowlist parameter
/// This makes it easier to test without relying on global state
fn is_command_allowed_with_allowlist(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should probably compare the IDs alongside the commands. Basically checking the pair

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think id has no meaning, so it just adds more work at the moment to the person maintaining the allowlist + entering things in goose by hand (when the command is what matters)

@shellz-n-stuff
Copy link
Contributor

Broadly I really really love this feature. It's such a huge security and trust improvement for customers

Copy link
Contributor

@shellz-n-stuff shellz-n-stuff left a comment

Choose a reason for hiding this comment

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

LGTM!

* main:
  ui: turn on extensions at startup (#1861)
  ui: models dropdown (#1860)
  fix: cli empty line (#1856)
  feat: Allow setting OpenAI timeout from config (#1819)
  feat: add retry for google (#1854)
  feat(extensions): add Java/JDK support for MCP servers (#1816)
  feat: extract `StdioProcessError(msg)` to try to display (#1855)
  fix: show window bugfix (#1840)
  fix: append the attachment path to the existing text in the input prompt (#1842)
  docs: updated docs for smart approval mode (#1853)
  styles: chat scroll interaction (#1837)
  ui: add description field  to modal (#1846)
  feat: use temp dir for extracting goose binary (#1838)
  ui: remove and update extensions (#1847)
  fix: disappearing user text when stopped (#1839)
Copy link

@wendytang wendytang left a comment

Choose a reason for hiding this comment

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

what's the best way to test the allowlist?

i tried setting an allowlist in the cli, but realized it was for goosed
image

Set the `GOOSE_ALLOWLIST` environment variable to the URL of your allowlist YAML file:

```bash
export GOOSE_ALLOWLIST=https://example.com/goose-allowlist.yaml
Copy link

@wendytang wendytang Mar 26, 2025

Choose a reason for hiding this comment

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

how would the env var be passed into goosed? is it going to be built into the app?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, built into app, similar to other ones.

@michaelneale michaelneale requested a review from wendytang March 26, 2025 23:07
@michaelneale
Copy link
Collaborator Author

@wendytang you have to run it from just run-ui - it does not affect the command line

Copy link

@wendytang wendytang left a comment

Choose a reason for hiding this comment

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

Nice! It is working very nicely

@wendytang
Copy link

wendytang commented Mar 27, 2025

did we want to show the error message in the notification?
image

@michaelneale
Copy link
Collaborator Author

@wendytang yeah - I think we could work on a design to present the message "not approved" as well, yes.

@michaelneale michaelneale merged commit 5d2e31d into main Mar 27, 2025
6 checks passed
@michaelneale michaelneale deleted the micn/allowlist-goosed branch March 27, 2025 00:36
salman1993 added a commit that referenced this pull request Apr 2, 2025
laanak08 added a commit that referenced this pull request Apr 3, 2025
* main:
  fix: handle the case sensitive in mac screenshot name (#2021)
  ui: better env var ux (#2006)
  feat: add ANTHROPIC_HOST configuration for Anthropic in cli and UI (#1776)
  Fix function params  (#2012)
  revert: "feat: allowlist optionally for goosed (#1848)"  (#2010)
  # feat(providers): Add support for generic GCP Vertex AI Claude and Gemini models (#1909)
  fix: fix empty result for view tool response (#2011)
  Blog: Top MCP servers I use  (#1951)
  docs: MCP for Nondevs Blog (#1910)
  styles: update markdown styles (#2005)
  fix(ollama): respect ollama URL and port (#2004)
  ui: refresh selected model (#2002)
  ui: fix radio button selection (#2001)
  use provider display names (#2000)
  ui: settings v2 click anywhere to exit models bottom bar (#1997)
  feat: better ErrorBoundary UI (#1993)
  feat: check tool annotation before permission check (#1957)
  fix: see last msg fully in shared session view (#1994)
ahau-square pushed a commit that referenced this pull request May 2, 2025
cbruyndoncx pushed a commit to cbruyndoncx/goose that referenced this pull request Jul 20, 2025
cbruyndoncx pushed a commit to cbruyndoncx/goose that referenced this pull request Jul 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow list for MCP launching

4 participants