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

fix: improper input validation #1506

Merged
merged 7 commits into from
Aug 17, 2018
Merged

fix: improper input validation #1506

merged 7 commits into from
Aug 17, 2018

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Aug 12, 2018

This PR adds a variety of fixes and tests for invalid inputs passed to the IPFS node that would crash the daemon.

The module err-code is used to add code's to error messages that the client can rely on and we can use to assert on in our tests.

refs #1406
also sent as part of this work libp2p/js-libp2p#232
related to https://github.com/ipfs/interface-ipfs-core/issues/300

@ghost ghost assigned alanshaw Aug 12, 2018
@ghost ghost added the status/in-progress In progress label Aug 12, 2018
@alanshaw alanshaw changed the title [WIP] fix: callback with error for invalid CIDs [WIP] fix: inproper input validation Aug 12, 2018
@alanshaw alanshaw changed the title [WIP] fix: inproper input validation [WIP] fix: improper input validation Aug 12, 2018
@alanshaw alanshaw changed the title [WIP] fix: improper input validation fix: improper input validation Aug 12, 2018
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Aside from the one multihash error hiding I found, I think this looks good.

Ideally we'd have the error codes in a single place to avoid typos. I know there were discussions in #1406 about centralizing those in their own repo. Perhaps we could add them to a single file in here for the interim?

This is a great step in the right direction for better error handling! 🎉

} catch (err) {
return callback(err)
return setImmediate(() => callback(errCode(err, 'ERR_INVALID_CID')))
Copy link
Contributor

Choose a reason for hiding this comment

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

If normalizeMultihash throws the error users are going to get confused since this will always say the cid is the problem. We should check if it's an unsupported hash error first.

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
@alanshaw
Copy link
Member Author

@jacobheun can I get a 👍 now? (will also wait for a jenkins 💚 too)

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Pending CI, 👍 looks good

@alanshaw
Copy link
Member Author

The module err-code is used to add code's to error messages that the client can rely on and we can use to assert on in our tests.

@diasdavid wanted to double check this was ok by you?

@alanshaw alanshaw merged commit 91a482b into master Aug 17, 2018
@alanshaw alanshaw deleted the fix/invalid-inputs branch August 17, 2018 07:47
@ghost ghost removed the status/in-progress In progress label Aug 17, 2018
@daviddias
Copy link
Member

@alanshaw what's the pros/cons of err-code that you see?

@alanshaw
Copy link
Member Author

alanshaw commented Aug 22, 2018

Pros:

  • Error messages aren't part of the public API
    • Allows us to fix typos, reword or just change error messages without breaking user code
    • Gives users something more stable to allow them to detect and deal with specific errors
  • Augmented Error object with a code property instead of Error sub-classes:
    • Doesn't require user code to import Error classes (just easier)
    • Can be easily serialized/deserialized for transport over HTTP for example
  • Enhances test quality
    • People know they shouldn't rely on error messages in tests but they don't know what they should do instead. We end up with tests of negative utility that just assert an error exists and no more. This can easily lead to false positives
  • Reduces the boilerplate to assigning a code property to an error instance
  • It's fairly standard practice to assign a code to an error and is even seen in Node.js also so it should be familiar to users and IPFS/libp2p developers

Note that when I mention "user" above I'm referring to developers using js-ipfs.

Cons:

  • We have to remember to add an error code to errors we create
  • It adds 22 lines of code to our bundle size :P

Additionally, I'm aware of the following difficulties in introducing this:

  • Not all errors will have an error code

    ...and they never will. We can't make dependencies add error codes and we can't add error codes to developer errors. With buy in from the JS teams we can perhaps add them to many of the projects we have control of. We'll not be able to add them in one shot, but if we can slowly add them then we'll gradually improve error handling in the code base and user land

  • We have no agreement with the go-ipfs team on what the codes should be

    A conversation to be had. I'm not sure it'll be possible/appropriate to talk about this on a per-error basis - we might have to find consensus on the code when we find errors created for similar reasons.

    It's ok to change the code...in either code base! It's the same as if we changed the message right now, but doing so should automatically cause the developer to consider it a change that might break user code and so we'll be able to make better choices about the version numbers we pick for our releases.

    Right now I bet that if we changed an error message we wouldn't think twice about it breaking user code regardless of the reality!

  • The HTTP API and the CLI does not communicate them to users

    I would like to send a PR at some point to make this happen.

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.

3 participants