Fixed #29528 -- Made URLValidator reject invalid characters in the username and password. #10097
Conversation
There was a problem hiding this comment.
Hi @timb07.
I think we need to call this out as a Backwards Incompatible change in releases/2.2.txt. (The change may be correct but people may be relying on the existing behaviour already.)
| @@ -94,7 +94,7 @@ class URLValidator(RegexValidator): | |||
|
|
|||
| regex = _lazy_re_compile( | |||
| r'^(?:[a-z0-9\.\-\+]*)://' # scheme is validated separately | |||
| r'(?:\S+(?::\S*)?@)?' # user:pass authentication | |||
| r'(?:[^\s:@/]+(?::[^\s:@/]*)?@)?' # user:pass authentication | |||
There was a problem hiding this comment.
Is it worth adding an invalid URL for each of the cases here (beyond / which is already added)?
There was a problem hiding this comment.
I don't feel particularly strongly either way.
There was a problem hiding this comment.
I'd say that we shouldn't be able to make incorrect changes to the regex without test failures. For example, the @ can be removed from the first group without failures and the second group can be reverted to \S without failures.
There was a problem hiding this comment.
I've added some test cases that should cover all possible regressions in this regex change.
| @@ -48,7 +48,6 @@ http://foo.bar/?q=Test%20URL-encoded%20stuff | |||
| http://مثال.إختبار | |||
| http://例子.测试 | |||
| http://उदाहरण.परीक्षा | |||
| http://-.~_!$&'()*+,;=:%40:80%2f::::::@example.com | |||
There was a problem hiding this comment.
Could we leave this, but just remove the offending ::::::?
There was a problem hiding this comment.
One of the other colons (in =:%40 or %40:80) would also need to be removed for this test case to pass.
|
It's a bug fix so it doesn't need a release note. An important consideration in a regex change like this is that it doesn't introduce the possibility of catastrophic backtracking. As far as I can tell, that's not the case since it's merely replace |
timgraham
changed the title
|
Hey @timb07 — Did you have any appetite for making the test changes here, or did you think them unnecessary, (or something else... 🙂)? (We should be able to wrap this up: let me know either way.) |
|
Hey @timb07. Thanks for making the extra changes. This looks great. 👍 |
This PR fixes issue 29528, and adds two new URLs to the test cases. It also removes one URL from the
valid_urls.txtfile that is invalid, and that this fix now correctly rejects.URLs with un-escaped "/", "@" or ":" characters in the username or password fields should be rejected. This fix replaces the
\Sset with its equivalent[^\s], and then adds the above characters to the exclusion set, giving[^\s:@/].