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

fix: dht validate if receiving stream #890

Merged
merged 6 commits into from
Dec 11, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"debug": "^4.1.0",
"detect-node": "^2.0.4",
"end-of-stream": "^1.4.1",
"err-code": "^1.1.2",
"flatmap": "0.0.3",
"glob": "^7.1.3",
"ipfs-block": "~0.8.0",
Expand Down Expand Up @@ -85,7 +86,7 @@
"eslint-plugin-react": "^7.11.1",
"go-ipfs-dep": "~0.4.18",
"gulp": "^3.9.1",
"interface-ipfs-core": "~0.90.0",
"interface-ipfs-core": "ipfs/interface-ipfs-core#fix/update-dht-responses",
"ipfsd-ctl": "~0.40.0",
"nock": "^10.0.2",
"pull-stream": "^3.6.9",
Expand Down
56 changes: 53 additions & 3 deletions src/dht/findpeer.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
'use strict'

const promisify = require('promisify-es6')
const streamToValue = require('../utils/stream-to-value')
const streamToValueWithTransformer = require('../utils/stream-to-value-with-transformer')

const errcode = require('err-code')

module.exports = (send) => {
return promisify((peerId, opts, callback) => {
Expand All @@ -17,10 +19,58 @@ module.exports = (send) => {
opts = {}
}

send.andTransform({
const handleResult = (res, callback) => {
// Inconsistent return values in the browser
if (Array.isArray(res)) {
res = res[0]
}

// Type 2 keys (inconsistencies between go core and js core)
if (res.Type !== 2 && res.type !== 2) {
const errMsg = `key was not found (type 2)`

return callback(errcode(new Error(errMsg), 'ERR_KEY_TYPE_2_NOT_FOUND'))
}

// inconsistencies between go core and js core
let id
let addrs

if (res.Responses) {
id = res.Responses[0].ID
addrs = res.Responses[0].Addrs
} else {
id = res.responses[0].id
addrs = res.responses[0].addrs
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies if I wasn't clear, interface-ipfs-core should return lowered property names but the HTTP API should be compatible with go-ipfs.

Here I expect us to be converting "Uppered" property names to lowered form. In js-ipfs the HTTP API should take the lowered property names from core and "Upper" them to be compatible. I know this is a PITA but this is consistent with all our APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change it! Thanks for clarifying.


// inconsistencies js / go - go does not add `/ipfs/{id}` to the address
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to fix this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, go-ipfs response contains only address. As a consequence, it will fail the following test:

ipfs/interface-ipfs-core/js/src/dht/findpeer.js#L48

I can change the test in interface-ipfs-core and remove this if you feel that it is less hacky.

Copy link
Contributor

@alanshaw alanshaw Dec 10, 2018

Choose a reason for hiding this comment

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

What I'm concerned about is that we're making assumptions about the format of the address. What does go-ipfs return when it's a circuit address? Would that cause a false positive?

Ideally both implementations should return the same format! Do you know the reason why go-ipfs returns the addr without the peer ID? Perhaps for a smaller payload? It would be pretty easy for js-ipfs to decapsulate the peer id from the addrs wouldn't it?

addrs = addrs.map((addr) => {
if (addr.split('/ipfs/') > -1) {
return addr
} else {
return `${addr}/ipfs/${id}`
}
})

callback(null, {
responses: [{
id,
addrs
}]
})
}

send({
path: 'dht/findpeer',
args: peerId,
qs: opts
}, streamToValue, callback)
}, (err, result) => {
if (err) {
return callback(err)
}

streamToValueWithTransformer(result, handleResult, callback)
})
})
}
39 changes: 36 additions & 3 deletions src/dht/findprovs.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
'use strict'

const promisify = require('promisify-es6')
const streamToValue = require('../utils/stream-to-value')
const streamToValueWithTransformer = require('../utils/stream-to-value-with-transformer')

const errcode = require('err-code')

module.exports = (send) => {
return promisify((cid, opts, callback) => {
Expand All @@ -17,10 +19,41 @@ module.exports = (send) => {
opts = {}
}

send.andTransform({
const handleResult = (res, callback) => {
// Inconsistent return values in the browser vs node
if (Array.isArray(res)) {
res = res[0]
}

// Type 4 keys (inconsistencies between go core and js core)
if (res.Type !== 4 && res.type !== 4) {
const errMsg = `key was not found (type 4)`

return callback(errcode(new Error(errMsg), 'ERR_KEY_TYPE_4_NOT_FOUND'))
}

// inconsistencies between go core and js core
const recResponses = res.Responses || res.responses

// providers array (handling inconsistencies)
const responses = recResponses.map((r) => ({
id: r.ID || r.id,
addrs: r.Addrs || r.addrs
}))

callback(null, { responses })
}

send({
path: 'dht/findprovs',
args: cid,
qs: opts
}, streamToValue, callback)
}, (err, result) => {
if (err) {
return callback(err)
}

streamToValueWithTransformer(result, handleResult, callback)
})
})
}
14 changes: 12 additions & 2 deletions src/dht/query.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,20 @@ module.exports = (send) => {
opts = {}
}

send.andTransform({
send({
path: 'dht/query',
args: peerId,
qs: opts
}, streamToValue, callback)
}, (err, result) => {
if (err) {
return callback(err)
}

if (typeof result.pipe === 'function') {
streamToValue(result, callback)
} else {
callback(null, result)
}
})
})
}
18 changes: 18 additions & 0 deletions src/utils/stream-to-value-with-transformer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict'

const streamToValue = require('./stream-to-value')

function streamToValueWithTransformer (response, transformer, callback) {
if (typeof response.pipe === 'function') {
streamToValue(response, (err, res) => {
if (err) {
return callback(err)
}
transformer(res, callback)
})
} else {
transformer(response, callback)
}
}

module.exports = streamToValueWithTransformer