Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace regexes with proper IPFS path/multihash validation #47

Closed
2 tasks done
lidel opened this issue Jan 17, 2016 · 8 comments
Closed
2 tasks done

Replace regexes with proper IPFS path/multihash validation #47

lidel opened this issue Jan 17, 2016 · 8 comments
Assignees
Labels
kind/discussion Topical discussion; usually not changes to codebase kind/enhancement A net-new feature or improvement to an existing feature topic/security Work related to security

Comments

@lidel
Copy link
Member

lidel commented Jan 17, 2016

  • Find a way of using NPM packages within JPM-based addon
  • Replace regexes with proper IPFS path/multihash validation via common library

As suggested in ipfs/notes#92 (comment) and #43 we should stop using regexes for detecting IPFS paths and come up with something more future-proof that involves js-multiaddr and js-multihash.

First step would be to create internal PoC in lib/ipfs-path-validation.js with isValidIPFSPath(path) method and then refactor addon to use it instead of gateways.IPFS_RESOURCE and protocols.IPFS_PATH regexes.

@lidel lidel added help wanted Seeking public contribution on this issue kind/discussion Topical discussion; usually not changes to codebase labels Jan 17, 2016
@fbaiodias
Copy link
Member

I've been updating on ipfs-chrome-station and ended up publishing is-ipfs to check if the hashes and urls are valid or not using js-multihash.
Let me know if you also want to use it and if there is anything I can add there to make it more helpful :)

@lidel
Copy link
Member Author

lidel commented Feb 8, 2016

A quick brain dump on this.

Reusing is-ipfs should be trivial, but in current Firefox ecosystem it is not. 😿

For some background, a quote from this discussion:

What mozilla did was creating a different CommonJS (and not up to date with the latest spec) environment than node. So no, it's not node.js and it doesn't claim to be, it just happens to use the same base API. It doesn't do any hard checks, yes, but it doesn't claim to be compatible with the "engine" "nodejs", which all packages on npm are assumed to be compatible with by default (which is not true, afaik).
It means that most npm packages aren't usable out of the box.

In short, even if a library does not use Node-specific APIs, module resolution is broken in subtle ways:

const isIPFS = require('is-ipfs')

ends with:

Message: Module `is-ipfs` is not found at resource://gre/modules/commonjs/is-ipfs.js

I did some experiments and relative paths work fine, for example:

const isIPFS = require('../node_modules/is-ipfs/index.js')

produces a new error this time for its dependency:

Message: Module `bs58` is not found at resource://gre/modules/commonjs/bs58.js

Ok, it should be possible to go over every dependency and fix require paths via sed/awk, but overriding files in node_modules feels dirty.
WebExtensions (#20) won't be ready any time soon, so it needs to be solved somehow. I'll look into this in spare time.

@Kubuxu
Copy link
Member

Kubuxu commented Feb 9, 2016

@lidel
Copy link
Member Author

lidel commented Feb 9, 2016

It "just works" for a limited number of modules that are tagged with jetpack.
Everything else is trial & error and requires manual workarounds I described (if you are lucky).

This mess is one of reasons why WebExtensions are developed as an alternative to current addon ecosystem.

@lidel
Copy link
Member Author

lidel commented Feb 9, 2016

Ok, deep into a rabbit hole: CommonJS implementation in Firefox is unable to load multihashes via relative paths
(is-ipfs, bs58 and base-x seem to load fine, or at least did not produce any error yet)

const isIPFS = require('../node_modules/is-ipfs')
const b58 = require('../node_modules/bs58/index.js')
const baseX = require('../node_modules/base-x/index.js')
const multihashes = require('../node_modules/multihashes/dist/multihashes.js')

multihashes package seems to be invisible to CommonJS from Firefox
(perhaps due to the lack of index.js?):

Message: Module `multihashes` is not found at resource://gre/modules/commonjs/multihashes.js

At this point I stopped. It does not seem to work for anything bigger than 'hello world' and there is a multitude of open tickets for NPM support:

It's just broken and I don't think it will be fixed for old SDK – all development goes into WebExtensions these days. It will probably be solved there.


I've read that alternative way to run NPM packages in Firefox is via Browserify shims.
I gave it a try and it works fine:

browserify -r is-ipfs -s module.exports > lib/is-ipfs.js
const isIPFS = require('./is-ipfs.js')
isIPFS.multihash('QmYjtig7VJQ6XsnUjqqJvj7QaMcCAwtrgNdahSiFofrE7o')
console.log: ipfs-firefox-addon: is-ipfs-test:true

There are two downsides:

  • All NPM dependencies are concatenated and wrapped in one big file (we probably can slim this down by removing shims for things that are provided by Firefox):
-rw-r--r-- 1 lidel lidel 53K Feb  9 19:07 lib/is-ipfs.js
  • When used extensively, addons have hard time going trough Review Process (example – this addon got rejected for 43K file)

I think I'll give it a try, following the feedback from linked example (avoiding redundant shims etc).

@lidel
Copy link
Member Author

lidel commented Feb 9, 2016

Ok, so Firefox SDK provides some Node-compatible libraries, but under different names.
Luckily JPM supports name overrides via package.json:

+"jetpack": {
+    "overrides": {
+        "buffer": "sdk/io/buffer"
+    }
+  },

Having that, I was able to hot-swap the implementation and remove all shims:

browserify -r is-ipfs -im --no-builtins -s module.exports > lib/npm/is-ipfs.js

Produced:

-rw-r--r-- 1 lidel lidel 7.7K Feb  9 22:25 lib/npm/is-ipfs.js

This contains only multihash-related code and should be easier to review 🎈

@lidel lidel closed this as completed in 7e98f04 Feb 9, 2016
@lidel lidel self-assigned this Feb 9, 2016
@lidel lidel added kind/enhancement A net-new feature or improvement to an existing feature topic/security Work related to security and removed help wanted Seeking public contribution on this issue labels Feb 9, 2016
@lidel lidel changed the title Replace regexes with proper IPFS path validation Replace regexes with proper IPFS path/multihash validation Feb 9, 2016
@fbaiodias
Copy link
Member

Oh, sorry I wasn't following this thread, but glad that you got it working :)

Do you think we should publish the browserified version too on is-ipfs?

@lidel
Copy link
Member Author

lidel commented Feb 10, 2016

No, npm package is enough. I have it as a dependency in package.json and automatically convert it to the Firefox-compatible form during the build step.

I'll leave it this way as a workaround until Firefox gets proper npm package support in new API called WebExtensions (#20).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/discussion Topical discussion; usually not changes to codebase kind/enhancement A net-new feature or improvement to an existing feature topic/security Work related to security
Projects
None yet
Development

No branches or pull requests

3 participants