fix no default samesite

[bharat-rajani]

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]

@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]

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]

LGTM - Just waiting for scanning/tests to finish

[bharat-rajani]

@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]

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]

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

[jaitaiwan]

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

[yhlee-tw]

@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)