Closed Bug 772953 Opened 8 years ago Closed 8 years ago

buglist urls should not contain a token

Categories

(Bugzilla :: Query/Bug List, defect)

4.2.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.2

People

(Reporter: glob, Assigned: glob)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #641131 - Flags: review?(dkl)
Comment on attachment 641131 [details] [diff] [review]
patch v1

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

r=dkl
Attachment #641131 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval4.2?
Target Milestone: --- → Bugzilla 4.2
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.
Flags: approval?
Flags: approval4.2?
Attached patch patch v2Splinter Review
Attachment #641131 - Attachment is obsolete: true
Attachment #649540 - Flags: review?(dkl)
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
Attachment #649540 - Flags: review?(dkl) → review+
Flags: approval?
Flags: approval4.2?
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).
Do not forget to address comment 6.
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval4.2?
Flags: approval4.2+
Flags: approval+
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Reopening! I can no longer save a query without getting the "Suspicious Action" error message.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is a regression, it blocks new releases.
Status: REOPENED → ASSIGNED
(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
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
oops, didn't see your comment until after my commit.  i'll fix forgetting searches too.
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.
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.
You need to log in before you can comment on or make changes to this bug.