Closed Bug 11610 Opened 21 years ago Closed 21 years ago

cookie failure - redirect processing should occur after header processing

Categories

(Core :: Networking, defect, P3)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: hshaw, Assigned: gagan)

References

()

Details

Our current processing of HTTP redirects causes many ecommerce
sites to work poorly with necko.

The sequence of events for many sites is (let's use login as example)

  o present form with user/password fields
  o user enters data/submits
  o server processes form data, authenticates user,
    responds with set-cookie and redirects to real login CGI
  o we process the redirect before firing the OnHeadersAvailable()
    so the set-cookie is lost and the real login CGI thinks cookies
    are disabled.

See
mozilla/netwerk/protocol/http/src/nsHTTPResponseListener::FinishedResponseHeade
rs
for more details.

Suggest processing headers available before processing redirect.

- Hubie
If we call FireOnHeaderAvail() before the ProcessRedirect(), we'll wind up
notifying the user's eventSink twice, which is not good. One option would be to
propagate the response headers over to the "new" response (the one coming from
the redirect); then we'd need to work out some kind of precedence scheme for
headers.
One thing to keep in mind is that (at least in the case of cookies)
you need need the Set-Cookie: headers to be processed (by the cookie
module) before the new redirect request goes out, else the new redirect
request won't have the correct Cookie: headers, causing login operation
(the example in this case) to fail.

BTW Just to see what would happen I stuck a FireOnHeaderAvail() before
the ProcessRedirect().  It's probably doing bad things as you describe,
but logons were working there weren't any catastrophic crashes.
Assignee: gagan → rpotts
Target Milestone: M10
I think it is reasonable to call FireOnHeadersAvailable twice. A client may have
other listeners for any header and hence it does need to know what the original
headers and the new (redirected) one's headers are. Assigning to rpotts and
marking for M10.
Assignee: rpotts → gagan
Target Milestone: M10 → M9
This should be handled for M9. Taking it over and fixing...
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Patch mailed to rpotts.
*** Bug 12262 has been marked as a duplicate of this bug. ***
Status: RESOLVED → REOPENED
Has this patch been checked in? I am using 8/20 M9 candidate builds and still
can not login to the URL above. Also bug 12262, which was marked as a dup, still
goes into an infinite loop.

Re-opening bug. My guess is that the patch was not checked in?
Resolution: FIXED → ---
Clearing Fixed resolution due to reopen.
Gagan, Looks like potts couldn't get your patch in for whatever reason.

Here's how you can do it yourself.

checkout the pertinant files from the branch.

cvs co -r SeaMonkey_M9_BRANCH mozilla/netwerk/protocol/http/whatever.x
(you can do this with entire dirs or modules too)

apply your changes to those files.

cvs stat the one's you changed to confirm that they've got the M9 sticky tag.

cvs commit them.

Jud
When I looked at the M9 branch for

netwerk/protocol/http/src/nsHTTPResponseListener.cpp

It looked like Rick did get the patch in

  revision 1.47.2.3
  date: 1999/08/21 08:54:21;  author: rpotts%netscape.com;
        state: Exp;  lines: +4 -3
  bug #11610.  Make sure that the OnHeadersAvailable notification is fired
  *even* if a http redirect will occur...

The good news, is most of the cookies aren't lost anymore, since
FireOnHeadersAvailable() gets called before the redirect.  The
bad news is the Set-Cookie:'s that come after the redirect (from
the subsequent redirect request) are now lost when they weren't
before.

Or maybe I'm tired and am not reading the code properly.

Re: bug #12262 , I think it may have been closed as a duplicate
a little too soon.  daria.mtv.com is a somewhat complicated page
that includes java.  I'm not sure if cookies are really the problem
or something else.  The cookies appeared to be set properly when I
traced the code.  I think that one requires more investigation and
should be reopened.

On a positive note, I took the patches from bug #'s 11610 and 11630
and put them in my local tip build (didn't check it in) and I can
login to various sites I couldn't before.

The particular site in this URL returns Set-Cookie: headers with
the original HTTP response *and* with the redirected HTTP response.

- Hubie
Target Milestone: M9 → M10
Moving to M10.  We need to get M9 out the door and this will give you folks time
to investigate.

paulmac, please provide M9 Release Note.
hey gagan,

I got your patch but I couldn't tell what was going on since it wasn't a
context diff :-(

It looked like you moved the call to ProcessStatusCode(...) after the call to
FireOnHeadersAvailable(...)but I wasn't sure...

Let me know what should be changed and I'll check it in...

-- rick
I would think this is pretty serious since we are breaking many sites that used
to work in M8.  IMO, this should not be pushed to M10 but should be considered
an M9 show stopper.
The current patch fixes most of the sites I've encountered.  There's far
fewer sites which set cookies after a redirect.

I don't think fixing the problem with setting cookies after the redirect
would be difficult to fix, but if you are really pressed for time, what's
currently there should work for most sites.
*** Bug 12087 has been marked as a duplicate of this bug. ***
Blocks: 12833
Target Milestone: M10 → M12
paulmac why was this reopened? are we still seeing this problem?
Status: REOPENED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
this was re-opened because the stated URL above was not working. However, it is
working now with 9/22 seamonkey, so marking fixed.
Status: RESOLVED → VERIFIED
Bulk move of all Necko (to be deleted component) bugs to new Networking

component.
You need to log in before you can comment on or make changes to this bug.