Skip to content

added some regex based checks for dangerous commands#38

Merged
lifeizhou-ap merged 1 commit intoblock:mainfrom
Kvadratni:main
Sep 4, 2024
Merged

added some regex based checks for dangerous commands#38
lifeizhou-ap merged 1 commit intoblock:mainfrom
Kvadratni:main

Conversation

@Kvadratni
Copy link
Contributor

This pull request enhances the safety of shell command execution by introducing a check for dangerous commands and updating the notification messages accordingly. The most important changes include adding a utility function to identify dangerous commands, modifying the shell command execution logic to incorporate this check, and adding tests to validate the new functionality.

Safety Enhancements:

Notification Improvements:

  • src/goose/toolkit/developer.py: Changed the status message from "running shell command" to "planning to run shell command" before the danger check, and moved the "running shell command" message to after the check. [1] [2]

Testing:

Import Adjustments:

Copy link
Collaborator

@baxen baxen left a comment

Choose a reason for hiding this comment

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

LGTM! Left a couple of small detail comments

bool: True if the command is dangerous, False otherwise.
"""
dangerous_patterns = [
r"\brm\b", # rm command
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think rm on a single file might be fine, but I am scared of rm -r - especially combined with the file path checks below. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

better safe than sorry.

Copy link

Choose a reason for hiding this comment

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

A bit late, but I think it'd be nice to add some user flexibility here, possibly via config setting somewhere. eg: one might also not want to allow curl, http, etc.

],
)
def test_dangerous_commands(command):
assert is_dangerous_command(command)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: probably want to assert some of the inverse, with safe commands too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's what the second test does.

@lifeizhou-ap lifeizhou-ap merged commit 466ce23 into block:main Sep 4, 2024
lifeizhou-ap added a commit that referenced this pull request Sep 4, 2024
…nctions

* main:
  Apply ruff and add to CI (#40)
  added some regex based checks for dangerous commands (#38)
  chore: Update publish github workflow to check package versions before publishing (#19)
lukealvoeiro added a commit that referenced this pull request Sep 4, 2024
…l-commit-title-pr

* origin/main:
  feat: show available toolkits (#37)
  adding in ability to provide per repo hints (#32)
  Apply ruff and add to CI (#40)
  added some regex based checks for dangerous commands (#38)
  chore: Update publish github workflow to check package versions before publishing (#19)
  chore: upgrade ai-exchange dependency (#36)
  fix: resuming sessions (#35)
  feat: upgrade `ai-exchange` to version `0.8.3` and fix tests (#34)
  fix: export metadata.plugins export should have valid module (#30)
  fix (#24)
  link to vs code extension (#20)
  Enable cli options for plugin (#22)
  Modified the readme to be more friendly to new users (#16)
  chore: gitignore generated lockfile (#15)
  add prompts (#11)
  conditionally publish only when config changes (#9)
lukealvoeiro added a commit that referenced this pull request Sep 9, 2024
* main:
  fix: typo in exchange method `rewind` (#54)
  fix: remove unsafe pop of messages (#47)
  chore: Update LICENSE (#53)
  chore(docs): update is_dangerous_command method description (#48)
  refactor: improve safety rails speed and prompt (#45)
  feat: make goosehints jinja templated (#43)
  ci: enforce PR title follows conventional commit (#14)
  feat: show available toolkits (#37)
  adding in ability to provide per repo hints (#32)
  Apply ruff and add to CI (#40)
  added some regex based checks for dangerous commands (#38)
  chore: Update publish github workflow to check package versions before publishing (#19)
  chore: upgrade ai-exchange dependency (#36)
  fix: resuming sessions (#35)
  feat: upgrade `ai-exchange` to version `0.8.3` and fix tests (#34)
  fix: export metadata.plugins export should have valid module (#30)
  fix (#24)
  link to vs code extension (#20)
  Enable cli options for plugin (#22)
  Modified the readme to be more friendly to new users (#16)
Kvadratni added a commit to Kvadratni/goose that referenced this pull request Sep 23, 2024
* origin/main:
  chore: release 0.9.0 (block#58)
  fix: goose should track files it reads and not overwrite changes (block#46)
  docs: Small dev notes for using exchange from source (block#50)
  fix: typo in exchange method `rewind` (block#54)
  fix: remove unsafe pop of messages (block#47)
  chore: Update LICENSE (block#53)
  chore(docs): update is_dangerous_command method description (block#48)
  refactor: improve safety rails speed and prompt (block#45)
  feat: make goosehints jinja templated (block#43)
  ci: enforce PR title follows conventional commit (block#14)
  feat: show available toolkits (block#37)
  adding in ability to provide per repo hints (block#32)
  Apply ruff and add to CI (block#40)
  added some regex based checks for dangerous commands (block#38)
  chore: Update publish github workflow to check package versions before publishing (block#19)

# Conflicts:
#	src/goose/toolkit/developer.py
#	src/goose/utils/check_shell_command.py
#	tests/utils/test_check_shell_command.py
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.

4 participants