Merge lp:~julian-edwards/launchpad/ppa-search-oops-bug-446157 into lp:launchpad

Proposed by Julian Edwards on 2009-10-14
Status: Merged
Merged at revision: not available
Proposed branch: lp:~julian-edwards/launchpad/ppa-search-oops-bug-446157
Merge into: lp:launchpad
Diff against target: 42 lines
2 files modified
lib/lp/registry/browser/distribution.py (+7/-0)
lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt (+14/-0)
To merge this branch: bzr merge lp:~julian-edwards/launchpad/ppa-search-oops-bug-446157
Reviewer Review Type Date Requested Status
Michael Nelson (community) code 2009-10-14 Approve on 2009-10-15
Review via email: mp+13361@code.launchpad.net
To post a comment you must log in.
Julian Edwards (julian-edwards) wrote :

= Summary =
Fix an oops when someone hand-hacks URLs searching PPAs.

== Proposed fix ==
We need to cope with a list value when expecting a string in the form data.
This is a trivial change in the view code, but I also filed a Foundations bugs
since we should be able to do this generically in the form setup.

== Pre-implementation notes ==
None, I suck.

== Tests ==
bin/test -cvvt xx-ubuntu-ppas

== Demo and Q/A ==
https://launchpad.dev/ubuntu/+ppas?name_filter=packages&name_filter=humanity

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt
  lib/lp/registry/browser/distribution.py

Michael Nelson (michael.nelson) wrote :

Thanks Julian. Just a suggestion for changing the comment below.

> === modified file 'lib/lp/registry/browser/distribution.py'
> --- lib/lp/registry/browser/distribution.py 2009-09-23 14:58:12 +0000
> +++ lib/lp/registry/browser/distribution.py 2009-10-15 08:23:35 +0000
> @@ -646,6 +646,15 @@
>
> def initialize(self):
> self.name_filter = self.request.get('name_filter')
> + if isinstance(self.name_filter, list):
> + # This happens if someone hand-hacks the URL so that it has
> + # more than one name_filter field.
> + #

Is that normal? I've normally just left a blank line there. I'm just
interested to know.

> + # XXX 2009-10-14 Julian bug=451424
> + # There really should be a way for the form to reject
> + # unexpected input like this before it hits the code here.
> + # See bug 451424.

As per our conversation, I think the issue here is more that we are not
using an LPFormView. We could inherit from LPFormView, have an interface
for the search form defined, use a safe_action and the data would be
validated automatically right? Up to you whether you want to go ahead
and actually do that, or simply update the above comment.

> + self.name_filter = " ".join(self.name_filter)

Great - at first I wondered why you didn't just grab name_filter[0]
(or [-1]), but the doctest has a good justification for joining the
terms.

> self.show_inactive = self.request.get('show_inactive')
>
> @property
>
> === modified file 'lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt'
> --- lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt 2009-09-23 14:58:12 +0000
> +++ lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt 2009-10-15 08:23:35 +0000
> @@ -246,6 +246,20 @@
> 1
>
>
> +=== Hand-hacked search URLs ==
> +
> +If the search term is specified more than once by someone hand-hacking the
> +URL, the page copes gracefully with this by searching for all the terms
> +specified.
> +
> + >>> anon_browser.open(
> + ... "http://launchpad.dev/ubuntu/+ppas"
> + ... "?name_filter=packages&name_filter=friends")
> + >>> [row] = find_tags_by_class(anon_browser.contents, 'ppa_batch_row')
> + >>> print extract_text(row)
> + PPA for Celso Providelo...
> +
> +
> == Owner's PPA pages ==
>
> Let's start by adding an extra package to Celso's archive:

--
Michael

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/distribution.py'
2--- lib/lp/registry/browser/distribution.py 2009-09-23 14:58:12 +0000
3+++ lib/lp/registry/browser/distribution.py 2009-10-15 10:29:13 +0000
4@@ -646,6 +646,13 @@
5
6 def initialize(self):
7 self.name_filter = self.request.get('name_filter')
8+ if isinstance(self.name_filter, list):
9+ # This happens if someone hand-hacks the URL so that it has
10+ # more than one name_filter field. We could do something
11+ # like form.getOne() so that the request would be rejected,
12+ # but we can acutally do better and join the terms supplied
13+ # instead.
14+ self.name_filter = " ".join(self.name_filter)
15 self.show_inactive = self.request.get('show_inactive')
16
17 @property
18
19=== modified file 'lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt'
20--- lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt 2009-09-23 14:58:12 +0000
21+++ lib/lp/soyuz/stories/ppa/xx-ubuntu-ppas.txt 2009-10-15 10:29:13 +0000
22@@ -246,6 +246,20 @@
23 1
24
25
26+=== Hand-hacked search URLs ==
27+
28+If the search term is specified more than once by someone hand-hacking the
29+URL, the page copes gracefully with this by searching for all the terms
30+specified.
31+
32+ >>> anon_browser.open(
33+ ... "http://launchpad.dev/ubuntu/+ppas"
34+ ... "?name_filter=packages&name_filter=friends")
35+ >>> [row] = find_tags_by_class(anon_browser.contents, 'ppa_batch_row')
36+ >>> print extract_text(row)
37+ PPA for Celso Providelo...
38+
39+
40 == Owner's PPA pages ==
41
42 Let's start by adding an extra package to Celso's archive: