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

Verifier.verify raises very general exceptions #51

Open
cjwatson opened this issue Apr 24, 2019 · 1 comment
Open

Verifier.verify raises very general exceptions #51

cjwatson opened this issue Apr 24, 2019 · 1 comment

Comments

@cjwatson
Copy link

Verifier.verify can raise at least:

  • MacaroonVerificationFailedException (various causes)
  • UnicodeDecodeError (if a first-party caveat isn't valid UTF-8)
  • TypeError (if a first-party caveat's signature fails binascii.unhexlify)
  • ValueError (if a third-party caveat's verification key isn't well-formed)
  • nacl.exceptions.CryptoError (if decrypting a third-party caveat's verification key fails)
  • perhaps other possibilities that I missed

The TypeError here is particularly egregious since it's an exception that one shouldn't normally need to catch but that could easily come up due to non-well-formed input data, but the others are a problem as well. This is worse than #50 because verification often requires supplying predicate functions and so the amount of code that can end up being enclosed in try: / except Exception: can be pretty large.

It should be guaranteed that Verifier.verify only raises MacaroonVerificationFailedException or its subclasses plus whatever might be raised by caller-supplied predicates; and the possible exceptions should be documented.

@ewjoachim
Copy link

ewjoachim commented Sep 22, 2020

Verifier.verify() can also raise whatever was raised in a callback added through satisfy_general(), and this is, I'd say, a good thing, because errors should never pass silently. But this could (and did) encourage people to raise in their satisfy_general instead of returning False, which is an API misuse.

Should arbitrary exceptions in callbacks be caught and turned into MacaroonVerificationFailedException or left as-is ?

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