Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http/cookiejar: strips additional information #39420

Open
kaos opened this issue Jun 5, 2020 · 7 comments
Open

net/http/cookiejar: strips additional information #39420

kaos opened this issue Jun 5, 2020 · 7 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@kaos
Copy link

kaos commented Jun 5, 2020

cookies = append(cookies, &http.Cookie{Name: e.Name, Value: e.Value})

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
Copy link
Contributor

dmitshur commented Jun 5, 2020

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

  • What did you do?
  • What did you expect to see?
  • What did you see instead?

@dmitshur dmitshur changed the title net/http/cookiejar: Strips additional information net/http/cookiejar: strips additional information Jun 5, 2020
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jun 5, 2020
@dmitshur dmitshur added this to the Backlog milestone Jun 5, 2020
@kaos
Copy link
Author

kaos commented Jun 6, 2020

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
Copy link

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
Copy link
Author

kaos commented Jul 12, 2021

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

@dmitshur dmitshur removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jul 13, 2021
@networkimprov
Copy link

cc @bradfitz @neild @fraenkel

@neild
Copy link
Contributor

neild commented Jul 21, 2021

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
Copy link
Contributor

vdobler commented Jul 27, 2021

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 #17587 : How to read data out of a Jar?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants