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.

Sign up for GitHub

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

Jump to bottom

Validate URLs in web commands before execution #2616

Merged
merged 11 commits into from
Apr 24, 2023
Merged

Validate URLs in web commands before execution #2616

merged 11 commits into from
Apr 24, 2023

Conversation

edcohen08
Copy link
Contributor

Background

This pr targets #1887 to validate urls before browsing. It was implemented in web_requests get_response but not for selenium or git.

Changes

Adds a validator for urls that just builds off of the validation that existed in get_response and adds it as a wrapper to add to commands that have a url as the first param.

Documentation

Documented in code

Test Plan

Still passes test suite and tested locally

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

hdkiller
hdkiller previously approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@hdkiller hdkiller left a comment

Choose a reason for hiding this comment

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

I've also tested this PR and it's works for me.

ntindle
ntindle reviewed Apr 20, 2023
View reviewed changes
autogpt/url_utils/validators.py
Comment on lines +65 to +100
local_prefixes = [
"file:///",
"file://localhost/",
"file://localhost",
"http://localhost",
"http://localhost/",
"https://localhost",
"https://localhost/",
"http://2130706433",
"http://2130706433/",
"https://2130706433",
"https://2130706433/",
"http://127.0.0.1/",
"http://127.0.0.1",
"https://127.0.0.1/",
"https://127.0.0.1",
"https://0.0.0.0/",
"https://0.0.0.0",
"http://0.0.0.0/",
"http://0.0.0.0",
"http://0000",
"http://0000/",
"https://0000",
"https://0000/",
]
Copy link
Member

@ntindle ntindle Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In a separate PR may want to drop everything that has a scheme that's not in [http, https, ws, git].

See the list here https://en.wikipedia.org/wiki/List_of_URI_schemes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree with that

@edcohen08 edcohen08 mentioned this pull request Apr 23, 2023
Closed
1 task
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 23, 2023
@github-actions
Copy link
Contributor

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

@edcohen08 edcohen08 force-pushed the validate-url-for-browse branch from ed95b82 to b84a561 Compare April 23, 2023 21:12
@codecov
Copy link

codecov bot commented Apr 23, 2023 •
edited
Loading

Codecov Report

Patch coverage: 72.22% and project coverage change: -7.76 ⚠️

Comparison is base (794a164) 49.40% compared to head (1435663) 41.64%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2616      +/-   ##
==========================================
- Coverage   49.40%   41.64%   -7.76%     
==========================================
  Files          63       64       +1     
  Lines        3004     3021      +17     
  Branches      494      505      +11     
==========================================
- Hits         1484     1258     -226     
- Misses       1400     1698     +298     
+ Partials      120       65      -55
Impacted Files Coverage Δ
autogpt/commands/git_operations.py 0.00% <0.00%> (ø)
autogpt/commands/web_selenium.py 0.00% <0.00%> (ø)
autogpt/url_utils/validators.py 76.92% <76.92%> (ø)
autogpt/app.py 31.68% <100.00%> (-15.26%) ⬇️
autogpt/commands/web_requests.py 81.25% <100.00%> (+9.37%) ⬆️

... and 13 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Apr 23, 2023
@github-actions
Copy link
Contributor

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

@Pwuts Pwuts linked an issue Apr 23, 2023 that may be closed by this pull request
Validate URL before executing browse_website #1887
Closed
1 task
@Pwuts Pwuts self-assigned this Apr 23, 2023
@Pwuts
Copy link
Member

Pwuts commented Apr 23, 2023

This will probably fix a lot of browsing issues, as the chrome driver will exit with really ugly errors even if the URL doesn't contain a schema.

@Pwuts Pwuts changed the title validate urls before any web ops Validate URLs in web commands before execution Apr 23, 2023
@github-actions github-actions bot removed the size/l label Apr 24, 2023
@Pwuts Pwuts merged commit 40a75c8 into Significant-Gravitas:master Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntindle ntindle ntindle left review comments

@Pwuts Pwuts Pwuts approved these changes

@hdkiller hdkiller hdkiller left review comments

Assignees

@Pwuts Pwuts

Labels
size/xl
Projects
AutoGPT development kanban
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate URL before executing browse_website
4 participants
@edcohen08 @Pwuts @ntindle @hdkiller