[bug] SameSiteNoneMode will not work for go versions 1.11 - 1.12

[andwun]

I found an interesting issue introduced with PR #123:

In this PR, the type SameSiteMode was added that mirrors type SameSite from go 1.13.
This seems to be a bad idea, since when considering the context, it advertises support for SameSiteNoneMode for go versions 1.11 - 1.12.

However the option SameSiteNoneMode has been introduced in go 1.13 and was never backported to the older versions (see the commit introducing SameSiteNoneMode at the go project).
Since this package relies on http.Cookie and http.SetCookie(), the code for actually writing out the SameSite=None option into the cookie is simply not present in go versions 1.11 - 1.12.

As a quick repro, try adding this test case to store_test.go and have it run with go versions 1.11 or 1.12 -- where it will fail:

// TestSameSiteNoneModeSet tests that setting SameSite Option to SameSiteNoneMode sets the SameSite
// attribute on the cookie in post go1.11 systems.
func TestSameSiteNoneModeSet(t *testing.T) {
	s := http.NewServeMux()
	s.HandleFunc("/", testHandler)
	r, err := http.NewRequest("GET", "/", nil)
	if err != nil {
		t.Fatal(err)
	}
	rr := httptest.NewRecorder()
	p := Protect(testKey, SameSite(SameSiteNoneMode))(s)
	p.ServeHTTP(rr, r)
	if rr.Code != http.StatusOK {
		t.Fatalf("middleware failed to pass to the next handler: got %v want %v",
			rr.Code, http.StatusOK)
	}
	if rr.Header().Get("Set-Cookie") == "" {
		t.Fatalf("cookie not set: got %q", rr.Header().Get("Set-Cookie"))
	}
	cookie := rr.Header().Get("Set-Cookie")
	if !strings.Contains(cookie, "SameSite=None") {
		t.Fatalf("cookie incorrectly does not have the SameSite attribute set: got %q", cookie)
	}
}

To prevent users from running into this issue, I think it would be best to remove the mirrored type SameSiteMode and to use http.SameSite, instead.

Alternatively, we could also close the loop by mirroring http.Cookie.String() and http.SetCookie() from go1.13, thus adding support for SameSiteNoneMode for the go versions where it's missing. This is something I wished would exist for gorilla/sessions. Of course, that would mean this needs to be maintained in the future.

Would be happy to implement a PR -- just let me know what you think.

[elithrar]

Would it make more sense to update the build tags to only support this in >= 1.13?

What is the benefit in using http.SameSite?

[andwun]

What is the benefit in using http.SameSite?

I see the benefit in that it would be apparent to users of this lib, that support for http.SameSiteNoneMode is not available in go 1.11 - 1.12.
With the current state, you can happily use csrf.SameSiteNoneMode but it will simply have no effect.

Would it make more sense to update the build tags to only support this in >= 1.13?

I think it would then be even easier to just use http.SameSite.
That's what is done in gorilla/sessions - so it seems odd anyway, that gorilla/csrf has it's own type SameSite without any real benefit.

[elithrar]

As before, the build tags specifying 1.11 was a bug.

#132 also attempts to address this, albeit differently - by not having a default SameSite mode.

I do want to do that going forward, as the default behaviour of this library should be to be as secure as possible. If you have older clients, you can walk it back as needed.

Does your suggestion address that last part?

(Frankly, I've had so little time to parse this issue, the PR, and the current state of SameSite Lax vs. None bugs out in the wild, so clarity is helpful)

[elithrar]

@andwun - are you interested in submitting a PR for this - and/or validating #132 - that is non-breaking - ?

The removal of csrf.SameSiteMode will break the public API, unfortunately.

[elithrar]

Partially resolved via #132

[stale]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.