diff --git a/apps/feedback/fields.py b/apps/feedback/fields.py new file mode 100644 index 00000000..91786957 --- /dev/null +++ b/apps/feedback/fields.py @@ -0,0 +1,16 @@ +from django.db.models.fields import URLField, CharField +from django.core.validators import URLValidator + +from .validators import ExtendedURLValidator + + +class ExtendedURLField(URLField): + """(Model) URLField that allows about: and chrome: URLs.""" + + def __init__(self, *args, **kwargs): + super(ExtendedURLField, self).__init__(*args, **kwargs) + + # Remove old URL validator, add ours instead. + self.validators = filter(lambda x: not isinstance(x, URLValidator), + self.validators) + self.validators.append(ExtendedURLValidator(verify_exists=verify_exists)) diff --git a/apps/feedback/forms.py b/apps/feedback/forms.py index bd830787..25e5bd64 100644 --- a/apps/feedback/forms.py +++ b/apps/feedback/forms.py @@ -4,17 +4,38 @@ from django import forms from django.conf import settings from django.core.exceptions import ValidationError +from django.core.validators import URLValidator from annoying.decorators import autostrip from tower import ugettext as _, ugettext_lazy as _lazy -from . import FIREFOX, MOBILE, LATEST_BETAS +from . import FIREFOX, MOBILE, LATEST_BETAS, fields from .models import Opinion from .validators import (validate_swearwords, validate_no_html, - validate_no_email, validate_no_urls) + validate_no_email, validate_no_urls, + ExtendedURLValidator) from .version_compare import version_int +class ExtendedURLField(forms.URLField): + """(Forms) URL field allowing about: and chrome: URLs.""" + def __init__(self, *args, **kwargs): + super(ExtendedURLField, self).__init__(*args, **kwargs) + + # Remove old URL validator, add ours instead. + self.validators = filter(lambda x: not isinstance(x, URLValidator), + self.validators) + self.validators.append(ExtendedURLValidator()) + + def to_python(self, value): + """Do not convert about and chorme URLs to fake http addresses.""" + if value and not (value.startswith('about:') or + value.startswith('chrome://')): + return super(ExtendedURLField, self).to_python(value) + else: + return value + + class FeedbackForm(forms.Form): """Feedback form fields shared between feedback types.""" description = forms.CharField(widget=forms.TextInput( @@ -28,7 +49,7 @@ class FeedbackForm(forms.Form): # NB: The ID 'id_url' is hard-coded in the Testpilot extension to # accommodate pre-filling the field client-side. # Do not change unless you know what you are doing. - url = forms.URLField(required=False, widget=forms.TextInput( + url = ExtendedURLField(required=False, widget=forms.TextInput( attrs={'placeholder': 'http://', 'id': 'id_url'})) def clean(self): @@ -46,9 +67,13 @@ def clean(self): def clean_url(self): """Sanitize URL input, remove PWs, etc.""" url = self.cleaned_data['url'] - parsed = urlparse.urlparse(url) - # Note: http/https is already enforced by URL field type. + # Do not mess with about: URLs (bug 600094). + if self.cleaned_data['url'].startswith('about:'): + return self.cleaned_data['url'] + + # Note: http/https/chrome is already enforced by URL field type. + parsed = urlparse.urlparse(url) # Rebuild URL to drop query strings, passwords, and the like. new_url = (parsed.scheme, parsed.hostname, parsed.path, None, None, diff --git a/apps/feedback/tests.py b/apps/feedback/tests.py index 4101120e..341eb2c9 100644 --- a/apps/feedback/tests.py +++ b/apps/feedback/tests.py @@ -1,3 +1,5 @@ +from datetime import datetime + from django import http from django.conf import settings from django.core.exceptions import ValidationError @@ -9,7 +11,7 @@ from . import FIREFOX, MOBILE from .utils import detect_language, ua_parse, smart_truncate -from .validators import validate_no_urls +from .validators import validate_no_urls, ExtendedURLValidator from .version_compare import simplify_version @@ -92,7 +94,7 @@ def test_smart_truncate(self): class ValidatorTests(TestCase): - def test_url(self): + def test_url_in_text(self): """Find URLs in text.""" patterns = ( ('This contains no URLs.', False), @@ -109,6 +111,18 @@ def test_url(self): else: validate_no_urls(pattern[0]) # Will fail if exception raised. + def test_chrome_url(self): + """Make sure URL validator allows chrome and about URLs.""" + v = ExtendedURLValidator() + + # These will fail if validation error is raised. + v('about:blank') + v('chrome://mozapps/content/downloads/downloads.xul') + + # These should fail. + self.assertRaises(ValidationError, v, 'about:') + self.assertRaises(ValidationError, v, 'chrome:bogus') + class VersionCompareTest(TestCase): def test_simplify_version(self): @@ -127,6 +141,8 @@ def test_simplify_version(self): class ViewTests(TestCase): fixtures = ['feedback/opinions'] + FX_UA = ('Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; ' + 'de; rv:1.9.2.3) Gecko/20100401 Firefox/%s') def test_enforce_user_agent(self): """Make sure unknown user agents are forwarded to download page.""" @@ -141,21 +157,20 @@ def test_enforce_user_agent(self): self.assertEquals(r.status_code, 302) # old version: redirect - FX_UA = ('Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; ' - 'de; rv:1.9.2.3) Gecko/20100401 Firefox/%s') r = self.client.get(reverse('feedback.sad'), - HTTP_USER_AGENT=FX_UA % '3.5') + HTTP_USER_AGENT=self.FX_UA % '3.5') self.assertEquals(r.status_code, 302) self.assertTrue(r['Location'].endswith(reverse('feedback.need_beta'))) # latest beta: no redirect - r = self.client.get(reverse('feedback.sad'), HTTP_USER_AGENT=(FX_UA % ( - product_details.firefox_versions['LATEST_FIREFOX_DEVEL_VERSION']))) + r = self.client.get( + reverse('feedback.sad'), HTTP_USER_AGENT=(self.FX_UA % ( + product_details.firefox_versions['LATEST_FIREFOX_DEVEL_VERSION']))) self.assertEquals(r.status_code, 200) # version newer than current: no redirect r = self.client.get(reverse('feedback.sad'), - HTTP_USER_AGENT=(FX_UA % '20.0')) + HTTP_USER_AGENT=(self.FX_UA % '20.0')) self.assertEquals(r.status_code, 200) settings.ENFORCE_USER_AGENT = old_enforce_setting @@ -168,3 +183,32 @@ def test_opinion_detail(self): r = self.client.get(reverse('opinion.detail', args=(29,))) eq_(r.status_code, 200) + def test_url_submission(self): + def submit_url(url, valid=True): + """Submit feedback with a given URL, check if it's accepted.""" + r = self.client.post( + reverse('feedback.sad'), { + # Need to vary text so we don't cause duplicates warnings. + 'description': 'Hello %d' % datetime.now().microsecond, + 'add_url': 'on', + 'positive': 'True', + 'url': url + }, HTTP_USER_AGENT=(self.FX_UA % '20.0'), follow=True) + # Neither valid nor invalid URLs cause anything but a 200 response. + eq_(r.status_code, 200) + if valid: + assert r.content.find('Thanks') >= 0 + assert r.content.find('Enter a valid URL') == -1 + else: + assert r.content.find('Thanks') == -1 + assert r.content.find('Enter a valid URL') >= 0 + + # Valid URL types + submit_url('http://example.com') + submit_url('https://example.com') + submit_url('about:me') + submit_url('chrome://mozapps/content/extensions/extensions.xul') + + # Invalid URL types + submit_url('gopher://something', valid=False) + submit_url('zomg', valid=False) diff --git a/apps/feedback/validators.py b/apps/feedback/validators.py index 52e2bbd8..79fa707c 100644 --- a/apps/feedback/validators.py +++ b/apps/feedback/validators.py @@ -3,6 +3,7 @@ from django.conf import settings from django.core.exceptions import ValidationError +from django.core import validators from django.utils.html import strip_tags from product_details import product_details @@ -71,3 +72,16 @@ def validate_no_urls(str): raise ValidationError( _('Your feedback seems to contain a URL. Please remove this and ' 'similar personal data from the text, then try again. Thanks!')) + + +class ExtendedURLValidator(validators.URLValidator): + """URL validator that allows about: and chrome: URLs.""" + regex = re.compile( + r'^about:[a-z]+$|' # about:something URLs + r'^chrome://[a-z][/?\.\S]+$|' # chrome://... URLs + r'^https?://' # http:// or https:// + r'(?:(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+[A-Z]{2,6}\.?|' # domain... + r'localhost|' # localhost... + r'\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})' # ...or ip + r'(?::\d+)?' # optional port + r'(?:/?|[/?]\S+)$', re.IGNORECASE)