Closed Bug 313414 Opened 14 years ago Closed 11 years ago
There is no way to do "sandboxed" http connections that don't modify the cookie list
1.23 KB, patch
|Details | Diff | Splinter Review|
5.07 KB, patch
|Details | Diff | Splinter Review|
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"
I think we should do this in any case
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 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
Probably too late to get this into 1.8, right?
yeah, I'd think so...
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...)
Assignee: darin → cbiesinger
Status: NEW → ASSIGNED
Attachment #200677 - Flags: review?(darin)
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).
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 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
Attachment #200467 - Attachment description: change order of cookie setting and OnExamineResponse → change order of cookie setting and OnExamineResponse (checked in)
Comment on attachment 200677 [details] [diff] [review] additional patch: channel properties minusing in favor of strong typing.
Attachment #200677 - Flags: review?(darin) → review-
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.
Attachment #200467 - Flags: approval1.8.1?
(In reply to comment #12) > (From update of attachment 200467 [details] [diff] [review] ) > 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.
Attachment #200467 - Flags: approval1.8.1? → branch-1.8.1?(darin)
Attachment #200467 - Flags: branch-1.8.1?(darin) → branch-1.8.1+
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
bug 389508 adds a LOAD_ANONYMOUS load flag, which I think addresses the remaining issues here.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Depends on: xxx
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.