Skip to content
This repository has been archived by the owner on Jun 16, 2020. It is now read-only.

IPFS Integration #1

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

IPFS Integration #1

wants to merge 11 commits into from

Conversation

daviddias
Copy link
Collaborator

@daviddias daviddias commented Jun 27, 2017

WIP PR for brave#9556 (comment)

Notes to self (or others that might want to contribute :))

Folder structure

.
├── ...
├── app        # Where the electron's main process things live
│   ├── # ...
│   ├── browser
│   │   ├── # ...
│   │   ├── ipfs.js   # The ipfs instance running on the main process
│   │   ├── # ...
│   │   ├── webtorrent.js  # The webtorrent instance running on the main process
│   │   └── # ...
│   ├── extensions # Where the extensions live, except the actual app that gets rendered to the user
│   │   ├── brave
│   │   ├── ipfs
│   │   └── torrent
│   └── # ...
├── js
│   ├── # ...
│   ├── ipfs # Where the IPFS extension app lives  (renderer process)
│   │   └── entry.js
│   ├── lib # Lot's of utils 
│   │   ├── appUrlUtil.js
│   │   ├── base64.js
│   │   ├── baseDomain.js
│   │   ├── classSet.js
│   │   ├── color.js
│   │   ├── cryptoUtil.js
│   │   ├── debounce.js
│   │   ├── eventUtil.js
│   │   ├── functional.js
│   │   ├── imageUtil.js
│   │   ├── importer.js
│   │   ├── openSearch.js
│   │   ├── promisify.js
│   │   ├── psl.js
│   │   ├── request.js
│   │   ├── text.js
│   │   ├── textCalculator.js
│   │   ├── throttle.js
│   │   ├── urlutil.js
│   │   └── zoom.js
│   └── webtorrent # The WebTorrent app (renderer process)
│       ├── components
│       └── entry.js
└── # ...

incognito: 'split',
// TODO(diasdavid) How to get this key??
key: 'MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAyWl+wMvL0wZX3JUs7GeZAvxMP+LWEh2bwMV1HyuBra/lGZIq3Fmh0+AFnvFPXz1NpQkbLS3QWyqhdIn/lepGwuc2ma0glPzzmieqwctUurMGSGManApGO1MkcbSPhb+R1mx8tMam5+wbme4WoW37PI3oATgOs2NvHYuP60qol3U7b/zB3IWuqtwtqKe2Q1xY17btvPuz148ygWWIHneedt0jwfr6Zp+CSLARB9Heq/jqGXV4dPSVZ5ebBHLQ452iZkHxS6fm4Z+IxjKdYs3HNj/s8xbfEZ2ydnArGdJ0lpSK9jkDGYyUBugq5Qp3FH6zV89WqBvoV1dqUmL9gxbHsQIDAQAB'
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⁉️

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -49,6 +49,8 @@ module.exports = {
},
braveExtensionId: 'mnojpmjdmbbfmejpflffifhffcmidifd',
torrentExtensionId: 'fmdpfempfmekjkcfdehndghogpnpjeno',
// TODO(diasdavid) how to these get calculated?
ipfsExtensionId: 'asdakdaklsjdaklsjdalsdaslkdjasds',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⁉️

