buglist urls should not contain a token

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Query/Bug List
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

4.2.1
Bugzilla 4.2
Dependency tree / graph
Bug Flags:
approval +
approval4.2 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 641131 [details] [diff] [review]
patch v1
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+
(Assignee)

Updated

5 years ago
Flags: approval?
(Assignee)

Updated

5 years ago
Flags: approval4.2?
Target Milestone: --- → Bugzilla 4.2
(Assignee)

Comment 3

5 years ago
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?
(Assignee)

Comment 4

5 years ago
Created attachment 649540 [details] [diff] [review]
patch v2
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+
(Assignee)

Updated

5 years ago
Flags: approval?
Flags: approval4.2?

Comment 6

5 years ago
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

5 years ago
Do not forget to address comment 6.
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval4.2?
Flags: approval4.2+
Flags: approval+
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 10

5 years ago
Reopening! I can no longer save a query without getting the "Suspicious Action" error message.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 11

5 years ago
This is a regression, it blocks new releases.
Blocks: 783053
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 13

5 years ago
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

5 years ago
oops, didn't see your comment until after my commit.  i'll fix forgetting searches too.
(Assignee)

Comment 15

5 years ago
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

5 years ago
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.