Password resets are allowed for 1 day longer than PASSWORD_RESET_TIMEOUT_DAYS

[Nick Zaccardi]

​An improper comparison (> rather than >=) in the password reset token checking, allows password reset tokens to be used one day longer than expected.

[Tim Graham]

​PR

[Tim Graham <timograham@…>]

In 95993a8:

Fixed #28248 -- Fixed password reset tokens being valid for 1 day longer than PASSWORD_RESET_TIMEOUT_DAYS.

[Luke Plant]

I believe this change is incorrect and should be reverted, according to a common sense interpretation of the setting.

The code rounds all timestamps to midnight (server time) providing a resolution of only 1 day. So if you generate a links 5 minutes before midnight and try to use it 6 minutes later, that counts as 1 day. With the current code, if you set the timeout setting to 1 day, such a link would be rejected.

In other words, new code interprets timeout of 1 day as "up to one day, could be almost zero". The old way interpreted as "at least 1 day, could be up to 2". I think the old way was better, much less likely to be an accidental usability issue. There is virtually no security concern with being too generous here (see my latest post on django-devs on related issue).

[Tim Graham <timograham@…>]

In 67a6ba39:

Reverted "Fixed #28248 -- Fixed password reset tokens being valid for 1 day longer than PASSWORD_RESET_TIMEOUT_DAYS."

This reverts commit 95993a89ce6ca5f5e26b1c22b65c57dcb8c005e9.

[Tim Graham <timograham@…>]

In e241b4e7:

[2.0.x] Reverted "Fixed #28248 -- Fixed password reset tokens being valid for 1 day longer than PASSWORD_RESET_TIMEOUT_DAYS."

This reverts commit 95993a89ce6ca5f5e26b1c22b65c57dcb8c005e9.

Backport of 67a6ba391bbcf1a4c6bb0c42cb17e4fc0530f6d2 from master

[Claude Paroz]

I think it would be great to add at least some part of Luke's comment in a code comment to prevent further reports about this issue.

[Tim Graham]

​PR to clarify the docs and code comment.

[Nick Zaccardi]

Sorry to be a moment late to respond. I think the reversion is the correct action for the 99+% and I support that direction. In fact, the fix I originally supplied does have the rounding bug. My apologies for that, I misunderstood the way the rounding work.

I do want to explain why this doesn't meet the 1% of use cases. When I originally reported this I was working on a password reset feature in a different app (a large corporate financial application) which has very specific policies on passwords, password resets, and the validity time of both. From a contractual perspective (regardless of user experience) >24hr link would be a break in policy or worse a violation of contractual obligation to implement a <24hr link. For most up to 2 days is fine, for some, regardless of the real-life implications of the policy, it is a big deal.

All that to say, why does this get rounded in the first place? why not just use:

validity_time = now() + {{ reset_timedelta }}

if validity_time < now():
    // is expired 

Thanks for all you folks do!

[Luke Plant]

Replying to Nick Zaccardi:

I do want to explain why this doesn't meet the 1% of use cases. When I originally reported this I was working on a password reset feature in a different app (a large corporate financial application) which has very specific policies on passwords, password resets, and the validity time of both. From a contractual perspective (regardless of user experience) >24hr link would be a break in policy or worse a violation of contractual obligation to implement a <24hr link. For most up to 2 days is fine, for some, regardless of the real-life implications of the policy, it is a big deal.

Thanks for filling in these background details. It is unfortunate that sometimes these policies exist which actually don't apply (for reasons that I described here - ​https://groups.google.com/d/msg/django-developers/65iOQunvkPY/pP5mF-44AQAJ )

However, your experience is still a significant data point in favour of the feature being asked for in that thread ( ​https://groups.google.com/forum/#!topic/django-developers/65iOQunvkPY ), namely, supporting a timeout of less than one day. Please do feel free to jump into that thread and add your 2 cents - we have to be pragmatic about complying with these kinds of regulations.

All that to say, why does this get rounded in the first place? why not just use:

It is mainly rounded to make the timestamp shorter (we only need to store a number of days, not seconds), which has an impact on the length of URL generated. That may sound like a dubious argument, but that was the initial rationale I believe! With some email clients that like to truncate URLs etc., it can make a difference. Maybe things are better these days...

[GitHub <noreply@…>]

In 0edff210:

Refs #28248 -- Clarified the precision of PASSWORD_RESET_TIMEOUT_DAYS.