#20003 closed Bug (fixed)
URLValidator does not accept urls with usernames or passwords in them
Reported by: | Owned by: | ||
---|---|---|---|
Component: | Core (Other) | Version: | 1.5 |
Severity: | Normal | Keywords: | URLValidator |
Cc: | gezuru@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In [1]: URLValidator()('https://user:pass@from django.core.validators import URLValidator In [2]: from django.core.validators importURLValidator()('https://user:pass@domain.com') --------------------------------------------------------------------------- ValidationError Traceback (most recent call last) <ipython-input-2-3adf7dbac94c> in <module>() ----> 1 URLValidator()('https://user:pass@domain.com') /Users/marshall/.virtualenvs/django/lib/python2.7/site-packages/django/core/validators.pyc in __call__(self, value) 72 raise e 73 url = urlparse.urlunsplit((scheme, netloc, path, query, fragment)) ---> 74 super(URLValidator, self).__call__(url) 75 else: 76 raise /Users/marshall/.virtualenvs/django/lib/python2.7/site-packages/django/core/validators.pyc in __call__(self, value) 42 """ 43 if not self.regex.search(smart_unicode(value)): ---> 44 raise ValidationError(self.message, code=self.code) 45 46 class URLValidator(RegexValidator): ValidationError: [u'Enter a valid value.']
Change History (24)
comment:1 by , 12 years ago
comment:2 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 12 years ago
Component: | Uncategorized → Core (Other) |
---|
comment:4 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 12 years ago
Has patch: | set |
---|
Proposed update: https://github.com/django/django/pull/998
Tests passing with sqlite.
comment:6 by , 12 years ago
See my comments on the pull request (https://github.com/django/django/pull/998#issuecomment-18097508)
comment:7 by , 11 years ago
Patch needs improvement: | set |
---|
comment:9 by , 11 years ago
These URLs could be used for a thorough test case: http://mathiasbynens.be/demo/url-regex
comment:10 by , 11 years ago
Cc: | added |
---|
comment:11 by , 10 years ago
Owner: | removed |
---|---|
Patch needs improvement: | unset |
Status: | assigned → new |
Another attempt at fixing this issue (including an extended test suite): https://github.com/django/django/pull/2873
comment:12 by , 10 years ago
Florian and I (and some other core devs I think) are wary of continuing to expand the regex for every use case out there. The validator allows specifying a custom regex
and I think we should consider discontinuing "enhancements" to the core regex and let users bring their own as needed. If we go this route, we should beef up the documentation about what is and isn't supported by default.
comment:13 by , 10 years ago
If that is the plan, the documentation really does need an update. The validator claims to validate URLs. Most people who use the URL Validator assume it will validate URLs. But in the current state it is broken and does not. It would be fine if it would consider some invalid URL edge cases valid (meaning it's not too strict), but if it returns a ValidationError for perfectly valid URLs, that's a clear bug to me.
Even if you look online for other ways to validate URLs in Python, you get pointed to the Django implementation: http://stackoverflow.com/q/7160737/284318
I think this is a bug that should be addressed somehow. Either by a fix (possibly with a different implementation than the approach I took) or by a deprecation of the validator.
comment:14 by , 10 years ago
I'm on Danilo's side on this issue, I don't see why we wouldn't improve the regex if we have a good patch with solid tests.
comment:15 by , 10 years ago
Are there possibly any security issues with including a username in the url?
comment:16 by , 10 years ago
@collinanderson I don't think so. The job of the URLValidator is to test whether an URL is valid according to some RFCs, not to decide whether a specific URL is a good idea in a specific case :)
It could have been a security issue if verify_exists
were still available (e.g. because the credentials would show up in network traffic), but as this functionality has been removed in 1.5 that's not a concern anymore.
comment:17 by , 10 years ago
Patch needs improvement: | set |
---|
The proposed change appears vulnerable to the "catastrophic backtracking situation" described in 9f8287a3f145fe5cbe71ef5573ea8898404727ad as the test added there now hangs with the proposed changes. This is one reason I'm reluctant to add regex complexity.
comment:18 by , 10 years ago
Patch needs improvement: | unset |
---|
I think I fixed it: https://github.com/django/django/pull/2873#issuecomment-52542276
comment:20 by , 10 years ago
Patch needs improvement: | unset |
---|
Pull request has been updated. All tests now pass on all supported Python versions.
comment:22 by , 10 years ago
I left some comments as well.
Any opinions on the proposed RegEx simplifications?
comment:23 by , 10 years ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Sorry, readline munged the display, what you should see for the first two lines are: