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

RESOLVED FIXED

Status

()

Core
Networking: HTTP
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Doron Rosenberg (IBM), Assigned: Biesinger)

Tracking

({fixed1.8.1})

Trunk
x86
All
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
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"
Created attachment 200467 [details] [diff] [review]
change order of cookie setting and OnExamineResponse (checked in)

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 3

12 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+
(Reporter)

Comment 4

12 years ago
Probably too late to get this into 1.8, right?
yeah, I'd think so...
Created attachment 200677 [details] [diff] [review]
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...)
Assignee: darin → cbiesinger
Status: NEW → ASSIGNED
Attachment #200677 - Flags: review?(darin)
hey, maybe that second patch can be used for bug 295558
Blocks: 295558

Comment 8

12 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).
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 11

12 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-
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

12 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.
Attachment #200467 - Flags: approval1.8.1? → branch-1.8.1?(darin)

Updated

12 years ago
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

Comment 15

11 years ago
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
Last Resolved: 9 years ago
Depends on: 389508
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.