Last Comment Bug 772953 - buglist urls should not contain a token
: buglist urls should not contain a token
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 4.2.1
: All All
: -- normal (vote)
: Bugzilla 4.2
Assigned To: Byron Jones ‹:glob›
: default-qa
Mentors:
Depends on: 754672
Blocks: bmo4.2 bz-release-423
  Show dependency treegraph
 
Reported: 2012-07-11 11:12 PDT by Byron Jones ‹:glob›
Modified: 2012-08-29 04:06 PDT (History)
2 users (show)
LpSolit: approval+
LpSolit: approval4.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (1.72 KB, patch)
2012-07-11 11:21 PDT, Byron Jones ‹:glob›
dkl: review+
Details | Diff | Review
patch v2 (3.68 KB, patch)
2012-08-06 22:03 PDT, Byron Jones ‹:glob›
dkl: review+
Details | Diff | Review

Description Byron Jones ‹:glob› 2012-07-11 11:12:57 PDT
sharing a buglist.cgi url is a reasonably common event.

currently the queries generated from the advanced search page for logged in users contains a token, which makes the url much longer and unwieldy.

http://example.com/buglist.cgi?list_id=2494300&short_desc=test&resolution=---&query_format=advanced&token=1342029085-524e85bf8eed9340b6a493650058c1d3&short_desc_type=allwordssubstr&product=Bugzilla

vs

http://example.com/buglist.cgi?list_id=2494300&short_desc=test&resolution=---&query_format=advanced&short_desc_type=allwordssubstr&product=Bugzilla


'token' needs to be added to the canonicalise_query call in buglist.cgi.
we could clear the token field with javascript unless the remasdefault checkbox is checked.
Comment 1 Byron Jones ‹:glob› 2012-07-11 11:21:53 PDT
Created attachment 641131 [details] [diff] [review]
patch v1
Comment 2 David Lawrence [:dkl] 2012-07-12 15:05:39 PDT
Comment on attachment 641131 [details] [diff] [review]
patch v1

Review of attachment 641131 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl
Comment 3 Byron Jones ‹:glob› 2012-07-25 01:08:07 PDT
while playing around with custom searches, i found a few other places where the token will get into the query_string.  i'll attach an updated patch soon.
Comment 4 Byron Jones ‹:glob› 2012-08-06 22:03:12 PDT
Created attachment 649540 [details] [diff] [review]
patch v2
Comment 5 David Lawrence [:dkl] 2012-08-20 15:25:14 PDT
Comment on attachment 649540 [details] [diff] [review]
patch v2

Review of attachment 649540 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works as expected. r=dkl

