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

Update SameSite policy of session cookie to Lax #3650

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

RemiRigal
Copy link
Contributor

Description

The session cookie connect.sid currently has a Strict SameSite policy, which prevents users to access pages from an external link. Requests with an external Referrer are systematically redirected to the login page because the session cookie is not added to the request. Changing this policy will not add vulnerabilities to the application.

Also, an alternative would be to set this policy to Lax only when the the CSRF Protection is disabled, let me know if this is preferable.

To-Dos

  • Successful build yarn build
  • Translation keys yarn i18n:extract
  • Database migration (if required)

Issues Fixed or Closed

@sct
Copy link
Owner

sct commented Oct 18, 2023

I haven't taken the time to look into fixing the issue, so thanks for doing this.

Since this is a security-related issue, I do think it makes sense to put this change behind having CSRF protection off. And then keep it Strict if CSRF protection is on.

@RemiRigal
Copy link
Contributor Author

Thank you for the fast feedback @sct !

I've updated the PR to set the SameSite policy based on the CSRF Protection setting. Let me know if anything else needs to be updated.

Copy link
Owner

@sct sct left a comment

Choose a reason for hiding this comment

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

LGTM. Let's give it a try!

@sct sct enabled auto-merge (squash) October 18, 2023 14:56
@sct sct merged commit c84ca43 into sct:develop Oct 18, 2023
6 checks passed
@RemiRigal RemiRigal deleted the feat/samesite branch October 18, 2023 15:08
@sct
Copy link
Owner

sct commented Oct 19, 2023

@all-contributors please add @RemiRigal for code

@allcontributors
Copy link
Contributor

@sct

I've put up a pull request to add @RemiRigal! 🎉

samicrusader pushed a commit to angelicdust/oversneedrr that referenced this pull request Nov 7, 2023
* update session cookie samesite policy to lax

* set cookie samesite policy based on csrf protection setting
lenaxia pushed a commit to lenaxia/overseerr-oidc that referenced this pull request Jan 3, 2024
* update session cookie samesite policy to lax

* set cookie samesite policy based on csrf protection setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue - Clicking "processing" link does not take you directly to a title in Overseerr like it should.
2 participants