Accept about: and chrome:// URL submissions as well as http/https. Bu…

…g 600094.

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))

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,

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)

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)