Closed Bug 214466 Opened 21 years ago Closed 21 years ago

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

Categories

(Bugzilla :: Query/Bug List, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: cbendell, Assigned: jouni)

Details

(Whiteboard: [needed for Win32bz])

Attachments

(1 file, 2 obsolete files)

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
Forgot to mention that this bug is reproduceable with the latest cvs checkout of bugzilla.
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?
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)
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)
I do not have any experience with IIS, but if Jouni doesn't mind, I think he might be able to help.
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: "change columns" does not change columns → nph redirect on column change breaks on IIS
Whiteboard: [needed for Win32bz]
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)
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.
Re: comment #4: for the record, you *can* do domain authentication with Apache. Look for mod_sspi
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?
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).
No, I meant the perl CGI.pm module.
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.
Target Milestone: --- → Bugzilla 2.18
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?
Assignee: justdave → jouni
Summary: nph redirect on column change breaks on IIS → Cookie setting w/ 3xx redirect is broken on IIS (colchange)
I thought the reason we got rid of the Refresh: header is that Internet Explorer didn't support it...
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 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...
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?
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
SERVER_SOFTWARE, actually. See <http://hoohoo.ncsa.uiuc.edu/cgi/env.html>. But yeah, that looks like a feasible approach.
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) (deleted) — Splinter Review
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.
Attachment #135259 - Flags: review?(bbaetz)
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.
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
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 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.
Attachment #135259 - Flags: review?(bbaetz)
Attached patch v2 (obsolete) (deleted) — Splinter Review
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.
Attachment #135259 - Attachment is obsolete: true
Attachment #135831 - Flags: review?(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)
Attachment #135831 - Flags: review?(justdave) → review-
I'm missing a quote there if you cut/paste
Attached patch v3 (deleted) — Splinter Review
Docs checked and tested to work on IIS5/W2k.
Attachment #135831 - Attachment is obsolete: true
Attachment #135835 - Flags: review?(justdave)
Attachment #135835 - Flags: review?(justdave) → review+
Flags: approval?
Flags: approval? → approval+
Checking in colchange.cgi; /cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v <-- colchange.cgi new revision: 1.38; previous revision: 1.37 done
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: