Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cross-origin protection #375
Cross-origin protection #375
Changes from 8 commits
576d9e0
c7173d1
1db6167
aadd9b9
a046a9a
81f7b83
7239586
bdeeab9
b82edf7
ebf226f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we want/need to handle "*" patterns as well in the allowed origins?
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.
yeah, I think so
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.
Yes we do.
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.
There are 3 possible states here:
Origin
header, which equates to the request being done either on the domain (no cross-origin shenanigans), or the request isn't coming from a browser at all (in which case the client can spoof the header to whatever it wants). We always allow these.protocol://hostname
value, if so we check it against the list (unless we allow all origins).null
value, which can be explicitly allowed by the list, but is generally advised against.There is no
*
origin.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.
I think you explained this in a previous comment which makes sense to me now :)
My initial thinking was that CORS responses can contain "*" in allowed origins, so perhaps we need to handle something like this.
I did a little reading about WebSockets (since I didn't really know anything about the WebSocket upgrade protocol) and, indeed, there is no such thing as CORS really when talking about WS connections by the sounds of it, and so the origin checking here can essentially take whatever form we like.
Given that, I have no problem with not handling * in an allowed origin; it could potentially be a future enhancement (so that you can say configure this to allow eg any connections from "https://*.mydomain.com") but I guess there's no need for that sort of thing in the first cut!