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 dev cookie on Safari #234

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

qingant
Copy link

@qingant qingant commented Aug 6, 2023

The behavior to set Secure=True when serving from localhost would cause failure to set cookie on Safari (tested on 17.0) and thus cause it hang on loading animation.

This could be an issue of Safari but pretty amount of developer who work on Mac would be prevented when they first try Solara on Mac if Safari was set as default browser.

@maartenbreddels
Copy link
Contributor

If this is really happening on Safarai 17 I think this is a browser bug (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#samesitesamesite-value says localhost is ok) , I do not have this issue on Safari 16.5.2.

But, as the comment explains, this is needed for iframe support, so we cannot merge this as is. We'll need to think how we can work around this issue, maybe some opt out?

@qingant
Copy link
Author

qingant commented Aug 7, 2023

If this is really happening on Safarai 17 I think this is a browser bug (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#samesitesamesite-value says localhost is ok) , I do not have this issue on Safari 16.5.2.

But, as the comment explains, this is needed for iframe support, so we cannot merge this as is. We'll need to think how we can work around this issue, maybe some opt out?

thanks. could you please point me to the direction where I could add the command options ? or , change to localStorage when Safari detected ? recent days, there is a couple of bugs around cookie in Safari (cookie manager for streamlit is also broken on Safari.)

@qingant
Copy link
Author

qingant commented Aug 7, 2023

@maartenbreddels

seems there is already an open issue: #133

It actually didn't work on my Safari 16.x version. I am guessing the reason that your browser works is because you already have cookie populated from previous run.

@maartenbreddels
Copy link
Contributor

You are correct, if i remove my cookie I get the same issue. This is pretty bad!

It seems we are doing a smart default thing (samesite=none and secure=true) to make more things work out of the box, but this default smart behaviour does not work on safari indeed.
I think we need to have something like

  1. A way to detect Safari (seems sth like if 'Safari' in user_agent and 'Chrome' not in user_agent and 'Chromium' not in user_agent:). If safari, do not do the smart default.
  2. manually turn on or off the samesite=none and secure=true settings. Turning it on can be useful if we are behind a proxy and fail to detect we are using https.

I usually start adding these kinds of options in the solara.server.settings module. If they are common I also expose it via the command line (I don't think we need it this time).

What do you think of this?

maartenbreddels pushed a commit that referenced this pull request Nov 14, 2023
The session cookie does not get set when solara runs in an iframe on solara. We fall back in the websocket handler to a new session id.
Fixes #133
Replaces #234
@maartenbreddels
Copy link
Contributor

This is 'kinda of' of replaced by #379
@iisakkirotko I think this means that developing with safari on localhost still would not give a stable session id, so .. we should still keep this PR open as a reminder we can still improve...

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.

None yet

2 participants