window.ipfs #333

alanshaw · 2017-12-12T23:23:05Z

This is basically the good work of @victorbjelkholm done in 6b0ac8b but cleaned up a little.

refs #330

This is a first pass effort at getting this to work.

Demo: https://youtu.be/t1ldUp_mjDk

What's in this PR?

window.ipfs property in your web pages!
- also adds window.Buffer if not there
- also adds window.Ipfs
Proxies to running js-ipfs or go-ipfs node (via js-ipfs-api) in the extension
Adds option to the preferences to enable/disable adding ipfs to the window

Access controls on certain ipfs functions prompt user for a allow/deny decision on first use
Adds a page for managing permissions

Adds tests for access control and permissions page
Adds nyc for code coverage

What's still todo (in a future PR)?

Add a custom confirm dialog. Firefox does not support window.confirm from the background process so it fails - all access to window.ipfs on Firefox will be automatically denied, however as a temporary workaround, Firefox users can use the manage permissions page to enable permissions after they have been auto denied
- Another reason this is needed is that we want to be able to grant access to all (or a subset of) the API for a given origin, window.confirm can't cover these UX requirements
Tighten access restrictions to origin/ipfs/hash - currently only restricted to origin
Level up ipfs-postmsg-proxy to support streaming properly and then also the progress options for files add. More info here

How does it work?

https://www.youtube.com/watch?v=aXLERp9-Mis

victorb · 2017-12-13T11:54:46Z

add-on/manifest.json

+          "http://*/*",
+          "https://*/*"
+        ],
+        "match_about_blank": true,


Don't think it's necessary to inject on blank pages no?

victorb

Amazing! Thanks for starting to implementing this! Will be a great feature.

Looks like something that can work, at least as a beginning. The most important part is to get streaming responses to work over the Ports, and is also where I got stuck. Try to implement the .cat method with streaming response, so we can make sure it's working before adding code for window.ipfs.

lidel · 2017-12-14T00:24:39Z

add-on/manifest.json

