search

widgetti / solara

A Pure Python, React-style Framework for Scaling Your Jupyter and Web Apps
https://solara.dev
MIT License
1.62k stars 105 forks source link

Make sure to set the correct cookie attributes when serving from localhost #102

Closed koenvo closed 12 months ago

koenvo commented 1 year ago

When developing a solara application that will run in an iframe there is an issue with the session_id cookie. The development server will run on http://localhost which requires SameSite=None; Secure to be set on the cookies.

maartenbreddels commented 1 year ago

Very cool, and high quality pr 😍 Do the test really test it since it is the same server and port? If you revert the changes, does the test fail?

koenvo commented 1 year ago

Very cool, and high quality pr 😍 Do the test really test it since it is the same server and port? If you revert the changes, does the test fail?

You're welcome. Happy to contribute!

Good call here. Normally I would write the failing test first 🙃

Output with the changes reverted:

FAILED tests/integration/server_test.py::test_run_in_iframe[flask-chromium] - playwright._impl._api_types.TimeoutError: Timeout 18000ms exceeded.
FAILED tests/integration/server_test.py::test_run_in_iframe[starlette-chromium] - playwright._impl._api_types.TimeoutError: Timeout 18000ms exceeded.
maartenbreddels commented 12 months ago

I did some local testing, and managed to pass the test after disabling your feature. I think it is because it is running on the same server.

Then I made it run on a different port, and it still passes the test (I can see with --headed and PWDEBUG=1 that the page really loads in the iframe). If I test this with python http.server like in the docs, it does fail.

I think there is still something happening I don't understand.

maartenbreddels commented 12 months ago

e.g. try changing localhost to xlocalhost in the check of starlette and flask and the test pass 🤷

koenvo commented 12 months ago

Interesting issue. Some things I found:

  1. The cookies are shared between tests. When a tests set the cookie successfully it is reused in next test. When I for example fix the flask server both flask and starlette tests succeed. When I only fix starlette the flask test will fail.
  2. about:black and http://localhost are considered cross-origins which is good for the test.
  3. When using a StaticFiles (starlette) on localhost it's seems not to be cross-origin for cookies on localhost on a different port (cookies are seem to be specified on domain only).
maartenbreddels commented 12 months ago

Ah, we can use page_session.context.clear_cookies() for this.

2. http://localhost are considered cross-origins which is good for the test.

I didn't know! Where did you find this?

Ok, I can confirm that now with clearing the cookies, it fails without your changes.

koenvo commented 12 months ago

Ah, we can use page_session.context.clear_cookies() for this. Right! I was thinking about restarting the entire webserver but this is what I was actually looking for.

  1. http://localhost are considered cross-origins which is good for the test.

I didn't know! Where did you find this?

Test it and use PWDEBUG to see if the cookies get accepted. By default the page_session.url is about:blank

maartenbreddels commented 12 months ago

Great work, thanks Koen!