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.

Sign up for GitHub

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
Merged

Catch TypeError and ValueError in verify functions #309

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
49286a6
Catch TypeError and ValueError in verify functions
algochoi Mar 25, 2022
df4f45b
Add unit tests around verify function
algochoi Mar 29, 2022
d48481f
Set bounds for dependency package versions
algochoi Mar 29, 2022
dbe2dab
Format files again using black
algochoi Mar 29, 2022
538c927
Revert bounds on package versions
algochoi Mar 29, 2022
de40e30
Fix boundaries on dependencies
algochoi Mar 29, 2022
c3b94a2
Fix boundaries again for package versions
algochoi Mar 29, 2022
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
4 changes: 2 additions & 2 deletions algosdk/future/transaction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2393,7 +2393,7 @@ def verify(self, message):
try:
verify_key.verify(message, subsig.signature)
michaeldiamant marked this conversation as resolved.
Show resolved Hide resolved
verified_count += 1
michaeldiamant marked this conversation as resolved.
Show resolved Hide resolved
except BadSignatureError:
except (BadSignatureError, ValueError, TypeError):
Copy link
Contributor

@tzaffi tzaffi Mar 28, 2022 •
edited
Loading

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:

py-algorand-sdk/test_unit.py

Line 2694 in f0c9430

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.

return False

if verified_count < self.threshold:
Expand Down Expand Up @@ -2562,7 +2562,7 @@ def verify(self, public_key):
try:
verify_key.verify(to_sign, base64.b64decode(self.sig))
return True
except (BadSignatureError, ValueError):
except (BadSignatureError, ValueError, TypeError):
return False

return self.msig.verify(to_sign)
Expand Down
2 changes: 1 addition & 1 deletion algosdk/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def verify_bytes(message, signature, public_key):
try:
verify_key.verify(prefixed_message, base64.b64decode(signature))
return True
except BadSignatureError:
except (BadSignatureError, ValueError, TypeError):
return False


Expand Down