Merge lp:~danilo/launchpad/bug-740640 into lp:launchpad

[Данило Шеган]

[r=henninge][ui=henninge][bug=740640] Protect against XSS in structural subscription overlay and add nicer client-side validation.

= Bug 740640 =

At the moment, structural subscription overlay allows XSS attacks.

We should stop them at the server-side and provide nice JS-based validation to inform the users of the conditions.

Since we want similar approach for client-side tags validation, this branch implements that as well.

== Pre-implementation notes ==

I've discussed approach to solving this with Gary: allowing everything in the subscription name, while probably the best approach, would complicate implementation in that existing infrastructure just populates the data from the server-side objects directly. Thus, we decided on black-listing potential XSS-attack characters for subscription name.

== Implementation details ==

For server-side, we implement "description" property through setter and getter.

Server-side changes:
 - bugs/model/bugsubscriptionfilter.py
 - bugs/browser/tests/test_bugsubscriptionfilter.py
 - bugs/model/tests/test_bugsubscriptionfilter.py

Client-side changes:
 - All the rest

== Tests ==

  bin/test -cvvt BugSubscriptionFilter.*description
  lib/lp/registry/javascript/tests/test_structural_subscription.html

== Demo and Q/A ==

See also https://devpad.canonical.com/~danilo/screencasts/inline-validation-error.ogv

  1. Go to https://launchpad.dev/+feature-rules and add "advanced-structural-subscriptions.enabled default 10 on"
  2. Go to http://launchpad.dev/firefox and click on "Subscribe to bug mail" (on the right) and try entering "<", ">", "&" or "\"" into the subscription name: check gets triggered on the 'change' event, which means when you focus on the next element
  3. Similarly, expand "more options" and try adding a few invalid tags (valid tags are "[a-z0-9][a-z0-9+.-]*", i.e. all lowercase or digits with +, ., -)
  4. Try saving with the errors still present

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/templates/product-index.pt
  lib/lp/registry/help/structural-subscription-name.html
  lib/lp/registry/help/structural-subscription-tags.html
  lib/lp/bugs/browser/structuralsubscription.py
  lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py
  lib/lp/registry/javascript/tests/test_structural_subscription.html
  lib/lp/app/javascript/lp.js
  lib/lp/registry/browser/product.py
  lib/lp/bugs/browser/tests/test_expose.py
  lib/lp/bugs/model/bugsubscriptionfilter.py
  lib/lp/registry/templates/product-portlet-license-missing.pt
  lib/lp/bugs/templates/bug-subscription-list.pt
  lib/lp/bugs/templates/bugtarget-subscription-list.pt
  lib/lp/services/inlinehelp/javascript/inlinehelp.js
  lib/lp/registry/javascript/tests/test_structural_subscription.js
  lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py
  lib/lp/registry/javascript/structural-subscription.js
  buildout-templates/bin/combine-css.in
  lib/lp/app/javascript/tests/test_accordionoverlay.html

./lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py
      82: E301 expected 1 blank line, found 0

[Henning Eggers (henninge)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Danilo,
thanks for this important fix, I am sorry it took a bit longer to review it.
Also thanks for the discussion on IRC, I really enjoyed getting behind what
this code does (and I almost fully succeeded ... ;-)

With the issues solved as discussed, this is good to land code-wise. Please
consider my other two suggestions below.

 review approve code

Cheers,
Henning

Am 28.03.2011 13:22, schrieb Данило Шеган:
> === modified file 'lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py'
> --- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-02-18 18:16:34 +0000
> +++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-03-28 11:22:31 +0000
> @@ -183,6 +183,15 @@
> self.assertEqual(
> u"It's late.", self.subscription_filter.description)
>
> + def test_modify_description_xss_safeguard(self):
> + # The description can be modified.
> +
> + # Modify, save, and start a new transaction.

Oops, something wrong with the comments here ...

> + 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-22 14:53:26 +0000
> +++ lib/lp/bugs/model/bugsubscriptionfilter.py 2011-03-28 11:22:31 +0000
> @@ -22,6 +22,7 @@
> 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 (
> @@ -62,7 +63,20 @@
>
> other_parameters = Unicode()
>
> - description = Unicode()
> + _description = Unicode('description')
> +
> + def _get_description(self):
> + return self._description
> +
> + def _set_description(self, description):
> + if ('<' in description or '>' in description or
> + '"' in description or '&' in description):

I don't like multi-line conditions. Can you please use an intermediate
variable? Ore would a regular expression be too expensive here?
re.find('[<>"&]', description), I think.

> + raise expose(ValueError(
> + 'BugSubscriptionFilter description cannot contain '
> + 'any of <, >, " or &.'))
> + self._description = description
> +
> + description = property(_get_description, _set_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-21 18:20:09 +0000
> +++ lib/lp/bugs/model/tests/test_bugsubscriptionf...

review: Approve (code)

[Henning Eggers (henninge)]

The only UI issue I see is the question if the error text for the tags is correct. Should it not mention that tags can only begin with an alphanumeric character? Otherwise we might have users wondering what they are doing wrong by entering ".foo".

review: Approve (ui,code)

[Данило Шеган (danilo)]

У пон, 28. 03 2011. у 15:24 +0000, Henning Eggers пише:
> Review: Approve code
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Danilo,
> thanks for this important fix, I am sorry it took a bit longer to review it.
> Also thanks for the discussion on IRC, I really enjoyed getting behind what
> this code does (and I almost fully succeeded ... ;-)
>
> With the issues solved as discussed, this is good to land code-wise. Please
> consider my other two suggestions below.
>
> review approve code
>
> Cheers,
> Henning
>
>
> Am 28.03.2011 13:22, schrieb Данило Шеган:
> > === modified file 'lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py'
> > --- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-02-18 18:16:34 +0000
> > +++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py 2011-03-28 11:22:31 +0000
> > @@ -183,6 +183,15 @@
> > self.assertEqual(
> > u"It's late.", self.subscription_filter.description)
> >
> > + def test_modify_description_xss_safeguard(self):
> > + # The description can be modified.
> > +
> > + # Modify, save, and start a new transaction.
>
> Oops, something wrong with the comments here ...

Fixed :)

> > === modified file 'lib/lp/bugs/model/bugsubscriptionfilter.py'
> > --- lib/lp/bugs/model/bugsubscriptionfilter.py 2011-03-22 14:53:26 +0000
> > +++ lib/lp/bugs/model/bugsubscriptionfilter.py 2011-03-28 11:22:31 +0000

> > @@ -62,7 +63,20 @@
> >
> > other_parameters = Unicode()
> >
> > - description = Unicode()
> > + _description = Unicode('description')
> > +
> > + def _get_description(self):
> > + return self._description
> > +
> > + def _set_description(self, description):
> > + if ('<' in description or '>' in description or
> > + '"' in description or '&' in description):
>
> I don't like multi-line conditions. Can you please use an intermediate
> variable? Ore would a regular expression be too expensive here?
> re.find('[<>"&]', description), I think.

I did consider using the re module (but for a very short while). It's
not about multi-line for me though, but mostly about performance: this
would do four passes over the string, and I suspect re would be even
faster for long strings. However, with short strings (like descriptions
will be), burden of loading the re module and compiling the expression
is probably more than that.

I'm happy to put it into an intermediate variable though, so I did that.

> > === modified file 'lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py'
> > --- lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py 2011-03-21 18:20:09 +0000
> > +++ lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py 2011-03-28 11:22:31 +0000
> > @@ -70,6 +70,21 @@
> > self.assertEqual(u"foo", bug_subscription_filter.other_parameters)
> > self.assertEqual(u"bar", bug_subscription_filter.description)
> >
> > + def test_description(self):
> > + """Test the description property."""
> > + bug_subscription_filter = BugSubscriptionFilter()
> > + bug_subscription_filter.description = u"foo"
> > + self.assertEqual(u"foo", bug_subscription_filter.descrip...

=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py'
--- lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-02-18 18:16:34 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py	2011-03-29 13:11:26 +0000
@@ -183,6 +183,14 @@
         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-23 15:55:44 +0000
+++ lib/lp/bugs/model/bugsubscriptionfilter.py	2011-03-29 13:11:26 +0000
@@ -22,6 +22,7 @@
 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 (
@@ -62,7 +63,22 @@
 
     other_parameters = Unicode()
 
-    description = 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)
 
     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-21 18:23:31 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py	2011-03-29 13:11:26 +0000
@@ -70,6 +70,33 @@
         self.assertEqual(u"foo", bug_subscription_filter.other_parameters)
         self.assertEqual(u"bar", bug_subscription_filter.description)
 
+    def test_description(self):
+        """Test the description property."""
+        bug_subscription_filter = BugSubscriptionFilter()
+        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-28 19:24:42 +0000
+++ lib/lp/registry/javascript/structural-subscription.js	2011-03-29 13:11:26 +0000
@@ -208,6 +208,9 @@
 
 function save_subscription(form_data) {
     var who;
+    var has_errors = check_for_errors_in_overlay(add_subscription_overlay);
+    if (has_errors)
+        return false;
     if (form_data.recipient[0] === 'user') {
         who = LP.links.me;
     } else {
@@ -218,10 +221,36 @@
 }
 namespace.save_subscription = save_subscription;
 
+function check_for_errors_in_overlay(overlay) {
+    var has_errors = false;
+    var errors = new Array();
+    for (var field in overlay.field_errors) {
+        if (overlay.field_errors[field]) {
+            has_errors = true;
+            errors.push(field);
+        }
+    };
+    if (has_errors) {
+        var error_text = errors.pop()
+        if (errors.length > 0) {
+            error_text = errors.join(', ') + ' and ' + error_text;
+        }
+
+        overlay.showError(
+            'Value for ' + error_text + ' is invalid.');
+        return true;
+    } else {
+        return false;
+    }
+};
+
 /**
  * Handle the activation of the edit subscription link.
  */
 function edit_subscription_handler(context, form_data) {
+    var has_errors = check_for_errors_in_overlay(add_subscription_overlay);
+    if (has_errors)
+        return false;
     var on = {success: function (new_data) {
         var filter = new_data.getAttrs();
         var description_node = Y.one(
@@ -237,6 +266,18 @@
     patch_bug_filter(context.filter_info.filter, form_data, on);
 }
 
+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 = {};
+    for (var field_name in overlay.field_validators) {
+        add_input_validator(overlay, overlay_id, field_name,
+                            overlay.field_validators[field_name]);
+    };
+};
+
 /**
  * Populate the overlay element with the contents of the add/edit form.
  */
@@ -252,9 +293,15 @@
         visible: false,
         form_submit_button: submit_button,
         form_cancel_button: Y.Node.create(cancel_button_html),
-        form_submit_callback: submit_callback
+        form_submit_callback: function(formdata) {
+            // Do not clean up if saving was not successful.
+            var save_succeeded = submit_callback(formdata);
+            if (save_succeeded !== false)
+                clean_up();
+        }
     });
     add_subscription_overlay.render(content_box_id);
+    setup_overlay_validators(add_subscription_overlay, overlay_id);
     // Prevent cruft from hanging around upon closing.
     function clean_up(e) {
         var filter_wrapper = Y.one('#' + FILTER_WRAPPER);
@@ -263,8 +310,6 @@
     }
     add_subscription_overlay.get('form_cancel_button').on(
         'click', clean_up);
-    add_subscription_overlay.get('form_submit_button').on(
-        'click', clean_up);
     add_subscription_overlay.on('cancel', clean_up);
 }
 
@@ -396,6 +441,11 @@
     };
 }
 
+/* We want to call 'resize' directly on tags container
+ * when we add a validation failed error message.
+ */
+var tags_container;
+
 /**
  * Create the accordion.
  *
@@ -426,6 +476,8 @@
         contentHeight: {method: "auto"}
     } );
 
+    tags_container = tags_ai;
+
     tags_ai.set("bodyContent",
         '<div>\n' +
         '<div>\n' +
@@ -774,6 +826,70 @@
     }, window);
 };
 
+/*
+ * Set up a validator function for a form input field.
+ * @method add_input_validator
+ * @param {Object} overlay Overlay object.
+ * @param {String} overlay_id Element ID of the containing overlay.
+ * @param {String} field_name Form <input> 'name' to set up a validator for.
+ * @param {String} validator Function which returns 'null' if there is
+      no error in the field value, and an error message otherwise.
+ */
+function add_input_validator(overlay, overlay_id, field_name, validator) {
+    var input = Y.one(overlay_id + ' input[name="'+field_name+'"]');
+    var field_container = input.get('parentNode');
+    var error_container = Y.Node.create('<div class="inline-warning"></div>');
+    field_container.appendChild(error_container);
+
+    input.on('change', function(e) {
+        var error_text = validator(input.get('value'));
+        if (error_text !== null) {
+            Y.lazr.anim.red_flash({node: input}).run();
+            error_container.setContent(error_text);
+            overlay.field_errors[field_name] = true;
+            // Accordion sets fixed height for the accordion item,
+            // so we have to resize the tags container.
+            if (field_name == 'tags')
+                tags_container.resize();
+            // Firefox prohibits focus from inside the 'focus lost' event
+            // handler (probably to stop loops), so we need to run
+            // it from a different context (which we do with setTimeout).
+            setTimeout(function() { input.focus(); input.select(); }, 1);
+        } else {
+            error_container.setContent('');
+            overlay.field_errors[field_name] = false;
+        }
+    });
+};
+
+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.
+    // As an extension, we also allow "-" (hyphen) in front of
+    // any tag to indicate exclusion of a tag, and we accept
+    // a space-separated list.
+    if (value.match(/^(\-?[a-z0-9][a-z0-9\+\.\-]*[ ]*)*$/) !== null) {
+        return null;
+    } else {
+        return ('Tags can only contain lowercase ASCII letters, ' +
+                'digits 0-9 and symbols "+", "-" or ".", and they ' +
+                'must start with a lowercase letter or a digit.');
+    }
+};
+// Export for testing
+namespace._get_error_for_tags_list = get_error_for_tags_list;
+
 function get_input_by_value(node, value) {
     // XXX broken: this should also care about input name because some values
     // repeat in other areas of the form
@@ -1180,7 +1296,6 @@
     // initialized except for the ones we added, so setupHelpTrigger
     // is idempotent.  Notice that this is old MochiKit code.
     forEach(findHelpLinks(), setupHelpTrigger);
-
 };               // setup
 
 }, '0.1', {requires: [
 
=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
--- lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-03-28 19:24:42 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-03-29 13:11:26 +0000
@@ -235,6 +235,102 @@
     suite.add(test_case);
 
     test_case = new Y.Test.Case({
+        name: 'Structural Subscription validation tests',
+
+        _should: {
+            error: {
+                }
+        },
+
+        setUp: function() {
+            // Monkeypatch LP to avoid network traffic and to allow
+            // insertion of test data.
+            window.LP = {
+                links: {},
+                cache: {}
+            };
+
+        },
+
+        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
+            // '+', '-', '.' in non-initial characters.
+            var tags = 'tag1 tag+2  tag.3 tag-4 5tag';
+            Assert.isNull(module._get_error_for_tags_list(tags));
+        },
+
+        assertHasErrorInTagsList: function(tags) {
+            var error_text = module._get_error_for_tags_list(tags);
+            Assert.isNotNull(error_text);
+            Assert.areEqual(
+                'Tags can only contain lowercase ASCII letters, ' +
+                    'digits 0-9 and symbols "+", "-" or ".", and they ' +
+                    'must start with a lowercase letter or a digit.',
+                error_text);
+        },
+
+
+        test_get_error_for_tags_list_uppercase: function() {
+            // Uppercase is not allowed in tags.
+            this.assertHasErrorInTagsList('Tag');
+        },
+
+        test_get_error_for_tags_list_invalid_characters: function() {
+            // Anything other than lowercase, digits or '+', '-' and '.'
+            // is invalid in tags.
+            this.assertHasErrorInTagsList('tag#!');
+        },
+
+        test_get_error_for_tags_list_special_characters: function() {
+            // Even if '+', '-' or '.' are allowed in tags,
+            // they must not be at the beginning of a tag.
+            this.assertHasErrorInTagsList('tag1 +tag2 -tag3 .tag4');
+        },
+    });
+    suite.add(test_case);
+
+    test_case = new Y.Test.Case({
         name: 'Structural Subscription interaction tests',
 
         _should: {