-
Notifications
You must be signed in to change notification settings - Fork 390
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
handle api tokens and xsrf #1847
Conversation
- XSRF checks are applied to GET requests, blocking the EventSource - switch to fetch-based EventSource implementation, since base EventSource doesn't allow headers (like websockets) - propagate api tokens for authenticated requests
// e.g. SecurityError in case of CSP Sandbox | ||
return null; | ||
} | ||
const xsrfTokenMatch = cookie.match("\\b_xsrf=([^;]*)\\b"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regexes in anything security sensitive always make me nervous haha. Can you either add a comment here explaining what's going on, or a direct link to the specific place from @jupyterlab/services this was stolen from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added link and explanation. It's still astonishing to me that there is no API for geting cookies by name in browsers, but it is {name}={anything but a semicolon}[; again]
I think we can make the breaking change for how params are passed now! And with the tests fixed, I think this is good to go. |
I have it working locally, but I've spent my whole day so far trying to get the tests to accurately describe that the code is working, but they keep failing with unhandled errors that should be handled as regular close events. |
explain regex
no reason to leave it undefined, even if we don't use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested successfully with #1851
Other than a typo this looks good to me
Co-authored-by: Simon Li <[email protected]>
Co-authored-by: Simon Li <[email protected]>
Thanks for testing, @manics. |
jupyterhub/binderhub#1847 Merge pull request #1847 from minrk/fetch-events
I deployed this to mybinder.org too, and it seems ok. |
Thanks for finishing this! |
fixes #1842