launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02502
[Merge] lp:~sinzui/launchpad/itemswidgets-term-title-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/itemswidgets-term-title-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#693689 canonical.widgets.itemswidgets do not escape term title
https://bugs.launchpad.net/bugs/693689
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/itemswidgets-term-title-1/+merge/48483
Escape the token and title in lp.app.widgets.itemswidgets renderer.
Launchpad bug: https://bugs.launchpad.net/bugs/693689
Pre-implementation: no one
Test command: ./bin/test -vv -t test_itemswidgets
The widgets in lp.app.widgets.itemswidgets do not escape data from the
vocabularies that they are given. Approximately none of the call sites realise
this, often passing in unsanitised data. This is mostly problematic for term
titles derived from user-provided display names, but probably other things as
well.
Some specific examples of this problem that I've tested are:
- the PPA dependency editor (bug #683406)
- the recipe build request form's series list
- +add-my-teams' team list
--------------------------------------------------------------------
RULES
* Update itemswidgets classes to use webapp.menu.escape to sanities
the title and token.
* LaunchpadRadioWidgetWithDescription is different since it is also
working with enum descriptions. Escape the title, token, and
description.
QA
* Create a team with markup in its name.
* Visit another team and choose to +add-my-teams.
* Verify the markup is escaped.
LINT
lib/lp/app/widgets/itemswidgets.py
lib/lp/app/widgets/tests/test_itemswidgets.py
IMPLEMENTATION
Added basic tests for render methods. Added tests for escaped content and
updated the classes. LaunchpadRadioWidgetWithDescription works with enums
instead of the common form of vocabularies, so while the tests look similar
to the other item widgets, all setup and assertions had to be reimplements
lib/lp/app/widgets/itemswidgets.py
lib/lp/app/widgets/tests/test_itemswidgets.py
--
https://code.launchpad.net/~sinzui/launchpad/itemswidgets-term-title-1/+merge/48483
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/itemswidgets-term-title-1 into lp:launchpad.
=== modified file 'lib/lp/app/widgets/itemswidgets.py'
--- lib/lp/app/widgets/itemswidgets.py 2011-02-01 21:03:45 +0000
+++ lib/lp/app/widgets/itemswidgets.py 2011-02-03 14:47:11 +0000
@@ -25,6 +25,8 @@
from lazr.enum import IEnumeratedType
+from canonical.launchpad.webapp.menu import escape
+
class LaunchpadDropdownWidget(DropdownWidget):
"""A Choice widget that doesn't encloses itself in <div> tags."""
@@ -50,6 +52,8 @@
kw = {}
if checked:
kw['checked'] = 'checked'
+ value = escape(value)
+ text = escape(text)
id = '%s.%s' % (name, index)
element = renderElement(
u'input', value=value, name=name, id=id,
@@ -70,6 +74,8 @@
kw = {}
if checked:
kw['checked'] = 'checked'
+ value = escape(value)
+ text = escape(text)
id = '%s.%s' % (name, index)
elem = renderElement(u'input',
value=value,
@@ -94,6 +100,8 @@
kw = {}
if checked:
kw['checked'] = 'checked'
+ value = escape(value)
+ text = escape(text)
id = '%s.%s' % (name, index)
elem = renderElement(u'input',
value=value,
@@ -144,7 +152,7 @@
"""Render the table row for the widget depending on description."""
if form_value != self._missing:
vocab_term = self.vocabulary.getTermByToken(form_value)
- description = vocab_term.value.description
+ description = escape(vocab_term.value.description)
else:
description = None
@@ -156,6 +164,7 @@
def renderItem(self, index, text, value, name, cssClass):
"""Render an item of the list."""
+ text = escape(text)
id = '%s.%s' % (name, index)
elem = renderElement(u'input',
value=value,
@@ -167,6 +176,7 @@
def renderSelectedItem(self, index, text, value, name, cssClass):
"""Render a selected item of the list."""
+ text = escape(text)
id = '%s.%s' % (name, index)
elem = renderElement(u'input',
value=value,
@@ -190,7 +200,7 @@
The `LaunchpadRadioWidget` does the rendering. Only the True-False values
are rendered; a missing value item is not rendered. The default labels
- are rendered as 'yes' and 'no', but can be changed by setting the widget's
+ are rendered as 'yes' and 'no', but can be changed by setting the widget's
true_label and false_label attributes.
"""
=== added file 'lib/lp/app/widgets/tests/test_itemswidgets.py'
--- lib/lp/app/widgets/tests/test_itemswidgets.py 1970-01-01 00:00:00 +0000
+++ lib/lp/app/widgets/tests/test_itemswidgets.py 2011-02-03 14:47:11 +0000
@@ -0,0 +1,217 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from zope.schema import Choice
+from zope.schema.vocabulary import (
+ SimpleTerm,
+ SimpleVocabulary,
+ )
+
+from lazr.enum import (
+ EnumeratedType,
+ Item,
+ )
+
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.widgets.itemswidgets import (
+ LabeledMultiCheckBoxWidget,
+ LaunchpadRadioWidget,
+ LaunchpadRadioWidgetWithDescription,
+ PlainMultiCheckBoxWidget,
+ )
+from lp.testing import (
+ TestCaseWithFactory,
+ )
+
+
+class ItemWidgetTestCase(TestCaseWithFactory):
+ """A test case that sets up an items widget for testing"""
+
+ layer = DatabaseFunctionalLayer
+
+ WIDGET_CLASS = None
+ SAFE_TERM = SimpleTerm('object-1', 'token-1', 'Safe title')
+ UNSAFE_TERM = SimpleTerm('object-2', 'token-2', '<unsafe> title')
+
+ def setUp(self):
+ super(ItemWidgetTestCase, self).setUp()
+ self.request = LaunchpadTestRequest()
+ self.vocabulary = SimpleVocabulary([self.SAFE_TERM, self.UNSAFE_TERM])
+ field = Choice(__name__='test_field', vocabulary=self.vocabulary)
+ self.field = field.bind(object())
+ self.widget = self.WIDGET_CLASS(
+ self.field, self.vocabulary, self.request)
+
+ def assertRenderItem(self, expected, term, checked=False):
+ markup = self.widget._renderItem(
+ index=1, text=term.title, value=term.token,
+ name=self.field.__name__, cssClass=None, checked=checked)
+ self.assertEqual(expected, markup)
+
+
+class TestPlainMultiCheckBoxWidget(ItemWidgetTestCase):
+ """Test the PlainMultiCheckBoxWidget class."""
+
+ WIDGET_CLASS = PlainMultiCheckBoxWidget
+
+ def test__renderItem_checked(self):
+ # Render item in checked state.
+ expected = (
+ '<input class="checkboxType" checked="checked" id="test_field.1" '
+ 'name="test_field" type="checkbox" value="token-1" /> '
+ 'Safe title ')
+ self.assertRenderItem(expected, self.SAFE_TERM, checked=True)
+
+ def test__renderItem_unchecked(self):
+ # Render item in unchecked state.
+ expected = (
+ '<input class="checkboxType" id="test_field.1" name="test_field" '
+ 'type="checkbox" value="token-1" /> Safe title ')
+ self.assertRenderItem(expected, self.SAFE_TERM, checked=False)
+
+ def test__renderItem_unsafe_content(self):
+ # Render item escapes unsafe markup.
+ expected = (
+ '<input class="checkboxType" id="test_field.1" name="test_field" '
+ 'type="checkbox" value="token-2" /> '
+ '<unsafe> &nbsp; title ')
+ self.assertRenderItem(expected, self.UNSAFE_TERM, checked=False)
+
+
+class TestLabeledMultiCheckBoxWidget(ItemWidgetTestCase):
+ """Test the LabeledMultiCheckBoxWidget class."""
+
+ WIDGET_CLASS = LabeledMultiCheckBoxWidget
+
+ def test__renderItem_checked(self):
+ # Render item in checked state.
+ expected = (
+ '<label for="field.test_field.1" style="font-weight: normal">'
+ '<input class="checkboxType" checked="checked" id="test_field.1" '
+ 'name="test_field" type="checkbox" value="token-1" /> '
+ 'Safe title</label> ')
+ self.assertRenderItem(expected, self.SAFE_TERM, checked=True)
+
+ def test__renderItem_unchecked(self):
+ # Render item in unchecked state.
+ expected = (
+ '<label for="field.test_field.1" style="font-weight: normal">'
+ '<input class="checkboxType" id="test_field.1" name="test_field" '
+ 'type="checkbox" value="token-1" /> Safe title</label> ')
+ self.assertRenderItem(expected, self.SAFE_TERM, checked=False)
+
+ def test__renderItem_unsafe_content(self):
+ # Render item escapes unsafe markup.
+ expected = (
+ '<label for="field.test_field.1" style="font-weight: normal">'
+ '<input class="checkboxType" id="test_field.1" name="test_field" '
+ 'type="checkbox" value="token-2" /> '
+ '<unsafe> &nbsp; title</label> ')
+ self.assertRenderItem(expected, self.UNSAFE_TERM, checked=False)
+
+
+class TestLaunchpadRadioWidget(ItemWidgetTestCase):
+ """Test the LaunchpadRadioWidget class."""
+
+ WIDGET_CLASS = LaunchpadRadioWidget
+
+ def test__renderItem_checked(self):
+ # Render item in checked state.
+ expected = (
+ '<label style="font-weight: normal">'
+ '<input class="radioType" checked="checked" id="test_field.1" '
+ 'name="test_field" type="radio" value="token-1" /> '
+ 'Safe title</label>')
+ self.assertRenderItem(expected, self.SAFE_TERM, checked=True)
+
+ def test__renderItem_unchecked(self):
+ # Render item in unchecked state.
+ expected = (
+ '<label style="font-weight: normal">'
+ '<input class="radioType" id="test_field.1" name="test_field" '
+ 'type="radio" value="token-1" /> Safe title</label>')
+ self.assertRenderItem(expected, self.SAFE_TERM, checked=False)
+
+ def test__renderItem_unsafe_content(self):
+ # Render item escapes unsafe markup.
+ expected = (
+ '<label style="font-weight: normal">'
+ '<input class="radioType" id="test_field.1" name="test_field" '
+ 'type="radio" value="token-2" /> '
+ '<unsafe> &nbsp; title</label>')
+ self.assertRenderItem(expected, self.UNSAFE_TERM, checked=False)
+
+
+class TestLaunchpadRadioWidgetWithDescription(TestCaseWithFactory):
+ """Test the LaunchpadRadioWidgetWithDescription class."""
+
+ layer = DatabaseFunctionalLayer
+
+ class TestEnum(EnumeratedType):
+ SAFE_TERM = Item('item-1', description='Safe title')
+ UNSAFE_TERM = Item('item-<2>', description='<unsafe> title')
+
+ def setUp(self):
+ super(TestLaunchpadRadioWidgetWithDescription, self).setUp()
+ self.request = LaunchpadTestRequest()
+ field = Choice(__name__='test_field', vocabulary=self.TestEnum)
+ self.field = field.bind(object())
+ self.widget = LaunchpadRadioWidgetWithDescription(
+ self.field, self.TestEnum, self.request)
+
+ def assertRenderItem(self, expected, method, enum_item):
+ markup = method(
+ index=1, text=enum_item.title, value=enum_item.name,
+ name=self.field.__name__, cssClass=None)
+ markup = ' '.join(markup.split())
+ self.assertEqual(expected, markup)
+
+ def test_renderSelectedItem(self):
+ # Render checked="checked" item in checked state.
+ expected = (
+ '<tr> <td rowspan="2">'
+ '<input class="radioType" checked="checked" id="test_field.1" '
+ 'name="test_field" type="radio" value="SAFE_TERM" /></td> '
+ '<td><label for="test_field.1">item-1</label></td> </tr> '
+ '<tr> <td class="formHelp">Safe title</td> </tr>')
+ self.assertRenderItem(
+ expected, self.widget.renderSelectedItem, self.TestEnum.SAFE_TERM)
+
+ def test_renderItem(self):
+ # Render item in unchecked state.
+ expected = (
+ '<tr> <td rowspan="2">'
+ '<input class="radioType" id="test_field.1" '
+ 'name="test_field" type="radio" value="SAFE_TERM" /></td> '
+ '<td><label for="test_field.1">item-1</label></td> </tr> '
+ '<tr> <td class="formHelp">Safe title</td> </tr>')
+ self.assertRenderItem(
+ expected, self.widget.renderItem, self.TestEnum.SAFE_TERM)
+
+ def test_renderSelectedItem_unsafe_content(self):
+ # Render selected item escapes unsafe markup.
+ expected = (
+ '<tr> <td rowspan="2">'
+ '<input class="radioType" checked="checked" id="test_field.1" '
+ 'name="test_field" type="radio" value="UNSAFE_TERM" /></td> '
+ '<td><label for="test_field.1">item-<2></label></td> </tr> '
+ '<tr> '
+ '<td class="formHelp"><unsafe> &nbsp; title</td> </tr>')
+ self.assertRenderItem(
+ expected,
+ self.widget.renderSelectedItem, self.TestEnum.UNSAFE_TERM)
+
+ def test_renderItem_unsafe_content(self):
+ # Render item escapes unsafe markup.
+ expected = (
+ '<tr> <td rowspan="2">'
+ '<input class="radioType" id="test_field.1" '
+ 'name="test_field" type="radio" value="UNSAFE_TERM" /></td> '
+ '<td><label for="test_field.1">item-<2></label></td> </tr> '
+ '<tr> '
+ '<td class="formHelp"><unsafe> &nbsp; title</td> </tr>')
+ self.assertRenderItem(
+ expected, self.widget.renderItem, self.TestEnum.UNSAFE_TERM)
