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

Problem with window.ipfs and Files/Settings #787

Closed
lidel opened this issue Sep 10, 2018 · 8 comments · Fixed by #845
Closed

Problem with window.ipfs and Files/Settings #787

lidel opened this issue Sep 10, 2018 · 8 comments · Fixed by #845
Assignees
Labels
kind/bug A bug in existing code (including security flaws) kind/discussion Topical discussion; usually not changes to codebase P1 High: Likely tackled by core team if no one steps up

Comments

@lidel
Copy link
Member

lidel commented Sep 10, 2018

Something that we should address before shipping the first iteration:

Background

When WebUI is bundled with IPFS Companion it detects and uses original API object available in WebExtension context, but if WebUI is loaded over HTTP it tries to use window.ipfs injected by IPFS Companion.

Problem

The problem with window.ipfs in current form is a limited subset of IPFS API and sensitive things like access to ipfs.config.* is blocked and access to ipfs.files.* is sandboxed.

Current UX is problematic. window.ipfs not only disables the Settings screen, but the sandboxing make it look like Files screen is broken.

User will be really confused if ipfs files ls shows files in the terminal, but WebUI claims it is empty.

Solution

Thoughts?

@lidel lidel added kind/bug A bug in existing code (including security flaws) revamp labels Sep 10, 2018
@lidel lidel changed the title Problem with window.ipfs and Files (when WebUI is not bundled with Companion) Problem with window.ipfs and Files Sep 10, 2018
@olizilla
Copy link
Member

It may be a good UX to display faded-out breadcrumbs prefixing root with the path of sandbox root.

I don't think we can ask window.ipfs what the sandbox path is currently. More critically, what's the UX we really want here? I feel that using window.ipfs with Web UI should get you the best experience possible, rather than a reduced functionality version.

You are right, the sandboxed version of WebUI is going to cause confusion and bug reports. I feel like we should put our energy into coming up with a mechanism to let apps get hold of an unsandboxed api. Right now we are blocking access to things from window.ipfs that can be trivially circumvented by making an XHR request from the page.

Possible routes through are

  1. Add a "give me full access" permission to window.ipfs that apps can request. This seems less annoying than being asked for permissions for each api individually.
  2. Have companion detect web ui somehow and redirect to the bundled one.
  3. Don't use window.ipfs in Web UI. (I don't really want this, but it's not a completely unreasonable idea given Web UI is bundled with a node, but want to move away from that hard coupling if possible)
  4. Other ideas!?

In the meantime we should fix the issue you identify in this issue. We still need to show a sensible UI if the user decides to not give web ui access to all the things.

@lidel
Copy link
Member Author

lidel commented Sep 11, 2018

  • (1) We can discuss adding "privileged" flag to window.ipfs 2.0
    • it could have visually more serious warning on the dialog popup asking for access, but we all know how it will end: every website will ask for full access and everyone will be trained to click OK, OK, OK 🙃
    • not saying it won't happen, maybe we will find a way to make it work. But it is a difficult decision that is ahead of us and won't happen before first iteration of WebUI ships.
  • (2) We.. actually could do that. 👀 Added value would be solving automatic updates for snapshot shipped with go-ipfs without the need for signing/secret management
    The most efficient mechanism would be to pass a hint in HTTP request or URL somehow (we want to avoid the need of parsing body of every request), a rough scheme (probably can be improved):
    1. WebUI could make a request to well known URL with its current version and if Companion is installed it could redirect this request to URL of another resource with info about version and URL of bundled WebUI.
    2. WebUI could then display UI informing user the new version is available "(click here to update)"
    3. If user clicks on it store store a flag so the next time the page is loaded AND companion is still present it won't ask user, but load the new version from companion.
  • (3) Disabling support for window.ipfs could be a temporary measure: I feel if we go with (2) the problem of window.ipfs 1.0 is no longer urgent and can be tackled as a part of window.ipfs 2.0 efforts in Q4.

Final thoughts:

  • The way I see WebUI itself is a privileged, trusted app and it should be developed with default assumption of having access to the full API.
  • Making WebUI compatible with current limitations of window.ipfs slows down the development because we need to think and address issues like this one.
  • If we decide to not use window.ipfs 1.0 in WebUI (2+3) don't think that is a problem: we already have parts of WebUI running as a separate apps with window.ipfs. (IPLD Explorer), so we are still dogfooding it.
  • If we decide to keep window.ipfs support (2-only) : hiding elements without any explanation produces bad UX when parts of UI are missing without any explanation (idea: we should keep Settings and just fade it out to indicate it is disabled).

@hacdias
Copy link
Member

hacdias commented Sep 11, 2018

Why not just webui.ipfs.io for everything? Go-ipfs would redirect to that path. Js-ipfs would redirect to that path. And then Companion would just need to whitelist that url (and the ipns version of it).

@lidel
Copy link
Member Author

lidel commented Sep 11, 2018

We want to avoid defaulting to webui.ipfs.io as the source of truth in go-ipfs and companion for various reasons, most important being:

  • regular website won't work in "offline" setting (eg. private network with no uplink) or when DNS is down (misconfiguration, network problems, censorship)
  • domain name relies on a trust in security guarantees of DNS and PKI (https certificate), which are weaker than content-addressing provided by IPFS (you need no trust, just verify hash matches)

@hacdias
Copy link
Member

hacdias commented Sep 12, 2018

@lidel gotcha and it makes sense. We need to have it embedded to make sure it works on private networks and in places where ipfs.io might be blocked, as well as when there is some DNS error.

@lidel lidel changed the title Problem with window.ipfs and Files Problem with window.ipfs and Files/Settings Sep 25, 2018
@olizilla
Copy link
Member

olizilla commented Sep 27, 2018

@lidel Do you think we should we be skipping window.ipfs in Web UI as a priority?

It's a shame, but it does seem like a source of confusion. If Companion is set up to redirect to it's bundled version, and as regular Web UI allows you pick your API endpoint, and there are additional gotchas in using window.ipfs here, most notably the files list silo-ing, it feel like that's the conclusion here.

@olizilla olizilla added P1 High: Likely tackled by core team if no one steps up kind/discussion Topical discussion; usually not changes to codebase and removed revamp labels Sep 27, 2018
@lidel
Copy link
Member Author

lidel commented Sep 27, 2018

@olizilla I think it makes sense to skip it for now. There is UI for customizing HTTP port now, so there would be no functional regression. We can revisit the decision when window.ipfs 2.0 lands (+we will take needs of webui into consideration while designing it).

(Alternative is to add a banner in Files screen explaining sandboxing (#818)).

@lidel
Copy link
Member Author

lidel commented Sep 29, 2018

To remove user confusion we should change API provider in web extension context from:

2018-09-29--16-20-02

to:

2018-09-29--16-20-30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) kind/discussion Topical discussion; usually not changes to codebase P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants