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

allow token-authenticated requests cross-origin by default #2920

Merged
merged 2 commits into from Oct 20, 2017

Conversation

Projects
None yet
4 participants
@minrk
Copy link
Member

minrk commented Oct 10, 2017

we already apply this logic in our server-side checks, but browsers check Access-Control-Allow-Origin headers themselves as well, meaning that token-authenticated requests can’t be made cross-origin without CORS headers from browsers, only scripts.

This makes default browser and server-side origin checks consistent

includes #2919 to avoid merge conflicts

@minrk minrk force-pushed the minrk:allow-origin-token branch from 12fcd69 to 014316c Oct 10, 2017

allow token-authenticated requests cross-origin by default
we already apply this logic in our server-side checks,
but browsers check `Access-Control-Allow-Origin` headers themselves as well,
meaning that token-authenticated requests can’t be made cross-origin without CORS headers from browsers,
only scripts.

This makes default browser and server-side origin checks consistent

@minrk minrk force-pushed the minrk:allow-origin-token branch from 014316c to 9acf6a8 Oct 11, 2017

@minrk

This comment has been minimized.

Copy link
Member Author

minrk commented Oct 11, 2017

My conflict-avoidance strategy was not successful, apparently. Rebased.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 11, 2017

I'm never entirely sure about these CORS issues, but I think I understand the basic idea, and I'm happy to trust the two of you. Do you want anyone else to review it? Do you want it in for 5.2?

@takluyver takluyver added this to the 5.3 milestone Oct 20, 2017

@takluyver takluyver merged commit 55aa80e into jupyter:master Oct 20, 2017

4 checks passed

codecov/patch 40% of diff hit (target 0%)
Details
codecov/project 79.09% (-0.03%) compared to a8c6b8b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Oct 26, 2017

We're going to try to release 5.2.1. Should this be included?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.