::: Bugzilla/CGI.pm
@@ +158,5 @@
>      # Delete leftovers from the login form
>      $self->delete('Bugzilla_remember', 'GoAheadAndLogIn');
>  
> +    # Delete the token if we're not updating the defaults
> +    unless (defined $self->param('remtype') && $self->param('remtype') eq 'asdefault') {

Nit: Bring && down to the next line
Comment 6 Frédéric Buclin 2012-08-21 02:12:38 PDT
Comment on attachment 649540 [details] [diff] [review]
patch v2

>=== modified file 'template/en/default/list/list.html.tmpl'

>+  [% IF cgi.param('token') != '' %]
>+    [% new_url = '' %]
>+    [% FOREACH name = cgi.param %]
>+      [% IF name != 'token' %]
>+        [% SET value = cgi.param(name) FILTER uri %]
>+        [% new_url = new_url _ name _ '=' _ value %]
>+        [% UNLESS loop.last %]
>+          [% new_url = new_url _ '&' %]
>+        [% END %]
>+      [% END %]
>+    [% END %]
>+  [% END %]

All this code can and should be replaced by a one-liner:

  [% new_url = cgi.canonicalise_query("token") %]

But I don't fully agree with your patch. I think it should be combined with the previous block:

  [% new_url = cgi.self_url %]
  [% IF quicksearch %]
    ....
  [% ELSIF cgi.param('token') %]
    [% new_url = cgi.canonicalise_query("token") %]
  [% END %]

This way we are sure we are not overriding what has been done in the [% IF quicksearch %] ... [% END %] block.

Please fix that on checkin (or upload another patch if it needs some additional testing again).
Comment 7 Frédéric Buclin 2012-08-21 02:13:42 PDT
Do not forget to address comment 6.
Comment 8 Byron Jones ‹:glob› 2012-08-28 08:44:21 PDT
committed to trunk:

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified buglist.cgi
modified Bugzilla/CGI.pm
modified js/custom-search.js
modified template/en/default/list/list.html.tmpl
modified template/en/default/search/search-advanced.html.tmpl
Committed revision 8359.

while the patch applies with minimal changes to 4.2, it doesn't work due to other changes.  working on fixing that now.
Comment 9 Byron Jones ‹:glob› 2012-08-28 09:37:01 PDT
for 4.2 i've implemented everything except for removing the token from the displayed url after you update your default query parameters, as this sits on top of changes from bug 730670.

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/4.2/
modified buglist.cgi
modified Bugzilla/CGI.pm
modified js/custom-search.js
modified template/en/default/search/search-advanced.html.tmpl
Committed revision 8124.
Comment 10 Frédéric Buclin 2012-08-28 11:05:19 PDT
Reopening! I can no longer save a query without getting the "Suspicious Action" error message.
Comment 11 Frédéric Buclin 2012-08-28 11:13:52 PDT
This is a regression, it blocks new releases.
Comment 12 David Lawrence [:dkl] 2012-08-28 21:18:55 PDT
(In reply to Frédéric Buclin from comment #11)
> This is a regression, it blocks new releases.

Ugh not sure how I missed that. Forgetting searches also fails as well.

=== modified file 'Bugzilla/CGI.pm'
--- Bugzilla/CGI.pm	2012-07-24 14:14:18 +0000
+++ Bugzilla/CGI.pm	2012-07-31 05:43:23 +0000
@@ -158,6 +158,11 @@
     # Delete leftovers from the login form
     $self->delete('Bugzilla_remember', 'GoAheadAndLogIn');
 
+    # Delete the token if we're not updating the defaults
+    unless (defined $self->param('remtype') && $self->param('remtype') eq 'asdefault') {
+        $self->delete("token");
+    }
+
     foreach my $num (1,2,3) {
         # If there's no value in the email field, delete the related fields.
         if (!$self->param("email$num")) {

Needs to be changed to:

    # Delete the token if we're not updating a default search,
    # saving a search, or forgetting a search.
    my $delete_token = 1;
    if (defined $self->param('remtype') 
        && ($self->param('remtype') eq 'asdefault'
            || $self->param('remtype') eq 'asnamed'))
    {
        $delete_token = 0;
    }
    if (defined $self->param('remaction') 
        && $self->param('remaction') eq 'forget')
    {
        $delete_token = 0;
    }
    $self->delete("token") if $delete_token;

dkl
Comment 13 Byron Jones ‹:glob› 2012-08-28 21:46:20 PDT
sorry about missing that.

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/CGI.pm
Committed revision 8361.

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/CGI.pm
Committed revision 8125.
Comment 14 Byron Jones ‹:glob› 2012-08-28 21:47:59 PDT
oops, didn't see your comment until after my commit.  i'll fix forgetting searches too.
Comment 15 Byron Jones ‹:glob› 2012-08-28 21:59:46 PDT
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/CGI.pm
Committed revision 8362.

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/CGI.pm
Committed revision 8126.
Comment 16 Frédéric Buclin 2012-08-29 04:06:08 PDT
I think a more robust fix would have been to extract the token at the top of buglist, before calling $cgi->redirect_search_url, else the code will break everytime we add a new token to buglist.cgi. The only problem is that the token would be lost if we get a redirect.

Note You need to log in before you can comment on or make changes to this bug.