#19070 closed Bug (fixed)
urlize template filter raises exception in some cases
| Reported by: | Owned by: | Sasha Romijn | |
|---|---|---|---|
| Component: | Template system | Version: | dev |
| Severity: | Normal | Keywords: | dceu13 |
| Cc: | eromijn@… | Triage Stage: | Unreviewed |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
In some cases, urlize template filter fails because it's trying to parse ipv6 url and exception is raised:
>>> from django.utils.html import urlize
>>> urlize('www.ee]')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "django/utils/functional.py", line 193, in wrapper
return func(*args, **kwargs)
File "django/utils/html.py", line 213, in urlize
url = smart_urlquote('http://%s' % middle)
File "django/utils/html.py", line 152, in smart_urlquote
scheme, netloc, path, query, fragment = urlsplit(url)
File "/usr/lib/python2.7/urlparse.py", line 183, in urlsplit
raise ValueError("Invalid IPv6 URL")
ValueError: Invalid IPv6 URL
'www.google.com]' also breaks, but 'google.com]' doesn't.
IMHO urlize filter should never raise exceptions. In my case it is used on plaintext user input which we have no control over, and I'm assuming that errors in Django's template rendering are always handled internally and silently. In this case I'd expect urlize() to catch the ValueError from urlsplit and handle it somehow, e.g. by skipping the url.
Happens in both 1.4 as well as master, 1.3 is not affected.
Attachments (3)
Change History (16)
by , 13 years ago
| Attachment: | 19070-1.diff added |
|---|
comment:1 by , 13 years ago
| Component: | Uncategorized → Template system |
|---|---|
| Has patch: | set |
| Triage Stage: | Unreviewed → Accepted |
| Type: | Uncategorized → Bug |
Attached a patch which fixes the specific square bracket issue (note that the issue is only accurate from Python 2.7).
The question remains whether we should swallow exceptions around smart_urlquote. Aymeric?
comment:2 by , 13 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
Patch reviewed and it's RFC.
Perhaps the question about swallowing exceptions shoud be discussed in django-developers or in separate ticket.
comment:3 by , 13 years ago
I'm going to commit the patch since it's the precise fix for this bug and we're getting affected by this - if we want to discuss the general exceptions issue let's do it elsewhere.
comment:4 by , 13 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:5 by , 13 years ago
| Resolution: | fixed |
|---|---|
| Status: | closed → reopened |
With the above patch applied / git HEAD:
>>> from django.utils.html import urlize
>>> urlize('www.ee]')
u'<a href="http://www.ee">www.ee</a>]'
>>> urlize('www[foo.com')
Traceback (most recent call last):
File "<console>", line 1, in <module>
File "/Users/tomi/Projects/feedify/venv/lib/python2.7/site-packages/django/utils/functional.py", line 176, in wrapper
return func(*args, **kwargs)
File "/Users/tomi/Projects/feedify/venv/lib/python2.7/site-packages/django/utils/html.py", line 168, in urlize
url = smart_urlquote('http://%s' % middle)
File "/Users/tomi/Projects/feedify/venv/lib/python2.7/site-packages/django/utils/html.py", line 107, in smart_urlquote
scheme, netloc, path, query, fragment = urlparse.urlsplit(url)
File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urlparse.py", line 182, in urlsplit
raise ValueError("Invalid IPv6 URL")
ValueError: Invalid IPv6 URL
>>>
pull request at https://github.com/django/django/pull/573 / patch attached.
by , 13 years ago
| Attachment: | 19070-2.diff added |
|---|
comment:6 by , 13 years ago
| Resolution: | → fixed |
|---|---|
| Status: | reopened → closed |
comment:7 by , 13 years ago
| Resolution: | fixed |
|---|---|
| Status: | closed → new |
It still seems possible to break urlize in Django 1.5.1:
Testcase:
>>> import django
>>> django.VERSION
(1, 5, 1, 'final', 0)
>>> from django.template.defaultfilters import urlize
>>> urlize('[http://168.192.0.1](http://168.192.0.1)')
Traceback (most recent call last):
File "<console>", line 1, in <module>
File "/Users/EmilStenstrom/Envs/kundo/lib/python2.7/site-packages/django/template/defaultfilters.py", line 45, in _dec
return func(*args, **kwargs)
File "/Users/EmilStenstrom/Envs/kundo/lib/python2.7/site-packages/django/template/defaultfilters.py", line 336, in urlize
return mark_safe(urlize_impl(value, nofollow=True, autoescape=autoescape))
File "/Users/EmilStenstrom/Envs/kundo/lib/python2.7/site-packages/django/utils/functional.py", line 194, in wrapper
return func(*args, **kwargs)
File "/Users/EmilStenstrom/Envs/kundo/lib/python2.7/site-packages/django/utils/html.py", line 212, in urlize
url = smart_urlquote(middle)
File "/Users/EmilStenstrom/Envs/kundo/lib/python2.7/site-packages/django/utils/html.py", line 153, in smart_urlquote
scheme, netloc, path, query, fragment = urlsplit(url)
File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/urlparse.py", line 182, in urlsplit
raise ValueError("Invalid IPv6 URL")
ValueError: Invalid IPv6 URL
comment:8 by , 13 years ago
| Has patch: | unset |
|---|---|
| Triage Stage: | Ready for checkin → Unreviewed |
| Version: | 1.4 → 1.5 |
@EmilStenstrom, I think opening a new ticket would have been the best course of action here since the commits for this ticket were already released, but moving this back to "Unreviewed" for the new issue.
comment:9 by , 13 years ago
On django (1, 6, 0, 'alpha', 0)
I get a different output
u'[<a href="http://168.192.0.1](http://168.192.0.1)" rel="nofollow">http://168.192.0.1](http://168.192.0.1)</a>'
I'm not entirely sure that this result is correct either though..
by , 13 years ago
| Attachment: | test_urlize.diff added |
|---|
This adds a test that shows that the behaviour of urlize described here does not fail on 1.6.0
comment:10 by , 13 years ago
| Has patch: | set |
|---|---|
| Keywords: | dceu13 added |
| Owner: | changed from to |
| Patch needs improvement: | set |
| Status: | new → assigned |
| Version: | 1.5 → master |
As this problem was reported, I think it would be good to add the test to the testsuite. It's a realistic use, and means we can be sure this does not break at any time. I'll clean the patch a little bit and make a PR.
comment:11 by , 13 years ago
| Cc: | added |
|---|---|
| Patch needs improvement: | unset |
Slight patch clean up, PR: https://github.com/django/django/pull/1161
comment:12 by , 13 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
Fixing the square bracket issue