Merge lp:~danilo/launchpad/bug-740640 into lp:launchpad
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Данило Шеган | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 12691 | ||||
Proposed branch: | lp:~danilo/launchpad/bug-740640 | ||||
Merge into: | lp:launchpad | ||||
Prerequisite: | lp:~danilo/launchpad/accordionoverlay | ||||
Diff against target: |
395 lines (+267/-5) 5 files modified
lib/lp/bugs/browser/tests/test_bugsubscriptionfilter.py (+8/-0) lib/lp/bugs/model/bugsubscriptionfilter.py (+17/-1) lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py (+27/-0) lib/lp/registry/javascript/structural-subscription.js (+119/-4) lib/lp/registry/javascript/tests/test_structural_subscription.js (+96/-0) |
||||
To merge this branch: | bzr merge lp:~danilo/launchpad/bug-740640 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Henning Eggers (community) | ui,code | Approve | |
Registry Administrators | ui | Pending | |
Review via email: mp+55123@code.launchpad.net |
Commit message
[r=henninge]
Description of the change
= 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/
- bugs/browser/
- bugs/model/
Client-side changes:
- All the rest
== Tests ==
bin/test -cvvt BugSubscription
lib/lp/
== Demo and Q/A ==
See also https:/
1. Go to https:/
2. Go to http://
3. Similarly, expand "more options" and try adding a few invalid tags (valid tags are "[a-z0-
4. Try saving with the errors still present
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
buildout-
lib/lp/
./lib/lp/
82: E301 expected 1 blank line, found 0
-----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 Данило Шеган: bugs/browser/ tests/test_ bugsubscription filter. py' bugs/browser/ tests/test_ bugsubscription filter. py 2011-02-18 18:16:34 +0000 bugs/browser/ tests/test_ bugsubscription filter. py 2011-03-28 11:22:31 +0000 on_filter. description) description_ xss_safeguard( self):
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -183,6 +183,15 @@
> self.assertEqual(
> u"It's late.", self.subscripti
>
> + def test_modify_
> + # 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"> <>" subscription_ filter. lp_save) l(400, error.response. status) statuses( self): l(set() , self.subscripti on_filter. statuses) bugs/model/ bugsubscription filter. py' bugs/model/ bugsubscription filter. py 2011-03-22 14:53:26 +0000 bugs/model/ bugsubscription filter. py 2011-03-28 11:22:31 +0000 database. sqlbase import sqlvalues launchpad. interfaces. lpstorm import IStore Level interfaces. bugsubscription filter import IBugSubscriptio nFilter interfaces. bugtask import ( 'description' ) n(self) : n(self, description):
> + error = self.assertRaises(
> + BadRequest, self.ws_
> + self.assertEqua
> +
> def test_modify_
> # The statuses field can be modified.
> self.assertEqua
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -22,6 +22,7 @@
> from canonical.
> from canonical.launchpad import searchbuilder
> from canonical.
> +from lazr.restful.error import expose
> from lp.bugs.enum import BugNotification
> from lp.bugs.
> from lp.bugs.
> @@ -62,7 +63,20 @@
>
> other_parameters = Unicode()
>
> - description = Unicode()
> + _description = Unicode(
> +
> + def _get_descriptio
> + return self._description
> +
> + def _set_descriptio
> + 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( nFilter description cannot contain ' _get_descriptio n, _set_description) self): bugs/model/ tests/test_ bugsubscription filter. py' bugs/model/ tests/test_ bugsubscription filter. py 2011-03-21 18:20:09 +0000 bugs/model/ tests/test_ bugsubscription f...
> + 'BugSubscriptio
> + 'any of <, >, " or &.'))
> + self._description = description
> +
> + description = property(
>
> def _get_statuses(
> """Return a frozenset of statuses to filter on."""
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/