Every now and then an exception is logged that reads Unable to find a valid code. that originates from the EmailMultifactorMethod. Further investigation reveals that is caused by two parallel requests being issued by a client where the first request appears to be cancelled by the client (HAProxy reports this as CD--).
Imagine two identical requests A and B arriving at the server:
- Request A is in
validate() and the form field successfully validates the code.
- Request B is in
validate() and the form field successfully validates the code.
- Request A enters
save(), looks the setup and fetches/invalidates the used code.
- Request B enters
save() after request A and once the lock is released, tries to fetch the code, finds none and panics.
Because request A was aborted by the client, the user only sees the result of request B: A hard exception instead of a meaningful error handling.
This is a design flaw in the processAuthenticationForm() implementation because it performs the validation again that already took place in validate(). However, due to the above race condition, this creates a TOCTOU situation where the code vanishes before the processAuthenticationForm() is fully executed.
Solution: processAuthenticationForm should not perform any validation at all because at this point the validation has already been passed. It should gracefully handle the code being absent because we know that the removal properly works. There was a good intention behind this highly defensive approach but in practice it creates a problem itself.
Every now and then an exception is logged that reads
Unable to find a valid code.that originates from theEmailMultifactorMethod. Further investigation reveals that is caused by two parallel requests being issued by a client where the first request appears to be cancelled by the client (HAProxy reports this asCD--).Imagine two identical requests A and B arriving at the server:
validate()and the form field successfully validates the code.validate()and the form field successfully validates the code.save(), looks the setup and fetches/invalidates the used code.save()after request A and once the lock is released, tries to fetch the code, finds none and panics.Because request A was aborted by the client, the user only sees the result of request B: A hard exception instead of a meaningful error handling.
This is a design flaw in the
processAuthenticationForm()implementation because it performs the validation again that already took place invalidate(). However, due to the above race condition, this creates a TOCTOU situation where the code vanishes before theprocessAuthenticationForm()is fully executed.Solution:
processAuthenticationFormshould not perform any validation at all because at this point the validation has already been passed. It should gracefully handle the code being absent because we know that the removal properly works. There was a good intention behind this highly defensive approach but in practice it creates a problem itself.