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: add field Cookie.Quoted bool #46443

Closed
gazerro opened this issue May 29, 2021 · 23 comments
Closed

net/http: add field Cookie.Quoted bool #46443

gazerro opened this issue May 29, 2021 · 23 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted
Milestone

Comments

@gazerro
Copy link
Contributor

gazerro commented May 29, 2021

For the RFC 6265, the double-quotes are part of the cookie value but the functions and methods in the standard library that operates on cookies treat them as if they were not part of it.

The SetCookie function does not allow to send a cookie, that conforms to the spec, with a double-quoted value and the (*Request).Cookie method strips the quotes from the value despite the double-quotes are part of it.

The syntax in the RFC 6265 is

cookie-pair   = cookie-name "=" cookie-value
...
cookie-value  = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )

but it has been implemented in the standard library as

cookie-pair   = cookie-name "=" ( cookie-value / ( DQUOTE cookie-value DQUOTE ) )
...
cookie-value  = *cookie-octet

The author of the RFC 6265 has confirmed in https://lists.w3.org/Archives/Public/ietf-http-wg/2017JanMar/0229.html that this was the intent.

The draft https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-02 added this note to the spec

Per the grammar above, the cookie-value MAY be wrapped in DQUOTE
characters.  Note that in this case, the initial and trailing DQUOTE
characters are not stripped.  They are part of the cookie-value, and
will be included in Cookie headers sent to the server.

and in the appendix reports this discussion https://issues.apache.org/jira/browse/HTTPCLIENT-1006.

@vdobler
Copy link
Contributor

vdobler commented May 29, 2021

For the RFC 6265, the double-quotes are part of the cookie value but the functions and methods in the standard library that operates on cookies treat them as if they were not part of it.

This is wrong. The optional double-quotes around a cookie are not part of the value. The standard library is correct.

@gazerro
Copy link
Contributor Author

gazerro commented May 30, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/marco/Library/Caches/go-build"
GOENV="/Users/marco/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/marco/go/pkg/mod"
GOOS="darwin"
GOPATH="/Users/marco/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.4"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/z9/xrln_qks56jbzxjbhs04fpm80000gn/T/go-build950868117=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Call an HTTP server that returns cookies with double-quoted values using http.Client.

https://play.golang.org/p/Ka8G_NWPu0n

What did you expect to see?

I expect to see

Cookie: [a=; b=v; c=""; d="v"]

The client sends the cookies to the server preserving the values ​​without stripping the double-quotes.

I also tested it, using this server, with all major browsers (Chrome, Firefox, Safari, Edge, Opera) on MacOS, Windows, iOS and Android with the latest versions and outdated versions (as Internet Explorer 6 on Windows XP, Safari 4 on MacOS Snow Leopard, iPhone 4, default browser on Android 4.4.2) and all browsers display

Cookie: [a=; b=v; c=""; d="v"]

What did you see instead?

Cookie: [a=; b=v; c=; d=v]

The client sends the cookies stripping the double-quotes.

@networkimprov
Copy link

cc @neild @empijei @fraenkel

@fraenkel
Copy link
Contributor

I believe there is some confusion over what the Cookie.Value represents. My reading is that its the cookie-octets. The double quotes are inserted as necessary (spaces and commas). There is no way to force the value to always be double quoted.

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 30, 2021
@neild
Copy link
Contributor

neild commented Jun 1, 2021

RFC 6265 is quite clear that double-quotes are part of the cookie-value when present. The standards-track RFC 6265-bis emphasizes this point, but the grammar in RFC 6265 is unambiguous.

Empirically, ("net/http".Cookie).Value contains the unquoted cookie-octets from the cookie-value. The (*Cookie.String) function adds surrounding DQUOTE characters if the value contains a space or comma and strips any existing DQUOTE characters from the Value. (Interestingly, the only time (*Cookie).String returns a quoted value is when the value is invalid under RFC 6265, which forbids spaces and commas in values.) The (*Request).Cookie function removes DQUOTE characters from cookies.

@gazerro
Copy link
Contributor Author

gazerro commented Jun 2, 2021

After some investigation, with this message I will explain the source of the problem

  • the Cookie.Value field does not represent the cookie value

and its consequences

  • a Cookie.Value value cannot be used to represent cookies with values in the form DQUOTE *cookie-octet DQUOTE
  • no implementation of the http.CookieJar interface can be standard compliant
  • the http.Client type does not conform the RFC 6265 when storing cookies

Also, I will propose some solutions.

Source of the problem

The Cookie.Value field does not represent the cookie value (cookie-value in the grammar of the RFC 6265) but *cookie-octect (as @fraenkel and @neild also noted, although not necessarily as a problem).

Consequences

Consequently, a Cookie.Value value, apart from non standard compliant cookies, cannot be used to represent cookies with values in the form DQUOTE *cookie-octet DQUOTE.

A type that implements the http.CookieJar interface, such as the cookiejar.Jar type, cannot be used to store cookies with DQUOTE because http.CookieJar methods receive and return cookies represented with http.Cookie values. The cookiejar.Jar type is documented as an in-memory RFC 6265-compliant http.CookieJar, but as I pointed out, no implementation of the http.CookieJar interface can conform to the standard.

http.Client uses the http.CookieJar interface to "insert relevant cookies into every outbound Request and is updated with the cookie values of every inbound Response" as required for user agents by RFC 6265. It also requires that the value be stored as is but this is not the case because a http.CookieJar cannot store cookies with value in the form DQUOTE *cookie-octet DQUOTE.

So, if a http.Client with a Jar is used, a cookie received as

Set-Cookie: name="value"

is sent to the server as

Cookie: name=value

instead of

Cookie: name="value"

Note that all the major browsers, latest and older versions, do not alter the cookie value sent to the server.

Solutions

I propose three alternative solutions

a) Standard compliant cookies received with a Set-Cookie header with a double-quoted value are discarded, as allowed by RFC 6265.

b) Add a Cookie.DoubleQuoted boolen field. It is set to true if a double-quoted cookie is parsed from a Set-Cookie header, and if it is true the Cookie.String adds double-quotes to the cookie value.

c) Change the meaning of the Cookie.Value field to represent a cookie value as defined in RFC 6265.

@vdobler
Copy link
Contributor

vdobler commented Jun 2, 2021

This issue is about a simple question: Does net/http.Cookie.Value represent the "semantic value" of a cookie or does it represent the raw data that RFC 6265 calls the "cookie-value".

RFC 6265 is not clear here (as it make much statements about how values should be interpreted) but common interpretation has been that the semantic value of a cookie can be optionally enclosed in double quotes or not enclosed. See e.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie stating "A cookie-value can optionally be wrapped in double quotes". This interpretation is shared by net/http which treats the double quotes as not being part of the value.

Unfortunately net/http.Cookie mentions RFC 6265 and maybe this lead to this confusion here. I still think that a= and a="" are semantically equivalent Cookie and Set-Cookie headers, both setting a cookie named "a" to an empty (semantic) value and anybody who needs to distinguish between them should process the raw HTTP headers himself and just cannot use net/http.Cookie which provides an interface to the "semantic" cookies, not the raw cookies as sent on the wire.

@neild
Copy link
Contributor

neild commented Jun 2, 2021

a) Standard compliant cookies received with a Set-Cookie header with a double-quoted value are discarded, as allowed by RFC 6265.

This is obviously not something we can or should do.

b) Add a Cookie.DoubleQuoted boolen field. It is set to true if a double-quoted cookie is parsed from a Set-Cookie header, and if it is true the Cookie.String adds double-quotes to the cookie value.

This seems like the simplest way to preserve double-quoted cookie-values.

c) Change the meaning of the Cookie.Value field to represent a cookie value as defined in RFC 6265.

We could safely change (*Cookie).String to preserve surrounding double quotes in the value when present. However, we clearly cannot change (*Request).Cookie to return a doubled-quoted Cookie.Value without breaking existing users. We could provide a way to configure cookie parsing (e.g., via a configuration flag on Request), but this seems more complicated than option b above.

Preserving the ability to round-trip a cookie-value seems worth the cost of a boolean on Cookie to me. It would be nice to know what the practical negative impact of not preserving double quotes is, however.

@gazerro
Copy link
Contributor Author

gazerro commented Jun 2, 2021

@neild I agree, the only viable option is b. Even if we could break existing users, this option does not force you to manage surrounding DQUOTE characters if you don't want to, and also allows you to adds surrounding DQUOTE characters if you need to.

It would be nice to know what the practical negative impact of not preserving double quotes is, however.

It's a good question. I honestly think no one knows. I also found this similar old issue #10195, closed but not solved.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@rsc rsc changed the title net/http: implementation of cookies does not conform to RFC 6265 for double-quoted values proposal: net/http: add field Cookie.DoubleQuoted bool Mar 1, 2024
@rsc rsc added this to Proposals Mar 1, 2024
@rsc rsc moved this to Incoming in Proposals Mar 1, 2024
@rsc
Copy link
Contributor

rsc commented Mar 1, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Mar 6, 2024

By analogy with url.URL.ForceQuery it seems like this should be Cookie.ForceQuote.
We do automatically quote when the value contains space or comma; I don't know if that's correct.
(The grammar seems to say commas are never valid.)

@neild
Copy link
Contributor

neild commented Mar 6, 2024

We automatically quote when the value contains space or comma, which is weird because spaces and commas are never valid in cookie values. I don't know if automatic quoting is correct or incorrect, but it doesn't seem useful, and it is confusing.

ForceQuote seems fine to me.

@vdobler
Copy link
Contributor

vdobler commented Mar 7, 2024

While spaces and commas are never valid in cookie values according to RFC 6265 all browsers (and even curl) support spaces and commas (if quoted) and people rely on this behaviour because a lot of systems, libraries and frameworks use them and they do work irl. (Firefox allows (almost) arbitrary UTF-8 strings as cookie values, at least when I tested it some years ago.)

I'm unsure about ForceQuote as this field is not only used to force quoting a value when sending but it also reports whether the received value was quoted or not. If you just read the documentation of net/url.URL` it's basically impossible to infer that ForceQuery is two-way (at least for me). But I'm fine with a consistent naming.

@rsc
Copy link
Contributor

rsc commented Mar 13, 2024

We can also just call it "Quoted". No need for Force.

@rsc rsc changed the title proposal: net/http: add field Cookie.DoubleQuoted bool proposal: net/http: add field Cookie.Quoted bool Mar 13, 2024
@rsc
Copy link
Contributor

rsc commented Mar 15, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is to add a new field ‘Quoted bool’ in http.Cookie.
When parsing a cookie, if double quotes are removed from the value, Quoted is set to true.
When printing a cookie, if Quoted is set to true, double quotes are added back to the value.
Double quotes are still also added implicitly for cookies that contain space or comma, for compatibility with older versions of Go.

@gazerro
Copy link
Contributor Author

gazerro commented Mar 20, 2024

Have all remaining concerns about this proposal been addressed?

The cookiejar.Jar type should work with the new Quoted field. So, when we use the SetCookies method to add cookies, the Cookies method should return the correct value for Quoted. For everything else, it doesn't seem like there's anything missing

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

Thanks for pointing that out @gazerro. We should definitely update the cookiejar implementation to preserve Quoted. I'm surprised it doesn't just use the Cookie type directly. Maybe that would make more sense. But yes, please consider updating cookiejar part of this proposal.

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is to add a new field ‘Quoted bool’ in http.Cookie.
When parsing a cookie, if double quotes are removed from the value, Quoted is set to true.
When printing a cookie, if Quoted is set to true, double quotes are added back to the value.
Double quotes are still also added implicitly for cookies that contain space or comma, for compatibility with older versions of Go.

The net/http/cookiejar implementation also has to be updated to preserve the Quoted field.

@nunogoncalves03
Copy link
Contributor

Since all the concerns seem to have been addressed, can I take on this proposal?

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Mar 28, 2024

@nunogoncalves03 The proposal has not been formally accepted yet. But, sure, you can send a patch for it that can be submitted when and if it is accepted. Thanks.

nunogoncalves03 added a commit to nunogoncalves03/go that referenced this issue Apr 1, 2024
The current implementation of the http package strips double quotes from the cookie-value during parsing, resulting in the serialized cookie not including them. This patch addresses this limitation by introducing a new field to track whether the original value was enclosed in quotes.

Additionally, the internal representation of a cookie in the cookiejar package has been adjusted to align with the new representation.

The syntax of cookies is outlined in RFC 6265 Section 4.1.1: https://datatracker.ietf.org/doc/html/rfc6265#section-4.1.1

Fixes golang#46443

Co-authored-by: Fábio Mata <[email protected]>
@rsc
Copy link
Contributor

rsc commented Apr 4, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add a new field ‘Quoted bool’ in http.Cookie.
When parsing a cookie, if double quotes are removed from the value, Quoted is set to true.
When printing a cookie, if Quoted is set to true, double quotes are added back to the value.
Double quotes are still also added implicitly for cookies that contain space or comma, for compatibility with older versions of Go.

The net/http/cookiejar implementation also has to be updated to preserve the Quoted field.

@rsc rsc moved this from Active to Likely Accept in Proposals Apr 4, 2024
nunogoncalves03 added a commit to nunogoncalves03/go that referenced this issue Apr 9, 2024
The current implementation of the http package strips double quotes
from the cookie-value during parsing, resulting in the serialized
cookie not including them. This patch addresses this limitation by
introducing a new field to track whether the original value was
enclosed in quotes.

Additionally, the internal representation of a cookie in the cookiejar
package has been adjusted to align with the new representation.

The syntax of cookies is outlined in RFC 6265 Section 4.1.1:
https://datatracker.ietf.org/doc/html/rfc6265\#section-4.1.1

Fixes golang#46443

Co-authored-by: Fábio Mata <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/577755 mentions this issue: net/http: add field Cookie.Quoted bool

@rsc rsc moved this from Likely Accept to Accepted in Proposals Apr 10, 2024
@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add a new field ‘Quoted bool’ in http.Cookie.
When parsing a cookie, if double quotes are removed from the value, Quoted is set to true.
When printing a cookie, if Quoted is set to true, double quotes are added back to the value.
Double quotes are still also added implicitly for cookies that contain space or comma, for compatibility with older versions of Go.

The net/http/cookiejar implementation also has to be updated to preserve the Quoted field.

@rsc rsc changed the title proposal: net/http: add field Cookie.Quoted bool net/http: add field Cookie.Quoted bool Apr 10, 2024
nunogoncalves03 added a commit to nunogoncalves03/go that referenced this issue Apr 12, 2024
The current implementation of the http package strips double quotes
from the cookie-value during parsing, resulting in the serialized
cookie not including them. This patch addresses this limitation by
introducing a new field to track whether the original value was
enclosed in quotes.

Additionally, the internal representation of a cookie in the cookiejar
package has been adjusted to align with the new representation.

The syntax of cookies is outlined in RFC 6265 Section 4.1.1:
https://datatracker.ietf.org/doc/html/rfc6265\#section-4.1.1

Fixes golang#46443

Co-authored-by: Fábio Mata <[email protected]>
nunogoncalves03 added a commit to nunogoncalves03/go that referenced this issue Apr 18, 2024
The current implementation of the http package strips double quotes
from the cookie-value during parsing, resulting in the serialized
cookie not including them. This patch addresses this limitation by
introducing a new field to track whether the original value was
enclosed in quotes.

Additionally, the internal representation of a cookie in the cookiejar
package has been adjusted to align with the new representation.

The syntax of cookies is outlined in RFC 6265 Section 4.1.1:
https://datatracker.ietf.org/doc/html/rfc6265\#section-4.1.1

Fixes golang#46443

Co-authored-by: Fábio Mata <[email protected]>
@dmitshur dmitshur modified the milestones: Unplanned, Go1.23 Jun 8, 2024
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. Proposal Proposal-Accepted
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.