@@ -427,6 +427,8 @@ const handleAppAction = (action) => {
appState = filtering.init(appState, action, appStore)
appState = basicAuth.init(appState, action, appStore)
appState = webtorrent.init(appState, action, appStore)
// TODO(diasdavid) - init IPFS in the main process
// appState = ipfs.init(appState, action, appStore)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⁉️ Should I do this here?

@@ -316,7 +323,8 @@ class Frame extends React.Component {
// In this case both the user display and the user think they're on this.props.location.
if (this.props.tabUrl !== this.props.location &&
!this.isAboutPage() &&
!isTorrentViewerURL(this.props.location)) {
!isTorrentViewerURL(this.props.location) &&
!isIPFSPath(this.props.location)) {
Copy link
Collaborator Author

@daviddias daviddias Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I got this code path right (it checks if location starts with dweb: or ipfs://), however, I never managed to get the log on 74 to print.

@daviddias daviddias force-pushed the feat/ipfs branch 3 times, most recently from 4e40ac2 to 9d4381a Compare June 28, 2017 09:47
js/ipfs/entry.js Outdated
@@ -0,0 +1,3 @@
// const ipc = window.chrome.ipcRenderer

console.log('woot! managed to capture the ipfs:// or dweb:// protocols')
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌟 if I get this one to run, I believe I know what to do next :)

if (getSetting(settings.IPFS_ENABLED)) {
extensionInfo.setState(config.ipfsExtensionId, extensionStates.REGISTERED)
loadExtension(config.ipfsExtensionId, getExtensionsPath('ipfs'), generateIPFSManifest(), 'component')
} else {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that getSetting(settings.IPFS_ENABLED)) prints true, which means that the extension should be loaded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 537 and 538 do nothing for some reason.

@daviddias daviddias force-pushed the feat/ipfs branch 3 times, most recently from e6be4bc to 4737315 Compare June 29, 2017 22:48
@@ -471,6 +531,17 @@ module.exports.init = () => {
extensionActions.extensionDisabled(config.torrentExtensionId)
}

// ipfsExtension
const isIPFSEnabled = getSetting(settings.IPFS_ENABLED)
console.log('IPFS: is extension enabled', isIPFSEnabled, config.ipfsExtensionId, getExtensionsPath('ipfs'))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prints:
IPFS: is extension enabled true interplanetaryfilesystemuniverse /Users/koruza/code/brave-browser-laptop/app/extensions/ipfs

@daviddias
Copy link
Collaborator Author

@jonathansampson @bridiver @NejcZdovc mind taking a look at what I might be failing when loading this extension?

// Returns the Chromium extension manifest for the ipfsExtension
// The ipfsExtension handles ipfs:// and dweb:
// Analagous to the PDFJS extension, it shows a special UI for that type of resource
let generateIPFSManifest = () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need all this. You can just create a regular manifest file and pass the path to the directory like non-component extensions

@daviddias
Copy link
Collaborator Author

Turning on tabbycat just started throwing uncaught exceptions:
image

Have you seen this behaviour?

@jonathansampson
Copy link

@diasdavid I have not seen that issue before, but when I ran Tabby Cat in Brave, we didn't have the about:preferences#extensions page. Are you able to share any further information about the Exception itself? Perhaps file/line-number?

You may even be able to break on exception from the Shift+F8 developer tools. Happy to help investigate further if needed.

@daviddias
Copy link
Collaborator Author

🗡 I've slain the first 🐉

image

callback('hi there!' + test() + IPFS) // eslint-disable-line
})
*/
callback('hi there!' + test() + IPFS) // eslint-disable-line
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm being unable to create an IPFS node as the background process crashes (for some reason) and I have no way to inspect it. Looking for a better way to debug this background extension.

@daviddias daviddias force-pushed the feat/ipfs branch 3 times, most recently from a3a481b to 6ea5fdf Compare August 22, 2017 06:49
@@ -45,7 +45,8 @@
"watch-all": "npm run watch & npm run watch-test",
"watch-test": "cross-env NODE_ENV=test webpack --watch",
"webpack": "webpack",
"win-renpm": "powershell ./tools/windows/re-npm.ps1"
"win-renpm": "powershell ./tools/windows/re-npm.ps1",
"ipfs": "wget https://unpkg.com/ipfs/dist/index.min.js -O app/extensions/ipfs/js/ipfs.min.js"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this step to make it simpler to reproduce my local set up.

@daviddias
Copy link
Collaborator Author

Update - Great news everyone!

We have successfully managed to run IPFS from a background process in a Brave Extension, capturing the ipfs:// protocol and resolving an hash from the IPFS network, see the image below for the full result.

image

The main issue we were facing was that the CSP was blocking our use of eval, which is exactly that @lidel while developing the Chrome and Firefox Web Extension with js-ipfs-api. For now, we removed that restriction for testing purposes but before we merge and release this feature, we want to:

  • make sure that we really need to use eval (can we change how the dependency that is injecting it works?)
  • if we absolutely need to use eval, can we ensure that it doesn't open any security vulnerability

@daviddias
Copy link
Collaborator Author

Streaming Responses Issue brave#10629

@@ -7,7 +7,7 @@
<!-- TODO: Don't allow img-src *, needed for favicons -->
<!-- TODO: Refactor away all unsafe-inline content -->
<!-- TODO: Replace suggestqueries.google.com and ac.duckduckgo.com and other search engines with a single config search engine -->
<meta http-equiv="Content-Security-Policy" content="default-src 'none'; form-action http://localhost:*; script-src 'self' http://localhost:*; connect-src 'self' https://s3.amazonaws.com/adblock-data/ https://s3.amazonaws.com/safe-browsing-data/ https://s3.amazonaws.com/tracking-protection-data/ https://s3.amazonaws.com/https-everywhere-data/ http://localhost:* ws://localhost:* https://suggestqueries.google.com https://ac.duckduckgo.com https://completion.amazon.com https://search.yahoo.com https://api.bing.com https://www.startpage.com https://infogalactic.com https://api.qwant.com https://ac.ecosia.org https://searx.me https://www.findx.com https://brave-download.global.ssl.fastly.net https://brave-laptop-updates.global.ssl.fastly.net https://brave-download.global.ssl.fastly.net https://laptop-updates-pre.brave.com https://brave-laptop-updates-pre.brave.com; style-src 'unsafe-inline'; font-src 'self' http://localhost:*; img-src 'self' * data: file: chrome-extension:; object-src 'self'; plugin-types application/browser-plugin">
<meta http-equiv="Content-Security-Policy" content="default-src 'none'; form-action http://localhost:*; script-src 'self' 'unsafe-eval' http://localhost:*; connect-src 'self' https://s3.amazonaws.com/adblock-data/ https://s3.amazonaws.com/safe-browsing-data/ https://s3.amazonaws.com/tracking-protection-data/ https://s3.amazonaws.com/https-everywhere-data/ http://localhost:* ws://localhost:* https://suggestqueries.google.com https://ac.duckduckgo.com https://completion.amazon.com https://search.yahoo.com https://api.bing.com https://www.startpage.com https://infogalactic.com https://api.qwant.com https://ac.ecosia.org https://searx.me https://www.findx.com https://brave-download.global.ssl.fastly.net https://brave-laptop-updates.global.ssl.fastly.net https://brave-download.global.ssl.fastly.net https://laptop-updates-pre.brave.com https://brave-laptop-updates-pre.brave.com; style-src 'unsafe-inline'; font-src 'self' http://localhost:*; img-src 'self' * data: file: chrome-extension:; object-src 'self'; plugin-types application/browser-plugin">
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the 'unsafe-eval'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is that needed? I think @diracdeltas will likely have an issue with it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately unsafe-eval removes most of the purpose of having a CSP - please avoid it by refactoring all inline script code into external files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it from here. It was not necessary

wss://nyc-1.bootstrap.libp2p.io
wss://nyc-2.bootstrap.libp2p.io
;
object-src 'self'; plugin-types application/browser-plugin"/>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to support a background page (and not just scripts) had to add manually all the endpoints a js-ipfs node connects on startup. This is a bit of annoyance as it will stop js-ipfs to connect dynamically to other public WebSockets endpoints unless the CSP can be relaxed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wss://*?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wow, it actually takes wildcards. I completely had discarded that option.

"page": "ipfs.html",
"persistent": true
},
"content_security_policy": "script-src 'self' 'unsafe-eval'"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the csp you have defined in the page above can be defined here instead

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the CSP is just for the IPFS extension, it should be defined here instead of in index-dev.html. i would still encourage refactoring such that unsafe-eval isn't needed.

Copy link
Collaborator Author

@daviddias daviddias Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bridiver when an extension has an index.html page, the extension loader ignores this CSP and applies the one in index.html. So yes, you are right, the index.html one is the only one necessary. My test yielded a false positive, it actually needs to have this in the manifest too.

@diracdeltas agreed that we should not have the need for unsafe-eval. We are working on it :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diasdavid I think the latest version of js-ipfs-api is ready to be tested without evals :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we just use js-ipfs. It seems that there still something. Need to test this more:

image

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is definitely still a protobuf thing

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we missed two modules

npm ls protocol-buffers
[email protected] /Users/dignifiedquire/opensource/ipfs/js-ipfs
├─┬ [email protected]
│ └─┬ [email protected]
│   └── [email protected]  deduped
└─┬ [email protected]
  └── [email protected]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 So it is evident. Nice, I was worried that it was npm false deduping. Thanks for catching that!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Woot 🚀 No more unsafe-eval!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🎉 🎉

@kyledrake
Copy link

Any updates here? Sounds like this is ready to go now that the eval issue is fixed. It would be nice to see this get merged into Brave and released.

@Jacalz
Copy link

Jacalz commented Nov 3, 2017

Yeah, I would love to see this too, have been waiting for a long time now 😅

@daviddias
Copy link
Collaborator Author

Good news everyone! @alanshaw and @olizilla are coming to help us polish the last quirks here! \o/

You can expect more activity starting this week this week. Especially when it comes to the UX for the user, cleaning out any last bugs and following up on outstanding tasks such as the needed streaming API.

@olizilla and @alanshaw welcome to this endeavor! Ping me through this thread or IRC if you have any questions! :)

@alanshaw
Copy link

Just a few of notes:

  • Webpack needs to be installed globally to build
  • wget needs to be installed for npm run ipfs to work
  • Document that npm run ipfs needs to be run!
  • [email protected] (wanted [email protected]) causes error Cannot find module 'ad-block/lib/regions'. I had to install [email protected] to successfully start (maybe this is fixed upstream?)

}
})

// TODO: bring back once brave supports registerURIHandlers

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diasdavid do you have any background info on why registerURIHandlers is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because an IPFS hash is, in reality, a URI and not a URL, we should be able to do ipfs:QmHash and not ipfs://QmHash.

This is still an ongoing proposal, read more at:

@ghost
Copy link

ghost commented Nov 14, 2017

we should be able to do ipfs:QmHash and not ipfs://QmHash.

Mh no, ipfs://QmHash is absolutely fine, and the URI variant is dweb:/ipfs/QmHash

@olizilla
Copy link

@lgierth i think @diasdavid is saying that in the context of this PR, the current impl can only do ipfs://QmHash style, and if we had the registerURIHandlers api available, we could do dweb:/ipfs/QmHash or ipfs:QmHash uris. (and that dweb is preferred)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants