Fixes window.ipfs is added after page load #368

alanshaw · 2018-02-01T16:37:27Z

fixes #362

also fixes whitelist by default from review feedback

alanshaw · 2018-02-01T16:41:03Z

add-on/src/lib/ipfs-proxy/acl-whitelist.json

+  "swarm.peers",
+  "swarm.addrs",
+  "swarm.localAddrs"
+]


Too permissive?

This is a whitelist right? Any other calls will by default be blocked? Think they are all fine, but I do worry about for example MFS being totally open for everyone with access to window.ipfs.

I suggest we remove all *.ls from whitelist for now. Adding / reading specific hash can be done without explicit permission, but listing internal node states should require explicit confirmation.

lidel · 2018-02-01T19:38:32Z

add-on/src/lib/ipfs-proxy/acl-whitelist.json

+  "swarm.peers",
+  "swarm.addrs",
+  "swarm.localAddrs"
+]


I suggest we remove all *.ls from whitelist for now. Adding / reading specific hash can be done without explicit permission, but listing internal node states should require explicit confirmation.

lidel · 2018-02-01T19:41:16Z

package.json

@@ -44,6 +46,7 @@
    "babel-preset-es2015": "6.24.1",
    "babel-preset-es2017": "6.24.1",
    "babelify": "8.0.0",
+    "brfs": "^1.4.4",


victorb · 2018-02-02T11:59:53Z

Tried it out, worked beautifully! Nice to see a IPFS app whose size is only 2.2K 👯

victorb · 2018-02-02T12:02:31Z

yarn.lock

@@ -5003,7 +5343,7 @@ level-js@^2.2.4:
    typedarray-to-buffer "~1.0.0"
    xtend "~2.1.2"

-level-js@timkuijsten/level.js#idbunwrapper:
+"level-js@github:timkuijsten/level.js#idbunwrapper":


Weird, somehow this changed from being a npm link (I think?) to being a github link, requiring git and therefore breaking the docker image.

Fixed it in 1a968ee. More context:

Same thing happened to me after a yarn cache clean (trying to get a git branch dependency to update).

Removing the node_modules folder and running yarn again seemed to work around the issue.
– yarnpkg/yarn#2083 (comment)

Closed since this seems to be resolved in recent versions.
– yarnpkg/yarn#2083 (comment)

alanshaw · 2018-02-05T08:47:31Z

@lidel good to merge now?

ipfs#368 (review)

lidel

@alanshaw after solving problem with git (1a968ee) now we are blocked by a problem with addon linter (npm run lint):

ERRORS:

Code             Message                       Description                                                                                            File                                        Line   Column
FILE_TOO_LARGE   File is too large to parse.   This file is not binary and is too large to parse. Files larger than 4MB will not be parsed. If your   dist/contentScripts/ipfs-proxy/content.js                
                                               JavaScript file has a large list, consider removing the list and loading it as a separate JSON file                                                             
                                               instead.

and indeed, our content scripts take more space than entire addon 🙃

