Opened 2 years ago

Closed 6 months ago

#20003 closed Bug (fixed)

URLValidator does not accept urls with usernames or passwords in them

Reported by: marshall@… Owned by: Tim Graham <timograham@…>
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 (23)

comment:1 Changed 2 years ago by marshall@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Sorry, readline munged the display, what you should see for the first two lines are:

In [1]: from django.core.validators import URLValidator

In [2]: URLValidator()('https://user:pass@domain.com')

comment:2 Changed 2 years ago by ptone

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 2 years ago by aaugustin

  • Component changed from Uncategorized to Core (Other)

comment:4 Changed 2 years ago by matiasb

  • Owner changed from nobody to matiasb
  • Status changed from new to assigned

comment:5 Changed 2 years ago by matiasb

  • Has patch set

Proposed update: https://github.com/django/django/pull/998
Tests passing with sqlite.

comment:6 Changed 2 years ago by apollo13

comment:7 Changed 2 years ago by timo

  • Patch needs improvement set

comment:8 Changed 19 months ago by aaugustin

#21532 was a duplicate.

comment:9 Changed 19 months ago by dbrgn

These URLs could be used for a thorough test case: http://mathiasbynens.be/demo/url-regex

comment:10 Changed 19 months ago by dbrgn

  • Cc gezuru@… added

comment:11 Changed 12 months ago by dbrgn

  • Owner matiasb deleted
  • Patch needs improvement unset
  • Status changed from assigned to new

Another attempt at fixing this issue (including an extended test suite): https://github.com/django/django/pull/2873

comment:12 Changed 11 months ago by timgraham

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 Changed 11 months ago by dbrgn

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 Changed 11 months ago by claudep

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 Changed 11 months ago by collinanderson

Are there possibly any security issues with including a username in the url?

comment:16 Changed 11 months ago by dbrgn

@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 Changed 11 months ago by timgraham

  • 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 Changed 11 months ago by dbrgn

  • Patch needs improvement unset

comment:19 Changed 10 months ago by timgraham

  • Patch needs improvement set

Some tests on Python 3 are failing.

comment:20 Changed 8 months ago by dbrgn

  • Patch needs improvement unset

Pull request has been updated. All tests now pass on all supported Python versions.

https://github.com/django/django/pull/2873

comment:21 Changed 7 months ago by loic

Left some comments on the PR.

comment:22 Changed 7 months ago by dbrgn

I left some comments as well.

Any opinions on the proposed RegEx simplifications?

comment:23 Changed 6 months ago by Tim Graham <timograham@…>

  • Owner set to Tim Graham <timograham@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 2e65d56156b622e2393dee1af66e9c799a51924f:

Fixed #20003 -- Improved and extended URLValidator

This adds support for authentication data (user:password) in URLs,
IPv6 addresses, and unicode domains.

The test suite has been improved by adding test URLs from
http://mathiasbynens.be/demo/url-regex (with a few adjustments,
like allowing local and reserved IPs).

The previous URL validation regex failed this test suite on 13
occasions, the validator was updated based on
https://gist.github.com/dperini/729294.

Note: See TracTickets for help on using tickets.
Back to Top