FAQ
Reviewers: bradfitz, volker.dobler,

Message:
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


Description:
net/http/cookiejar: new package.

This CL defines the API. Implementation will come in follow-up CLs.

Update issue 1960.

Please review this at http://codereview.appspot.com/6849092/

Affected files:
A src/pkg/net/http/cookiejar/jar.go
A src/pkg/net/http/cookiejar/storage.go
A src/pkg/net/http/cookiejar/storage_test.go

Search Discussions

  • Rsc at Nov 25, 2012 at 3:58 pm
    LGTM



    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go
    File src/pkg/net/http/cookiejar/jar.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode5
    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/jar.go#newcode44
    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/jar.go#newcode56
    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/storage.go
    File src/pkg/net/http/cookiejar/storage.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode17
    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/storage.go#newcode23
    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/storage.go#newcode25
    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/storage.go#newcode68
    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/storage_test.go
    File src/pkg/net/http/cookiejar/storage_test.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go#newcode12
    src/pkg/net/http/cookiejar/storage_test.go:12: testCases :=
    map[string]bool{
    make global var validStorageKeyTests

    https://codereview.appspot.com/6849092/
  • Dr Volker Dobler at Nov 25, 2012 at 8:51 pm
    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/jar.go
    File src/pkg/net/http/cookiejar/jar.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode44
    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/jar.go#newcode56
    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/jar.go#newcode56
    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/storage.go
    File src/pkg/net/http/cookiejar/storage.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode66
    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?

    https://codereview.appspot.com/6849092/
  • Russ Cox at Nov 25, 2012 at 10:21 pm
    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
  • Nigeltao at Nov 25, 2012 at 11:22 pm
    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/jar.go
    File src/pkg/net/http/cookiejar/jar.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode56
    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/storage.go
    File src/pkg/net/http/cookiejar/storage.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode66
    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/storage.go#newcode68
    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/storage_test.go
    File src/pkg/net/http/cookiejar/storage_test.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go#newcode12
    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.

    https://codereview.appspot.com/6849092/
  • Dr Volker Dobler at Nov 26, 2012 at 12:26 am
    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/jar.go
    File src/pkg/net/http/cookiejar/jar.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode56
    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/storage.go
    File src/pkg/net/http/cookiejar/storage.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode66
    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.

    https://codereview.appspot.com/6849092/
  • Nigeltao at Nov 26, 2012 at 12:54 am
    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go
    File src/pkg/net/http/cookiejar/jar.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/jar.go#newcode56
    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/storage.go
    File src/pkg/net/http/cookiejar/storage.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage.go#newcode66
    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...

    https://codereview.appspot.com/6849092/
  • Nigel Tao at Nov 26, 2012 at 3:38 am

    On Mon, Nov 26, 2012 at 11:54 AM, 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/
  • Rsc at Nov 26, 2012 at 3:52 am
    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/storage_test.go
    File src/pkg/net/http/cookiejar/storage_test.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go#newcode12
    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/storage.go
    File src/pkg/net/http/cookiejar/storage.go (right):

    https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/storage.go#newcode12
    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.

    https://codereview.appspot.com/6849092/
  • Nigeltao at Nov 26, 2012 at 4:01 am
    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go
    File src/pkg/net/http/cookiejar/storage_test.go (right):

    https://codereview.appspot.com/6849092/diff/10001/src/pkg/net/http/cookiejar/storage_test.go#newcode12
    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/storage.go
    File src/pkg/net/http/cookiejar/storage.go (right):

    https://codereview.appspot.com/6849092/diff/10002/src/pkg/net/http/cookiejar/storage.go#newcode12
    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.

    https://codereview.appspot.com/6849092/
  • Dr Volker Dobler at Nov 26, 2012 at 7:58 am
    LGTM

    I wonder if cookiejar should be located under exp:
    Handling IDNs properly will require exp/norm.

    https://codereview.appspot.com/6849092/
  • Nigel Tao at Nov 27, 2012 at 7:48 am

    On Mon, Nov 26, 2012 at 6:58 PM, 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.
  • Nigeltao at Nov 27, 2012 at 7:27 am
    *** 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


    http://codereview.appspot.com/6849092/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 23, '12 at 6:48a
activeNov 27, '12 at 7:48a
posts13
users3
websitegolang.org

3 users in discussion

Nigeltao: 7 posts Rsc: 3 posts Dr Volker Dobler: 3 posts

People

Translate

site design / logo © 2022 Grokbase