214466 - Cookie setting w/ 3xx redirect is broken on IIS (colchange)

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Colin Bendell]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624

Changing the columns using the "change columns" link at the bottom of the result
list does not change the columns displayed.  I have tested this in IE6 and
Mozilla 1.5

Reproducible: Always

Steps to Reproduce:
1. performed query
2. selected 'change columns'
3. selected different columns and submitted the page
4. results display the original columns, and not the new column list
5. 'change columns' displays only the original column selections, not the new
selection

Comment 1

[Colin Bendell]

Forgot to mention that this bug is reproduceable with the latest cvs checkout of
bugzilla.

Comment 2

[Jussi Sirpoma]

I could not reproduce this in my bugzilla or in
http://landfill.bugzilla.org/bugzilla-tip/

Do you have cookiepath parameter set up correctly? Does the value of COLUMNLIST
cookie change for you when you change the columns?

Comment 3

[Colin Bendell]

The problem is that the Set-Cookie directives are not coming through with the 
304 redirect directive.  If I disable the redirect, forcing the page to return 
200 OK, the cookies do get set and everything behaves from there on in as 
expected.

The only difference I can see from out configuration and landfill.bugzilla.org 
is that we are fronting bugzilla with IIS (5.0).  I have confirmed that IIS is 
not mangling the 304 with its own error page.  I am going to attempt to revert 
to the 1.34 method of outputting the header information (instead of using the 
cgi pm)

Comment 4

[Colin Bendell]

I'm no closer to understanding why the Set-Cookie headers aren't propagated to 
the user, but if I replace the $cgi->redirect method with a refresh header ( 
print "Refresh: 0; URL=buglist.cgi?$::FORM{'rememberedquery'}\n"; ) then 
everything works as I would expect because a 200 OK is sent to the user and 
the browser follows the redirect.

Is there a test environment that has IIS fronting Bugzilla instead of Apache?  
(I'm using IIS only because I want Domain authentication for Bugzilla and View 
CVS)  

Comment 5

[Jussi Sirpoma]

I do not have any experience with IIS, but if Jouni doesn't mind, I think he
might be able to help.

Comment 6

[Jouni Heikniemi]

There's no public IIS based Bugzilla test installation. I've had IIS test
platforms for quite some time, but they're never stable enough to be allowed
public use ;-) 

Your problem is called an IIS bug. I had all forgotten about this, but it's one
part in the huge mess about how IIS handles the details of NPH CGIs. 

http://support.microsoft.com:80/support/kb/articles/q176/1/13.asp&NoWebContent=1

The workaround is not practically usable in Bugzilla.

See bug 201816 for what caused this. This will break column changing when
updating Bugzilla on all Windows/IIS installations (apart from Windows 2003
server with IIS 6, which is rumored to work correctly). 

Bbaetz, do you have a proposal on how to fix this? Give up on NPH mode? 

Comment 7

[Bradley Baetz (:bbaetz)]

I thought that IE didn't support Refresh - thats one of the reasons it got moved.

Note that NPH mode won't work either - see
http://support.microsoft.com/default.aspx?scid=kb;en-us;280341

Now that we use CGI.pm for everything, we could run in non-parsed header mode, I
guess - can we do that w/o the nph- prefix? In CGI.pm, theres a commented out line:

# $NPH++ if defined($ENV{'SERVER_SOFTWARE'}) && $ENV{'SERVER_SOFTWARE'}=~/IIS/;

if you uncomment that, does that fix things, and does it break anything else?
(buglists on mozilla/ns4 being the main one which may break)

Comment 8

[Colin Bendell]

This is more a philosophical thought: What about setting support for bugzilla 
to IIS 5 and greater? IIS 5 is available for NT4 and that kb #280341 only 
relates to IIS 4.

Presumeably all these problems are resolved with IIS 6 since it was a complete 
rewrite, but that just might me being optimistic.

Comment 9

[Dave Miller [:justdave]]

Re: comment #4:  for the record, you *can* do domain authentication with Apache.
 Look for mod_sspi

Comment 10

[Colin Bendell]

Responding to #4:
IE (IE 5.0+) _does_ support the refresh.  Perhaps an old version (-4.0) didn't?

I couldn't find the line in CGI.pm you are suggesting to comment out.  This is 
in Bugzilla::CGI.pm right?

Comment 11

[Jacob Steenhagen]

