fix: Network errors during pwnedpasswords.com check causing recursive loop #589
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The old code no error when ignoreNetworkErrors was set to true, but did not set a hash result which caused an infinite loop. This fixes #316.
jakhog
changed the title
aeneasr
reviewed
Jul 20, 2020
| if s.ignoreNetworkErrors { | ||
| return nil | ||
| } | ||
| return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to check if password has been breached before: %s", err)) | ||
| return s.ignoreNetworkErrors, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to check if password has been breached before: %s", err)) |
There was a problem hiding this comment.
Nice! Typically you would use the error as a value in "idiomatic" Go to avoid the mix of two return types. While the correctness of the code is obvious, it's just more "Go-style" to write it like this:
var ErrNetworkFailure = errors.new("unexpected network error")
// func () fetch() error {
// ...
if err != nil {
s.log().WithError(err).Errorf("Unable to check if password has been breached before")
return ErrNetworkFailure
}
// ...
Comment on lines
169
to
172
| if shouldIgnore, err := s.fetch(hpw); err != nil { | ||
| if shouldIgnore { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
And then you do this:
Suggested change
| if shouldIgnore, err := s.fetch(hpw); err != nil { | |
| if shouldIgnore { | |
| return nil | |
| } | |
| if err := s.fetch(hpw); errors.Is(err, ErrNetworkFailure) { | |
| return nil | |
| } else if err != nil { | |
| return err | |
| } |
|
Thanks for the feedback! I was away on vacation so haven't seen this until now, but seems like you already fixed it so thank you 馃槃 |
|
No problem, I hope you had a good vacation and were able to regenerate :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issue
On network errors, the password validator ends up in a recursion loop until either the fault disappears - or more likely the stack overflows. Related to #316, @aeneasr .
Proposed changes
Change the validator code so that when
ignoreNetworkErrors = true, a failure during the pwnedpasswords.com check returns no error, but the validation of the same password later would retry the online check.Added test cases that previously crashed or timed out the test suite (because of the loop), but they pass now. I also took the liberty to add a few more tests for the online password leak check.
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further comments
I implemented a
FakeHTTPClientto use for these tests and feel like it should really live somewhere else to potentially be reused, but I couldn't find a good home for it. The internal/testhelpers package doesn't work as it causes an import cycle, any suggestions?This is also the first Go test I've written, so happy with any feedback if something is not perfect 馃檪