-
Notifications
You must be signed in to change notification settings - Fork 43.1k
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
feat(agent): Add URL whitelisting/blacklisting to config and URL validation #6848
base: master
Are you sure you want to change the base?
Conversation
β Deploy Preview for auto-gpt-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
remove falcon from the PR |
### White/Black Listing URLS | ||
################################################################################ | ||
##WEB_POLICY - Specifies whether the url list is black list(0) or a whilte list(1) | ||
#WEB_POLICY=0 |
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.
I think we need a policy for users who want to continue using AutoGPT as normal. This looks like it forces them to choose between whitelisting and blacklisting, which some users might not want either. There needs to be a default option that simply disables this feature.
##WEB_POLICY - Specifies whether the url list is black list(0) or a whilte list(1) | ||
#WEB_POLICY=0 | ||
##URL_LIST - Comma separated URLs to be black or whitelisted, sets according to WEB_POLICY value | ||
#URL_LIST=https://www.google.com,http://www.google.com |
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.
The only thing I don't like about putting it here, is that these lists might get pretty long. That's why in my version I was attempting to parse files.
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.
This still needs some work and testing before we can consider merging it
##WEB_POLICY - Specifies whether the url list is black list(0) or a whilte list(1) | ||
#WEB_POLICY=0 |
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.
- Formatting diverges from the rest of the file
- Rather make this an enum with values
blacklist
andwhitelist
# White/Black Listing URLs | ||
web_policy: Optional[str] = os.getenv("WEB_POLICY", 0) | ||
url_list: Optional[list] = os.getenv("URL_LIST",[]).split(",") |
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.
This is not how the config of the application works, as you can see from how all the other config attributes are defined.
|
||
P = ParamSpec("P") | ||
T = TypeVar("T") | ||
|
||
config = Config |
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.
this won't work
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.
@Pwuts can you please help with why this wont work?
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.
from autogpt.config import ConfigBuilder
config = ConfigBuilder.build_config_from_env()
web_policy = config.web_policy
url_list = config.url_list
will this work? @Pwuts
@@ -2,10 +2,11 @@ | |||
import re | |||
from typing import Any, Callable, ParamSpec, TypeVar | |||
from urllib.parse import urljoin, urlparse | |||
from autogpt.config import Config |
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.
this is gonna throw a linting error
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.
Added flake8 to VS code, sorry there was some issue with flake earlier.
raise ValueError("URL Not Whitelisted") | ||
elif url in url_list: | ||
raise ValueError("URL Blacklisted.") |
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.
inconsistent formatting
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.
I have fixed the linting this update the PR with the above config changes @Pwuts
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
Background
This PR refers to the solution for enhancement suggested here: #5289
This basically takes account white/black listing of URLs during context setting in agents.
Changes ποΈ
I have added 2 env variables for web policy and URL list. With a combination of both, a URL is white or blacklisted by the read_webpage function.
PR Quality Scorecard β¨
+2 pts
+5 pts
+5 pts
+5 pts
-4 pts
+4 pts
+5 pts
-5 pts
agbenchmark
to verify that these changes do not regress performance? β+10 pts