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: invalid byte '"' in Cookie.Value; dropping invalid bytes #18627

Closed
sinylei opened this issue Jan 12, 2017 · 36 comments

Comments

Projects
None yet
10 participants
@sinylei
Copy link

commented Jan 12, 2017

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

go root# go version
go version go1.7.1 darwin/amd64

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

root# go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7.1/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.1/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build065605285=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

Hi,
I am trying to access a Web server using net/http.
But the request was rejected by the server due to the cookie not accepted, missing a pair of "".
My application report in console log as below:
net/http: invalid byte '"' in Cookie.Value; dropping invalid bytes

The cookie I sent in my request looks like this:
Cookie: _LSID=2543ebb4-1213-47f8-b31a-a17a31ec93d3,Fy4oMdFmUw5wt1vD/gm0jDBV3hje6omX1VDusKRcf/Bj5Qenv4uTaPE2pRWvt7JNXyO0bFdrQfd0w4TYzxIXGQ==

The cookie required and accepted by server:
cookie: _LSID="2543ebb4-1213-47f8-b31a-a17a31ec93d3,Fy4oMdFmUw5wt1vD/gm0jDBV3hje6omX1VDusKRcf/Bj5Qenv4uTaPE2pRWvt7JNXyO0bFdrQfd0w4TYzxIXGQ=="

The difference is a pair of char(") at the both sides of the Cookie value string.

What did you expect to see?

Could you remove the Cookie Value authentication for char '"'

What did you see instead?

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

@kennygrant

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

See also #7243 which discusses these restrictions on cookie values.

@sinylei

This comment has been minimized.

Copy link
Author

commented Jan 12, 2017

import (
    "github.com/mozillazg/request"
    "net/http"
)

func checkServer(user) {
    c := &http.Client{}
    
    req := request.NewRequest(c)
    req.Cookies = map[string]string {
//        "_LSID" : user.LSID,
        "_LSID" : `"` + user.LSID + `"`,
    }
...
}

My code looks above using a third package request which is a wrapper of original net/http.
The user.LSID is string like as below:
2543ebb4-1213-47f8-b31a-a17a31ec93d3,Fy4oMdFmUw5wt1vD/gm0jDBV3hje6omX1VDusKRcf/Bj5Qenv4uTaPE2pRWvt7JNXyO0bFdrQfd0w4TYzxIXGQ==

The server which is out of my control requires the cookie as below.
_LSID="2543ebb4-1213-47f8-b31a-a17a31ec93d3,Fy4oMdFmUw5wt1vD/gm0jDBV3hje6omX1VDusKRcf/Bj5Qenv4uTaPE2pRWvt7JNXyO0bFdrQfd0w4TYzxIXGQ=="

The server requires Double quotes in the cookie value and mandatory.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

@sinylei i meant a code sample that compiled.

@sinylei

This comment has been minimized.

Copy link
Author

commented Jan 12, 2017

@davecheney sorry, I can not provide you a code sample since it is not a standalone app, it is a subsystem of a project, which is original written by nodejs. Now I am trying to rewrite some subsystem using golang to improve the performance.
Briefly, this subsystem can not work independently. Another process does the 'User Login' to the Web server and transfer the user session cookie to this subsystem. This subsystem initialized another request to the Web server using this cookie.

I understand that golang has the restrictions on cookie value to avoid any potential security issue for server-side application developed by golang. But for the client application, the language should provide the developer flexibility on cookie values that required by some server. Actually, nodejs has no such restrictions on cookie values. I read the source code of net/http and found the restriction is here:

https://golang.org/src/net/http/cookie.go#L320

@opennota

This comment has been minimized.

Copy link

commented Jan 13, 2017

The cookies with quoted values added via AddCookie are unquoted, those added via Header.Add are not.

package main

import (
	"fmt"
	"log"
	"net/http"
	"net/http/httptest"
	"net/http/httputil"
)

func main() {
	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		data, _ := httputil.DumpRequest(r, false)
		fmt.Println(string(data))
	}))
	c := http.Cookie{
		Name:  "name1",
		Value: `"quoted"`,
		Path:  "/",
	}
	r, _ := http.NewRequest("GET", s.URL, nil)
	r.AddCookie(&c)
	r.Header.Add("Cookie", `name2="quoted"`)
	_, err := http.DefaultClient.Do(r)
	if err != nil {
		log.Fatal(err)
	}
}

Output:

2017/01/13 08:28:38 net/http: invalid byte '"' in Cookie.Value; dropping invalid bytes
GET / HTTP/1.1
Host: 127.0.0.1:42767
Accept-Encoding: gzip
Cookie: name1=quoted
Cookie: name2="quoted"
User-Agent: Go-http-client/1.1
@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

If Go is violating RFCs, somebody should cite which RFCs we're violating, and then we can fix.

