Skip to content
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

Add wildcard support for shell deny/allow lists #5145

Conversation

soheil
Copy link
Contributor

@soheil soheil commented Aug 27, 2023

This Pull Request introduces an enhancement in the mechanism we use to control shell commands. Specifically, in the way we handle the allowlist and denylist of commands in the config.

The changes are as follows:

  1. A new process is introduced to identify shell commands in both the allowlist (shell_allowlist) and the denylist (shell_denylist) that contains wildcard characters (*, [, ], ?). These identified commands are placed into two new lists: wildcard_shell_allowlist and wildcard_shell_denylist.

  2. The new logic first checks if there are any wildcard commands in the allowlist or denylist. If there are:

    • For allowlist control mechanism, it checks if the given command matches any pattern in the wildcard_shell_allowlist. If it does, it returns True, indicating this command is allowed to run. If not, it returns whether the command is in the non-wildcard allowlist.

    • For other mechanisms, it checks if the given command matches any pattern in the wildcard_shell_denylist. If it does, it returns False, indicating this command is forbidden. If not, it returns whether the command is not in the non-wildcard denylist.

This change aims to improve the flexibility of the shell command control.

For example previously adding npm start to deny list would have no effect. Since AG only look at the first part of the command here that would simply be npm therefore a match would not happen, unless npm itself is banned completelty, which is not desirable for things that do not require use interaction like npm install. After this PR you can add npm start* to the deny list, it detects the wildcard * character and it tries to match this item in the deny list against the entire command not just the fist part of it. So if the command were npm start --abc it would still be denied.

Example denylist in .env

SHELL_DENYLIST=sudo,su,npm start*

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thoroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes.
  • I have run the following commands against my code to ensure it passes our linters:
    black .
    isort .
    mypy
    autoflake --remove-all-unused-imports --recursive --ignore-init-module-imports --ignore-pass-after-docstring autogpt tests --in-place

@netlify
Copy link

netlify bot commented Aug 27, 2023

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit f1c4ec2
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/64eabda4e60c6700084382be

@codecov
Copy link

codecov bot commented Aug 27, 2023

Codecov Report

Attention: Patch coverage is 33.33333% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 50.73%. Comparing base (5f9cc58) to head (f1c4ec2).
Report is 17 commits behind head on master.

❗ Current head f1c4ec2 differs from pull request most recent head 6d1e9b0. Consider uploading reports for the commit 6d1e9b0 to get more accurate results

Files Patch % Lines
autogpt/commands/execute_code.py 33.33% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5145      +/-   ##
==========================================
+ Coverage   45.51%   50.73%   +5.22%     
==========================================
  Files         139      128      -11     
  Lines        6530     5523    -1007     
  Branches      917      765     -152     
==========================================
- Hits         2972     2802     -170     
+ Misses       3408     2508     -900     
- Partials      150      213      +63     
Flag Coverage Δ
Linux ?
Windows ?
autogpt-agent ?
macOS ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Sep 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@Pwuts Pwuts force-pushed the add-wildcard-support-for-deny-allow-lists branch from f1c4ec2 to 71ee24f Compare March 22, 2024 17:52
@soheil soheil requested a review from a team as a code owner March 22, 2024 17:52
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Mar 22, 2024
Copy link

netlify bot commented Mar 22, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit 6d1e9b0
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/65fdc89db7063000083d5d17

@Pwuts Pwuts force-pushed the add-wildcard-support-for-deny-allow-lists branch from dfa5fad to 4e4f0bf Compare March 22, 2024 18:02
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 22, 2024
@Swiftyos Swiftyos closed this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoGPT Agent conflicts Automatically applied to PRs with merge conflicts function: run shell commands size/m
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants