Closed Bug 191020 Opened 22 years ago Closed 22 years ago

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


(Bugzilla :: Query/Bug List, defect)

Not set



Bugzilla 2.18


(Reporter: gerv, Assigned: gerv)



(1 file, 1 obsolete file)

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.

Attached patch Patch v.1 (obsolete) — Splinter Review
Here's the fix.

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.
The code needs to come after this code:

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).

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.
Attachment #112910 - Flags: review-
Attached patch Patch v.2Splinter Review
How's this?

Attachment #112910 - Attachment is obsolete: true
Attachment #113357 - Flags: review?(bbaetz)
Comment on attachment 113357 [details] [diff] [review]
Patch v.2

r=bbaetz, assuming you tested/etc
Attachment #113357 - Flags: review?(bbaetz) → review+
Requesting approval.

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

Flags: approval?
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).
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.
Lucky we found it before implementing any form of query sharing, then :-)

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

Group: webtools-security
and approved.  check it in.
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 2.18

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

Closed: 22 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.