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

Catch TypeError and ValueError in verify functions #309

Merged
merged 7 commits into from
Mar 30, 2022

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Mar 25, 2022

pynacl's VerifyKey() can now return a TypeError or ValueError (related PR). Our SDK should reflect this change by catching those errors as well, or users may encounter an unexpected exception.

Picked some major release versions and set that as boundaries. Hopefully users are using venv or conda as well...

Also added the black formatter updates so CI can run normally.

I didn't change the v1 transaction methods, but the problem is there as well (I hope no one uses them because we haven't updated them for a very long time).

@algochoi algochoi marked this pull request as ready for review March 28, 2022 13:51
@algochoi algochoi requested review from jasonpaulos and tzaffi March 28, 2022 21:22
@@ -2393,7 +2393,7 @@ def verify(self, message):
try:
verify_key.verify(message, subsig.signature)
verified_count += 1
except BadSignatureError:
except (BadSignatureError, ValueError, TypeError):
Copy link
Contributor

@tzaffi tzaffi Mar 28, 2022

Choose a reason for hiding this comment

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

I'm guessing that most users simply want to know that verify() has failed, deal with it, and won't really care why this has occurred (at least not in their runtime handling). For such users, swallowing these two exceptions makes life easier.

But could there be something about these different errors that could allow users to have more nuanced recovery strategies? I kind of doubt it, but it's worth knowing before we just add the blanket conversion from (BadSignatureError, ValueError, TypeError) to False.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more about it, ValueError and TypeError sounds like they could be generated by bad user input/usage. If that is the case, then we really wouldn't want to swallow these errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - I also thought about this and couldn't reach a conclusive decision. Our current unit tests do seem to test the ValueError here by testing the wrong number of bytes:

def test_verify(self):

I'm not exactly sure how we want verify to behave in these cases so I wanted other people's inputs as well. If we choose to include this change, I can also add some test cases to check the two new error types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Summarizing a verbal with @algochoi, @tzaffi, and @jasonpaulos:

  • We agreed the expedient path is to keep the PR's behavior and add unit tests validating behavior.
  • We left with a loose agreement that revisiting the return type out-of-band is worthwhile. For example, moving from boolean to a richer data type will simplify disambiguating failure cases.

As of c3b94a2, I think the PR matches what we verbally agreed to. Since I didn't open the thread, I'll leave as is without resolving.

Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@algochoi Thanks for the effort to clean up API inconsistencies here. ☕

@algochoi algochoi merged commit aba9f4c into develop Mar 30, 2022
@algochoi algochoi deleted the algochoi/verify-type-catching branch March 30, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants