-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 CORS Support #1529
Fix CORS Support #1529
Conversation
@whyrusleeping @cryptix PTAL |
|
||
func skipAPIHeader(h string) bool { | ||
switch h { | ||
default: |
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.
seeing default first and implicit continues always weirds me out in the logic. maybe:
switch h{
case "Access-Control-Allow-Origin":
return true
case "Access-Control-Allow-Methods":
return true
case "Access-Control-Allow-Credentials":
return true
default:
return false
}
just to be explicit?
LGTM |
#1529 (comment) License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
#1529 (comment) License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
CORS headers do not prevent CSRF attacks, so this doesn't restore the security of #1521. Here is an attack example, if your daemon is on this branch then visiting the page will pin something to your node: http://localhost:8080/ipfs/QmYeRGr8PY3yigx2aTvK8B9e4HVUy3h4b2r3tsZHaYBduD (Note that the attack can come from any website you visit, it doesn't have to be an IPFS-hosted page). Pinning arbitrary things is only a sort-of scary attack, but more critical attacks could be possible too (e.g. making someone publish the attacker's hash for their IPNS names). |
@mappum |
@mappum To prevent CSRF, we should have some kind of token (a CSRF token of a JSON WebToken), blocking by referer header doesn't make it impenetrable and it can be annoying for web app devs. |
+1 for token, though it can also be annoying in its own right. maybe:
could get the token async? not sure. @mappum and @diasdavid would you discuss and decide? you know more about all the constraints than me |
I would really like to fix + merge this so we can release 0.3.6 soon-- it has lots of important bugfixes. |
This commit fixes + improves CORS support License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
it used to be here for a CSRF check. but we now have CORS checks. License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
this commit adds the ability to specify arbitrary HTTP headers for either the Gateway or the API. simply set the desired headers on the config: ipfs config --json API.HTTPHeaders.X-MyHdr '["meow :)"]' ipfs config --json Gateway.HTTPHeaders.X-MyHdr '["meow :)"]' License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
#1529 (comment) License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
this commit makes the API handler short circuit the request if the CORS headers say its not allowed. (the CORS handler only sets the headers, but does not short-circuit) It also makes the handler respect the referer again. See security discussion at #1532 License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
this commit introduces more serious CORS tests that check status response codes, and run real HTTP requests. License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
aaaand done. |
ipfs#1529 (comment) License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
ipfs/kubo#1529 (comment) License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>
Fix CORS Support This commit was moved from ipfs/kubo@9e4d6e1
This PR