If Go is not violating RFCs, somebody should provide evidence that the RFCs are wrong (that popular clients & servers behave otherwise, reliably, with details). Then we can file bugs against the RFCs and add workarounds.

@bradfitz bradfitz added this to the Unplanned milestone Jan 13, 2017

@opennota

This comment has been minimized.

Copy link

commented Jan 13, 2017

According to the RFC 6265 section 4.1.1,

cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )

where DQUOTE is a double quote.

So double-quoted cookie values are explicitly allowed.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

@opennota, yes, but Request.Cookies contains the unescaped values. The double quote (") character is for the wire representation. You can't just put the wire representation into the Request.Cookies map values.

Are you saying that this bug is that Go should add double quotes around cookie values in more cases?

@opennota

This comment has been minimized.

Copy link

commented Jan 13, 2017

@bradfitz

Are you saying that this bug is that Go should add double quotes around cookie values in more cases?

No. I'm okay with the current behaviour, where one can send double quoted cookie values by adding them directly to the request headers. But at least nodejs (as the reporter says) behaves differently. How about other languages and platforms?

@opennota

This comment has been minimized.

Copy link

commented Jan 13, 2017

Perhaps Go could recognize and do not remove double quotes around the cookie values in Request.Cookies? net/http server doesn't seem to unescape values like %22 or \", so it should be okay, shouldn't it.

@sinylei

This comment has been minimized.

Copy link
Author

commented Jan 13, 2017

@opennota You provide a really good workaround, that put such quoted string into Header portion. Header related function does not do such value validation. Thank you very much!

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

Previous discussion was in #7243.

The Go code is currently:

// http://tools.ietf.org/html/rfc6265#section-4.1.1                                                                                             
// cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )                                                                          
// cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E                                                                             
//           ; US-ASCII characters excluding CTLs,                                                                                              
//           ; whitespace DQUOTE, comma, semicolon,                                                                                             
//           ; and backslash                                                                                                                    
// We loosen this as spaces and commas are common in cookie values                                                                              
// but we produce a quoted cookie-value in when value starts or ends                                                                            
// with a comma or space.                                                                                                                       
// See https://golang.org/issue/7243 for the discussion.                                                                                        
func sanitizeCookieValue(v string) string {
        v = sanitizeOrWarn("Cookie.Value", validCookieValueByte, v)
        if len(v) == 0 {
                return v
        }
        if v[0] == ' ' || v[0] == ',' || v[len(v)-1] == ' ' || v[len(v)-1] == ',' {
                return `"` + v + `"`
        }
        return v
}

It sounds like this bug is suggesting that we need to double-quote the Set-Cookie value on the wire in more cases. Perhaps if there's a comma anywhere in the value string.

@sinylei

This comment has been minimized.

Copy link
Author

commented Feb 3, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

@odeke-em, you interested in this one?

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

Say no more fam, I got it.

@odeke-em odeke-em self-assigned this Feb 3, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

@odeke-em, still got it? :)

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

Yap yap, I'll work on it tonight after this lab exercise ie in about 4 hours and send in a CL, otherwise I'll ping you for help.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

So am interpreting this issue differently, and we have 2 different options:

  1. Skip validation for '"' entirely, as the reporter requests, regardless of the number of double quotes.
  2. Skip validation of '"' only if they are in pairs because we have the definition of a cookie value
    to correctly follow http://tools.ietf.org/html/rfc6265#section-4.1.1 which defines a cookie-value as:
// cookie-value      = *cookie-octet / ( dQuote *cookie-octet dQuote )
// cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
//           ; US-ASCII characters excluding CTLs,
//           ; whitespace dQuote, comma, semicolon,
//           ; and backslash

That is: if a cookie value has double quotes, they must be in pairs surrounding a cookie octet.

What do y'all think? 1. Is trivial. 2. requires a bit more of cleverness but I've implemented it as well.

EDIT: Just to be clear, with 2. The qualm code in this issue will be accepted

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Feb 9, 2017

CL https://golang.org/cl/36642 mentions this issue.

@opennota

This comment has been minimized.

Copy link

commented Feb 9, 2017

@odeke-em

I've mailed https://go-review.googlesource.com/c/36642.

Am I mistaken, or you code assumes that cookie-octet can be a double quote (it cannot)?

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

@opennota let's comment on the CL :) but cookie-octet in my CL cannot be a double quote, it can be the empty string surrounded by a pair of double quotes e.g "" but not """.

@opennota

This comment has been minimized.

Copy link

commented Feb 9, 2017

I just don't get this parse " octet " in pairs business. (And can't log in to comment on the CL at the moment, sorry.)
Isn't it that cookie-value either surrounded by dquotes (so there are only two of them) or doesn't contain dquotes at all?

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

Yes you are right, after doing a second reading of the definition, I see that I might have misinterpreted the definition: I read cookie-value instead of cookie-octet so just only 2 of DQUOTE as you mention. Thank you @opennota, I'll comment that on the CL. For now, I've got a crap ton of assignments to work on due for Friday afternoon but I'll update/fix the CL on Friday afternoon when free, or anyone can feel free to drop in.

@vdobler

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

The cookie-value in question is

2543ebb4-1213-47f8-b31a-a17a31ec93d3,Fy4oMdFmUw5wt1vD/gm0jDBV3hje6omX1VDusKRcf/Bj5Qenv4uTaPE2pRWvt7JNXyO0bFdrQfd0w4TYzxIXGQ==

which consists of valid cookie-octets and an invalid internal comma ",". This comma is illegal according to RFC 6265 in a cookie value, be it unquoted or quoted.

RFC 6265 never requires double quoting cookie values.
We use double quoted cookie values to allow malformed cookie values which are common in the wild, especially cookie values containing spaces and commas because browsers do handle these cookie values properly and lots of systems produce formally illegal cookie values (as no browser complains) and we should deal with them. It turned out (see issue #7243) that all browser accept:

  • unquoted cookie values with internal spaces or commas
  • quoted cookie values with leading or trailing spaces and commas

We decided to allow formally malformed cookie values with spaces and commas and send them in quoted form iff required by browsers (i.e. leading or trailing), but send internal spaces/commas unquoted.

It seems as if there are clients out in the wild which require the cookie-values to be quoted also for internal commas (and probably spaces).

Formally there is no bug as commas are disallowed in cookie-values by RFC 6265.

Alternatives are:

  1. We always quote all cookie values, even wellformed ones. This increases network traffic a tiny bit. This should have no drawback on RFC 6265 compliant agents/servers. This is dangerous as lots of cookie values in the wild are wellformed and unquoted and compliance with RFC 6265 seems not too strong.

  2. We decide this is a non-bug, especially as there is a workaround for non compliant third party systems: Add the Cookie or Set-Cookie header manually.

  3. Send cookie values in quoted form also if the cookie-value contains internal (non-leading, non-trailing) spaces or commas.

@ddo

This comment has been minimized.

Copy link

commented Feb 21, 2017

@bradfitz

The double quote (") character is for the wire representation. You can't just put the wire representation into the Request.Cookies map values.

Sorry but why not?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

@vdobler, option (3) sounds good to me.

@ddo

This comment has been minimized.

Copy link

commented Feb 22, 2017

we should handle cookies like what browsers do not RFCs

  • #validCookieValueByte should allow " \ and ,
  • cookie name and value must be trim space, #readSetCookies only trim parts that split from ;
  • #parseCookieValue no need allowDoubleQuote since we allow " in cookie name and value
  • (optional) not sure is it faster if we use strings.SplitN instead of strings.Index(name, "=")
@vdobler

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

@ddo

  • Different browsers behave differently. Doing "like what browser do" is not well defined.
  • Most browser do not allows " in cookie values (because that interferes with the wire format).
  • Trimming spaces from cookie values is more than strange. Why would you like to do that?
@ddo

This comment has been minimized.

Copy link

commented Feb 22, 2017

i tested some cases with chrome and firefox

0=qweqweqw
1=
11=    (some spaces in value)
2=""
22="      "
3="
4=,
44=","
55="   w   "
555=     w    (some spaces prefix and suffix value)
5555=      w   w     (some spaces prefix and suffix value)
55555="   w   w    "
6="\"
66=\
7=haha
      7  7    =haha   (some spaces prefix and suffix name)
"  7  7"=haha
9="
99="""
999="   "   "
9999=""   "
99999="      ""
"""=1
"=1
""=1
,=1
/=1

Google Chrome

screenshot from 2017-02-22 20-24-03

Firefox

screenshot from 2017-02-22 20-26-07

Note: <space><space>w<space><space><space>w<space> gonna be w<space><space><space>w, both 2 browsers show like w<space>w but when you send the next request the cookie in headers is correct one w<space><space><space>w. like strings#TrimSpace

@gopherbot

This comment has been minimized.

Copy link

commented Feb 22, 2017

CL https://golang.org/cl/37328 mentions this issue.

@vdobler

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2017

@ddo Chrome is very liberal in some regards, Firefox in others (e.g. UTF-8 values). If only Chrome had to be supported it would be easy.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 23, 2017

@vdobler I'll let you take over the CL, as I don't seem to fully understand the deeper problem. Thanks for looking into it.

@JamesCullum

This comment has been minimized.

Copy link

commented Oct 19, 2017

Still having that issue, Microsoft is using JSON in a cookie value as well, using double quotes. While I did code a workaround (extracting the raw value from the header), is there some way to supress the error message?

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

@JamesCullum please open a new issue, this one is closed.

@golang golang locked and limited conversation to collaborators Oct 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.