launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/revert-xss-workaround into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~danilo/launchpad/revert-xss-workaround/+merge/55738

= Revert XSS fix for 740640 =

Bug 740640 fix was unnecessary since in the meantime lazr.restful has been changed to provide the fix automatically.

== Proposed fix ==

Revert parts of that branch that we don't need anymore.  We only leave tags-validation in the subscription overlay.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py
  lib/lp/bugs/model/bugsubscriptionfilter.py
  lib/lp/registry/javascript/tests/test_structural_subscription.js
  lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py
  lib/lp/registry/javascript/structural-subscription.js
-- 
https://code.launchpad.net/~danilo/launchpad/revert-xss-workaround/+merge/55738
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/revert-xss-workaround into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py'
--- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-03-29 10:52:55 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-03-31 11:57:30 +0000
@@ -183,14 +183,6 @@
         self.assertEqual(
             u"It's late.", self.subscription_filter.description)
 
-    def test_modify_description_xss_safeguard(self):
-        # The description can be modified but raises an
-        # exception if you try potential XSS attack characters.
-        self.ws_subscription_filter.description = u"&gt; <>"
-        error = self.assertRaises(
-            BadRequest, self.ws_subscription_filter.lp_save)
-        self.assertEqual(400, error.response.status)
-
     def test_modify_statuses(self):
         # The statuses field can be modified.
         self.assertEqual(set(), self.subscription_filter.statuses)

=== modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
--- lib/lp/bugs/model/bugsubscriptionfilter.py	2011-03-29 10:52:55 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2011-03-31 11:57:30 +0000
@@ -22,7 +22,6 @@
 from canonical.database.sqlbase import sqlvalues
 from canonical.launchpad import searchbuilder
 from canonical.launchpad.interfaces.lpstorm import IStore
-from lazr.restful.error import expose
 from lp.bugs.enum import BugNotificationLevel
 from lp.bugs.interfaces.bugsubscriptionfilter import IBugSubscriptionFilter
 from lp.bugs.interfaces.bugtask import (
@@ -63,22 +62,7 @@
 
     other_parameters = Unicode()
 
-    _description = Unicode('description')
-
-    def _get_description(self):
-        return self._description
-
-    def _set_description(self, description):
-        has_html_chars = (
-            '<' in description or '>' in description or
-            '"' in description or '&' in description)
-        if has_html_chars:
-            raise expose(ValueError(
-                'BugSubscriptionFilter description cannot contain '
-                'any of <, >, " or &.'))
-        self._description = description
-
-    description = property(_get_description, _set_description)
+    description = Unicode('description')
 
     def _get_statuses(self):
         """Return a frozenset of statuses to filter on."""

=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py	2011-03-29 10:52:55 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py	2011-03-31 11:57:30 +0000
@@ -76,27 +76,6 @@
         bug_subscription_filter.description = u"foo"
         self.assertEqual(u"foo", bug_subscription_filter.description)
 
-    def test_description_xss_safeguard_tags(self):
-        """Test that description property disallows a HTML tags."""
-        bug_subscription_filter = BugSubscriptionFilter()
-        self.assertRaises(ValueError,
-                          setattr, bug_subscription_filter, 'description',
-                          u'<script>')
-
-    def test_description_xss_safeguard_ampersand(self):
-        """description property disallows a few HTML characters."""
-        bug_subscription_filter = BugSubscriptionFilter()
-        self.assertRaises(ValueError,
-                          setattr, bug_subscription_filter, 'description',
-                          u'test & blow up')
-
-    def test_description_xss_safeguard_quote(self):
-        """description property disallows a few HTML characters."""
-        bug_subscription_filter = BugSubscriptionFilter()
-        self.assertRaises(ValueError,
-                          setattr, bug_subscription_filter, 'description',
-                          u'test "double" quotes')
-
     def test_defaults(self):
         """Test the default values of `BugSubscriptionFilter` objects."""
         # Create.

=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js	2011-03-30 11:36:37 +0000
+++ lib/lp/registry/javascript/structural-subscription.js	2011-03-31 11:57:30 +0000
@@ -268,7 +268,6 @@
 
 function setup_overlay_validators(overlay, overlay_id) {
     overlay.field_validators = {
-        name: get_error_for_subscription_name,
         tags: get_error_for_tags_list
     };
     overlay.field_errors = {};
@@ -862,17 +861,6 @@
     });
 };
 
-function get_error_for_subscription_name(value) {
-    if (value.search(/[\<\>\"\&]/) == -1) {
-        return null;
-    } else {
-        return ('Subscription name cannot contain any of the following ' +
-                'characters: &lt; &gt; &quot; &amp;');
-    }
-};
-// Export for testing
-namespace._get_error_for_subscription_name = get_error_for_subscription_name;
-
 function get_error_for_tags_list(value) {
     // See database/schema/trusted.sql valid_name() function
     // which is used to validate a single tag.

=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
--- lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-03-30 11:36:37 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-03-31 11:57:30 +0000
@@ -252,46 +252,6 @@
 
         },
 
-        test_get_error_for_subscription_name_valid: function() {
-            // Valid name makes get_error_for_subscription_name return null.
-            var name = 'Test + name!';
-            Assert.isNull(module._get_error_for_subscription_name(name));
-        },
-
-        assertHasErrorInSubscriptionName: function(subscription_name) {
-            var error_text = module._get_error_for_subscription_name(
-                subscription_name);
-            Assert.isNotNull(error_text);
-            Assert.areEqual(
-                'Subscription name cannot contain any of the following ' +
-                    'characters: &lt; &gt; &quot; &amp;',
-                error_text);
-        },
-
-        test_get_error_for_subscription_name_left_angle_bracket: function() {
-            // For invalid names get_error_for_subscription_name returns
-            // an error message instead.
-            this.assertHasErrorInSubscriptionName('<Test');
-        },
-
-        test_get_error_for_subscription_name_right_angle_bracket: function() {
-            // For invalid names get_error_for_subscription_name returns
-            // an error message instead.
-            this.assertHasErrorInSubscriptionName('Test>');
-        },
-
-        test_get_error_for_subscription_name_ampersand: function() {
-            // For invalid names get_error_for_subscription_name returns
-            // an error message instead.
-            this.assertHasErrorInSubscriptionName('Test & fail');
-        },
-
-        test_get_error_for_subscription_name_quote: function() {
-            // For invalid names get_error_for_subscription_name returns
-            // an error message instead.
-            this.assertHasErrorInSubscriptionName('Test "failure"');
-        },
-
         test_get_error_for_tags_list_valid: function() {
             // Valid tags list is a space-separated list of tags
             // consisting of all lowercase and digits and potentially