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

Categories

(Core :: Networking: HTTP, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: doronr, Assigned: Biesinger)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files)

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
Attachment #200467 - Flags: superreview?(darin)
Attachment #200467 - Flags: review?(darin)
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
Attachment #200467 - Flags: superreview?(darin)
Attachment #200467 - Flags: superreview+
Attachment #200467 - Flags: review?(darin)
Attachment #200467 - Flags: review+
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)
hey, maybe that second patch can be used for bug 295558
Blocks: 295558
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] [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.
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
Marking fixed1.8.1 based on comment 14.
Keywords: fixed1.8.1
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.