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.

Sign up for GitHub

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

Jump to bottom

Expand session cookie configuration #190

Closed
cemerick opened this issue Jan 4, 2022 · 3 comments
Closed

Expand session cookie configuration #190

cemerick opened this issue Jan 4, 2022 · 3 comments

Comments

@cemerick
Copy link

cemerick commented Jan 4, 2022

It appears that the default of SameSite=Strict results in cookies that are sent as part of a redirection response (either 303 or 302) not being retained or sent by the browser afterwards; setting SameSite=Lax yields expected behavior. (There are some other workarounds described in https://stackoverflow.com/questions/42216700, FWIW.)

It seems reasonable to suspect that there are a variety of contexts where the various session implementations using Cookie.set_cookie's defaults will be inappropriate. Rather than suggest a narrow addition of SameSite configurability to address the above specific issue, maybe it is more reasonable to expose all of set_cookie's options via each of the session middlewares?

(Maybe this is just another bullet point for #13.)
@aantron
Copy link
Owner

aantron commented Jan 4, 2022

Exposing the set_cookie options seems reasonable. I am a little scared of having every session middleware have a lot of options, but it does seem like these things should be readily configurable.

@ulrikstrid
Copy link
Collaborator

A solution to work around the issue is to redirect with a 200 which can be done like so:

let redirect_200 url =
  let html =
    Printf.sprintf
      {|<html><head><meta http-equiv="refresh" content="0;URL='%s'"/></head><body><p>Moved to <a href="%s">%s</a>.</p></body></html>|}
      url url url
  in
  Dream.html html

This worked for my OIDC scenario at least, not sure if it's too hacky or not, but at least it works.

@aantron
Copy link
Owner

aantron commented Feb 16, 2022

The above commit changes the default for Dream's cookies from SameSite=Strict to SameSite=Lax. It has been pointed out to me by @andreypopp, and I further confirmed, that Lax is the right default for all cookies.

I think that solves the underlying issue here. Please comment, though, if you think the session middleware does need more configuration options for any other reason!

@aantron aantron mentioned this issue Mar 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@cemerick @ulrikstrid @aantron and others