fix no default samesite

gorilla / sessions

Package gorilla/sessions provides cookie and filesystem sessions and infrastructure for custom session backends.

https://gorilla.github.io

BSD 3-Clause "New" or "Revised" License

2.93k stars 371 forks source link

fix no default samesite #276

Closed bharat-rajani closed 5 months ago

bharat-rajani commented 6 months ago

What type of PR is this? (check all applicable)

[ ] Refactor
[ ] Feature
[X] Bug Fix
[ ] Optimization
[ ] Documentation Update
[ ] Go Version Update
[ ] Dependency Update

Description

This PR sets the SameSite cookie attribute to Lax in the Set-Cookie header. The SameSite=Lax value provides a reasonable balance between security and usability for websites.

Reference: https://owasp.org/www-community/SameSite

Related Tickets & Documents

Related Issue # https://github.com/gorilla/sessions/issues/256
Closes # https://github.com/gorilla/sessions/issues/256

Added/updated tests?

[X] Yes
[ ] No, and this is why: please replace this line with details on why tests have not been included
[ ] I need help with writing tests

Run verifications and test

[X] make verify is passing
[X] make test is passing

Fixes: https://github.com/gorilla/sessions/issues/256

bharat-rajani commented 6 months ago

@jaitaiwan https://github.com/gorilla/sessions/pull/276#pullrequestreview-2014240474

Sure I can add Samesite as None. However, some browsers are mandating additional Secure attribute when SameSite=None. ( chrome, firefox )

I think it will be good to add Secure along with SameSite=None, let me know what you think!

jaitaiwan commented 6 months ago

I think that's a good idea. We can release it under a major version tag so it doesn't break folk's installations either way.

jaitaiwan commented 6 months ago

LGTM - Just waiting for scanning/tests to finish

bharat-rajani commented 6 months ago

@jaitaiwan I believe vulncheck issues are unrelated and we can tackle them separately. Also, I ran linter locally and I don't see issues which are somehow reported in github workflow.

docker run -t --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.53.3 golangci-lint run -v

Can you help?

apoorvajagtap commented 6 months ago

The linter is passing for me as well. I think this could be a false positive, I can't even find the existence of max reported in the error. I think we can ignore the lint errors, and as @bharat-rajani mentioned we can work through the vuln failures seperately. @jaitaiwan WDYT?

jaitaiwan commented 5 months ago

We'll merge this once we've fixed #277 I think

jaitaiwan commented 2 months ago

I think this might be missing from v1.4.0 can you check @bharat-rajani or @apoorvajagtap ?

yhlee-tw commented 1 month ago

@jaitaiwan the new default None, like what @bharat-rajani has mentioned, breaks setup where they disable Secure (e.g. local HTTP server) . it's an easy fix but I thought it was planned for a major version bump (or at least, mentioned in release notes)