I ran into this IIS header bug on an internal project at work before. It was the
last bit of ammunition I needed to get them to let me put Apache on that box.
Unfortunately, there are some things that require IIS so if you use any of them
 ont that box switching is not really an option (OWA on Exchange server leaps to
mind).

Comment 12

[Bradley Baetz (:bbaetz)]

No, I meant the perl CGI.pm module.

Comment 13

[Jouni Heikniemi]

Re comment 8, Colin:
Requiring IIS 5 would be fine with me if it helped, but it doesn't. The KB
article you refer to is a separate problem. Even though that one is fixed in IIS
5, the nph handling still has problems. It works (at least theoretically) if you
rename a cgi to "nph-something.cgi", but there's quite a lot of work to be done
if one wishes that to be made for all the pages that use server side push...

Jake, you're right in hinting that using Apache with this kind of software is
massively easier. However, there is the point of not having to maintain two
different web servers even if you're not absolutely forced to (by OWA for
example). Of course, all this is relevant only if you're running production IISs
anyway.

Comment 14

[Jouni Heikniemi]

Taking the bug, and ccing some core posse for comments. To summarize the previous:

IIS functions incorrectly when trying to set cookies simultaneously with a 3xx
redirect. This would be fixed by using nph mode for the cgi, but for IIS, that
apparently requires renaming the cgi to be "nph-colchange.cgi". That's pretty
much impossible, so let's forget it.

Bbaetz's proposal of hacking cgi.pm in comment 7 didn't help, and toying with
IIS seems to be very difficult. I think we have two major alternatives: 

1) make IIS users patch this by hand, essentially making this wontfix (but this
is somewhat contrary to the roadmap's "out of the box" target)

2) introduce a fallback iis compatibility hack so that instead of using an HTTP
3xx redirect, we use the old-fashioned Refresh header

As for number 2: I don't know if we can automagically detect the web server
we're running under. This would mean making people set "IIS mode" on somewhere.
That's ugly, but maybe acceptable.


If we decide to create this sort of "compatibility mode", how should we switch
it on? I'm thinking of a Win32-only localconfig switch or something of equal
caliber. Would you find this acceptable? Better alternatives?

If we use refresh headers for IIS only, do you see other problems involved?

Comment 15

[Dave Miller [:justdave]]

I thought the reason we got rid of the Refresh: header is that Internet Explorer
didn't support it...

Comment 16

[Jouni Heikniemi]

Yeah, but see comment 10 on that. I haven't confirmed it though, but I've never
encountered the Refresh problem with IE myself, so I'd think it's a pre-5.0
problem if something.

Comment 17

[Dave Miller [:justdave]]

Comment 6 seems to indicate that this bug is fixed in IIS6.  Is that correct?

If that's the case, then I have no problem with providing the Refresh: header or
the even older <meta name="refresh"> foo (whichever IE is more likely to
support) with the setting of a param or a variable in localconfig or something.

refreshhack   Older versions (prior to version 6) of Microsoft Internet
              Information Server (IIS) had a bug which prevented them from
              properly setting cookies at the same time as issuing a 3xx
              redirect. If you are using a version of IIS older than version 6,
              you will need to turn this on to make changing columns in
              buglists work.

              ( ) On  (*) Off

Something along those lines...

Comment 18

[Bradley Baetz (:bbaetz)]

you can look at ENV{SERVER_VERSION}. Can we do this in the CGI.pm redirect
function? - ie convert the 30x to a 200 + refresh if we're IIS?

Comment 19

[Gervase Markham [:gerv]]

It would be much better if this didn't have to be a param - and, from what
bbaetz says, it probably doesn't - which is good.

Gerv

Comment 20

[Jouni Heikniemi]

SERVER_SOFTWARE, actually. See <http://hoohoo.ncsa.uiuc.edu/cgi/env.html>. But
yeah, that looks like a feasible approach.

Comment 21

[Jouni Heikniemi]

Attachment: v1

Ok, this is try 1. 

- Instead of changing Bugzilla::CGI->redirect, I hacked colchange. This is 
  because we use quite a few redirects in Bugzilla altogether, and this IIS 
  hack only concerns those places where cookies are set.

- This also uses the old mechanism on IIS6. I don't have it available for 
  testing, so I can't confirm if the correct approach works or not. I don't
  think IIS 6 is an issue anyway, and I suggest we fix (optimize) it when we
  can properly test this. 

- I tried the now-revived refresh header on a couple of IEs (5.something and 
  6.0) and it worked fine on both. This may have a problem on older IEs, but
  I don't know if it's a problem. I guess we'll hear if somebody hits a 
  problem. Writing out meta refresh tags would complicate the solution 
  somewhat, so I don't think it's worth doing if it's only IE 3 that suffers
  from this.

Comment 22

[Jouni Heikniemi]

Oh, forgot to say: Tested on Windows 2k / IIS 5.0 and RH 8 / Apache 1.3.27. Of
course, the patch shouldn't affect platforms other than IIS.

Comment 23

[Colin Bendell]

You shouldn't have worry about supporting IE4 or IE3 since they make up less 
than 1% of the browser market.
http://www.w3schools.com/browsers/browsers_stats.asp
http://www.webreference.com/stats/browser.html

Comment 24

[Dave Miller [:justdave]]

from 'perldoc CGI' :

       GENERATING A REDIRECTION HEADER

          print $query->redirect('http://somewhere.else/in/movie/land');

       Sometimes you don't want to produce a document yourself, but simply
       redirect the browser elsewhere, perhaps choosing a URL based on the
       time of day or the identity of the user.

       The redirect() function redirects the browser to a different URL.  If
       you use redirection like this, you should not print out a header as
       well.

       One hint I can offer is that relative links may not work correctly when
       you generate a redirection to another document on your site.  This is
       due to a well-intentioned optimization that some servers use.  The
       solution to this is to use the full URL (including the http: part) of
       the document you are redirecting to.

       You can also use named arguments:

           print $query->redirect(-uri=>'http://somewhere.else/in/movie/land',
                                  -nph=>1);

       The -nph parameter, if set to a true value, will issue the correct
       headers to work with a NPH (no-parse-header) script.  This is important
       to use with certain servers, such as Microsoft IIS, which expect all
       their scripts to be NPH.

Notice the comment about IIS at the end.

Comment 25

[Jouni Heikniemi]

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

Cancelling review request as bug 223473 bitrotted the patch anyway. Also to
re-investigate the nph stuff pasted by justdave; I've got some ideas.

Comment 26

[Jouni Heikniemi]

Attachment: v2

As much as I hate to admit it, I haven't been able to come up with a better
fix. What Dave pasted is fine, except for one fact: It somehow assumes IIS
really treats all cgis as NPH. But in fact IIS only treats CGIs in nph _if_
their names are prefixed with "nph-". Oddly enough, even when I created
nph-colchange.cgi to handle the submit, it didn't work. Well, this is, of
course, a sum of several issues. The root cause is the fact that IIS's CGI
handling simply sucks when it comes to stuff like this. 

I suggest we proceed with this sort of a patch unless somebody else can help
with this. I can't make it break on anything, and it's not like I was close to
a breakthrough that would make the fix cleaner or meaner... If I come up with a
better solution later on, I'll be sure to contribute it.

Comment 27

[Dave Miller [:justdave]]

Comment on attachment 135831 [details] [diff] [review]
v2

we can't print headers to stdout anymore.  Use $cgi->header().

print $cgi->header(-type => "text/html",
	     -refresh => "0; URL=$vars->{'redirect_url'});

Something to that effect (double-check the docs for the correct syntax)

Comment 28

[Dave Miller [:justdave]]

I'm missing a quote there if you cut/paste

Comment 29

[Jouni Heikniemi]

Attachment: v3

Docs checked and tested to work on IIS5/W2k.

Comment 30

[Jouni Heikniemi]

Checking in colchange.cgi;
/cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v  <--  colchange.cgi
new revision: 1.38; previous revision: 1.37
done

Comment 31

[John Neilson]

Hope it doesn't mess anything up responding to an old bug like this, but I spent the day wrestling with colchange.cgi on a newly-installed 2.2 site and ended up finding this bug...

Short synopsis: when I would make a few changes to the list of columns and submit it, the server would return a "404 Not Found" error on colchange.cgi.  If I forced the script to output a HTTP header up top then colchange.cgi worked and I could see the cookies it was trying to set, and it would display the "next" page, but the cookies obviously weren't getting set because they were printing to the output page.  Any time the debug header was removed the 404 error returned.

Anyway, after finding this bug I found the lines in the code where you change the cookie-setting depending on what SERVER_SOFTWARE is being used.  On a hunch I commented out the ifs and forced it to use the alternative code, and it worked fine from that point.

My SERVER_SOFTWARE in this case is "iPlanet-WebServer-Enterprise/4.1".  

I don't know if you want to add that to the if statement going forward or what, but consider this another data point on this bug (if I should open this as a New bug instead please let me know).

Thanks.