feat: allow list of MCPs #1821
feat: allow list of MCPs #1821
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds optional allowlisting for MCP commands by introducing new functions to download and validate an allowlist file. Key changes include the addition of functions to download the allowlist, validate commands against the allowlist in multiple modules, and updates to dependencies in Cargo.toml to support blocking requests and testing.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/goose/src/agents/extension.rs | Introduces functions is_command_allowed and download_allowlist with new error variants for allowlist handling; minor spelling fix in a comment. |
| crates/goose/src/config/extensions.rs | Validates extension commands before saving configuration by calling validate_command. |
| crates/goose/src/agents/capabilities.rs | Validates the extension command before adding it to the client. |
| crates/goose/Cargo.toml | Updates dependencies by adding the blocking feature for reqwest and the mockito dependency for testing. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an optional allowlisting mechanism for MCP commands, ensuring that only approved commands are executed. Key changes include:
- Adding new error variants and functions (is_command_allowed and download_allowlist) for allowlist verification.
- Integrating command validation into extension configuration and capabilities.
- Adding tests and updating dependencies to support blocking HTTP requests and mocking.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/goose/src/agents/extension.rs | Added allowlist functions, error variants, and tests for MCP checks. |
| crates/goose/src/config/extensions.rs | Integrated command validation before saving extension configuration. |
| crates/goose/src/agents/capabilities.rs | Validated extension commands prior to transport creation. |
| crates/goose/Cargo.toml | Updated dependencies to support blocking requests and testing. |
| // Only log a warning if both download failed AND we couldn't read the file | ||
| warn!( | ||
| "Failed to download allowlist AND couldn't read existing file at {}: {:?}", | ||
| path_str, | ||
| download_result.err() | ||
| ); |
There was a problem hiding this comment.
In cases where both downloading and reading the allowlist file fail, the function only logs a warning and then allows the command, which contradicts the expectations in test_download_allowlist_failure. Consider returning an error instead of silently succeeding.
| // Only log a warning if both download failed AND we couldn't read the file | |
| warn!( | |
| "Failed to download allowlist AND couldn't read existing file at {}: {:?}", | |
| path_str, | |
| download_result.err() | |
| ); | |
| // Return an error if both download failed AND we couldn't read the file | |
| return Err(Box::new(ExtensionError::AllowlistAccessFailed( | |
| format!( | |
| "Failed to download allowlist AND couldn't read existing file at {}: {:?}", | |
| path_str, | |
| download_result.err() | |
| ), | |
| ))); |
| if let Some(extensions) = yaml.get("extensions") { | ||
| if let Some(extensions_array) = extensions.as_sequence() { | ||
| // Create a list of allowed commands | ||
| let allowed_commands: Vec<String> = extensions_array |
There was a problem hiding this comment.
We probably want to check the pair of command and ID. It makes it slightly more rigid which I think is a good property for this use-case
There was a problem hiding this comment.
the id is not defined by the extension (it is just system local) so can't really do much with it.
| let path_str = allowlist_path.to_string_lossy().to_string(); | ||
|
|
||
| // Always try to download the allowlist if URL is set | ||
| let download_result = download_allowlist(&url); |
There was a problem hiding this comment.
A few issues here:
- We probably don't want to download the file and save it to disk every request. We can probably do this entirely in memory
- We can probably cache the allowlist to make this whole thing more performant!
There was a problem hiding this comment.
yes good point, will be trying a differnet approach via the goose-server crate next
|
closing in favour of #1848 |
still WIP: optional allowlisting of MCP/commands that mcp client will launch