Issue 6849092: code review 6849092: net/http/cookiejar: new package. (Closed)
|
Created:
11 years, 5 months ago by nigeltao Modified:
11 years, 4 months ago Reviewers:
CC:
bradfitz, volker.dobler, rsc, golang-dev Visibility:
Public. |
Descriptionexp/cookiejar: new package.
This CL defines the API. Implementation will come in follow-up CLs.
Update issue 1960.
Patch Set 1 #Patch Set 2 : diff -r f2755950769b https://code.google.com/p/go #Patch Set 3 : diff -r f2755950769b https://code.google.com/p/go #Patch Set 4 : diff -r f2755950769b https://code.google.com/p/go #Patch Set 5 : diff -r f2755950769b https://code.google.com/p/go #
Total comments: 22
Patch Set 6 : diff -r cda840e2befc https://code.google.com/p/go #
Total comments: 2
Patch Set 7 : diff -r cda840e2befc https://code.google.com/p/go #Patch Set 8 : diff -r 801594ac5efd https://code.google.com/p/go #Patch Set 9 : diff -r 801594ac5efd https://code.google.com/p/go #
MessagesTotal messages: 13
Hello bradfitz@golang.org, dr.volker.dobler@gmail.com (cc: golang-dev@googlegroups.com, rsc@golang.org), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/jar.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/jar.go:5: // Package cookiejar provides a http.CookieJar implementation that conforms Package cookiejar implements an RFC 6265-compliant http.CookieJar. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/jar.go:44: // Jar implements the http.CookieJar interface from the net/http package. Do you want people to modify the jar after construction? I am wondering if we should have a cookiejar.New(*Options) instead. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly cookies? Do we post-process the return I don't understand this TODO. Both the Cookies and SetCookies methods have a URL so it seems like they can tell HTTP from HTTPS. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:17: // A key is invalid if it contains a byte that is not '-', not '.', is outside A valid key must use only bytes in the character class [a-z0-9.-] and must have at least one non-. byte. Note that this excludes any key containing a capital ASCII letter as well as the empty string. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:23: // The Add method has undefined behavior if the entry's Data has zero length. An added Entry will always have non-empty Data. (maybe move down to the Add method comment) https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:25: // A Jar will not call a Storage's methods, other than Lock, unless it has I think this might work better as comments on the Lock and Unlock methods, if you expand sync.Locker (which most people have probably not seen): // A client must call Lock before calling other methods and must call // Unlock when finished. Between the calls to Lock and Unlock, a client // can assume that other clients are not modifying the Storage. Lock() Unlock() https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:68: Data []byte You spend a lot of time telling people not to mutate the []byte. This is ASCII data, though, right? So maybe it would be okay to use string? https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage_test.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage_test.go:12: testCases := map[string]bool{ make global var validStorageKeyTests
Sign in to reply to this message.
Nothing is fundamentally wrong with this API, but I admit I don't see what the rationals for this API are. Having a Storage with opaque Data forces Jar to serialize Cookies and the filter functions f to decode this serialization before doing actual work. Is it about squeezing the booleans HostOnly, Secure and HttpOnly into a bitfield and compressing Value to minimize the memory footprint of an in-memory Storage? Or is it just a way to avoid an additional type StoredCookie which would share several fields with http.Cookie? Having LastAccess exposed in Entry and managed by Storage is nice, but only if LastAccess is used somewhere: The only two task where LastAccess is ever used is a) display in a GUI and b) during compactification of the jar when LastAccess helps to decide which cookies to drop from the jar. Such functions should be part of the API, at least if we are heading for a general "suits-everything" cookie jar. Just to be sure: The general design is fixed to "Jar delegates all cookie storage to an interface type Storage"? Or are other designs like "Package cookiejar provides an efficent in-memory cookie jar as well as functions to build specialized cookie jars." or "The way chromium/firefox does." still discussed? https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/jar.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/jar.go:44: // Jar implements the http.CookieJar interface from the net/http package. Russ's Options could be used for stuff like allowing host cookies on IP addresses and safeguarding against excessive long Value. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly cookies? Do we post-process the return On 2012/11/25 15:58:51, rsc wrote: > I don't understand this TODO. Both the Cookies and SetCookies methods have a URL > so it seems like they can tell HTTP from HTTPS. It is (I assume) about telling HTTP(S) from FTP and other protocols, not about telling HTTP from HTTPS (which is controlled by Secure field). https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly cookies? Do we post-process the return Allowing an ftp server to set a cookie which gets sent to a HTTP request (and the other way around) might be allowed in theory, but most probably this is a bad idea. I would be strict (and safe): "Jar will neither accept cookies from, nor send cookies to any URL which is neither HTTP nor HTTPS." https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:66: Creation time.Time Why is Creation treated special here? Created is set once like Name or Path and not used for retrieval (it is not even used for cleanup or shrinking the jar). Just because LastAccess is handled special?
Sign in to reply to this message.
There is only one Jar implementation here. The Storage interface is structured so that it is as simple as possible to implement and as difficult as possible to implement incorrectly. Russ
Sign in to reply to this message.
The rationale for this design, as bradfitz mused earlier, is that the implementor of an SQLiteStorage or a LevelDBStorage shouldn't have to know anything about HTTP cookies and all its subtleties. Note that in this proposal, storage.go does not import "net/http". I haven't decided yet whether Jar has its own in-memory storage. This design does allow for a CachingStorage to wrap another Storage, so it doesn't have to be the responsibility of Jar per se. On a related note, a Jar may want its own in-memory storage for non-persistent cookies. I'm still exploring this space. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/jar.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly cookies? Do we post-process the return On 2012/11/25 15:58:51, rsc wrote: > I don't understand this TODO. Both the Cookies and SetCookies methods have a URL > so it seems like they can tell HTTP from HTTPS. Heh, the HttpOnly name is unfortunate, but out of our control. It has nothing to do with FTP, from Go's point of view. It means that the cookie is used for HTTP(S) requests, but is not viewable from a page's javascript (via document.cookie). It mitigates XSS attacks. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:66: Creation time.Time On 2012/11/25 20:50:58, volker.dobler wrote: > Why is Creation treated special here? Created is set once > like Name or Path and not used for retrieval (it is not > even used for cleanup or shrinking the jar). Just because > LastAccess is handled special? It's here because when a newly set cookie replaces an existing cookie (replaces means same name/domain/path), then the new cookie is stored with the creation time of the old cookie. (RFC 6265 Section 5.3 Step 11). On second thoughts, creation time could arguably be encoded in the Data []byte, but it could be easier to manage as a separate Entry field. This API isn't set in stone yet; presumably implementing a MemStorage and possibly a SQLiteStorage or LevelDBStorage will shake out the design. LastAccess is a separate field because sending an HTTP request scans the cookies and updates the LastAccess of each to time.Now. Being a separate field means that the Data bytes don't need a decode-modify-encode step for this common operation. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:68: Data []byte On 2012/11/25 15:58:51, rsc wrote: > You spend a lot of time telling people not to mutate the []byte. > This is ASCII data, though, right? So maybe it would be okay to use string? Yeah, string might be better. I was trying to avoid []byte <--> string conversions to and from storage, but that's possibly premature optimization. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage_test.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage_test.go:12: testCases := map[string]bool{ On 2012/11/25 15:58:51, rsc wrote: > make global var validStorageKeyTests Why? I prefer local variables over global variables, and I think it's just as readable.
Sign in to reply to this message.
I see.
Why not taking the identifying (name,domain,path)-tuple
out of the opaque Data like this:
type Entry {
Creation, LastAccess time.Time
Id string // e.g. Name + ";" + Domain + ";" + Path
Data []byte // just Value and flags left
}
and Storage must treat any Entry with the same Id
as identical in the set of a given key? This wouldn't
make a DB-backed Storage much more complicated and
Storage can be required to manage Creation as it know
knows if two cookies are the same (even if it doesn't
need to know anything about cookies at all).
This would remove the need of Jar to query Storage
first before Add'ing a cookie.
https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/...
File src/pkg/net/http/cookiejar/jar.go (right):
https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/...
src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly
cookies? Do we post-process the return
On 2012/11/25 23:22:35, nigeltao wrote:
> On 2012/11/25 15:58:51, rsc wrote:
> > I don't understand this TODO. Both the Cookies and SetCookies methods have a
> URL
> > so it seems like they can tell HTTP from HTTPS.
>
> Heh, the HttpOnly name is unfortunate, but out of our control.
>
> It has nothing to do with FTP, from Go's point of view. It means that the
cookie
> is used for HTTP(S) requests, but is not viewable from a page's javascript
(via
> document.cookie). It mitigates XSS attacks.
Yes, access through document.cookie is one of those
"non-HTTP" APIs RFC 6265 forbids in 5.4.1 bullet 4.
Am I wrong in assuming that every other (incl. FTP or
jet-to-invent protocols) are excluded too?
https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/...
File src/pkg/net/http/cookiejar/storage.go (right):
https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/...
src/pkg/net/http/cookiejar/storage.go:66: Creation time.Time
On 2012/11/25 23:22:35, nigeltao wrote:
> On 2012/11/25 20:50:58, volker.dobler wrote:
> > Why is Creation treated special here? Created is set once
> > like Name or Path and not used for retrieval (it is not
> > even used for cleanup or shrinking the jar). Just because
> > LastAccess is handled special?
>
> It's here because when a newly set cookie replaces an existing cookie
(replaces
> means same name/domain/path), then the new cookie is stored with the creation
> time of the old cookie. (RFC 6265 Section 5.3 Step 11).
>
> On second thoughts, creation time could arguably be encoded in the Data
[]byte,
> but it could be easier to manage as a separate Entry field. This API isn't set
> in stone yet; presumably implementing a MemStorage and possibly a
SQLiteStorage
> or LevelDBStorage will shake out the design.
>
> LastAccess is a separate field because sending an HTTP request scans the
cookies
> and updates the LastAccess of each to time.Now. Being a separate field means
> that the Data bytes don't need a decode-modify-encode step for this common
> operation.
LastAccess as a separate field is perfectly fine,
It is managed by Storage. But who is going to manage
Creation? If Storage is responsible for updating
than Storage must have know-how about the opaque Data;
otherwise it cannot know if two cookies are the same.
If Jar manages this field, then it can be envoded in Data
as well. If Jar manages Creation, then Jar will have
to look up any cookie with Entries before Add'ing
them to choose the right Creation time.
Sign in to reply to this message.
https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/jar.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/jar.go:56: // TODO: how do we reject HttpOnly cookies? Do we post-process the return On 2012/11/26 00:26:39, volker.dobler wrote: > Yes, access through document.cookie is one of those > "non-HTTP" APIs RFC 6265 forbids in 5.4.1 bullet 4. > Am I wrong in assuming that every other (incl. FTP or > jet-to-invent protocols) are excluded too? I think that net/http/cookiejar would only care about HTTP(S). FTP and other transport protocols are out of scope. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:66: Creation time.Time On 2012/11/26 00:26:39, volker.dobler wrote: > LastAccess as a separate field is perfectly fine, > It is managed by Storage. But who is going to manage > Creation? If Storage is responsible for updating > than Storage must have know-how about the opaque Data; > otherwise it cannot know if two cookies are the same. > If Jar manages this field, then it can be envoded in Data > as well. If Jar manages Creation, then Jar will have > to look up any cookie with Entries before Add'ing > them to choose the right Creation time. I think the principle is that Storage is as dumb as possible. Again, I'd like the programmer who implements SQLiteStorage not to have to know anything about HTTP cookies. Yes, a Jar will need to scan existing cookies before Add'ing new ones. We could move name/domain/path into an Entry instead, but it would make a Storage more complicated. Hmm...
Sign in to reply to this message.
On Mon, Nov 26, 2012 at 11:54 AM, <nigeltao@golang.org> wrote: > We could move name/domain/path into an Entry instead, but it would make > a Storage more complicated. Hmm... I've uploaded a version that puts the domain/name/path (which I've called a SubKey) into an Entry. PTAL at https://codereview.appspot.com/6849092/
Sign in to reply to this message.
I haven't thought enough about whether the Subkey needs to be exposed. https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage_test.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage_test.go:12: testCases := map[string]bool{ On 2012/11/25 23:22:35, nigeltao wrote: > On 2012/11/25 15:58:51, rsc wrote: > > make global var validStorageKeyTests > > Why? I prefer local variables over global variables, and I think it's just as > readable. Because 99% of the other tests do that. Some of the reasons: it avoids some indentation, it makes it easier to scan forward to find where the table is done (completely unindented lines), and it makes the table available to some future test function that might be able to reuse the cases. When I see a local variable I like this I expect it contains a value that can only be computed once the test has started running. There's no such value here. https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage.go (right): https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:12: // entries. Each entry consists of a sub-key, creation time, last access time, s/sub-key/subkey/ subkey is a fine English word without needing a hyphen. Because of that fact, the name below should be Subkey not SubKey.
Sign in to reply to this message.
https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage_test.go (right): https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage_test.go:12: testCases := map[string]bool{ On 2012/11/26 03:52:55, rsc wrote: > On 2012/11/25 23:22:35, nigeltao wrote: > > On 2012/11/25 15:58:51, rsc wrote: > > > make global var validStorageKeyTests > > > > Why? I prefer local variables over global variables, and I think it's just as > > readable. > > Because 99% of the other tests do that. Some of the reasons: it avoids some > indentation, it makes it easier to scan forward to find where the table is done > (completely unindented lines), and it makes the table available to some future > test function that might be able to reuse the cases. > > When I see a local variable I like this I expect it contains a value that can > only be computed once the test has started running. There's no such value here. Done. https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/... File src/pkg/net/http/cookiejar/storage.go (right): https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/... src/pkg/net/http/cookiejar/storage.go:12: // entries. Each entry consists of a sub-key, creation time, last access time, On 2012/11/26 03:52:55, rsc wrote: > s/sub-key/subkey/ > subkey is a fine English word without needing a hyphen. > Because of that fact, the name below should be Subkey not SubKey. Done.
Sign in to reply to this message.
LGTM I wonder if cookiejar should be located under exp: Handling IDNs properly will require exp/norm.
Sign in to reply to this message.
On Mon, Nov 26, 2012 at 6:58 PM, <dr.volker.dobler@gmail.com> wrote: > I wonder if cookiejar should be located under exp: > Handling IDNs properly will require exp/norm. Hmm... I'll move it to exp/cookiejar for now.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=69e2f3d33300 *** exp/cookiejar: new package. This CL defines the API. Implementation will come in follow-up CLs. Update issue 1960. R=bradfitz, dr.volker.dobler, rsc CC=golang-dev http://codereview.appspot.com/6849092
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