+      {
+        "matches": [
+          "file://*/*",
+          "http://*/*",


A single entry with <all_urls> would be better here:

A match pattern is essentially a URL that begins with a permitted scheme (http, https, file, or ftp, and that can contain '*' characters. The special pattern <all_urls> matches any URL that starts with a permitted scheme. (src #1, src #2)

We'll eventually inject programmatically due to the need to turn this feature off, but yes that would be better.

alanshaw · 2017-12-14T15:46:46Z

Extracting this proxy idea for window.ipfs into a module, using interface-ipfs-core tests to establish whether it works or not: https://github.com/tableflip/ipfs-postmsg-proxy

victorb · 2018-01-23T16:53:13Z

add-on/src/contentScripts/ipfs-proxy/content.js

+const injectScript = require('./inject-script')
+
+function init () {
+  const port = browser.runtime.connect({ name: 'ipfs-proxy' })


If other extensions know this name, is it possible they can connect to it? That would be bad, as they could bypass the permissions you've setup, if I understand correctly.

It would! I'll check it out.

...although thinking about it, the access control is all done in the extension background process, so it doesn't matter who's connecting and asking for stuff, it still has to be authorised by the user.

I'll double check but I think it might be ok.

Ok so, the background process is listening to runtime.onConnect:

Fired when a connection is made with either an extension process or a content script.
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/onConnect

It would need to listen to runtime.onConnectExternal to receive connections from other extensions.

Aside from that, even if connections from other extensions were allowed, the authorization is done in the background process, so the user would still need to have approved the function to be called before the proxy server let it through to the IPFS instance.

olizilla · 2018-01-24T10:28:21Z

add-on/src/lib/ipfs-companion.js

    registerListeners()
    await setApiStatusUpdateInterval(options.ipfsApiPollMs)
-    await storeMissingOptions(options, optionDefaults, browser.storage.local)
+    await storeMissingOptions(
+      await browser.storage.local.get(),


Can you explain this change? The code looks fine, just curious.

Yes! Previously storeMissingOptions wasn't doing anything.

On first run, options will be the same as optionDefaults. Due to the line earlier:

const options = await browser.storage.local.get(optionDefaults)

Regarding the parameter to get:

A key (string) or keys (an array of strings or an object specifying default values) to identify the item(s) to be retrieved from storage
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/StorageArea/get#Parameters

...so, it finds {} in storage and then assigns optionDefaults to it.

So storeMissingOptions is given two identical objects and does nothing.

Similarly for subsequent runs, options will be assigned any missing defaults so there's never anything to store!

Instead, we get a fresh copy of the current options, without defaults applied.

Nice catch! I think I've made this bug while porting from legacy SDK to WebExtension APIs.
Due to the "fallback to defaults" it worked anyway, so we never noticed :):)

...probably should be in a separate PR - apologies

olizilla · 2018-01-24T10:43:40Z

add-on/src/lib/ipfs-proxy/access-control.js

+      const acl = await this.getAcl()
+
+      acl[origin] = acl[origin] || {}
+      acl[origin][permission] = allow


Guard against the forbidden origins like __proto__? 👻

It's less pretty, but might be safer to structure the acl with data that comes from the outside as values rather than keys...

I think a Map instead of an plain old object would be safer right?

olizilla · 2018-01-24T11:12:02Z

add-on/src/lib/ipfs-proxy/index.js

+  'swarm.disconnect'
+]
+
+function createIpfsProxy (getIpfs, getState) {


This function is doing some intricate plumbing. It's very cool. Please add some docs to pin down some of the helpful background context that is in your head so future us can pick it up.

Things like "this is when a tab is created, but also caveat, caveat" and "This end is talking from proxy to ipfs node, this end is content script to proxy.", those sort of things.

olizilla · 2018-01-24T11:19:33Z

package.json

@@ -37,7 +40,10 @@
  "private": true,
  "preferGlobal": false,
  "devDependencies": {
+    "babel-preset-env": "^1.6.1",


Pin those deps.

olizilla · 2018-01-24T11:20:48Z

package.json

@@ -18,13 +15,19 @@
    "build:copy": "run-s build:copy:*",
    "build:copy:src": "shx mkdir -p add-on/dist && shx cp -R add-on/src/* add-on/dist",
    "build:copy:wx-polyfill-lib": "shx cp node_modules/webextension-polyfill/dist/browser-polyfill.min.js add-on/dist/contentScripts/browser-polyfill.min.js",
-    "build:js": "browserify -p prundupify -t browserify-css -t [ browserify-package-json --global ] add-on/src/background/background.js add-on/src/options/options.js add-on/src/popup/browser-action/index.js add-on/src/popup/quick-upload.js -p [ factor-bundle -o add-on/dist/background/background.js -o add-on/dist/options/options.js -o add-on/dist/popup/browser-action/browser-action.js -o add-on/dist/popup/quick-upload.js ] -o add-on/dist/ipfs-companion-common.js",
+    "build:js": "browserify -p prundupify -t browserify-css -t [ browserify-package-json --global ] add-on/src/background/background.js add-on/src/options/options.js add-on/src/popup/browser-action/index.js add-on/src/popup/quick-upload.js add-on/src/pages/proxy-acl/index.js -p [ factor-bundle -o add-on/dist/background/background.js -o add-on/dist/options/options.js -o add-on/dist/popup/browser-action/browser-action.js -o add-on/dist/popup/quick-upload.js -o add-on/dist/pages/proxy-acl/proxy-acl.js ] -o add-on/dist/ipfs-companion-common.js",


Note to self... capture the browserify config in a file so we can re-use it for watchify.

olizilla · 2018-01-24T11:29:00Z

test/functional/lib/ipfs-proxy/access-control.test.js

+    expect(acl).to.deep.equal({})
+
+    const sets = [
+      ['http://ipfs.io', 'ipfs.files.add', true],


Worth ensuring that urls are normalised correctly...

are

http://ipfs.io

http://ipfs.io/

http://ipfs.io/ipfs/QmHash

https://ipfs.io/

equivalent for permissions purposes? are

https://www.ipfs.io

https://ipfs.io

different for permissions purposes?

The origin for permissions purposes is extracted out of the URL that is reported by the port so this information doesn't come from "outside", and nor does the permission name (comes from ACL_FUNCTIONS) or allow/deny decision (comes from window.confirm).

So the origin is normalised already by virtue of parsing the URL and extracting the origin. For permissions we're using the same-origin policy, so URLs for a different origin need different approval.

An origin is defined as a combination of URI scheme, host name, and port number
https://en.wikipedia.org/wiki/Same-origin_policy

http://ipfs.io and http://ipfs.io/ are normalised to http://ipfs.io

http://ipfs.io/ipfs/QmHash not supported as a separate "origin" yet (mentioned in "What's still todo (in a future PR)?" in the description) so origin would be http://ipfs.io

https://ipfs.io is a different origin for permissions (because scheme differs)

https://www.ipfs.io and https://ipfs.io, also a different origin for permissions (because host differs)

ah sure, yes, I was trying to say that there is an implicit dependency for this to function correctly on the url having been normalised before it gets passed on to the ACL, but there is no test to assert it.

With the new changes the ACL will error out if the user tries to set a value that isn't a valid origin as normalised by URL, so i guess that covers it.

✨ 🚀 ✨

olizilla

Very nice work @alanshaw

olizilla · 2018-01-24T11:44:52Z

~~npm run build fails with:~~

Ignore me. I didn't have the latest commit. It builds fine under both npm and yarn now.

olizilla · 2018-01-24T12:20:53Z

No window.ipfs when run in chrome atm.

alanshaw · 2018-01-24T16:38:42Z

@olizilla review feedback done, is good now?

olizilla · 2018-01-24T17:19:24Z

add-on/src/lib/ipfs-proxy/index.js

@@ -5,6 +5,8 @@ const browser = require('webextension-polyfill')
 const { createProxyServer, closeProxyServer } = require('ipfs-postmsg-proxy')
 const AccessControl = require('./access-control')

+// These are the functions that require an allow/deny decision by the user.
+// All other exposed IPFS functions are available to call without authorization.


Perhaps one for a next PR, but ongoing maintenance would be simpler if we flip this to a whitelist of methods that can be called without access control... as the ipfs api changes we can jump to the latest release and then figure out later if a new api should be whitelisted.

olizilla · 2018-01-24T17:54:52Z

It's working in Chrome again now! Is very nice.

lidel

Fantastic work @alanshaw 🚀 👍

Added some notes in code and below:

acls
Before we merge this I think we should switch ACLs from whitelist-by-default to prompt-by-default. Better to sort this out from the beginning.

window.confirm
I don't think a window.confirm prompt is enough in the long run, even if we ignore issue with Firefox.

User should be able to select one of four options:
(i) allow method (ii) deny method
(iii) allow everything for this origin (iv) deny everything for this origin

We could simplify UI for this by going with two buttons (allow aX/deny X) and a checkbox "remember choice for all API functions for this origin", which when checked changes "X" from function name to "everything", or something like that.

I was thinking about providing custom page via browserAction, but we can't trigger "onclick" on it programmatically.

Here's an idea: what if we display a prompt in.. a new tab, that is closed right after user picks allow/deny?

lidel · 2018-01-24T21:25:19Z

add-on/_locales/en/messages.json

@@ -265,6 +265,18 @@
    "message": "DNSLINK Support",
    "description": "An option title on the Preferences screen (option_dnslink_title)"
  },
+  "option_ipfsProxy_title": {
+    "message": "window.ipfs",
+    "description": "An option title for enabling/disabling the IPFS proxy"


Please append key names at the end of descriptions, eg:

"description": "An option title for enabling/disabling the IPFS proxy (option_ipfsProxy_title)"

This is so that crowdin users can see it as additional hint about message's context.
(Crowdin UI hides key names for some reason 🤔 )

lidel · 2018-01-24T21:56:08Z

add-on/src/contentScripts/ipfs-proxy/page.js

+const _Buffer = Buffer
+
+window.Buffer = window.Buffer || _Buffer
+window.Ipfs = window.Ipfs || Ipfs


I wonder if we should expose both window.Ipfs and window.ipfs.
Naming is quite unfortunate :'(

Is there a good use case for exposing Ipfs ?

Maybe you want an IPFS node configured with particular swarm peers so using the one provided by the extension is not possible? ...and you don't want to download the IPFS lib because you're on a poor connection?

Maybe...for experiments?

Are they good enough?

I'd say yes, it would make developer's life easier. 👍
But perhaps it should be renamed to window.IpfsApi or something like that?

@diasdavid, would appreciate your thoughts on this from js-ipfs-api perspective

lidel · 2018-01-24T22:32:55Z

add-on/src/lib/ipfs-proxy/index.js

+const AccessControl = require('./access-control')
+
+// These are the functions that require an allow/deny decision by the user.
+// All other exposed IPFS functions are available to call without authorization.


While this is fine for a PoC, real life implementation has to be the other way around.
Namely, we should require ACL for everything by default, and have a short whitelist of safe functions that do not require permission.
That way we will not introduce security issues when version bump of js-ipfs(-api) introduces a new function.

I agree and I'm happy to switch, but can this be done in a separate PR so we can get this merged?

Just FYI, bumping js-ipfs alone wouldn't expose new functions, you'd have to also add the new functions to ipfs-postmsg-proxy for them to become available.

I think it can be a separate PR.
As long as the checkbox enabling window.ipfs is disabled by default, we can merge it. 👍

Yep, window.ipfs checkbox is disabled by default

lidel · 2018-01-24T22:49:15Z

package.json

@@ -48,8 +54,11 @@
    "fs-promise": "2.0.3",
    "geckodriver": "1.10.0",
    "husky": "0.14.3",
+    "ignore-styles": "^5.0.1",


Let's drop ^ :)
(here, in nyc and p-queue)

Yep, no problem

lidel · 2018-01-24T23:02:14Z

add-on/src/lib/ipfs-proxy/access-control.js

+  constructor (storage, key = 'ipfsProxyAcl') {
+    super()
+    this._storage = storage
+    this._key = key


What if we store ACL keys with a prefix?
eg. set this._key = 'ipfsProxyAcl.' + key (or add/remove a prefix in getAcl and _setAcl)
It removes risk of overriding storage keys + makes it easier to browse/sort local storage keys when debugging the extension.

So, just to confirm our keys would look like 'ipfsProxyAcl.http://google.co.uk for example?

Currently things look sort of like this:

{ ipfsProxyAcl: { 'http://google.co.uk': { 'files.add': true }, 'https://ipfs.io': { 'files.add': false, 'object.new': false } } }

...and we'd be altering it to be something like:

{ 'ipfsProxyAcl.http://google.co.uk': { 'files.add': true }, 'ipfsProxyAcl.https://ipfs.io': { 'files.add': false, 'object.new': false } }

I like this and I think it makes things simpler, the only issue I can think of is getting the entire ACL for the manage permissions page might be tricky - we'd have to read everything in storage into memory and filter just the keys we need.

I think that's a better trade of though since atm we're reading the entire ACL into memory for every getAccess and setAccess, which will be far more commonly called than getAcl.

Sounds like a plan 👌
I think its the only thing that blocks merging this PR, other things can be addressed in separate PRs

lidel · 2018-01-24T23:14:59Z

add-on/src/lib/ipfs-proxy/index.js

+  let allow
+
+  try {
+    allow = window.confirm(msg)


This is just FYI, but just like you said this won't work on Firefox. Modals are not allowed from background page.
Confirmed with ipfs.add – it results in no prompt, access is automatically denied:

Browser Console confirms prompt was not allowed:

There are related tickets at bugzilla tl;dr this is by design and they suggest to display prompt via content script:

We don't intend to support alert() in background pages.
If you want to alert data based on a context menu item, you can do so from a content script.
– https://bugzilla.mozilla.org/show_bug.cgi?id=1203394#c41

I'm telling you that it won't be accessible from background pages. Alerts are modal dialogs, and they're meant to be tied to a specific, user-visible window. If you want to trigger an alert from a context menu operation, it needs to be triggered from the appropriate content window.
– https://bugzilla.mozilla.org/show_bug.cgi?id=1203394#c43

Yep, as I also mentioned in the description, the workaround for Firefox right now will be to go to the "manage permissions" page and change the "deny" to "allow".

I will send a separate PR for a custom confirm dialog with options to allow/deny all.

@alanshaw I've noticed and found how Greasemonkey displays confirmation dialog (under Firefox):

https://github.com/greasemonkey/greasemonkey/blob/9ce5dabc74496fa6a631d991302ad4fd80730f1d/src/bg/user-script-detect.js#L76-L99

(HTML page loaded in new browser window with 'type': 'popup')

What is also interesting is Android detection via chrome.runtime.getPlatformInfo :)

alanshaw · 2018-01-25T12:52:21Z

@lidel is it good to go now?

daviddias

👍

lidel · 2018-01-25T23:15:18Z

Yes! 💯 Fantastic work @alanshaw! 👌 🚀 ✨

victorb reviewed Dec 13, 2017

View reviewed changes

olizilla mentioned this pull request Dec 13, 2017

Expose IPFS API as window.ipfs #330

Closed

7 tasks

lidel reviewed Dec 14, 2017

View reviewed changes

alanshaw force-pushed the feat/global-ipfs branch from cd992d9 to 5518545 Compare January 9, 2018 09:05

alanshaw force-pushed the feat/global-ipfs branch from bd84eb7 to 0e42bdf Compare January 22, 2018 14:51

alanshaw and others added 15 commits January 23, 2018 15:43

adds window.ipfs object to pages

5e9e2a1

uses postmsg-rpc module

2d13c37

updates dependencies

51c0afe

uses ipfs-postmsg-proxy

c1d4c98

updates to [email protected]

f21fcac

add POC access control for proxied IPFS functions

e061b6f

adds access control list management page

0eeb425

i18n for proxy ACL page

c0eb828

fixes i18n getMessage call

ab8300f

adds IPFS to page only when option is set

d49cf04

wip adds access control tests

521e909

more wip access control tests

87af28f

adds remaining tests and fixes for access control

de6de37

adds proxy-acl page tests

01ae150

cleanups

9b29486

alanshaw force-pushed the feat/global-ipfs branch from cc5db5c to 9b29486 Compare January 23, 2018 15:52

fixes permissions page url and logo src

9b0009d

alanshaw changed the title ~~[WIP] window.ipfs~~ window.ipfs Jan 23, 2018

alanshaw requested review from olizilla, lidel and victorb January 23, 2018 16:46

victorb reviewed Jan 23, 2018

View reviewed changes

olizilla reviewed Jan 24, 2018

View reviewed changes

olizilla requested changes Jan 24, 2018

View reviewed changes

alanshaw added 5 commits January 24, 2018 12:23

fixes execute content script in chrome

af59463

changes access control implementation to use Map and validate parameters

3886340

adds tests for access control param validation

cbeed1a

pins dependencies

d02c787

adds comments and reorganises code for better readability

9aa0f8e

olizilla reviewed Jan 24, 2018

View reviewed changes

olizilla approved these changes Jan 24, 2018

View reviewed changes

lidel requested changes Jan 24, 2018

View reviewed changes

alanshaw added 4 commits January 25, 2018 10:05

pin all the dependencies!

4aa6408

adds key names at the end of descriptions

c5a7ee1

changes acl storage structure

9237bfb

stringifies ACL data because chrome does not store arrays!?

5dd1116

lidel approved these changes Jan 25, 2018

View reviewed changes

daviddias approved these changes Jan 25, 2018

View reviewed changes

lidel merged commit e038ed9 into ipfs:master Jan 25, 2018

alanshaw deleted the feat/global-ipfs branch January 26, 2018 07:01

alanshaw mentioned this pull request Feb 1, 2018

Fixes window.ipfs is added after page load #368

Merged

lidel mentioned this pull request Feb 8, 2018

Custom dialog for window.ipfs ACLs (make it work under Firefox) #377

Closed

window.ipfs #333

window.ipfs #333

Conversation

alanshaw commented Dec 12, 2017 • edited Loading

What's in this PR?

What's still todo (in a future PR)?

How does it work?

Choose a reason for hiding this comment

victorb left a comment

Choose a reason for hiding this comment

lidel Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olizilla Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olizilla left a comment

Choose a reason for hiding this comment

olizilla commented Jan 24, 2018 • edited Loading

olizilla commented Jan 24, 2018 • edited Loading

alanshaw commented Jan 24, 2018

Choose a reason for hiding this comment

olizilla commented Jan 24, 2018

lidel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

alanshaw commented Jan 25, 2018

daviddias left a comment

Choose a reason for hiding this comment

lidel commented Jan 25, 2018 • edited Loading

alanshaw commented Dec 12, 2017 •

edited

Loading

lidel Dec 14, 2017 •

edited

Loading

olizilla Jan 24, 2018 •

edited

Loading

olizilla commented Jan 24, 2018 •

edited

Loading

olizilla commented Jan 24, 2018 •

edited

Loading

lidel Jan 25, 2018 •

edited

Loading

lidel Jan 25, 2018 •

edited

Loading

lidel Jan 29, 2018 •

edited

Loading

lidel commented Jan 25, 2018 •

edited

Loading