Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: disable cors by default #3275

Merged
merged 10 commits into from
Sep 14, 2020
Merged

fix: disable cors by default #3275

merged 10 commits into from
Sep 14, 2020

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Sep 9, 2020

Brings js-ipfs into line with go-ipfs by not having CORS on by default, instead requiring the user to explicitly configure it.

BREAKING CHANGE:

Brings js-ipfs into line with go-ipfs by not having CORS on by default,
instead requiring the user to explicitly configure it.

BREAKING CHANGE:

- CORS origins will need to be [configured manually](https://github.com/ipfs-inactive/js-ipfs-http-client#cors)
docs/CONFIG.md Outdated Show resolved Hide resolved
packages/ipfs/test/http-api/cors.js Show resolved Hide resolved
achingbrain and others added 5 commits September 9, 2020 15:14
Co-authored-by: Marcin Rataj <lidel@lidel.org>
Browsers send OPTIONS requests as preflight CORS checks when the
client has done something like specified custom headers.

Removes the blanket 405 handlers in order to use HAPI's default
404 when the preflight request-method is for a resource that does
not exist, though retain the 405 behaviour if a request with a non-POST
or non-OPTION method is actually received.
"The origin server MUST generate an
   Allow header field in a 405 response containing a list of the target
   resource's currently supported methods."

https://tools.ietf.org/html/rfc7231#section-6.5.5
@achingbrain
Copy link
Member Author

achingbrain commented Sep 10, 2020

I've changed the default behaviour of the non-POST 405 responses.

The previous implementation was adding extra handlers for GET, DELETE, etc that just throw 405s, but it caused Hapi's built in CORS module to treat the presence of a handler for those methods as confirmation that an actual request made to that resource would have a chance of succeeding so was returning 200s for preflight requests with a Access-Control-Request-Method header of GET, for example.

To me it doesn't make sense to send a 405 in response to a preflight request as you're saying the OPTIONS method isn't allowed, which it is.

I've moved the 405-throwing behaviour to an onRequest handler, so a preflight OPTIONS request to a resource with an Access-Control-Request-Method that isn't POST will now 404. Sending the actual non-POST request will 405 as before, though it should really 404 as well, but this is RPC not REST so 🤷 .

TBH it's not a big deal as the pre-flight response is opaque to the client js if it doesn't have a 2xx status code.

As an added bonus we now send an Allow header along with the 405 response to be RFC7231 compliant.

@lidel
Copy link
Member

lidel commented Sep 10, 2020

I want to test this against https://webui.ipfs.io +/- CORS, but we are in the middle of release, so need to wait a bit before I get to it. Will post my review later.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CORS works as expected, but I think there is a regression related to /webui on API port:

  • 💚 https://webui.ipfs.io is unable to use API at http://127.0.0.1:5002 without CORS safelisting
  • 💚 https://webui.ipfs.io is able to use API at http://127.0.0.1:5002 when CORS safelisting is done via jsipfs config --json API.HTTPHeaders.Access-Control-Allow-Origin '["https://webui.ipfs.io"]' before starting the daemon
  • 💔 The WebUI URL printed to the console during jsipfs daemon start is broken: http://127.0.0.1:5002/webui
    returns 405 Method Not Allowed

docs/CONFIG.md Outdated Show resolved Hide resolved
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OPTIONS, WebUI (local or remote) look fine now.
Small nit below, but other than that lgtm.

docs/CONFIG.md Outdated Show resolved Hide resolved
@achingbrain achingbrain merged commit 3ff833d into master Sep 14, 2020
@achingbrain achingbrain deleted the fix/disable-cors-by-default branch September 14, 2020 12:18
SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
Co-authored-by: Marcin Rataj <lidel@lidel.org>

Brings js-ipfs into line with go-ipfs by not having CORS on by default, instead requiring the user to explicitly configure it.

BREAKING CHANGE:

- CORS origins will need to be [configured manually](https://github.com/ipfs/js-ipfs/blob/master/packages/ipfs-http-client/README.md#cors) before use with ipfs-http-client
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants