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

wip: proxy IPFS API instead of disabling web security #738

Closed
wants to merge 4 commits into from
Closed

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Dec 8, 2018

Update: we decided to remove the CORS headers and disable web security for now until we solve the issue of uploading big files with this PR (#740).

Using ipfs-postmsg-proxy it is possible to avoid needing the CORS headers #728.

I needed to block ipfsd-ctl to version 0.40 because 0.40.1 uses the newest IPFS HTTP Client which is not backwards compatible with the .files API. Otherwise we would have got errors. We should update ipfs-redux-bundle and Web UI. It will be mostly find and replace. Will open a PR soon.

TODO:

  • ipfs-redux-bundle detects Desktop and shows it as the provider feat: add ipfs-desktop support ipfs-inactive/ipfs-redux-bundle#20
  • ipfs-redux-bundle gets a new version
  • Web UI is updated to use the latest ipfs-redux-bundle
  • update Web UI to use the newest HTTP Client (can be done in another PR)
  • remove the 'lock' on ipfsd-ctl (blocked by the above)

Closes #728.

@ghost ghost assigned hacdias Dec 8, 2018
@ghost ghost added the in progress label Dec 8, 2018
@hacdias hacdias added this to the v0.6 milestone Dec 8, 2018
License: MIT
Signed-off-by: Henrique Dias <[email protected]>
@hacdias
Copy link
Member Author

hacdias commented Dec 8, 2018

Here I'm using a Web UI version that accepts window.ipfs so you can try it out guys. /cc @ipfs-shipyard/gui

@olizilla I wonder why Web UI is set not to try window.ipfs. Already know the answer.

@olizilla could we release a new version of redux-bundle with this PR: ipfs-inactive/ipfs-redux-bundle#20 ?

@olizilla
Copy link
Member

olizilla commented Dec 9, 2018

Fast work @hacdias. I'm seeing issues trying to upload large files (>180MB), they crash the webui window.

@olizilla
Copy link
Member

olizilla commented Dec 9, 2018

Using the webui in Firefox 63 and Chrome 70, I can upload large files without issue. Perhaps Electron ships with an older version of chromium that has issues with that?

Thinking out loud (totally up for discussion on this) perhaps the best solution is the simplest one? what if the desktop menubar opens the webui in the users default browser, via http://127.0.0.1:5001/webui

The Web UI would be pulled from IPFS on first usage and be available offline after that. Desktop would be helping the user by showing that IPFS is running from the taskbar, and the menu bar would mean they dont have to remember the awkward http://127.0.0.1:5001/webui/ url. but otheriwse there is no magic, and the users expectation of the webui would be that of a website, as we'd be showing it in their browser.

@lidel @hacdias what do you think?

@olizilla
Copy link
Member

olizilla commented Dec 9, 2018

of note, if we use http://127.0.0.1:5001/webui we don't need a postmsg-proxy and we don't need to set CORS headers.

It means the user needs to fetch the webui on first run, which is a downside, but we could fix that by shipping with a tar of the actual ipfs blocks for webui, pinned at the version of the ipfs impl that we ship with, and just put those blocks into the blockstore during install.

@olizilla
Copy link
Member

olizilla commented Dec 9, 2018

I also wanna call out that I'm super impressed @hacdias that you got all the changes to make it work via postmsg proxy done so quickly. This still may be the right solution, but I just want to check that we're not missing a simpler one.

@lidel
Copy link
Member

lidel commented Dec 9, 2018

Awesome work @hacdias! It is a neat proof the proxy works fine in electron apps as well! We should update README at ipfs-postmsg-proxy with MVP code snippet for electron – mind PRing?

I'm seeing issues trying to upload large files (>180MB), they crash the webui window.

(cc ipfs/ipfs-webui#858) this feels like a regression around stream-http, ipfs-companion fixed it in ipfs-inactive/js-ipfs-http-client#868.

if we use http://127.0.0.1:5001/webui we don't need a postmsg-proxy and we don't need to set CORS headers.

It means the user needs to fetch the webui on first run, which is a downside, but we could fix that by shipping with a tar of the actual ipfs blocks for webui, pinned at the version of the ipfs impl that we ship with, and just put those blocks into the blockstore during install.

This sounds cool, offline use case would still work, we avoid proxying and actually use IPFS here as an added benefit :)

I'm more than okay with this, but to avoid playing with raw blocks (is block format the same for js-ipfs and go-ipfs? did it change in the past?) we could alternatively have regular .tar and ipfs add --pin=true on first start .

Note that there is a benefit of "native feel" when Web UI is opened in Electron window, so we may decide to continue doing that.

@hacdias
Copy link
Member Author

hacdias commented Dec 9, 2018

@olizilla it seems that the latest stable release of Electron used Chromium 66.0.3359.181 which seems a far behind 70. Although I can't confirm that's the source of the issue. It's probably what @lidel said.

@olizilla @lidel I can see only two issues with using the Web UI from the daemon:

  1. We don't have as much control over the version of Web UI that Desktop will be shipped with.
  2. It won't feel as native.

On the other hand:

  1. We have less to worry (proxy compatibility with newest JS HTTP Client).
  2. We don't need to control the creation of more windows.

I enjoy it either way. It's not something that I think that changes the user experience a lot. If we use the user default's browser there might be more compatibility issues but I doubt anyone is using an obsolete browser. It might be even more updated than Electron's Chromium version.

@hacdias
Copy link
Member Author

hacdias commented Dec 9, 2018

Just as as something I remembered now: if we use user default browser how will we inject IPFS Desktop settings into Web UI?

@lidel
Copy link
Member

lidel commented Dec 9, 2018

What if we open Web UI from http://127.0.0.1:5001/webui (API port) in Electron's BrowserWindow ?

This way:

  • there is no location bar and it continues to feel like desktop app
  • we are able to inject window.ipfsDesktop for settings set/get
  • no need for proxy nor setting CORS headers
  • (optional, requires upstream change) we could look into opening specific Web UI version via http://127.0.0.1:5001/ipfs/<cid>
    • right now anything but /webui one returns 404
      • is there a good reason for that? if so, perhaps we could at least whitelist /ipns/webui.ipfs.io?

@olizilla
Copy link
Member

olizilla commented Dec 9, 2018

@hacdias @lidel good points! Let's try opening http://127.0.0.1:5001/webui in the electron window. I just did a quick test locally, and the electron window didn't crash when adding a file > 200MB

@hacdias
Copy link
Member Author

hacdias commented Dec 9, 2018

@olizilla just to make sure: http://127.0.0.1:5001/webui is the latest available version? If not, will we wait for the next release of go-ipfs to have ipfs-redux-bundle working with IPFS Desktop?

I see the version is hardcoded into go-ipfs so it would be a bit of an hassle to have the latest version running. In this case we'll want to update the redux bundle to work with Desktop (ipfs-inactive/ipfs-redux-bundle#20).

I think @lidel idea of using /ipns/webui.ipfs.io would be better if we don't want to bundle Web UI directly.

@olizilla
Copy link
Member

So the situation is

  • we want to avoid setting custom CORS config for IPFS Desktop. It's not obvious how it could be abused, but we'd rather not take the chance.
  • proxying the ipfs api works, but there is currently an unresolved issue with uploading large files.
  • Using the default /webui url works with much less code, but leaves us tied to the go-ipfs release schedule.
  • /ipns/webui.ipfs.io isn't currently whitelisted under the go-ipfs api like /ipfs/ is, so isn't an option for today, but we could request it for future.
  • Disabling CORS checks for the electron window is still an option.

A proposal

  • let's debug why large file uploads are failing in this PR.
  • in the meantime, on master let's disable the web-security / CORS checks on the electron window, remove the logic that adds custom CORS config.

That way we can release newer versions webui without waiting for a go-ipfs release, and avoid making changes to the ipfs cors config. It seems preferable to take on a tiny risk of someone hacking their copy of Desktop to use it as a general purpose web-browser, than leaving a possible CORS hack open in a config file that may live on beyond the lifetime of the desktop app.

Questions

  • Should we use a bundled version of webui or hardcode a cid to fetch via IPFS? Using IPFS to fetch the latest webui seems preferable to manually bundling webui, but we need to check how long it takes to render on first use. Can we fetch it on install?

  • Using /ipns/webui.ipfs.io is appealling but there is value in pinning the version of webui to the version of ipfs that we ship with, as it guarantees that webui will be compatible with the ipfs version for the lifetime of the app. Do we want to roll out updates to the desktop webui on merge to master, seperately from shipping releases to desktop? I think hardcoding a CID for webui in each Desktop release (like we do know, but loaded over ipfs, and same as go/js-ipfs does) works best for stability, as we'd be testing the combo of go-ipfs and webui that we explicitly want to release. We could add an option for "try with latest webui" to the desktop settings for dev mode, but I wouldn't want to push that on everyone by default.

@hacdias
Copy link
Member Author

hacdias commented Dec 10, 2018

proxying the ipfs api works, but there is currently an unresolved issue with uploading large files.

As @lidel pointed out, it might not be related to the proxy but to ipfs/ipfs-webui#858.

Should we use a bundled version of webui or hardcode a cid to fetch via IPFS? Using IPFS to fetch the latest webui seems preferable to manually bundling webui, but we need to check how long it takes to render on first use. Can we fetch it on install?

I think it would be nicer to fetch it via IPFS. We could fetch it on the first run before starting up menubar.

in the meantime, on master let's disable the web-security / CORS checks on the electron window, remove the logic that adds custom CORS config.

If y'all agree I can do that for now in a separate PR so we can merge this one later while the big file issue isn't resolved.

@olizilla
Copy link
Member

The webui supports large file uploads via the http api. The update to the module formerly known as js-ipfs-api mentioned in ipfs/ipfs-webui#858 landed in webui 2 months ago. It may be related to the same issue, but we need to investigate.

If y'all agree I can do that for now in a separate PR so we can merge this one later while the big file issue isn't resolved.

@hacdias please go ahead!

@hacdias hacdias changed the title wip: remove need for using CORS headers wip: proxy IPFS API instead of disabling web security Dec 10, 2018
@hacdias
Copy link
Member Author

hacdias commented Dec 11, 2018

Closing this PR since we decided to disable webSecurity on the Browser Window that runs Web UI. This way there are no CORS checks and we can talk to the daemon directly. There aren't other security issues since we're not executing JavaScript from other websites.

@hacdias hacdias deleted the no-cors branch December 13, 2018 23:46
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

Successfully merging this pull request may close these issues.

3 participants