313414 - There is no way to do "sandboxed" http connections that don't modify the cookie list

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Doron Rosenberg (IBM)]

Right now, a xpcom http connection will send and store the cookies the user has
stored.  For example, in my Gmail notifier extension, whenever I do a
httpconnection to do a screenscrape, I may be overwritting the user's cookies
for Gmail, which isn't good :)

The "http-on-modify-request" observer topic allows one to modify the cookies
being sent prior to the connection being established, but there is no way to
modify/remove cookies on the response object before they get processed.

"http-on-examine-response" is sent after cookies are processed
(http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#779).

Talked to biesi on IRC about this, and he suggested we could move
"http-on-examine-response" before the cookie setting.  Or perhaps send another
topic for that?

Aso:

biesi	wonders if we should also have a flag for necko not to set cookies
biesi	chan.nsIWritablePropertyBag2.setPropertyAsBoolean("cookies-enabled", false);
biesi	hm, should probably be split into two parts
biesi	"send-cookies" and "store-received-cookies"

Comment 1

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

Attachment: change order of cookie setting and OnExamineResponse (checked in)

I think we should do this in any case

Comment 2

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

to elaborate on that patch a bit... it seems unlikely to me that anyone would
depend on the cookie already being set by the time OnExamineResponse is being
called. Setting the cookie does not affect the behaviour of this channel in any
way, it only affects new connections.

Comment 3

[Darin Fisher]

Comment on attachment 200467 [details] [diff] [review]
change order of cookie setting and OnExamineResponse (checked in)

Yeah, I agree with this.  The idea of that notification is to allow the observer to modify the response received from the server.  r+sr=darin

Comment 4

[Doron Rosenberg (IBM)]

Probably too late to get this into 1.8, right?

Comment 5

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

yeah, I'd think so...

Comment 6

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

Attachment: additional patch: channel properties

do you think this is a good idea too? it allows configuring sending/processing of cookies per channel.

I haven't tested this yet, but if you agree with this I'll write a unit test for this (if I can come up with a good way to test the process-cookies property...)

Comment 7

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

hey, maybe that second patch can be used for bug 295558

Comment 8

[Darin Fisher]

biesi: i'm not sure about using properties for that.  i like supporting properties for user data, but i'm not sure about using properties to implement new APIs.  it might make more sense as a nsIHttpChannel2::cookieFlags.  i know i've been a proponent of using properties for this in the past, but i'm starting to have second thoughts about that.  for instance, storing the content length as a property is not so great since it would be copied when a redirect occurs.  this means of course that the property should not be set until after it is determined that the channel will not redirect.  i'd like to hold off on introducing property based APIs.  i think there are key advantages to defining interfaces (ability to detect support for a feature at runtime being a big one, but strong typing is another).

Comment 9

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

I filed a bug somewhere about flags for properties to allow them to be marked as noncopyable, but yeah, lack of strong typing is indeed a reason why I didn't like them too much initially, though I don't mind that so much lately...

I want to note here that copying these specific properties has definite advantages, because that means that for a redirect, the cookie settings will be automatically followed for the new channel too.

Comment 10

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

Comment on attachment 200467 [details] [diff] [review]
change order of cookie setting and OnExamineResponse (checked in)

Checking in netwerk/protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChannel.cpp
new revision: 1.263; previous revision: 1.262
done

Comment 11

[Darin Fisher]

Comment on attachment 200677 [details] [diff] [review]
additional patch: channel properties

minusing in favor of strong typing.

Comment 12

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

Comment on attachment 200467 [details] [diff] [review]
change order of cookie setting and OnExamineResponse (checked in)

I think this patch would be good in 1.8.1. It allows extensions to block setting certain cookies easily, and is low risk.

Comment 13

[Doron Rosenberg (IBM)]

(In reply to comment #12)
> (From update of attachment 200467 [details] [diff] [review] [edit])
> I think this patch would be good in 1.8.1. It allows extensions to block
> setting certain cookies easily, and is low risk.
> 

Indeed, this would be great for extension authors.

Comment 14

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

order change patch checked in on 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.5; previous revision: 1.256.2.4
done

Comment 15

[Jesse Ruderman]

Marking fixed1.8.1 based on comment 14.

Comment 16

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

bug 389508 adds a LOAD_ANONYMOUS load flag, which I think addresses the remaining issues here.