feat: return errors in exported functions instead of panicking #37
Conversation
Exported funcs should ideally return errors instead of panicing
Underneath it all, generateMySecret was panicking, so returning an error from there requires us to bubble the error all the way up to these 2 exported functions. This is good, it means clients consuming this library can handle the errors gracefully and at their convinience.
CamiloGarciaLaRotta
changed the title
| s.generateMySecret() | ||
| _, err := s.generateMySecret() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("generating secret: %w", err) | ||
| } |
There was a problem hiding this comment.
👋🏼 @jpgoldberg I'm curious why we need to call s.generateMySecret() before calling s.makeB() or s.makeA(). Those methods will themselves call s.generateMySecret() as part of their execution:
https://github.com/1password/srp/blob/d2080afd070083afda9374fdc3917e82727af74b/internal.go#L115-L121
Unless I'm missing something, I think we can remove this call to s.generateMySecret(). CI did not fail without it
|
You are absolutely right about this. I do not recall why I choose to panic on those in the first place. It may have been that I was trying to maintain some compatibility with how these were being called by other code. This would be a "breaking change", so it needs to be tested carefully in the code that makes use of this package. |
|
Just to point it out:
The "don't panic" idiom says (emphasis added):
Events like failure to fetch random bytes from the system source isn't really a normal failure, and indicates the system is in a pretty bad state. For exceptional circumstances like that (where there is truly nothing we can do to proceed), I think panic is quite appropriate. Equally, sometimes we know that something is going to succeed (e.g. the hash writers we use don't return errors under any circumstance, but it's part of the signature to fulfil the writer API contract. There isn't really much value in making callers deal with a potential error that can't actually occur, so panic is also probably okay here because we are asserting we know that a write to the hash writer can't fail (not the case for all writers, but for the hash writer it is). Agreed that for simpler error handling, we should not be panicing though. |
3 participants
👋🏽 hi team
What
This PR proposes braking changes to some of the exported functions⚠️
Namely,
NewSRPClient(),NewSRPServer(), andKDFRFC5054().Concretely, it changes the signature of these methods so that they return an error (a common Go idiom), rather than panicking. This allows consumers of the library to handle the errors gracefully and at their convinience.
What approach did you choose and why?
I identified all the places where we panic. I refactored it to emit an error instead. I then worked my way upwards so that the error gets bubbled up all the way to the exported functions.
What should reviewers focus on?
I believe there is much to be gained from this breaking change:
So the questions I think we should focus on are (if the changes are approved):
If something goes wrong, what are the mitigation strategies?
Consumers of this library can use the previous tag or a commit SHA prior to this PR.
Note that I am not an official collaborator on this repo, so my commits won't trigger CI builds. If you want to see the CI for these commits, see my fork: https://github.com/CamiloGarciaLaRotta/srp/pull/2/commits
Performance Impact
None
Observability
N/A