Closed
Bug 313414
Opened 19 years ago
Closed 16 years ago
There is no way to do "sandboxed" http connections that don't modify the cookie list
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: doronr, Assigned: Biesinger)
References
Details
(Keywords: fixed1.8.1)
Attachments
(2 files)
1.23 KB,
patch
|
darin.moz
:
review+
darin.moz
:
superreview+
darin.moz
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
5.07 KB,
patch
|
darin.moz
:
review-
|
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"
Assignee | ||
Comment 1•19 years ago
|
||
I think we should do this in any case
Attachment #200467 -
Flags: superreview?(darin)
Attachment #200467 -
Flags: review?(darin)
Assignee | ||
Comment 2•19 years ago
|
||
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•19 years ago
|
||
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
Attachment #200467 -
Flags: superreview?(darin)
Attachment #200467 -
Flags: superreview+
Attachment #200467 -
Flags: review?(darin)
Attachment #200467 -
Flags: review+
Assignee | ||
Comment 6•19 years ago
|
||
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 | ||
Comment 7•19 years ago
|
||
hey, maybe that second patch can be used for bug 295558
Blocks: 295558
Comment 8•19 years ago
|
||
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).
Assignee | ||
Comment 9•19 years ago
|
||
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.
Assignee | ||
Comment 10•19 years ago
|
||
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 11•19 years ago
|
||
Comment on attachment 200677 [details] [diff] [review] additional patch: channel properties minusing in favor of strong typing.
Attachment #200677 -
Flags: review?(darin) → review-
Assignee | ||
Comment 12•19 years ago
|
||
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?
Reporter | ||
Comment 13•19 years ago
|
||
(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.
Updated•19 years ago
|
Attachment #200467 -
Flags: approval1.8.1? → branch-1.8.1?(darin)
Updated•19 years ago
|
Attachment #200467 -
Flags: branch-1.8.1?(darin) → branch-1.8.1+
Assignee | ||
Comment 14•19 years ago
|
||
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
Assignee | ||
Comment 16•16 years ago
|
||
bug 389508 adds a LOAD_ANONYMOUS load flag, which I think addresses the remaining issues here.
You need to log in
before you can comment on or make changes to this bug.
Description
•