--- ipfs-companion/add-on/dist ---
    8.0 MiB [##########] /contentScripts
    3.9 MiB [####      ]  ipfs-companion-common.js
    1.2 MiB [#         ] /background
  404.0 KiB [          ] /popup 
  184.0 KiB [          ] /pages
   64.0 KiB [          ] /options

--- ipfs-companion/add-on/dist/contentScripts/ipfs-proxy ---
    4.1 MiB [##########]  content.js
    3.9 MiB [######### ]  page.js
    4.0 KiB [          ]  inject-script.js

alanshaw · 2018-02-05T10:25:28Z

eek, wtf?! I'm guessing that's because we're adding window.Ipfs as well. I think we could get rid of half of that by removing dist/contentScripts/ipfs-proxy/page.js after dist/contentScripts/ipfs-proxy/content.js is generated (page.js is inlined into content.js so isn't actually used). Would that be acceptable or should we remove window.Ipfs altogether?

lidel · 2018-02-05T10:36:31Z

I am afraid removing page.js is not enough: content.js itself is >4MiB.
We need to remove window.Ipfs for now, or find a way to go under linter limit somehow.

alanshaw · 2018-02-05T10:51:08Z

I can bring content.js down to 2MB if I uglifyify page.js (this includes window.Ipfs) - is that cool?

lidel · 2018-02-05T11:02:03Z

Yes, we can minimize the output as long we have reproducible builds (#306).

alanshaw · 2018-02-07T10:25:22Z

@lidel what did you do to fix the yarn issue?

lidel · 2018-02-07T11:00:04Z

@alanshaw removing the node_modules folder and running yarn again seemed to work around the issue (I did it on a linux box, if that matters. idea from: yarnpkg/yarn#2083 (comment))

alanshaw · 2018-02-07T13:58:47Z

🙄 that's what I did...

lidel · 2018-02-07T14:12:48Z

Did you run via npm run yarn-build to guarantee correct yarn is used?
Perhaps you have an older version installed globally. 🤔

alanshaw · 2018-02-07T14:21:37Z

I did yes. I'm not sure how we can avoid this when our dependencies have github dependencies e.g.

https://github.com/libp2p/js-libp2p-crypto/blob/71339e08e75f6c92485e22a95caf96164cbb1cad/package.json#L45

alanshaw · 2018-02-07T14:34:24Z

@lidel I'm not sure your commit in 1a968ee fixed the build - I still see lots of github: dependencies in that file.

Temporary fix until ipfs#372 is merged

lidel · 2018-02-07T23:03:09Z

@alanshaw I've just added git to the image for now and it fixed the build.
(CI is being reworked in #372, but i did not want to port that PR here, just fixed alpine image so I can merge this PR)

This change enables Firefox users to disable creation of window.ipfs, solving fingerprinting issue raised in: #451 The key difference between tabs.executeScript and contentScripts API is that the latter provides guarantee to execute before anything else. Chrome does not provide contentScripts API and we need to statically load content_script via manifest. More info on the underlying issue: #368 #362 (comment)

This PR enables Firefox users to disable creation of `window.ipfs` attribute via _Preferences_ screen, solving fingerprinting issue raised in #451. It requires webpack, so should be merged after #498 ### Background Existing `tabs.executeScript` API with `runAt: 'document_start'` flag was executing too late and page scripts were unable to detect `window.ipfs` correctly. More info on the underlying issue: #368 #362 (comment) As a workaround that worked in both Chrome and Firefox, we were forced to always load content script via manifest definition. This meant no control over when it is loaded and no easy off switch. If `window.ipfs` was disabled via _Preferences_, it was throwing an Error on access, but remained in the scope. Mozilla added [`contentScripts`](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contentScripts/register) API in Firefox 59. The key difference between `tabs.executeScript` and `contentScripts` API is that the latter provides guarantee to execute before anything else on the page, passing [our synchronous smoke test](https://ipfs-shipyard.github.io/ipfs-companion/docs/examples/window.ipfs-fallback.html). ### Known Issues Chrome does not provide `contentScripts` API so it will continue to statically load `content_script` via manifest. Hopefully with time, other browser vendors will adopt the new API.

alanshaw requested review from olizilla, lidel and victorb February 1, 2018 16:40

alanshaw commented Feb 1, 2018

View reviewed changes

lidel requested changes Feb 1, 2018

View reviewed changes

lidel mentioned this pull request Feb 1, 2018

window.ipfs injected after page loaded #362

Closed

alanshaw added 3 commits February 2, 2018 09:28

fixes window.ipfs is added after page load

8b6cc0e

removes hats

40a9778

removes *.ls from whitelist

4604290

alanshaw force-pushed the fix/window.ipfs-added-after-page-load branch from 8c1fded to 4604290 Compare February 2, 2018 09:31

victorb reviewed Feb 2, 2018

View reviewed changes

adds tests for preAcl

a2b1377

alanshaw changed the title ~~[WIP] Fixes window.ipfs is added after page load~~ Fixes window.ipfs is added after page load Feb 5, 2018

alanshaw and others added 2 commits February 5, 2018 08:58

removes .only

6296a78

yarn fix: remove dependency on git binary

1a968ee

ipfs#368 (review)

lidel requested changes Feb 5, 2018

View reviewed changes

alanshaw added 2 commits February 5, 2018 20:59

uglifies proxy content script and removes unused (inlined) page script

0f7503c

Merge branch 'master' into fix/window.ipfs-added-after-page-load

3b4e4f7

Fix Jenkins by adding git to alpine image

e618995

Temporary fix until ipfs#372 is merged

lidel approved these changes Feb 7, 2018

View reviewed changes

lidel merged commit 6891f74 into ipfs:master Feb 7, 2018

lidel mentioned this pull request Jun 8, 2018

feat: disable creation of window.ipfs attribute in Firefox #495

Merged

alanshaw deleted the fix/window.ipfs-added-after-page-load branch June 13, 2018 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes window.ipfs is added after page load #368

Fixes window.ipfs is added after page load #368

alanshaw commented Feb 1, 2018 •

edited

Loading

alanshaw Feb 1, 2018

victorb Feb 1, 2018

lidel Feb 1, 2018 •

edited

Loading

lidel Feb 1, 2018 •

edited

Loading

lidel Feb 1, 2018

alanshaw Feb 2, 2018

victorb commented Feb 2, 2018

victorb Feb 2, 2018

lidel Feb 5, 2018 •

edited

Loading

alanshaw commented Feb 5, 2018

lidel left a comment •

edited

Loading

alanshaw commented Feb 5, 2018

lidel commented Feb 5, 2018 •

edited

Loading

alanshaw commented Feb 5, 2018

lidel commented Feb 5, 2018

alanshaw commented Feb 7, 2018

lidel commented Feb 7, 2018

alanshaw commented Feb 7, 2018

lidel commented Feb 7, 2018

alanshaw commented Feb 7, 2018

alanshaw commented Feb 7, 2018

lidel commented Feb 7, 2018

Fixes window.ipfs is added after page load #368

Fixes window.ipfs is added after page load #368

Conversation

alanshaw commented Feb 1, 2018 • edited Loading

alanshaw Feb 1, 2018

Choose a reason for hiding this comment

victorb Feb 1, 2018

Choose a reason for hiding this comment

lidel Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

lidel Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

lidel Feb 1, 2018

Choose a reason for hiding this comment

alanshaw Feb 2, 2018

Choose a reason for hiding this comment

victorb commented Feb 2, 2018

victorb Feb 2, 2018

Choose a reason for hiding this comment

lidel Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

alanshaw commented Feb 5, 2018

lidel left a comment • edited Loading

Choose a reason for hiding this comment

alanshaw commented Feb 5, 2018

lidel commented Feb 5, 2018 • edited Loading

alanshaw commented Feb 5, 2018

lidel commented Feb 5, 2018

alanshaw commented Feb 7, 2018

lidel commented Feb 7, 2018

alanshaw commented Feb 7, 2018

lidel commented Feb 7, 2018

alanshaw commented Feb 7, 2018

alanshaw commented Feb 7, 2018

lidel commented Feb 7, 2018

alanshaw commented Feb 1, 2018 •

edited

Loading

lidel Feb 1, 2018 •

edited

Loading

lidel Feb 1, 2018 •

edited

Loading

lidel Feb 5, 2018 •

edited

Loading

lidel left a comment •

edited

Loading

lidel commented Feb 5, 2018 •

edited

Loading