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

URL validator: accept user/password, use urllib.urlparse #847

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ianw
Copy link

@ianw ianw commented Jun 24, 2024

I was hitting an issue in a build tool that was not letting me specify a URL to clone a git tree with a personal access token (e.g. [1]) in a wtform URL field.

I started looking at expanding the original regex, but there are tricks like multiple "@"'s in passwords that are hard to get right. I think that for this purpose, urllib.urlparse (urlparse/urlsplit doesn't seem to matter here) will just "do the right thing".

The test-cases are expanded with some coverage of username/passwords.

[1] https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html#clone-repository-using-personal-access-token

I was hitting an issue in a build tool that was not letting me specify
a URL to clone a git tree with a personal access token (e.g. [1]) in a
wtform URL field.

I started looking at expanding the original regex, but there are
tricks like multiple "@"'s in passwords that are hard to get right.  I
think that for this purpose, urllib.urlparse (urlparse/urlsplit
doesn't seem to matter here) will just "do the right thing".

The test-cases are expanded with some coverage of username/passwords.

[1] https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html#clone-repository-using-personal-access-token
@ThiefMaster
Copy link

This absolutely needs to be opt-in. In most cases people do NOT want to accept such URLs.

@ianw
Copy link
Author

ianw commented Jun 24, 2024

This absolutely needs to be opt-in. In most cases people do NOT want to accept such URLs.

I think my concern with that would be the alternative case, where you have opted-out, then implies that the URL will be sanitised in some way. There's certainly been CVE level issues in things like urlparse with things like newlines being translated through incoming URLs etc.

So my counter argument would be to just not play that game at all -- have this as a RFC-level validator around URLs and leave security up to the app when it has what it knows is a valid url?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants