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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@johejo
Copy link

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

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 29, 2021
@gopherbot
Copy link

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

@johejo
Copy link
Author

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

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 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 as completed Feb 1, 2021
@golang golang locked and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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

4 participants