[FIX] Fix tests for complete_signup #543
Conversation
Instead of using the time that the HMAC was created as the expiry time, use a separate expiry time in the future. This prevents a race condition where these tests will sometimes fail if they run more than 1 second as the HMAC would have expired by then. The expiry time is set to one year from the time that the set up code runs. That should hopefully be enough time to run the tests, seeing as, if these tests run for more than a year, we probably have bigger problems.
Currently test_if_link_expired uses a HMAC that is incorrect because it uses a different expiry time than the one uses to generate the HMAC. On top of this, it uses an expiry time in the future to test the response when the HMAC is expired, when it should be using an expiry time in the past. This causes it to appear to still work because the complete_signup request fails as expected, but that happens because the hash is incorrect, not because the link is expired. This commit fixes that bug to use the correct expiry time and HMAC combination.
Add some log statements to make it easier to debug why signup is failing. This doesn't leak any data that the user wouldn't already have access to (they would probably already know what the expiry time was, and if it wasn't past the expiry time, it's somewhat obvious that any invalid registration response is caused by a bad HMAC), so this probably wouldn't make us any more vulnerable to chosen ciphertext attacks that rely on knowing when HMAC authentication failed (if any such attacks even exist).
pranavrajpal
requested review from
canihavesomecoffee and
thealphadollar
as code owners
Generating the HMAC should use the expiry time in the data, not the time that the HMAC was created, because complete_signup uses the expiry time to determine the data used to generate the HMAC.
|
I don't really understand why the CI is failing. It seems to run all of the tests without failures, and then fails when it's trying to generate the coverage report with the error:
I'm not very familiar with using Codecov, and I can't seem to find much information about this error online. Does anyone have more information about why this is happening? |
Codecov Report
@@ Coverage Diff @@
## master #543 +/- ##
==========================================
+ Coverage 80.94% 80.98% +0.03%
==========================================
Files 36 36
Lines 3589 3591 +2
Branches 408 408
==========================================
+ Hits 2905 2908 +3
Misses 549 549
+ Partials 135 134 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Hey Pranav, thanks for the fix. It was a temporary issue that got fixed by rerun of the CI. |
Please prefix your pull request with one of the following: [FEATURE] [FIX] [IMPROVEMENT].
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
This contains a few fixes for some tests for
complete_signupin mod_auth. Those fixes are:test_if_link_expiredto actually test what would happen when using an expired link. Originally, it was testing what happened when sending an invalid HMAC with a time in the future, which coincidentally resulted in the same outcome. (fixed in cfa6f71)I also added some logging to
complete_signupfor when the HMAC or expiry date is invalid to make it easier to debug problems like this if they ever happen again.