Ipfs-companion: Remove use of unsafe-eval

[lidel]

We allowed unsafe-eval in browser extension because browserified bundle ipfs-api.min.js relies on eval. Unfortunately Mozilla does not accept extensions with unsafe-eval:

The use of 'unsafe-eval' is not allowed in the manifest.json's CSP as it can cause major security issues. [feedback from review]

[..] extensions with 'unsafe-eval', 'unsafe-inline', remote script, or remote sources in their CSP are not allowed for extensions listed on addons.mozilla.org due to major security issues. [[MDN](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/content_security_policy)]

We need to address it somehow.

Any ideas @ipfs/javascript-team?
Is it possible to create own bundle without eval?
We can start using webpack if needed.

[styfle]

Can you link to the code that is doing the eval? And what is it evaluating?

[daviddias]

searching for eval on the min file reveals that is something that Webpack injects as part of its bundling:

Which gets in because of the script loader https://github.com/webpack/webpack/issues/4899

Investigating how to disable it (it's not actually present in our config file https://github.com/ipfs/aegir/blob/master/config/webpack.js)

[yfdyh000]

https://github.com/substack/vm-browserify/blob/bfd7c5f59edec856dc7efe0b77a4f6b2fa20f226/example/run/bundle.js#L430

[lidel]

FYSA we need to resolve this two-three weeks before Firefox 57 is released, otherwise users will be left dry without working browser extension :(

[daviddias]

I believe we used the script-loader in the past due to our usage of node-forge which we don't use anymore.

@dignifiedquire sounds like the removal of script-loader can be one of the features for aegir@next? What do you say?

[dignifiedquire]

I do not think the issue is the script-loader, I searched through all deps and it does not get installed and still eval is present in the built file.

[dignifiedquire]

I believe I found the offender. The shim for vm relies on eval, and it seems this is used in the dependency asn1.

[dignifiedquire]

Here: https://github.com/indutny/asn1.js/blob/master/lib/asn1/api.js#L21 to be exact

[daviddias]

Need to think more about this, but I'm guessing that we can get rid of this https://github.com/substack/vm-browserify/blob/master/index.js#L105

[dignifiedquire]

Looking at this PR it seems that things should work, it will try to use the eval and fail due to csp but still work. @lidel is the issue that any warning with eval will be rejected, or did you actually experienced failures because of the csp restrictions?

[lidel]

Mozilla rejected our submission due to unsafe-eval in our CSP:

https://github.com/ipfs/ipfs-companion/blob/bbeaddeedf1a4781cd0dd40ba822911ee16269c9/add-on/manifest.json#L78

They may not care if eval is in minified file as long as unsafe-eval is removed from CSP.

Which I can't do because ipfs-api (v14.0.4) does not work in WebExtension context without it.

@dignifiedquire It fails to load under both Firefox and Chrome:

[dignifiedquire]

@lidel @diasdavid I have an experimental build which should improve things, could you try this out and let me know if it helps? (based on master if js-ipfs-api)
dist.zip

[lidel]

@dignifiedquire I replaced ipfs-api.min.js with index.js from your .zip and retested under Chrome/Firefox, both still fail due to CSP:

Seems that src String causes Function.apply to fail due to CSP:

[daviddias]

@lidel you never had issues with connect-src on the CSP since you always dial to a localhost node, right?

If we were to try #248, we would need to figure out connect-src as it would block dials to WebSocket nodes.

[lidel]

@diasdavid correct, localhost and HTTP-only (no WSS), so there were no issues as long unsafe-eval is added.

[daviddias]

@lidel @dignifiedquire what's the status here, any other ideas?

[lidel]

No _good_ ideas on my end, just status update:

Firefox release is blocked due to unsafe-eval in manifest.json/content_security_policy
The eval seems to be required by both js-ipfs-api and is-ipfs. Both are built by AEgir.

The best case would be to find an upstream fix: the problem is not limited to this extension, as you've seen in https://github.com/diasdavid/browser-laptop/pull/1#pullrequestreview-59048553 use of eval always causes weird looks. If we could remove its use completely, that would solve us headache across all projects.

There is a possibility we could find a workaround and sandbox eval, somehow.
I did some research at https://discourse.mozilla.org/c/add-ons, but found no working solution.
I've seen some iframe-based sandboxing attempts, but as far I can read into them, they address different issues and still require unsafe-eval and/or unsafe-inline.

[dignifiedquire]

Update: Found out the Function.apply is from generator-function, which is a dependency of https://github.com/mafintosh/protocol-buffers which we use for protobuf support. Currently working on removing the use of that library.

[dignifiedquire]

Step 1: https://github.com/ipfs/js-ipfs-unixfs/pull/18

[dignifiedquire]

Step 2: https://github.com/ipld/js-ipld-dag-pb/pull/39

[dignifiedquire]

Step 3: https://github.com/libp2p/js-libp2p-crypto/pull/107

[dignifiedquire]

@lidel build without protocol-buffers lib and no Function.apply dist 2.zip. Want to give it another spin?

[lidel]

@dignifiedquire this version works without unsafe-eval in manifest.json, extension loaded and functionality seems to work fine 👍 🎉

FYI there was one CSP error:

it was triggered by this line of dist/index.js:

var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval

but from what I was able to check it does not impact anything we use in browser extension.

As long as we don't have unsafe-eval in manifest.json it should pass the review at Mozilla, but if we can eliminate CSP errors from Browser Console completely, that would be even better.

Is it technically possible?

[dignifiedquire]

Great to hear that @lidel. That eval is coming from our new protobuf library: https://github.com/dcodeIO/protobuf.js/issues/593 but should hopefully resolved soonish.

[dignifiedquire]

@lidel latest release of js-ipfs-api should be free of eval, hopefully. Please give it another try :)

[lidel]

@dignifiedquire The latest one at npmjs.com is [email protected] and it still produces error due to Function.apply. I re-tested version from dist 2.zip and it still works fine.

I guess either you did not publish the latest version yet or there is some kind of regression since dist 2.zip

[daviddias]

@lidel I was lucky with js-ipfs, you might have to do a fresh npm install and fresh npm run build with ipfs-api.

[lidel]

Just to eliminate possibility of me going insane 🙃

Does:

curl -s https://unpkg.com/[email protected]/dist/index.js | grep -B 10 Function.apply 

return:

var src = 'return ('+line.toString()+')'

    var keys = Object.keys(scope || {}).map(function(key) {
      return key
    })

    var vals = keys.map(function(key) {
      return scope[key]
    })

    return Function.apply(null, keys.concat(src)).apply(null, vals)

on your boxes as well?

[dignifiedquire]

Seems something went wrong in the release, unpkg version shows Function.apply but when I build on master with fresh deps it the apply is gone

[daviddias]

One more time 🎶

[lidel]

[email protected] works indeed! 🚀

[dignifiedquire]

[email protected] works indeed! 🚀

AWESOME 🎉