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

[request] Don't throw RuntimeException on invalid signatures #79

Open
peret opened this issue Sep 7, 2017 · 4 comments
Open

[request] Don't throw RuntimeException on invalid signatures #79

peret opened this issue Sep 7, 2017 · 4 comments

Comments

@peret
Copy link

peret commented Sep 7, 2017

First of all, thanks for providing this package :) I'm using libsodium to verify a signature in my Android app and was really surprised just now when I realized that VerifyKey.verify() (more precisely: Util.isValid()) throws a RuntimeException. At least for me, this was really unexpected and I think it's also not the most elegant solution. Basically, if I want to handle this case gracefully (instead of crashing my app), I have to catch all RuntimeExceptions and compare the message with a constant string that could potentially change in future versions. A few ideas on alternative ways to handle this (in my personal order of preference):

  1. Just let it return false when the signature isn't valid and let developers handle what should happen in that case. After all, it is a boolean method, so just by looking at the interface I would expect it to return true if the signature is valid and false otherwise. Imho, there's no need to throw an exception at all.
  2. Don't use RuntimeException but create a new (checked!) exception class, so that at least I will be prompted to handle that case while writing the code.
  3. At the very minimum, make it a new subclass of RuntimeException, so I don't need to catch all the RuntimeExceptions, but can easily catch "InvalidSignatureException" or similar.

Is there any chance you would be making one of these changes in a future version? Or is there a particular reason you went for the RuntimeException?

@joshjdevl
Copy link
Owner

The code in question Util.isValid is code remnant brought in from an earlier fork.

In this case bool return is better. Would you be willing to contribute a patch?

Thanks.

@peret
Copy link
Author

peret commented Sep 8, 2017

I'm happy to provide a patch, I'm just not sure when I will get around to doing that. Also, this change might break existing code in a subtle way, i.e. if people are relying on the fact that an invalid signature will throw an exception and don't check the return value at all. Need to make sure to communicate that properly somehow...

@joshjdevl
Copy link
Owner

Good points! Let's deprecate the existing method and add a new method?

@joshjdevl
Copy link
Owner

Marked as deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants