fix: throw on invalid multiaddr to constructor (#934)

- dont assume that invalid multiaddrs are valid hosts
- if multiaddrOrHost starts with a /, assume its a multiaddr
- throw if that multiaddr is invalid.

Previously, we'd try and use invalid multiaddrs as the host,
which would lead to requests like http:///dnsaddr/foobar.com:5001/api

License: MIT
Signed-off-by: Oli Evans <oli@tableflip.io>

src/index.js

@@ -7,35 +7,35 @@ const getConfig = require('./utils/default-config')
 const sendRequest = require('./utils/send-request')
 
 function ipfsClient (hostOrMultiaddr, port, opts) {
-  const config = getConfig()
-
-  try {
-    const maddr = multiaddr(hostOrMultiaddr).nodeAddress()
-    config.host = maddr.address
-    config.port = maddr.port
-  } catch (e) {
-    if (typeof hostOrMultiaddr === 'string') {
-      config.host = hostOrMultiaddr
-      config.port = port && typeof port !== 'object' ? port : config.port
+  // convert all three params to objects that we can merge.
+  let hostAndPort = {}
+
+  if (!hostOrMultiaddr) {
+    // autoconfigure host and port in browser
+    if (typeof self !== 'undefined') {
+      const split = self.location.host.split(':')
+      hostAndPort.host = split[0]
+      hostAndPort.port = split[1]
+    }
+  } else if (multiaddr.isMultiaddr(hostOrMultiaddr)) {
+    hostAndPort = toHostAndPort(hostOrMultiaddr)
+  } else if (typeof hostOrMultiaddr === 'object') {
+    hostAndPort = hostOrMultiaddr
+  } else if (typeof hostOrMultiaddr === 'string') {
+    if (hostOrMultiaddr[0] === '/') {
+      // throws if multiaddr is malformed or can't be converted to a nodeAddress
+      hostAndPort = toHostAndPort(multiaddr(hostOrMultiaddr))
+    } else {
+      // hostOrMultiaddr is domain or ip address as a string
+      hostAndPort.host = hostOrMultiaddr
     }
   }
 
-  let lastIndex = arguments.length
-  while (!opts && lastIndex-- > 0) {
-    opts = arguments[lastIndex]
-    if (opts) break
-  }
-
-  Object.assign(config, opts)
-
-  // autoconfigure in browser
-  if (!config.host &&
-    typeof self !== 'undefined') {
-    const split = self.location.host.split(':')
-    config.host = split[0]
-    config.port = split[1]
+  if (port && typeof port !== 'object') {
+    port = { port: port }
   }
 
+  const config = Object.assign(getConfig(), hostAndPort, port, opts)
   const requestAPI = sendRequest(config)
   const cmds = loadCommands(requestAPI, config)
   cmds.send = requestAPI
@@ -44,4 +44,13 @@ function ipfsClient (hostOrMultiaddr, port, opts) {
   return cmds
 }
 
+// throws if multiaddr can't be converted to a nodeAddress
+function toHostAndPort (multiaddr) {
+  const nodeAddr = multiaddr.nodeAddress()
+  return {
+    host: nodeAddr.address,
+    port: nodeAddr.port
+  }
+}
+
 module.exports = ipfsClient

test/constructor.spec.js

@@ -1,6 +1,7 @@
 /* eslint-env mocha */
 'use strict'
 
+const multiaddr = require('multiaddr')
 const chai = require('chai')
 const dirtyChai = require('dirty-chai')
 const expect = chai.expect
@@ -9,18 +10,74 @@ chai.use(dirtyChai)
 const f = require('./utils/factory')
 const ipfsClient = require('../src/index.js')
 
-function clientWorks (client, done) {
-  client.id((err, id) => {
-    expect(err).to.not.exist()
+describe('ipfs-http-client constructor tests', () => {
+  describe('parameter permuations', () => {
+    it('none', () => {
+      const ipfs = ipfsClient()
+      if (typeof self !== 'undefined') {
+        const { hostname, port } = self.location
+        expectConfig(ipfs, { host: hostname, port })
+      } else {
+        expectConfig(ipfs, {})
+      }
+    })
 
-    expect(id).to.have.a.property('id')
-    expect(id).to.have.a.property('publicKey')
-    done()
+    it('opts', () => {
+      const host = 'wizard.world'
+      const port = '999'
+      const protocol = 'https'
+      const ipfs = ipfsClient({ host, port, protocol })
+      expectConfig(ipfs, { host, port, protocol })
+    })
+
+    it('mutliaddr dns4 string, opts', () => {
+      const host = 'foo.com'
+      const port = '1001'
+      const protocol = 'https'
+      const addr = `/dns4/${host}/tcp/${port}`
+      const ipfs = ipfsClient(addr, { protocol })
+      expectConfig(ipfs, { host, port, protocol })
+    })
+
+    it('mutliaddr ipv4 string', () => {
+      const host = '101.101.101.101'
+      const port = '1001'
+      const addr = `/ip4/${host}/tcp/${port}`
+      const ipfs = ipfsClient(addr)
+      expectConfig(ipfs, { host, port })
+    })
+
+    it('mutliaddr instance', () => {
+      const host = 'ace.place'
+      const port = '1001'
+      const addr = multiaddr(`/dns4/${host}/tcp/${port}`)
+      const ipfs = ipfsClient(addr)
+      expectConfig(ipfs, { host, port })
+    })
+
+    it('host and port strings', () => {
+      const host = '1.1.1.1'
+      const port = '9999'
+      const ipfs = ipfsClient(host, port)
+      expectConfig(ipfs, { host, port })
+    })
+
+    it('host, port and api path', () => {
+      const host = '10.100.100.255'
+      const port = '9999'
+      const apiPath = '/future/api/v1/'
+      const ipfs = ipfsClient(host, port, { 'api-path': apiPath })
+      expectConfig(ipfs, { host, port, apiPath })
+    })
+
+    it('throws on invalid mutliaddr', () => {
+      expect(() => ipfsClient('/dns4')).to.throw('invalid address')
+      expect(() => ipfsClient('/hello')).to.throw('no protocol with name')
+      expect(() => ipfsClient('/dns4/ipfs.io')).to.throw('multiaddr must have a valid format')
+    })
   })
-}
 
-describe('ipfs-http-client constructor tests', () => {
-  describe('parameter permuations', () => {
+  describe('integration', () => {
     let apiAddr
     let ipfsd
 
@@ -40,39 +97,26 @@ describe('ipfs-http-client constructor tests', () => {
       ipfsd.stop(done)
     })
 
-    it('opts', (done) => {
-      const splitted = apiAddr.split('/')
-      clientWorks(ipfsClient({
-        host: splitted[2],
-        port: splitted[4],
-        protocol: 'http'
-      }), done)
-    })
-
-    it('mutliaddr, opts', (done) => {
-      clientWorks(ipfsClient(apiAddr, { protocol: 'http' }), done)
-    })
-
-    it('host, port', (done) => {
-      const splitted = apiAddr.split('/')
-
-      clientWorks(ipfsClient(splitted[2], splitted[4]), done)
-    })
-
-    it('specify host, port and api path', (done) => {
-      const splitted = apiAddr.split('/')
-
-      clientWorks(ipfsClient({
-        host: splitted[2],
-        port: splitted[4],
-        'api-path': '/api/v0/'
-      }), done)
+    it('can connect to an ipfs http api', (done) => {
+      clientWorks(ipfsClient(apiAddr), done)
     })
+  })
+})
 
-    it('host, port, opts', (done) => {
-      const splitted = apiAddr.split('/')
+function clientWorks (client, done) {
+  client.id((err, id) => {
+    expect(err).to.not.exist()
 
-      clientWorks(ipfsClient(splitted[2], splitted[4], { protocol: 'http' }), done)
-    })
+    expect(id).to.have.a.property('id')
+    expect(id).to.have.a.property('publicKey')
+    done()
   })
-})
+}
+
+function expectConfig (ipfs, { host, port, protocol, apiPath }) {
+  const conf = ipfs.util.getEndpointConfig()
+  expect(conf.host).to.equal(host || 'localhost')
+  expect(conf.port).to.equal(port || '5001')
+  expect(conf.protocol).to.equal(protocol || 'http')
+  expect(conf['api-path']).to.equal(apiPath || '/api/v0/')
+}