191020 - buglist.cgi doesn't always get query names right for filename to save

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Gervase Markham [:gerv]]

buglist.cgi works out the filename to put in the Content-Disposition header
using the old style of params to buglist.cgi, and before we do the old->new
conversion. It needs to move after that bit of code, and it'll start working again.

There's also a missing /g on the whitespace-removal regexp. I don't know how
serious a problem that is - the comment says it'll let people mess with the HTTP
headers. CCing justdave and bbaetz for an assessment.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here's the fix.

Gerv

Comment 2

[Bradley Baetz (:bbaetz)]

The /g is because of \r\n, but I think we input test that anyway. We should
probably s/\s/_/g instead

I don't follow why this has to be moved, though - that code doesn't use the
params (or teh CGI) object.

Comment 3

[Gervase Markham [:gerv]]

The code needs to come after this code:
http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/buglist.cgi#234

Currently, we use 
$::FORM{'cmdtype'} = "dorem" && $::FORM{'remaction'} = "run"
from query.cgi to indicate a named query. The current generate-a-name code
checks only for $::FORM{'cmdtype'} eq 'runnamed', the old interface, and so will
only work with stored queries created before the change.

So, we need to make it look for the new form values (dorem and run), and then
move it after the code where the old form values are changed to the new form
values (line 234).

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

Comment on attachment 112910 [details] [diff] [review]
Patch v.1

Oh, that backwards code. I thought you were talking about the CGI stuff.

Anyway, I'll review this if you s// it to _ instead of nothing. Thats sort of
an established convention.

Quoting the name so that we can include spaces directly (and filtering out
\r\n) can be a different bug, if anyone feels its worth it.

Mopre importantly (and the reason I'm minusing this) is the fact the 
$::FORM{'cmdtype'} ||= "" needs to be done earlier, else you'll get uninited
var warnings when we try to do the compat code conversion.

Comment 5

[Gervase Markham [:gerv]]

Attachment: Patch v.2

How's this?

Gerv

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 113357 [details] [diff] [review]
Patch v.2

r=bbaetz, assuming you tested/etc

Comment 7

[Gervase Markham [:gerv]]

Requesting approval.

BTW, bbaetz: is the whitespace thing actually a security hole, or can we remove
that designation?

Gerv

Comment 8

[Dave Miller [:justdave]]

I'm going to hold off on approval till I know for sure if this is a security
thing or not.  Now that we have this nifty approval thing we can sit on it and
check it in when we're ready to release the advisory so it doesn't show up in
the cvs logs on bosai before we release it if it needs to have an advisory.

Whitespace in itself won't mess with HTTP headers, but linefeeds will (which
might be nailed by the whitespace removal).

Comment 9

[Bradley Baetz (:bbaetz)]

Gerv: We trim on entry, but don't strip out newlines, so it may be. Although
since you can only affect your own queries, I don't think it lets you do
anything more than insert headers/content into your own browser.

Comment 10

[Gervase Markham [:gerv]]

Lucky we found it before implementing any form of query sharing, then :-)

Making non-confidential - the only person you can hack here is yourself.

Gerv

Comment 11

[Dave Miller [:justdave]]

and approved.  check it in.

Comment 12

[Gervase Markham [:gerv]]

Fixed.

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.220; previous revision: 1.219
done

Gerv