201816 - use CGI.pm for header output

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Bradley Baetz (:bbaetz)]

We should use CGI.pm for all our output. Patch coming after a bit more testing.

Important note - this does not work in a utf8 locale under perl 5.8, because of
perl core bug 21951, http://bugs6.perl.org/rt2/Ticket/Display.html?id=21951

Since the webserver tends to run in the "C" locale, that shouldn't be an issue,
although it is when doing cmdline testing if you have utf8 set up (which I was
doing, which is how I noticed this). Once theres a workarround we can use it, or
require a fixed version of CGI. Do we need to do that before this is checked in?
If so, I can probably make a workarround which would work for bugzilla. I'd
rather not, though.

CGI.pm version bump is so that we can have no charset sent, rather than a blank
charset (ie |Content-Type: text/html; charset=|)

The content-type conversion semi-automated via perl -pi, although I went through
everything manually and did cookies entirely manually.

I got rid of the headers_done test in the ThrowError* code by using
:unique_headers in CGI.pm - when trying to print the headers, it will just
return if they're already been printed. This obviously only works if everywhere
uses CGI.pm, and noone prints manually.

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: patch

OK, here we go. This is streightforward, really. Only issue is wher eI used
Bugzilla->cgi vs a local $cgi var:

edit* and friends got Bugzilla->cgi, since they need rewrites to be any use for
mod_perl anyway.

Subroutines in cgi scripts can't use the script-local var from within a
subroutine because of how mod_perl wraps the script - they end up as closures.
See mod_perl docs for details. (We do get a warning about this under mod_perl,
so its not a hidden bug). So if printing the header was the only use the sub
had for a cgi object, I just used Bugzilla->cgi.

I did add |use Bugzilla| lines for all but edit*. If I missed some, it doesn't
matter for now since CGI.pl brings it in.

I also changed the cookie expiration dates to have the correct day, and changed
them while I was at it.

Comment 2

[Tobias Burnus]

One shouldn't forget to update
  http://www.bugzilla.org/docs/html/extraconfig.html#content-type
if this gets in.
(Setting   $self->charset('');  in Bugzilla/CGI.pm is less clumpsy than the
previous perl -pi -e  -- and I paves the way for UTF-8/bug 126266 :-)

Comment 3

[Bradley Baetz (:bbaetz)]

Oh, and I moved the contenttypes stuff out of localconfig while I was there.

Comment 4

[Dave Miller [:justdave]]

I'd rather not break utf8 personally... what's the workaround?

Comment 5

[Bradley Baetz (:bbaetz)]

Its not UTF8 which breaks per se; its running under the utf8 locale which does.
I haven't had a response to my bug report. I'll try to work out a work arround
this weekend.

Comment 6

[Joel Peshkin]

Comment on attachment 120352 [details] [diff] [review]
patch

Well, this reads OK, but I tried it out and it broke my logincookes.  I have to
log in after every page.  Cookiepath is "/" for me which always worked in the
past.

Comment 7

[Joel Peshkin]

Incidentally, the cookie it tried to set on my machine is....
"<param>cookiepath</param>"
:-)

Comment 8

[Joel Peshkin]

This is still pretty toxic.  In prder to get the correct cookie set, CGI.pm
needs to refer to Bugzilla::Config::Param() instead of merely Param().  Even so,
and even after clearing cookies, this patch forces me to log in for every page.
 If I back the patch out, that does not happen.

Traces show the logincooke being sent to the browser and then sent back to the
server when the next page is requested, but the cookie does not seem to be
accepted with the patch in place.

Did you try this using perl 5.6.1 ??



Checking perl modules ...
Checking for       AppConfig (v1.52)   ok: found v1.52
Checking for             CGI (v2.88)   ok: found v2.91
Checking for    Data::Dumper (any)     ok: found v2.102
Checking for    Date::Format (v2.21)   ok: found v2.21
Checking for             DBI (v1.32)   ok: found v1.32
Checking for      DBD::mysql (v2.1010) ok: found v2.1017
Checking for      File::Spec (v0.82)   ok: found v0.82
Checking for      File::Temp (any)     ok: found v0.12
Checking for        Template (v2.08)   ok: found v2.08
Checking for      Text::Wrap (v2001.0131) ok: found v2001.0131

The following Perl modules are optional:
Checking for              GD (v1.20)   ok: found v1.38
Checking for     Chart::Base (v0.99)   ok: found v0.99
Checking for     XML::Parser (any)     ok: found v2.31
Checking for       GD::Graph (any)     ok: found v1.35
Checking for GD::Text::Align (any)     ok: found v1

Checking user setup ...  

Comment 9

[Joel Peshkin]

... and just to make this a bit more interesting, the correct cookie values for
Bugzilla_Login and Bugzilla_Loginccokie are 1 and 133 respectively. (with the
latter incrementing each time I do a new login)  I see these values being sent
to the browser and back to the server.

  I added 
    print STDERR qq[login "$login" "$login_cookie"\n];      
  to Authenticate()

When I run without this patch, I get...
login "1" "133"
When I apply this patch, I get...
login "2" "2"
login "2" "2"  

I have no idea where these "2"s are coming from.

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: fix cookie fetching

I really don't like overloading routines for fetch+get, especially when its not
easy to tell which is which from the arguments directly.

Anyway, this works, subject to the caveat in the comments.

Oh, and I sent a patch to the CGI.pm author with a workarround for that utf8
bug.

Comment 11

[Joel Peshkin]

Comment on attachment 121058 [details] [diff] [review]
fix cookie fetching

r=joel
Works much better.  Do elaborate (to developers??) on what the patch you
submitted to the CGI.pm author is.

Comment 12

[Bradley Baetz (:bbaetz)]

Nah, I'll just wait for upstream to apply it and then bump the required version.
If it takes a while, then I'll note it. Bascically, I replaces \s with |
\r\n\t|, and everything works. This skips out the unicode space chars, but thats
ok (It still works if I manually add those in, so its something else odd going
on here)

Comment 13

[Bradley Baetz (:bbaetz)]

Attachment: require CGI-2.92

CGI 2.92 was released today, so require that to fix the bug

Comment 14

[Bradley Baetz (:bbaetz)]

Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92

Actually, we need 2.93 instead

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92

Actually, we need 2.93 instead because 2.92 breaks all mod_perl-1 apps using
CGI.pm. I've changed that locally.

joel, can you rubberstamp this, please? Its basically identical to the version
you already r='d.

Comment 16

[Joel Peshkin]

Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92

r=joel

Comment 17

[Dave Miller [:justdave]]

Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92

because of the size of this patch I'd like a second review on it before it goes
in.  Might be me if I get time, but I'll leave it blank in case someone else
beats me to it, since I probably won't get to it right away.

Comment 18

[Dave Miller [:justdave]]

Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92

I'm obviously not going to get time to touch this...  we just did a release, so
we have time to get stable again.  Joel's usually decent at this stuff anyway. 
If it has problems let's file new bugs.

Comment 19

[Bradley Baetz (:bbaetz)]

Checked in. Have fun!