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

Remove the need for setting CORS headers #728

Closed
lidel opened this issue Dec 5, 2018 · 4 comments
Closed

Remove the need for setting CORS headers #728

lidel opened this issue Dec 5, 2018 · 4 comments
Assignees

Comments

@lidel
Copy link
Member

lidel commented Dec 5, 2018

Issue extracted from #722 (review)

Summary

Right now IPFS Desktop is automatically adding CORS headers to $IPFS_PATH/config:

https://github.com/ipfs-shipyard/ipfs-desktop/blob/8f912ce816e2cd2e1c634fe8cdab54e2eb37cedf/src/utils/daemon.js#L59-L64

Without the above, Web UI won't load.
We should look into ways we can make it work without the need for touching API.HTTPHeaders, eg. see how IPFS Companion deals with the same problem.

Background Discussion

@olizilla asked:

I wonder how this could be abused. A fake scheme is a neat trick, but I don't know enough to reason about how it could be exploited if we added it to the list of allowed origins. I guess a site would have have been loaded from the webui:// scheme. What happens if a web-ext is allowed to registered itself as handling that scheme?

What are our other options here?
My main concern is about updating the users config for an external daemon. @lidel what do you think?

@lidel suggested:

Actually.. we have a lot of control over the browser (request headers etc) in IPFS Desktop, just like we do in IPFS Companion. This means we should not need to set the CORS header in daemon.

Electron (Chromium engine) is adding Origin: null to fetch requests made from webui:// and internal contexts to HTTP API which acts as a sort-of-false-positive trigger for CORS validation, resulting in 403 Forbidden:

2018-12-04--16-08-50

Note: IPFS Companion works around this by removing Origin header from requests sent to the API from js-ipfs-http-client running in its background page:

This makes those requests "no longer CORS" in view of go-ipfs.
I believe if we add similar fix to IPFS Desktop the need for origins.push(...) will disappear.

@olizilla
Copy link
Member

olizilla commented Dec 5, 2018

I think the issue here is that we're using the js-ipfs-http-client code from the webui to make the http requests, rather than passing in a proxy for an ipfs instances as companion does

https://github.com/ipfs-shipyard/ipfs-desktop/blob/f06cc5919682ba80009bceb07c39a210dfd63e66/src/hooks/webui/preload.js#L34

This technique is nice and simple, but we may have to switch it out for an injected proxy object if we want to avoid CORS dancing.

@hacdias
Copy link
Member

hacdias commented Dec 5, 2018

@olizilla perhaps we could make a proxy to communicate directly with the API on the main process and take advantage of the fact that IPFS Redux Bundle can use an window.ipfs object. Although, right now, it is disabled in Web UI.

Edit: I can find some issues of making a proxy between the main and renderer processes: there are some values returned by the API that aren't serializable such as Big.js numbers, file uploads, etc...

@hacdias hacdias mentioned this issue Dec 5, 2018
15 tasks
@lidel
Copy link
Member Author

lidel commented Dec 5, 2018

AFAIK Electron is using postMessage-like interface for communication between processes.
Take a look at ipfs-postmsg-proxy, it solves serialization issues in ipfs-companion around window.ipfs in page context talking to API running in separate webextension process.

@hacdias hacdias added this to the v0.6 milestone Dec 5, 2018
@ghost ghost assigned hacdias Dec 7, 2018
@ghost ghost added the in progress label Dec 7, 2018
@hacdias
Copy link
Member

hacdias commented Dec 13, 2018

Although our solution might not be considered perfect, we don't need to set the CORS headers anymore. I'm closing this!

@hacdias hacdias closed this as completed Dec 13, 2018
@ghost ghost removed the in progress label Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants