Closed
Bug 772953
Opened 12 years ago
Closed 12 years ago
buglist urls should not contain a token
Categories
(Bugzilla :: Query/Bug List, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 4.2
People
(Reporter: glob, Assigned: glob)
References
Details
Attachments
(1 file, 1 obsolete file)
3.68 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #641131 -
Flags: review?(dkl)
Comment 2•12 years ago
|
||
Comment on attachment 641131 [details] [diff] [review] patch v1 Review of attachment 641131 [details] [diff] [review]: ----------------------------------------------------------------- r=dkl
Attachment #641131 -
Flags: review?(dkl) → review+
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?
Attachment #641131 -
Attachment is obsolete: true
Attachment #649540 -
Flags: review?(dkl)
Comment 5•12 years ago
|
||
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+
Comment 6•12 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•12 years ago
|
||
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: 12 years ago
Resolution: --- → FIXED
Comment 10•12 years ago
|
||
Reopening! I can no longer save a query without getting the "Suspicious Action" error message.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 12•12 years ago
|
||
(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•12 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
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•12 years ago
|
||
oops, didn't see your comment until after my commit. i'll fix forgetting searches too.
Assignee | ||
Comment 15•12 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•12 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.
Description
•