FAQ
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/

Search Discussions

  • 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 26, '12 at 12:26a
activeNov 27, '12 at 7:48a
posts8
users3
websitegolang.org

3 users in discussion

Nigeltao: 5 posts Dr Volker Dobler: 2 posts Rsc: 1 post

People

Translate

site design / logo © 2021 Grokbase