fix no default samesite

[bharat-rajani]

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

• Refactor
• Feature
• 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 # SameSite is not set in the default path #256
• Closes # SameSite is not set in the default path #256

Added/updated tests?

• 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

• make verify is passing
• make test is passing

Fixes: #256

[jaitaiwan]

From what I read of your linked comments, it seems like we should set the default to "None" for backwards compatible behaviour.

[bharat-rajani]

@jaitaiwan #276 (review)

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)