window.ipfs injected after page loaded

[victorb]

Great work so far on window.ipfs, it's amazing to have IPFS embedded directly in the pages without having to build or do anything! Looking forward to perfecting it.

One issue I hit was with my test page from my previous experiments: https://github.com/VictorBjelkholm/js-ipfs-in-the-browser/blob/master/testpage.html

I've made two versions. One is the original which checks if window.ipfs is defined, and then does a ipfs.id() call. This does not work as the window.ipfs has not been injected when that code is being executed. https://ipfs.io/ipfs/QmW9EQSoL4WSPXy8UwZJjJ7xmrhiEY4UzWZ85LYSvf3e4j

A second version I made introduces a setTimeout of 500 milliseconds. This one works, as the script has had time to inject and load the script before the code is being executed. https://ipfs.io/ipfs/QmY49HH5pABacU5gSdXB12vQim6U2oXrTVefD3oTMvRCQv

If you look at my previous example, I define in the manifest.json that the script should be injected at the document_start (https://github.com/VictorBjelkholm/js-ipfs-in-the-browser/blob/de2bdc82d3dbe05060814ea94c666e6c3d290c76/manifest.json#L21), so pages will instantly be able to access window.ipfsweb. I'm guessing ipfs-companion is not doing this, as we don't want to inject ipfs.window if the option is not turned on.

My thinking is that we should rather go the manifest.json way, and inject the content-script always, but only do the expose if the option is turned on. This would solve the problem.

[RangerMauve]

What about emitting an event on document called ipfsloaded so that code can listen on it?

[dryajov]

@RangerMauve that's a great idea IMO

[victorb]

Hm, I'm not sure I agree. So the usage I would like to see is something like this:

if (windows.ipfs) {
  startApp(window.ipfs)
} else {
  require('ipfs').then((ipfs) => {
    const node = new Ipfs()
    node.once('ready', () => {
      startApp(node)
    })
  })
}

This little snippet would load extra code + wait for node to get ready before starting the application. But if window.ipfs already exists, we can just use that. This would either load fast because we don't have to load the js-ipfs bundle or it'll load a bit slower.

Now, if we have a ipfsloaded event, how am I supposed to know if the node is gonna be loaded or not? When I'm check window.ipfs, I can know directly if it's available or not. I would have to resolve to using setTimeout or something (which would also be infeasible as different computers would expose window.ipfs at different speeds), meaning I would have to purposefully make my application slower.

So I would argue we need to expose window.ipfs at document_start, unless there is another solution.

[alanshaw]

I agree with @victorbjelkholm it should just be available as soon as user scripts start running on the web page. The content script is run at document_start but I think that the tab updated event when status === 'complete' is probably fired after document_start so I reckon that's the reason for the short delay.

We can't use the manifest to define that the content script should run at document_start as the content script can't determine if it should attach ipfs to the window without either talking to browser storage or asking the background process via postMessage - both async operations which would also cause this issue.

I reckon it can be fixed, and will look into it asap.

[dryajov]

@alanshaw @victorbjelkholm I agree, if windows.ipfs can be made available at the right time, then there is no need for the event, it's only in the case we can't control when it becomes available to the page/script that it would make sense. 👍

[ghost]

We can't use the manifest to define that the content script should run at document_start as the content script can't determine if it should attach ipfs to the window

Is this to make the config option work? If the option is disabled, we could just have all commands fail as if the permission wasn't granted.

[victorb]

@lgierth problem is that if we inject the script, we can check the permissions but those functions are async, giving us the same problem as we have now. They won't finish until the page has been loaded probably.

[RangerMauve]

@victorbjelkholm What about injecting a window.ipfs and having it point at the proxy, but making the proxy queue up operations until the permissions have loaded? This would probably confuse scripts that plan on using their own IPFS instance as a fallback (like the example given against the event)

[alanshaw]

TLDR;

There's no way of programmatically adding window.ipfs before any other scripts on the page run, in Chrome.

A proposal (mostly thanks to @lgierth for the suggestion):

1. Add window.ipfs to all pages (by declaring content_scripts in manifest.json)
2. Change the meaning of the window.ipfs preference checkbox to be a blanket deny all permission
3. Change the default value for the window.ipfs preference checkbox to be checked

---

The gory details:

I was assuming that runAt: 'document_start' would work as documented, but I never checked, and as a slow human pawing at the keyboard I inevitably never experienced the problem when trying it out in the browser console. Anyway, here's what I did and found out:

I tried executing the content script when the tab updated status was "loading" but this did not work, the content script is not executed before other scripts in any browser.

In Chrome, there seems to be no event we can listen to where using browser.tabs.executeScript will guarantee execution of a script before anything else is executed: https://bugs.chromium.org/p/chromium/issues/detail?id=478183.

I also tried executing the script in browser.webNavigation.onCommitted and that worked for Firefox but not Chrome.

A bug linked from that bug suggests that we might be able to use chrome.declarativeContent to work around this, but:

WARNING: This action is still experimental and is not supported on stable builds of Chrome.
https://developer.chrome.com/extensions/declarativeContent#type-RequestContentScript

😢 😭

browser.contentScripts would be the most excellent and ideal way 🚀 ...it works in Firefox, but it's not implemented in Chrome.

So, according to my research it's not possible in Chrome atm, so I'm proposing the following changes:

1. Add window.ipfs to all pages (by declaring content_scripts in manifest.json)

   Declaring content_scripts in manifest.json is the only way we'll get a window.ipfs object before any other script executes. This will allow web pages to do a simple if/else check for window.ipfs.

2. Change the meaning of the window.ipfs preference checkbox to be a blanket deny all permission

   If you've installed the IPFS companion extension you should expect it to add IPFS to your windows, if you want to disable all access to your instance then you can. Note that if you uncheck the checkbox you won't be prompted for permission when attempts are made - they'll be automatically denied.

   For developers using the added instance there's not a lot of change here since they're going to have to check for a denied permission for operations they want to do anyway. The only real difference here is that they could be denied access to read operations as well as write operations.

3. Change the default value for the window.ipfs preference checkbox to be checked

   This will allow read operations by default, and prompt the user for an allow/deny decision for write operations. This is in the spirit of running an IPFS node.

[olizilla]

Yep, I'd find it easier to reason about if installing companion meant window.ipfs would always exist. Operations on it can be queued, or throw if disabled, but the property should exist. We need to get to a point where @victorbjelkholm's usage example just works.

I'm ok with completely removing the companion setting to toggle window.ipfs. We can add a "Deny All" to the permissions page if needed.

[lidel]

I removed my comment at #368 (review) that was arguing for disabling property (sorry, been reading github in reverse order today 🙃 )

I think at this stage I am 👍 for enabling window.ipfs by default, now that we have ACL.
(Firefox UI for ACL dialog remains to be fixed, but we have a plan).

"Companion === window.ipfs" is the right messaging (in the long term).

I think we should rename the property at the Preferences page to something like "Allow websites to ask for access to window.ipfs". That way it may be enabled by default, but users could uncheck it to deny access for all sites.

Question: what happens if my IPFS node goes down, but I still have window.ipfs on a page? Are we okay with current behaviour of js-ipfs-api?

[victorb]

Question: what happens if my IPFS node goes down, but I still have window.ipfs on a page? Are we okay with current behaviour of js-ipfs-api?

Then the calls would fail and the developer of the application needs to handle the error.

Regarding ACL per site

The address for checking the permissions needs to be the full path, but without the hash and query parameters. Otherwise applications running on a gateway gets access to other apps running on the same gateway.