Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Error: multihash too short. must be > 3 bytes. #2867

Closed
obo20 opened this issue Jul 2, 2018 · 10 comments
Closed

Error: multihash too short. must be > 3 bytes. #2867

obo20 opened this issue Jul 2, 2018 · 10 comments
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) pkg:http-client Issues related only to ipfs-http-client status/ready Ready to be worked

Comments

@obo20
Copy link
Contributor

obo20 commented Jul 2, 2018

If you submit a request to get a file using ipfs.files.cat, and you have a hash that is greater than 3 bytes, you still get the error message: "Error: multihash too short. must be > 3 bytes."

For example:

ipfs.files.cat(/ipfs/QmUzdHisA7ouM8DM5rVAKursVK7t7tKeqDYGvmaCdJw5jEeesdlfjsldkfjsldkfjsdlkfjskldfj will return that error message

@obo20
Copy link
Contributor Author

obo20 commented Jul 2, 2018

From looking at the package itself, the only code I could find with this error message was as follows:

if (buf.length < 3) { throw new Error('multihash too short. must be > 3 bytes.') }

From the looks of it, it should be working, so I'm not sure why this is erroring out like it is

@alanshaw
Copy link
Member

alanshaw commented Jul 3, 2018

@obo20 TLDR; the error message is deceptive but it's failing because QmUzdHisA7ouM8DM5rVAKursVK7t7tKeqDYGvmaCdJw5jEeesdlfjsldkfjsldkfjsdlkfjskldfj is not a valid CID.


ipfs.files.cat calls cleanCID on /ipfs/QmUzdHisA7ouM8DM5rVAKursVK7t7tKeqDYGvmaCdJw5jEeesdlfjsldkfjsldkfjsdlkfjskldfj here:

https://github.com/ipfs/js-ipfs-api/blob/a8f9a43064930c816b0e73dbc1674a313cf1baca/src/files/cat.js#L16

...and falls through to:

https://github.com/ipfs/js-ipfs-api/blob/a8f9a43064930c816b0e73dbc1674a313cf1baca/src/utils/clean-cid.js#L16

cid.split('/')[0] is the empty string, so the call to CID.validateCID fails and throws the error you're seeing.

Back in ipfs.files.cat your input is passed to ipfsPath() which returns false because your CID is invalid.

https://github.com/ipfs/js-ipfs-api/blob/a8f9a43064930c816b0e73dbc1674a313cf1baca/src/files/cat.js#L18

Finally, the error we got from CID.validateCID is passed back to your callback.

@0x-r4bbit
Copy link
Contributor

I guess this should be fixed with a more descriptive error message (that handles the case of invalid CIDs)?

@alanshaw
Copy link
Member

alanshaw commented Jul 4, 2018

I would like to use something like explain-error to give better context to errors throughout js-ipfs, js-ipfs-api and it's dependencies.

For the time being I think in this case the best solution would be to fix files/cat.js to only call cleanCID if the input is not determined to be a valid IPFS path as cleanCID only expects input as a buffer, CID instance, string representation of a CID or finally a string in the form QmXEmhrMpbVvTh61FNAxP9nU7ygVtyvZA8HZDUaqQCAb66/a.txt. We can use ipfsPath from is-ipfs for this.

That way we'll get an error message that makes much more sense in the context of the input.

@0x-r4bbit
Copy link
Contributor

0x-r4bbit commented Jul 4, 2018

@alanshaw I'm looking into this and was wondering what you prefer:

For the time being I think in this case the best solution would be to fix files/cat.js to only call cleanCID if the input is not determined to be a valid IPFS path

This means, we basically wanna check for Buffer, isCID, cidString and ipfsPath beforehand. However, with how it is now, that would mean we'd only conditionally clean the hash e.g.:

    try {
      if (checkValidInput(hash)) {
        hash = cleanCID(hash)
      }
    } catch (err) {
      if (!v.ipfsPath(hash)) {
        return callback(err)
      }
    }

with checkValidInput() looking something like

function checkValidInput(input) {
  return (typeof input === 'string' && (v.cid(input) || v.ipfsPath(input))) || (Buffer.isBuffer(input) || CID.isCID(input))
}

In other words, if hash doesn't pass that check, whatever hash is at this point will be send to IPFS (and probably result in an error as it won't be a valid hash).

Is ^ that error the one we wanna propagate here?

Another option would obviously to make it error right at the point we know input isn't valid. Something like:

