149845 - buglist.cgi checks for ORDER validity are wrong

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Bradley Baetz (:bbaetz)]

buglist.cgi has:

foreach my $fragment (split(/[,\s]+/, $order)) {
                next if $fragment =~ /^asc|desc$/i;

This is wrong, because /^asc|desc$/i is /(^asc)|(desc$)/, not /^(asc|desc)$/, as
it should be. This should be fixed for 2.16, as even though I don't think its
exploitable, its letting something through we didn't want to.

The split regexp allows ,, though, which is also wrong.

We should do what I suggested mattyt do for 2.14.2 patch does, and split on /,/.
The grep should then be changed to check for trim($_) =~ /$fragment(\s+asc|desc))?/

The grep will go thorugh all the coumns, too, while we only want want to check
for any single match - is there a better way to do this?

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: v1

OK, try this. The regexp I wrote in my earlier comment was obviously wrong,
though - the one on the patch actually works.

I also moved the @columnnames generation out of the foreach - I don't see why
it has to be regenerated each time.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 86770 [details] [diff] [review]
v1

+		 $fragment = trim($fragment);

Nit: This can be rolled into the regular expression with \s* at the beginning
and end.

r=myk

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 86770 [details] [diff] [review]
v1

Oops, I meant to give this 2xr=myk but got distracted.	Here it is now.

Comment 4

[Matthew Tuck [:CodeMachine]]

Personally I think that trim is better style.  We should we wary of turning the
whole world into a regexp, even if we can, as they tend to be harder to read.

Comment 5

[Bradley Baetz (:bbaetz)]

Checked in, trunk + branch:

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


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

Comment 6

[Dave Miller [:justdave]]

2.14.2 is out, removing security group.