Skip to content
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.

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

fix no default samesite #276

Merged
merged 2 commits into from
Jun 15, 2024
Merged

Conversation

bharat-rajani
Copy link
Member

@bharat-rajani bharat-rajani commented Apr 21, 2024

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

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

Copy link
Member

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

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

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

.gitignore Outdated Show resolved Hide resolved
@bharat-rajani
Copy link
Member Author

bharat-rajani commented Apr 22, 2024

@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
Copy link
Member

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
Copy link
Member

LGTM - Just waiting for scanning/tests to finish

@bharat-rajani
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member

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

@jaitaiwan jaitaiwan merged commit ef99c78 into gorilla:main Jun 15, 2024
7 of 10 checks passed
@jaitaiwan
Copy link
Member

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

@yhlee-tw
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SameSite is not set in the default path
4 participants