golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.97k stars 17.67k forks source link

net/http/cookiejar: strips additional information #39420

Open kaos opened 4 years ago

kaos commented 4 years ago

https://github.com/golang/go/blob/50bd1c4d4eb4fac8ddeb5f063c099daccfb71b26/src/net/http/cookiejar/jar.go#L219

Is there a purpose to leave out everything apart from the name and value here? I'd like to know how long time a cookie has left to live..

dmitshur commented 4 years ago

Thanks. To make this issue more actionable, can you please provide answers to these questions:

kaos commented 4 years ago

Certainly. (my misconception was based on me not realising that the jar prunes all expired cookies.) If that is in the documentation, I overlooked it.

What did you do?

func (reporter *CDIReporter) sessionValid() bool {
    for _, cookie := range reporter.Client.Jar.Cookies(reporter.CDIUrl) {
        if cookie.Name == "SessionID" {
            return time.Now().Before(cookie.Expired)
        }
    }

    return false
}

What did you expect to see?

I expected to get a true result from sessionValid() just after acquiring the session cookie.

What did you see instead?

I got false. (from the return inside the for loop, not after, as there was a matching cookie.)

Now, I've resolved it like this (but I miss being able to know for how long the session is going to be valid, so I can refresh it deterministically, rather than on a hunch/fixed interval/etc..)

func (reporter *CDIReporter) sessionValid() bool {
    for _, cookie := range reporter.Client.Jar.Cookies(reporter.CDIUrl) {
        if cookie.Name == "SessionID" {
            // the cookie is deleted from the jar when expired
            return true
        }
    }

    return false
}
boggydigital commented 4 years ago

I believe I hit the same issue. Use case: I have a CLI application that is trying to preserve and reuse session cookies between individual runs, so on application shutdown I'm serializing cookies from the jar and then on relaunch re-populating jar with deserialized cookies.

Current behavior is not a show-stopper, since I can set some sensible defaults for other properties - however it's not clear what's the intent for stripping all extra information from a cookie.

Looking around GitHub I found couple instances where:

On a positive note: I saw many cases where all folks are using were indeed name/value.

kaos commented 3 years ago

@dmitshur still waiting for more info? Or this just haven't got updated labels..? ;)

networkimprov commented 3 years ago

cc @bradfitz @neild @fraenkel

neild commented 3 years ago

I'm not familiar with why the current behavior was chosen and may be missing something, but I don't see any reason why (*cookiejar.Jar).Cookies couldn't preserve additional fields. The http package only uses the Name and Value and will ignore other fields in cookies returned by an http.CookieJar implementation.

cc @vdobler, who wrote the initial implementation of Cookies.

vdobler commented 3 years ago

The reason why net/http/cookiejar.Jar behaves the way it does is because it is the minimal implementation usable in net/http.Client.Jar. It is not a "serves any purpose" cookie jar. A lot of things are left out, from handling HttpOnly attributes to persisting (and reloading) the state. Russ once put it (paraphrasing):

I'd expect net/http to be able to log into \<major newspaper> and download the crossword puzzle."

net/http/cookiejar.Jar doesn't do more than what is needed for that task.

E.g. RFC 6265 requires to delete expired cookies the moment they expire (if I remember correctly). Jar doesn't; which is "okay" as you do not have a direct API to read the Jar content.

There where several long discussions with Nigel on if / how to provide persistence. All fruitless. There are forks of net/http/cookiejar which do provide persisting a Jar and reloading the persisted data.

While technically true that *Jar.Cookies could populate extra fields I think this is dangerous as *http.Cookie.String behaves differently depending on which fields are present.

A real fix would be a solution to https://github.com/golang/go/issues/17587 : How to read data out of a Jar?