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

Jump to bottom

net/http: readCookies unable to parse out cookies that are not well written #39087

Closed
shidawuhen opened this issue May 15, 2020 · 8 comments
Closed

net/http: readCookies unable to parse out cookies that are not well written #39087

shidawuhen opened this issue May 15, 2020 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@shidawuhen
Copy link

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

$ go version
go version go1.13.10 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="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/pangzhiqiang/Library/Caches/go-build"
GOENV="/Users/pangzhiqiang/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="micode.be.xiaomi.com"
GONOSUMDB="micode.be.xiaomi.com"
GOOS="darwin"
GOPATH="/Users/pangzhiqiang/data/code/golang/myproject"
GOPRIVATE="micode.be.xiaomi.com"
GOROOT="/usr/local/Cellar/go@1.13/1.13.10_1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go@1.13/1.13.10_1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/pangzhiqiang/data/code/golang/myproject/asap/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0h/652yzkf17qd7n446g954_1j40000gn/T/go-build435664031=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

  1. server: create a go server, and create an api, this api is to display the cookies passed by the client
package main

import (
	"net/http"

	_ "asap/docs"

	"github.com/gin-gonic/gin"
)

func setupRouter() *gin.Engine {
	r := gin.Default()

	// Ping test
	r.GET("/ping", ping)

	return r
}

// @Summary 接口探活
// @Produce  json
// @Param lang query string false "en"
// @Success 200 {string} string "ok"
// @Router /ping [get]
func ping(c *gin.Context) {
	cookies := c.Request.Cookies()
	cookieInfo := ""
	for _, cookie := range cookies{
		cookieInfo += cookie.Name + ":" + cookie.Value + "\n"
	}
	c.String(http.StatusOK, cookieInfo )

}

func main() {
	r := setupRouter()
	// Listen and Server in 0.0.0.0:8080
	r.Run(":9090")
}

2.client：

<?php
$url = "http://127.0.0.1:9090/ping";
$cookie = "; xmuuid=XMGUEST-FCF117BF-4D1B-272F-829D-25E19826D4F8;type=protobuf";
$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, $url);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
curl_setopt($ch, CURLOPT_COOKIE, $cookie);
$output = curl_exec($ch);
curl_close($ch);
var_dump($output) ;

What did you expect to see?

string(66) "xmuuid:XMGUEST-FCF117BF-4D1B-272F-829D-25E19826D4F8
type:protobuf
"

What did you see instead?

string(0) ""

Reason

readCookies can't parse this formate

if splitIndex := strings.Index(line, ";"); splitIndex > 0 {
				part, line = line[:splitIndex], line[splitIndex+1:]
			} else {
				part, line = line, ""
			}
@gopherbot
Copy link

Change https://golang.org/cl/233978 mentions this issue: net/http: fix readCookies unable to parse out cookies that are not well written

@odeke-em
Copy link
Member

Thank you for filing this issue @shidawuhen and welcome to the Go project!

So firstly, what's the reason why we should be allowing malformed cookies? Do other servers allow these malformed cookies? What does Node.js do? Python? Java (Netty or Jetty) do?

Kindly pinging some cookie and security experts because this could use their eyes @vdobler @katiehockman @FiloSottile.

@odeke-em odeke-em added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 18, 2020
@vdobler
Copy link
Contributor

vdobler commented May 18, 2020

This is one of several issues reported about our strict handling of cookie-list with empty elements.

The Cookie header set with the PHP client code is malformed according to https://tools.ietf.org/html/rfc6265#section-5.4 .
We try to parse malformed stuff if it is very common in the following sense: It must be working on A) all modern browsers and it must be handled by B) all dominant HTTP libraries in other languages.

@shidawuhen Your Cookie header is handcrafted and thus would not qualify for A and not for B. Can you provide evidence that any browser sends such Cookie headers based on normally set cookies or that any HTTP library produces such Cookie headers?

@shidawuhen
Copy link
Author

@odeke-em @vdobler
We have an Android app with a large number of users.

When the APP requests the api of our server, the format of the cookie is "token = abc; xmuuid = def;".

The token is the authentication information of the user login.

When the user is not logged in, the cookie is "; xmuuid = def;"

Of course, this is indeed because Android developer did not write it very well, but the version <= go1.12 is supported, and the problem has recently been discovered by the service upgrade go version to go 1.13. The old version supports but the new version does not support, which does bring about this problem.

And we use the beego framework, it is not easy to handle cookies in the service.

@gopherbot
Copy link

Change https://golang.org/cl/234961 mentions this issue: net/http: fix readCookies unable to parse out cookies that are not well written

@gopherbot
Copy link

Change https://golang.org/cl/235141 mentions this issue: net/http: clarify that AddCookie appends to whatever the Cookie header contains

@vdobler
Copy link
Contributor

vdobler commented Jul 27, 2020

Was fixed by https://golang.org/cl/235141 .

@odeke-em
Copy link
Member

Thanks for the reminder @vdobler, and for working on it too.

@seankhliao seankhliao mentioned this issue Dec 21, 2020
Closed
@golang golang locked and limited conversation to collaborators Jul 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@vdobler @odeke-em @gopherbot @shidawuhen