[Django] #28248: Password resets are allowed for 1 day longer than PASSWORD_RESET_TIMEOUT_DAYS
Django
PASSWORD_RESET_TIMEOUT_DAYS
-----------------------------------------+------------------------
Reporter: Nick Zaccardi | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
[https://github.com/django/django/blob/7afb47646920ab3835dfa1750257dace01883a4b/django/contrib/auth/tokens.py#L45
An improper comparison] (> rather than >=) in the password reset token
checking, allows password reset tokens to be used one day longer than
expected.
--
Ticket URL: <https://code.djangoproject.com/ticket/28248>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/8560 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:1>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Component: contrib.auth | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"95993a89ce6ca5f5e26b1c22b65c57dcb8c005e9" 95993a8]:
{{{
#!CommitTicketReference repository=""
revision="95993a89ce6ca5f5e26b1c22b65c57dcb8c005e9"
Fixed #28248 -- Fixed password reset tokens being valid for 1 day longer
than PASSWORD_RESET_TIMEOUT_DAYS.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:2>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Luke Plant):
* status: closed => new
* resolution: fixed =>
Comment:
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:3>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* version: 1.11 => 2.0
* severity: Normal => Release blocker
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:4>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Component: contrib.auth | Version: 2.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"67a6ba391bbcf1a4c6bb0c42cb17e4fc0530f6d2" 67a6ba39]:
{{{
#!CommitTicketReference repository=""
revision="67a6ba391bbcf1a4c6bb0c42cb17e4fc0530f6d2"
Reverted "Fixed #28248 -- Fixed password reset tokens being valid for 1
day longer than PASSWORD_RESET_TIMEOUT_DAYS."
This reverts commit 95993a89ce6ca5f5e26b1c22b65c57dcb8c005e9.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:5>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Component: contrib.auth | Version: 2.0
Severity: Release blocker | Resolution: fixed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"e241b4e7970e45c21d14df30ce9f6f02b9b7cdce" e241b4e7]:
{{{
#!CommitTicketReference repository=""
revision="e241b4e7970e45c21d14df30ce9f6f02b9b7cdce"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:6>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Component: contrib.auth | Version: 2.0
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* resolution: fixed => invalid
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:7>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Component: contrib.auth | Version: 2.0
Severity: Release blocker | Resolution: invalid
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:8>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Component: contrib.auth | Version: 2.0
Severity: Release blocker | Resolution: invalid
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
[https://github.com/django/django/pull/9141 PR] to clarify the docs and
code comment.
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:9>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Component: contrib.auth | Version: 2.0
Severity: Release blocker | Resolution: invalid
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:10>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Component: contrib.auth | Version: 2.0
Severity: Release blocker | Resolution: invalid
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Luke Plant):
Replying to [comment:10 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...
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:11>
Django
PASSWORD_RESET_TIMEOUT_DAYS
Reporter: Nick Zaccardi | Owner: nobody
Component: contrib.auth | Version: 2.0
Severity: Release blocker | Resolution: invalid
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"0edff2107f9cdd89737d2d33d1a40362ecde894c" 0edff210]:
{{{
#!CommitTicketReference repository=""
revision="0edff2107f9cdd89737d2d33d1a40362ecde894c"
Refs #28248 -- Clarified the precision of PASSWORD_RESET_TIMEOUT_DAYS.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28248#comment:12>