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: no reversibility in the serialization and deserialization for cookie samesite default mode #43992

Closed
johejo opened this issue Jan 29, 2021 · 4 comments

Comments

@johejo
Copy link

@johejo johejo commented Jan 29, 2021

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

$ go version
go version go1.16rc1 linux/amd64

Does this issue reproduce with the latest release?

no.
reproduced on 1.16rc1

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/heijo/.cache/go-build"
GOENV="/home/heijo/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/heijo/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/heijo/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/heijo/ghq/go.googlesource.com/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/heijo/ghq/go.googlesource.com/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16rc1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/heijo/ghq/go.googlesource.com/go/src/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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3979720791=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I was investigating changes in the behavior of the http.cookie samesite in 1.16.

package main

import (
	"net/http"
	"net/http/httptest"
	"testing"
)

func TestSameSiteDefaultMode(t *testing.T) {
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		http.SetCookie(w, &http.Cookie{Name: "n", Value: "v", SameSite: http.SameSiteDefaultMode})
	})
	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()
	handler.ServeHTTP(rec, req)

	resp := rec.Result()
	got := resp.Cookies()[0].SameSite
	if got != http.SameSiteDefaultMode {
		t.Errorf("shoud be default mode (%v) but got %v", http.SameSiteDefaultMode, got)
	}
}

What did you expect to see?

pass test

On 1.15.7, pass

What did you see instead?

On 1.16rc1 fails

--- FAIL: TestSameSiteDefaultMode (0.00s)
    main_test.go:20: shoud be default mode (1) but got 0
FAIL
exit status 1

Related #36990 542693e

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 30, 2021

Change https://golang.org/cl/288274 mentions this issue: net/http: adapt SameSiteDefaultMode behavior match the specification

@johejo
Copy link
Author

@johejo johejo commented Feb 1, 2021

cc @euank @FiloSottile @rsc @empijei

Isn't this a release-blocker?
At least I think it should not stay the current behavior for 1.16.

There is also an option to revert CL256498 .

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Feb 1, 2021

Can you elaborate on why the current behavior is problematic?

A missing SameSite attribute had always parsed as Cookie.SameSite = 0, as far as I can tell. What changed is there used to be, in a draft of the specification, an explicit "default" value. That value is gone in the final spec and the default is just represented by not specifying a SameSite attribute, so SameSiteDefaultMode and 0 now serialize to the same.

You CL changes parsing to set Cookie.SameSite = SameSiteDefaultMode when the attribute is missing. That would make Cookie.SameSite = 0 not round-trip, inevitably. It also feels like more likely to confuse applications which might have handled an explicit SameSiteDefaultMode differently from a missing attribute.

@johejo
Copy link
Author

@johejo johejo commented Feb 1, 2021

Really Thanks. I finally understood the intention.
It might be better to have a little more supplement in doc or release notes.

@johejo johejo closed this Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants