Fixed #26308 -- Prevented crash with binary URLs in is_safe_url() #6242
Conversation
| @@ -4,9 +4,10 @@ Django 1.9.4 release notes | |||
|
|
|||
| *Under development* | |||
|
|
|||
| Django 1.9.4 fixes several bugs in 1.9.3. | |||
| Django 1.9.4 fixes a regression in the 1.9.3 security release. | |||
There was a problem hiding this comment.
If you feel this should be released asap, then let's just say "a regression on Python 2 in the 1.9.3 security release where utils.http.is_safe_url() crashes on bytestring URLs (:ticket:...)" and remove the "Bugfixes" section.
There was a problem hiding this comment.
I find the issue rather critical for any Python 2 deployment, don't you?
| @@ -290,6 +290,7 @@ def is_safe_url(url, host=None): | |||
| url = url.strip() | |||
| if not url: | |||
| return False | |||
| url = force_text(url, errors='replace') | |||
There was a problem hiding this comment.
Do you still want to remove this when dropping Python 2? If there isn't something like six.PY2 will we remember? I guess force_text() still has some value in resolving lazy objects but that's not why it's used here. Just wondering...
| @@ -290,6 +290,8 @@ def is_safe_url(url, host=None): | |||
| url = url.strip() | |||
| if not url: | |||
| return False | |||
| if six.PY2: | |||
| url = force_text(url, errors='replace') | |||
There was a problem hiding this comment.
I'm wondering about errors='replace' here. The function below considers control characters. Is it possible that some of the bad characters we're trying to detect here will be turned into replacement characters, and then overlooked by _is_safe_url below? The Unicode replacement character is U+FFFD, which is category "So".
There was a problem hiding this comment.
The control character issue is that <a href="\x08//example.com"> will redirect to example.com but is_safe_url() would consider it safe before 011a543.
In Firefox and Chrome, <a href="\ufffd//example.com"> will be treated as path relative and redirect to something like http://localhost:8000/page-where-link-appears/%EF%BF%BD//example.com, so I don't see a problem.
This fixes a regression introduced by c5544d2. Thanks John Eskew for the reporti and Tim Graham for the review.
|
For anyone patching other distributions of Django, this fix was amended with 552f038. |
This fixes a regression introduced by c5544d2.