250375 - Cookies set using nsHttpChannel::setRequestHeader get cleared by the cookie service

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Doron Rosenberg (IBM)]

Trying to set cookies using nsIHttpChannel::setRequestHeader doesn't work since
when the channel is opened, the cookies get cleared and loaded in from the
cookie service.

Comment 1

[Darin Fisher]

In AsyncOpen, we call AddCookieToRequest.  That function calls
mRequestHead.SetCookie passing PR_FALSE, indicating that any existing value for
the Cookie header should be overwritten.  This function is also called from
DoAuthRetry.  We cannot simply merge cookies in AddCookieToRequest since that
might cause us to add the same cookie twice when calling AddCookieToRequest from
DoAuthRetry.  We cannot get around this by simply passing PR_FALSE when
AddCookieToRequest is called from DoAuthRetry either.  I think the right
solution is to store the value of the Cookie header prior to calling
AddCookieToRequest from AsyncOpen.  In AddCookieToRequest we should merge the
stored value with whatever the cookie service provides.

Comment 2

[[e-mail]]

darin, you make it too easy for us :)

-> me

Comment 3

[[e-mail]]

>We cannot get around this by simply passing PR_FALSE when AddCookieToRequest is
>called from DoAuthRetry either.

hmm, can you elaborate? i'd note that the cookieservice's GetCookieString
delimits multiple cookies with ";", whereas nsHttpHeaderArray::SetHeader uses
"," when merging headers, so that could be a reason why we can't use the merging
functionality. but i don't think that's what you were getting at...

Comment 4

[[e-mail]]

Attachment: concatenate headers in AddCookiesToRequest

this does (i think) what darin said in comment 1, but i still don't see why the
merging wouldn't work.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

what's the status of this patch?

Comment 6

[Darin Fisher]

Hrm... so with the current patch, wouldn't we lose the cookies set via
setRequestHeader if the channel goes through the DoAuthRetry code path?  It
seems like we need to store the cookies set via setRequestHeader in some member
variable so that we can merge them again after we have overwritten the value of
the "Cookie" request header.  Does this make sense?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: unit test for this bug

adds a unit test for this bug. The reason I didn't add it to
test_http_headers.js is that this testcase depends on a working network
connection. (but maybe that would not be a problem?)

should we have an XFAIL list for tests that are currently known to fail?

A makefile target that runs these tests would also be nice

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 192217 [details] [diff] [review]
unit test for this bug

I have a better testcase...

Comment 9

[Darin Fisher]

I think we should make the tinderbox turn orange if any of these tests fail.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: test + patch

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

this patch differs from the original patch in this bug in that it appends the
user-set headers after the ones from the cookie service. I did it that way
assuming that later headers would override earlier ones, which seems like the
desired behaviour for explicitly-set headers vs implicit ones.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #9)
> I think we should make the tinderbox turn orange if any of these tests fail.

filed bug 304162

Comment 13

[Darin Fisher]

Comment on attachment 192221 [details] [diff] [review]
test + patch

Looks good, r+sr=darin

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 192221 [details] [diff] [review]
test + patch

low-risk patch to allow XMLHttpRequest users (and other necko-using code) to
set a Cookie header

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 192221 [details] [diff] [review]
test + patch

actually, I have a slightly better testcase

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: test + patch, v1.1

the only difference is that the unit test now only does its second part if the
first one succeeded.

Comment 17

[Chris Beard]

Comment on attachment 192300 [details] [diff] [review]
test + patch, v1.1

moving r+sr forward and approving

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in except for the test_all.sh change, which got checked in as part of
bug 63368

HEAD:
Checking in netwerk/protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <-- 
nsHttpChannel.cpp
new revision: 1.257; previous revision: 1.256
done
Checking in netwerk/protocol/http/src/nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.71; previous revision: 1.70
done
Checking in netwerk/test/Makefile.in;
/cvsroot/mozilla/netwerk/test/Makefile.in,v  <--  Makefile.in
new revision: 1.86; previous revision: 1.85
done
Checking in netwerk/test/unit/test_http_headers.js;
/cvsroot/mozilla/netwerk/test/unit/test_http_headers.js,v  <--  test_http_headers.js
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_cookie_header.js,v
done
Checking in netwerk/test/unit/test_cookie_header.js;
/cvsroot/mozilla/netwerk/test/unit/test_cookie_header.js,v  <-- 
test_cookie_header.js
initial revision: 1.1
done

MOZILLA_1_8_BRANCH:
Checking in netwerk/protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <-- 
nsHttpChannel.cpp
new revision: 1.256.2.1; previous revision: 1.256
done
Checking in netwerk/protocol/http/src/nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.70.4.1; previous revision: 1.70
done
Checking in netwerk/test/Makefile.in;
/cvsroot/mozilla/netwerk/test/Makefile.in,v  <--  Makefile.in
new revision: 1.85.2.1; previous revision: 1.85
done
Checking in netwerk/test/unit/test_cookie_header.js;
/cvsroot/mozilla/netwerk/test/unit/test_cookie_header.js,v  <-- 
test_cookie_header.js
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in netwerk/test/unit/test_http_headers.js;
/cvsroot/mozilla/netwerk/test/unit/test_http_headers.js,v  <--  test_http_headers.js
new revision: 1.5.2.1; previous revision: 1.5
done