Fix #1523 by adding SameSite mode for CSRF settings

[pr0head]

No description provided.

[codecov]

Codecov Report

Merging #1524 (0807357) into master (829e821) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1524      +/-   ##
==========================================
+ Coverage   85.77%   85.89%   +0.12%     
==========================================
  Files          29       29              
  Lines        2017     2021       +4     
==========================================
+ Hits         1730     1736       +6     
+ Misses        181      180       -1     
+ Partials      106      105       -1     

Impacted Files	Coverage Δ
middleware/csrf.go	80.28% <100.00%> (+4.16%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 829e821...0807357. Read the comment docs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[misterion]

Hey, guys, why this request was closed? The problem is still here and should be fixed.

[sgtsquiggs]

Please reopen this PR - this solves a real issue.

[pafuent]

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

[pr0head]

Fixed

[pr0head]

@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

[lammel]

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?

[pr0head]

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

[lammel]

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.

[pr0head]

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.

[pafuent]

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
)

[sgtsquiggs]

Thanks for the work getting this in!

[lammel]

Workaround for Go 1.12 as suggested by @aldas has been added to master so tests should be fine by now.