Skip to content

Fix #1523 by adding SameSite mode for CSRF settings #1524

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 3, 2021
Merged

Fix #1523 by adding SameSite mode for CSRF settings #1524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8b2c77b
Fix #1523 by adding SameSite mode for CSRF settings
pr0head Mar 4, 2020
cb15226
Merge branch 'master' of https://github.com/labstack/echo
pr0head Dec 3, 2020
dc147d9
Fix #1523 by adding secure cookie if SameSite mode is None
pr0head Dec 3, 2020
53b38de
Merge branch 'master' of https://github.com/labstack/echo
pr0head Dec 11, 2020
0807357
Merge branch 'master' of https://github.com/labstack/echo
pr0head Dec 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions middleware/csrf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ type (
// Indicates if CSRF cookie is HTTP only.
// Optional. Default value false.
CookieHTTPOnly bool `yaml:"cookie_http_only"`

// Indicates SameSite mode of the CSRF cookie.
// Optional. Default value SameSiteDefaultMode.
CookieSameSite http.SameSite `yaml:"cookie_same_site"`
}

// csrfTokenExtractor defines a function that takes `echo.Context` and returns
Expand All @@ -67,12 +71,13 @@ type (
var (
// DefaultCSRFConfig is the default CSRF middleware config.
DefaultCSRFConfig = CSRFConfig{
Skipper: DefaultSkipper,
TokenLength: 32,
TokenLookup: "header:" + echo.HeaderXCSRFToken,
ContextKey: "csrf",
CookieName: "_csrf",
CookieMaxAge: 86400,
Skipper: DefaultSkipper,
TokenLength: 32,
TokenLookup: "header:" + echo.HeaderXCSRFToken,
ContextKey: "csrf",
CookieName: "_csrf",
CookieMaxAge: 86400,
CookieSameSite: http.SameSiteDefaultMode,
}
)

Expand Down Expand Up @@ -105,6 +110,9 @@ func CSRFWithConfig(config CSRFConfig) echo.MiddlewareFunc {
if config.CookieMaxAge == 0 {
config.CookieMaxAge = DefaultCSRFConfig.CookieMaxAge
}
if config.CookieSameSite == http.SameSiteNoneMode {
config.CookieSecure = true
}

// Initialize
parts := strings.Split(config.TokenLookup, ":")
Expand Down Expand Up @@ -157,6 +165,9 @@ func CSRFWithConfig(config CSRFConfig) echo.MiddlewareFunc {
if config.CookieDomain != "" {
cookie.Domain = config.CookieDomain
}
if config.CookieSameSite != http.SameSiteDefaultMode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The article on the issue say this:

Cookies with SameSite=None must also specify Secure, meaning they require a secure context.

So maybe it could be good to set cookie.Secure=true when SameSite=None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pafuent Maybe you can correct the testing strategy to take at least GO version 1.13 as a basis?
The http.SameSiteNoneMode constant appeared only in 1.13 version of GO(golang/go#32546), which is why GitHub Actions throws an error when checking: dc147d9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would mean this is a breaking change, as for vecho 4 we still consider Go v1.12 supported.
Do we rely on the actual implementation of SameSiteNoneMode in Golang, er just using the const for comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lammel Are you suggesting to declare local constants that will be analogous in GoLang?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is what I meant. Go v1.12 will not have any changes in that regard anymore.
So in case we do not rely an additional functionality other than the constant I would prefer to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, there will be a conflict.
The passed value in the config (config.CookieSameSite) is will be set as cookie.SameSite. Cookie, in turn, is a http.Cookie structure and an attempt to set a value from a local constant will result in an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something but maybe we can do something like this:

package main

import (
	"fmt"
)

type SameSite int

const (
	SameSiteDefaultMode SameSite = iota + 1
	SameSiteLaxMode
	SameSiteStrictMode
)

const (
	SameSiteNoneMode SameSite = SameSiteStrictMode + 1
)

func main() {
	var ss SameSite
	ss = SameSiteLaxMode
	fmt.Println(ss)
	ss = SameSiteNoneMode
	fmt.Println(ss)
}

The tricky part is how to write it in order to select our version of the constant or the Go 1.13.
One option is to use build constraints. So only for Go 1.12 you define an Echo constant SameSiteDefaultMode SameSite with a value equal to 4, for all the other versions, you define the same constant but equal to http.SameSiteDefaultMode

Something like this (please test it, I didn't had the time to do it)

samesite.go

// +build !go1.12

package middleware

import (
	"net/http"
)

const (
	SameSiteNoneMode http.SameSite = http.SameSiteNoneMode
)

samesite_1_12.go

// +build go1.12

package middleware

import (
	"net/http"
)

const (
	SameSiteNoneMode http.SameSite = 4
)

cookie.SameSite = config.CookieSameSite
}
cookie.Expires = time.Now().Add(time.Duration(config.CookieMaxAge) * time.Second)
cookie.Secure = config.CookieSecure
cookie.HttpOnly = config.CookieHTTPOnly
Expand Down
77 changes: 77 additions & 0 deletions middleware/csrf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package middleware

import (
"fmt"
"net/http"
"net/http/httptest"
"net/url"
Expand Down Expand Up @@ -81,3 +82,79 @@ func TestCSRFTokenFromQuery(t *testing.T) {
assert.Error(t, err)
csrfTokenFromQuery("csrf")
}

func TestCSRFSetSameSiteMode(t *testing.T) {
e := echo.New()
req := httptest.NewRequest(http.MethodGet, "/", nil)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

csrf := CSRFWithConfig(CSRFConfig{
CookieSameSite: http.SameSiteStrictMode,
})

h := csrf(func(c echo.Context) error {
return c.String(http.StatusOK, "test")
})

r := h(c)
assert.NoError(t, r)
assert.Regexp(t, "SameSite=Strict", rec.Header()["Set-Cookie"])
}

func TestCSRFWithoutSameSiteMode(t *testing.T) {
e := echo.New()
req := httptest.NewRequest(http.MethodGet, "/", nil)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

csrf := CSRFWithConfig(CSRFConfig{})

h := csrf(func(c echo.Context) error {
return c.String(http.StatusOK, "test")
})

r := h(c)
assert.NoError(t, r)
assert.NotRegexp(t, "SameSite=", rec.Header()["Set-Cookie"])
}

func TestCSRFWithSameSiteDefaultMode(t *testing.T) {
e := echo.New()
req := httptest.NewRequest(http.MethodGet, "/", nil)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

csrf := CSRFWithConfig(CSRFConfig{
CookieSameSite: http.SameSiteDefaultMode,
})

h := csrf(func(c echo.Context) error {
return c.String(http.StatusOK, "test")
})

r := h(c)
assert.NoError(t, r)
fmt.Println(rec.Header()["Set-Cookie"])
assert.NotRegexp(t, "SameSite=", rec.Header()["Set-Cookie"])
}

func TestCSRFWithSameSiteModeNone(t *testing.T) {
e := echo.New()
req := httptest.NewRequest(http.MethodGet, "/", nil)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

csrf := CSRFWithConfig(CSRFConfig{
CookieSameSite: http.SameSiteNoneMode,
})

h := csrf(func(c echo.Context) error {
return c.String(http.StatusOK, "test")
})

r := h(c)
assert.NoError(t, r)
assert.Regexp(t, "SameSite=None", rec.Header()["Set-Cookie"])
assert.Regexp(t, "Secure", rec.Header()["Set-Cookie"])
}