try {
  if (checkValidInput(hash)) {
    hash = cleanCID(hash)
  } else {
    throw new Error('something something, invalid input')
  }
} catch (err) {
  if (!v.ipfsPath(hash)) {
    return callback(err)
  }
}

In fact, we can get rid off the try catch block all together then:

 if (checkValidInput(hash)) {
    hash = cleanCID(hash)
 } else {
    return callback(new Error('something something, invalid input'))
 }

However, at this point we don't know exactly why hash is invalid, unless we check every type again explicitly, which is why I'm assuming you prolly want the (probably) invalid hash to be send to IPFS so it will propagate a more descriptive error 👇

That way we'll get an error message that makes much more sense in the context of the input.

0x-r4bbit referenced this issue in 0x-r4bbit/js-ipfs-api Jul 4, 2018
…is valid

Fixes #799

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
@0x-r4bbit
Copy link
Contributor

0x-r4bbit commented Jul 4, 2018

Here's a proposal. Happy to change it to your needs: ipfs-inactive/js-ipfs-http-client#804

0x-r4bbit referenced this issue in 0x-r4bbit/js-ipfs-api Jul 4, 2018
…is valid

Fixes #799

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
@0x-r4bbit
Copy link
Contributor

@alanshaw you're prolly super busy with things, but maybe you'll find 2 minutes to give my last comment here a quick thought? :)

I'll then update the PR accordingly and serve it on a golden plate (and stop annoying you with it).

@alanshaw
Copy link
Member

Apologies I completely forgot about this one - I have commented on your PR now, let me know what you think!

@0x-r4bbit
Copy link
Contributor

No worries. We've all got stuff to do :)

I've read your comment and am working on updating the PR. Trying to come up with a test as well, so we can verify the expected behavior. Stay tuned.

0x-r4bbit referenced this issue in 0x-r4bbit/interface-ipfs-core Jul 18, 2018
To verify changes discussed in https://github.com/ipfs/js-ipfs-api/issues/799#issuecomment-402086401, this commit
adds a test that checks `files.cat()` throws a meaningful error when a key or ipfs path with
unsupported encoding is given.

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
0x-r4bbit referenced this issue in 0x-r4bbit/js-ipfs-api Jul 18, 2018
… `cat()`

A test to verify this is available at ipfs-inactive/interface-js-ipfs-core#330 and needs to be merged first.

Fixes #799

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
0x-r4bbit referenced this issue in 0x-r4bbit/js-ipfs-api Jul 18, 2018
… `cat()`

A test to verify this is available at ipfs-inactive/interface-js-ipfs-core#330 and needs to be merged first.

Fixes #799

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
0x-r4bbit referenced this issue in 0x-r4bbit/interface-ipfs-core Jul 18, 2018
To verify changes discussed in https://github.com/ipfs/js-ipfs-api/issues/799#issuecomment-402086401, this commit
adds a test that checks `files.cat()` throws a meaningful error when a key or ipfs path with
unsupported encoding is given.

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
0x-r4bbit referenced this issue in 0x-r4bbit/js-ipfs-api Jul 18, 2018
A test to verify this is available at ipfs-inactive/interface-js-ipfs-core#330 and needs to be merged first.

Fixes #799

License: MIT
Signed-off-by: Pascal Precht pascal.precht@gmail.com
@achingbrain achingbrain transferred this issue from ipfs-inactive/js-ipfs-http-client Mar 9, 2020
@achingbrain achingbrain added kind/bug A bug in existing code (including security flaws) exp/novice Someone with a little familiarity can pick up pkg:http-client Issues related only to ipfs-http-client help wanted Seeking public contribution on this issue labels Mar 9, 2020
@jacobheun jacobheun added the status/ready Ready to be worked label Jun 25, 2020
@achingbrain
Copy link
Member

Calling

await all(ipfs.cat('/ipfs/QmUzdHisA7ouM8DM5rVAKursVK7t7tKeqDYGvmaCdJw5jEeesdlfjsldkfjsldkfjsdlkfjskldfj'))

now fails with

Error: invalid character 'l' in 'QmUzdHisA7ouM8DM5rVAKursVK7t7tKeqDYGvmaCdJw5jEeesdlfjsldkfjsldkfjsdlkfjskldfj'

so the error message is at least less cryptic than it used to be.

If a more descriptive error message is desired, a PR should be opened against multiformats/js-cid as that's where the string is turned into a CID.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) pkg:http-client Issues related only to ipfs-http-client status/ready Ready to be worked
Projects
None yet
Development

No branches or pull requests

5 participants