308876 - Iff ssl is "always", whining (and other non-interactive scripts?) get a 302-moved error

Status: RESOLVED FIXED

(Bugzilla :: Whining, defect, P2)

Description

[Joel Peshkin]

When whining runs on a site with the ssl set to "always", the ssl-forcing code
tries to redirect even though whining is a non-cgi thing.

Comment 1

[A. Karl Kornel]

I'm not noticing it with whine.pl, but I do see it if I run collectstats.pl.

This is due to Bugzilla/CGI.pm lines 67-69 (lines 58-60 on 2.20).  Possible patch is coming soon.

Comment 2

[A. Karl Kornel]

Attachment: 2.20 Patch v1

The line that (I think) causes the problem is

> if (Param('sslbase') ne '' and Param('ssl') eq 'always') {

I change it to 

> if (Param('sslbase') ne '' and Param('ssl') eq 'always' and i_am_cgi()) {

i_am_cgi simply checks for the SERVER_SOFTWARE environment variable (apparently
it has to be defined when CGI is being used) and returns 1 if found.  2.20
doesn't currently have i_am_cgi, so I also patch Bugzilla/Util.pm to include
it.

I'm requesting review from someone who knows this area.  This is also going to
need a patch on tip, which I'll submit once this patch is approved.

Comment 3

[A. Karl Kornel]

Since I don't have much to do while I wait for review, here's an explanation of how this problem 
appeared in the first place.  This is all 2.20-specific, and it helps to have the sources for collectstats.pl 
(http://lxr.mozilla.org/bugzilla2.20/source/collectstats.pl), whine.pl (http://lxr.mozilla.org/
bugzilla2.20/source/whine.pl), and CGI.pm (http://lxr.mozilla.org/bugzilla2.20/source/Bugzilla/
CGI.pm) handy.

Both whine.pl and New Charts rely on saved search URLs, and since the CGI and Bugzilla::CGI modules 
can parse a URL, the job of doing so is given to them.  whine.pl references namedqueries (saved 
searches) to extract the URLs while New Charts stores the search URLs in its own tables.  In either case, 
those URLs may be http:// URLs, if SSL was not used when the site was first set up.  However, now SSL 
is mandatory (all of the time, not just for authenticated sessions).  Of course, data sets and saved 
searches created after SSL use became mandatory won't show this bug.

Whine.pl creates its CGI object on line 461, while collectstats.pl does so on line 488.  The interesting 
thing here is that CGI objects are not always created.  For whine.pl, CGI objects are only created if a 
whining query needs to be executed, while collectstats.pl only does so for New Charts data, which 
means if no data sets exist collectstats never needs to create a CGI object.  Again, you won't always see 
this bug!

When the CGI objects are created by whine.pl and collectstats.pl they pass in the URL to be parsed, 
instead of allowing CGI to fetch the URL itself.  On line 49 of CGI.pm, right at the start of the new 
subroutine, the CGI superclass (from which Bugzilla::CGI inherits, see line 30) is given the URL during 
its initialization.  Then, on line 58, we check to see if ssl must always be used (it must) and if sslbase 
has a value (it does).  That test being true, the require_https subroutine is called.

require_https starts at line 221.  It first checks to see if the current protocol is 'https'.  This is a bit of a 
mistake here, because the CGI module does not know that we are not being executed in a CGI 
environment, because we passed it a URL to parse (normally it would get and parse the URL itself).  So, 
not knowing any better, it parses the URL and sets the result of the protocol subroutine to 
'http' (remember, our saved search URL was created before SSL use became mandatory).  This means 
the test on line 223 passes.  The test on line 225 also passes ($url is the value of the sslbase parameter 
and was passed to require_https on line 59).  The rest of the new URL is given by CGI (appended to the 
sslbase), and on line 231 the CGI module is commanded to redirect the browser (which doesn't exist 
here) to the new URL.  CGI does so by sending the 302 Moved response and the new URL in a Location: 
header.  Since we are redirecting, we are not expect us to do any more work and we exit (line 232).

So, there you are.  All you need to do is start with an installation that did not force the use of SSL at all.  
One or more saved searches or data sets are created by a user not using SSL.  Then at some point SSL 
use became mandatory, and this bug rears its ugly head.

The solution, of course, if to recognize that we are not being executed because of some remote web 
client.  The i_am_cgi module in Bugzilla::Util takes care of making this determination, and so I change 
Bugzilla::CGI to be sure that i_am_cgi is true before calling require_https.  Unfortunately, i_am_cgi 
appeared after 2.20 froze, so I had to copy it from 2.22 to 2.20.  It's small, and it's easier to understand 
a call to i_am_cgi than a check to see if the SERVER_SOFTWARE environment variable exists.  Of course, 
the patch for tip will be smaller, as i_am_cgi already exists there 8-)

Comment 4

[Joel Peshkin]

Comment on attachment 196378 [details] [diff] [review]
2.20 Patch v1

r=joel

Comment 5

[A. Karl Kornel]

Attachment: Tip Patch v1

Basically the same as the 2.20 patch, except i_am_cgi already exists on tip.

Comment 6

[Frédéric Buclin]

tip:

Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.19; previous revision: 1.18
done


2.20rc2:

Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.16.2.1; previous revision: 1.16
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.28.2.3; previous revision: 1.28.2.2
done