launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03084
[Merge] lp:~danilo/launchpad/bug-740640 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/bug-740640 into lp:launchpad with lp:~danilo/launchpad/accordionoverlay as a prerequisite.
Requested reviews:
Launchpad UI Reviewers (launchpad-ui-reviewers): ui
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-740640/+merge/55123
= 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
--
https://code.launchpad.net/~danilo/launchpad/bug-740640/+merge/55123
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-740640 into lp:launchpad.
=== 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.
+ self.ws_subscription_filter.description = u"> <>"
+ 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):
+ 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>')
+
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 @@
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 {
@@ -217,7 +220,33 @@
}
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;
+ }
+};
+
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(
@@ -233,6 +262,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]);
+ };
+};
+
function create_overlay(content_box_id, overlay_id, submit_button,
submit_callback) {
// Create the overlay.
@@ -245,9 +286,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);
@@ -256,8 +303,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);
}
@@ -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'});
+ // 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: < > " &');
+ }
+};
+// 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 @@
// 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 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: < > " &',
+ 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: < > " &',
+ error_text);
+ },
+
+ test_get_error_for_subscription_name_ampersand: function() {
+ // For invalid names get_error_for_subscription_name returns
+ // an error message instead.
+ var name = 'Test & fail';
+ 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: < > " &',
+ error_text);
+ },
+
+ test_get_error_for_subscription_name_quote: function() {
+ // For invalid names get_error_for_subscription_name returns
+ // an error message instead.
+ var name = 'Test "failure"';
+ 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: < > " &',
+ error_text);
+ },
+
+ 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));
+ },
+
+ test_get_error_for_tags_list_uppercase: function() {
+ // Uppercase is not allowed in tags.
+ var tags = 'Tag';
+ 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 ".".',
+ error_text);
+ },
+
+ test_get_error_for_tags_list_invalid_characters: function() {
+ // Anything other than lowercase, digits or '+', '-' and '.'
+ // is invalid in tags.
+ var tags = 'tag#!';
+ 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 ".".',
+ error_text);
+ },
+
+ 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.
+ var tags = 'tag1 +tag2 -tag3 .tag4';
+ 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 ".".',
+ error_text);
+ },
+ });
+ suite.add(test_case);
+
+ test_case = new Y.Test.Case({
name: 'Structural Subscription interaction tests',
_should: {
