Cookies set on a 401 are not sent back the when a auth is resent.

VERIFIED FIXED in mozilla0.9.4

Status

()

Core
Networking: Cookies
P3
major
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: J. Nick Koston, Assigned: Paul Chen)

Tracking

({topembed})

Trunk
mozilla0.9.4
x86
Linux
topembed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.3) Gecko/20010802
BuildID:    

When a 401 is sent with a cookie in the headers mozilla doesn't send back the
cookie until you hit cancel and reload the page.




Reproducible: Always
Steps to Reproduce:
1. Send 401 header and a cookie
2. Cookie doesn't get sent back until cancel is pressed and page is reloaded.


Actual Results:  Cookie not sent.

Expected Results:  Cookie should be sent.

This basiclly breaks the a few select few auth schemes.

Comment 1

17 years ago
Need more details here.  In particular a detailed set of steps for reproducing 
this, including the specific URL used.

Comment 2

17 years ago
http://www.burst.net/~nick/mozilla_cookie_bug/setcookie.cgi

Wrote a script to demo this.

Comment 3

17 years ago
Confirming and accepting
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → mozilla0.9.4

Comment 4

17 years ago
Here's what's happening:

1. Go to http://www.burst.net/~nick/mozilla_cookie_bug/setcookie.cgi
2. Click on "here"
3. This causes a GET to http://www.burst.net/~nick/mozilla_cookie_bug/auth.php
4. An authentication dialog is brought up although the page has not yet
   finished loading
5. Fill in the authentication ("test", "test") and press OK
6. The nsHTTPChannel::SetRequestHeader is entered to set cookie header
7. mIsPending is true so this routine does not execute (contains NS_ENSURE_TRUE)
8. Therefore the cookie header has not been updated and the server gets the
   wrong value for the cookie.

So the problem is caused by mIsPending being true in step 7.  It was set to TRUE 
by a preceding call to nsHTTPChannel::AsyncOpen and should have been set back to 
FALSE by a call to nsHTTPChannel::OnStopRequest.  But the call to OnStopRequest 
never occured because the page never finished loading.

A simple patch that would fix this is simply to remove the following line at the 
head of nsHttpChannel::SetRequestHeader

   NS_ENSURE_TRUE(!mIsPending, NS_ERROR_IN_PROGRESS);

So I'm requesting a review of this patch.  If the reviewer feels that there is a 
more fundamental problem here (such as why the page never stops loading), please 
suggest a patch to fix the underlying problem.

cc'ing some networking people since the patch is in nsHTTPChannel, as well as 
some layout people since the problem is caused by the page not finishing loading 
at the time that the authentication dialog is displayed.

rpotts, darin, please review

Comment 5

17 years ago
Created attachment 45853 [details] [diff] [review]
bypass NS_ENSURE_TRUE in SetRequestHeader

Comment 6

17 years ago
that assertion is there for a very good reason... a better fix would be to
toggle the mIsPending flag around the call to OnModifyRequest.

Comment 7

17 years ago
Created attachment 45855 [details] [diff] [review]
alternate solution

Comment 8

17 years ago
Yes, this latter patch also fixes the problem.

r=morse

rpotts, please sr.  Thanks.

Comment 9

17 years ago
Is this the only situation where an existing channel is reused?  I'm wondering
if there are any other situations where it is allowable to modify 'request
attributes' after a connection has been initiated...

If not, then i guess this approach is ok...  

Do we need to worry about the possibility of an OnModifyRequest listener calling
Cancel(...) ?  If so, could mIsPending get incorrectly set to PR_TRUE?  Or will
the proxy listener code defer calling nsHttpChannel::OnStopRequest(...) until
after nsHttpChannel::ProcessAuthentication() has returned...

If Cancelling is not an issue, then r/sr=rpotts

Comment 10

17 years ago
darin, can you answer the questions that rick raised?

Comment 11

17 years ago
authentication is the only case in which we reuse an existing http channel.. i
also confirmed that it is the only other place in which we call OnModifyRequest.

cancelation during an OnModifyRequest callback is almost a NOP because both
mTransaction and mCacheReadRequest would be NULL (mStatus is however set). 
nsHttpChannel::Cancel should probably return an error if !mIsPending, since it
doesn't make any sense to cancel a transaction that is not pending, or we could
just make it a NOP.
(Assignee)

Comment 12

17 years ago
nav triage team:

Marking p3
Priority: -- → P3

Comment 13

17 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 14

17 years ago
we'll need this on the 0.9.2 branch as well (after a day or so of shakeout on
the trunk).
Status: RESOLVED → REOPENED
Keywords: topembed
Resolution: FIXED → ---
(Assignee)

Comment 15

17 years ago
taking from morse since he's going on vacation
Assignee: morse → pchen
Status: REOPENED → NEW
(Assignee)

Comment 16

17 years ago
fix checked into branch
Status: NEW → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 17

17 years ago
verified-on-trunk:

WinNT4 2001082803
Linux rh6 2001082908
Mac os9 2001082908
Whiteboard: verified-on-trunk

Comment 18

17 years ago
*** Bug 99092 has been marked as a duplicate of this bug. ***

Comment 19

17 years ago
verified:

Linux rh6 2001-09-24-04-0.9.4
Status: RESOLVED → VERIFIED

Updated

17 years ago
Whiteboard: verified-on-trunk
You need to log in before you can comment on or make changes to this bug.