-----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_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.description)
> +
> +    def test_description_xss_safeguard(self):
> +        """description property disallows a few HTML characters."""
> +        bug_subscription_filter = BugSubscriptionFilter()
> +        def set_description(filter, value):
> +            filter.description = value
> +        self.assertRaises(ValueError,
> +                          setattr, bug_subscription_filter, 'description',
> +                          u'<script>')

You are being more thorough on the JS test (testing each character), why not here?

> +
>      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 11:22:14 +0000
> +++ lib/lp/registry/javascript/structural-subscription.js	2011-03-28 11:22:31 +0000
> @@ -207,6 +207,9 @@
> @@ -217,7 +220,33 @@
> @@ -233,6 +262,18 @@
> @@ -256,8 +303,6 @@
> @@ -766,6 +811,67 @@
>      }, 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;
> +            field_container.get('parentNode').get('parentNode').setStyles(
> +                {height: 'auto'});

We discussed this on IRC and I'd prefer to avoid the double parentNode because
that heavily depends on the page structure. (ok, a many things do).
My suggestion add a class to the right containers and do.
  Y.all(overlay_id + ' .classname').setStyles(...);
And please add a comment why this is necessary.

> +            // 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 ".".');
> +    }
> +};
> +// 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
> @@ -1134,7 +1240,6 @@
> === modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
> --- lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-03-28 11:22:14 +0000
> +++ lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-03-28 11:22:31 +0000
> @@ -228,6 +228,123 @@
>      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));
> +        },
> +
> +        test_get_error_for_subscription_name_left_angle_bracket: function() {
> +            // For invalid names get_error_for_subscription_name returns
> +            // an error message instead.
> +            var name = '<Test';
> +            var error_text = module._get_error_for_subscription_name(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_right_angle_bracket: function() {
> +            // For invalid names get_error_for_subscription_name returns
> +            // an error message instead.
> +            var name = 'Test>';
> +            var error_text = module._get_error_for_subscription_name(name);
> +            Assert.isNotNull(error_text);
> +            Assert.areEqual(
> +                'Subscription name cannot contain any of the following ' +
> +                    'characters: &lt; &gt; &quot; &amp;',
> +                error_text);
> +        },

You agreed to make this nicer by avoiding all the repetitive code. Thank you!

[...]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2Qpg4ACgkQBT3oW1L17ihX7wCgihq6nImEZRmxnAFPxbezjrID
u4YAn3jZzffRWJbyoRx8thqOcaybuqBe
=rGVT
-----END PGP SIGNATURE-----