PST-010: A recovery link stays usable for its full hour after the person has changed their password or signed in #739

New issue

Closed

opened 2026-10-10 10:55:49 +00:00 by tiagoagueda · 1 comment

tiagoagueda commented

2026-10-10 10:55:49 +00:00

Owner

Found in the security audit of 2026-10-10 (SECURITY-AUDIT-2026-10-10.md), run against main at c30851b from 8 to 10 October 2026, as PST-010. Severity: Low.

The text below is the audit's entry, unchanged apart from links to the other issues it names. The finding was produced by automated review and checked by a second automated pass; re-check it before acting on it.

Severity: Low
Category: credential-lifecycle
Location: src/postulo/accounts/recovery.py (46-66)
Found by: authentication and sessions
Related: PST-011 (#740)

Who can do it, and what they need. Whoever ends up holding a recovery link after the person it was made for no longer needs it: a chat log, a screenshot, a shoulder surfer. Precondition: the link was issued in the last hour and not revoked by hand.

How it works.

Administrator issues a link; the person recovers by another route or simply remembers their password and signs in, or changes it under Settings.
Nothing revokes the link: spend() only checks what _link() checked (used_at, revoked_at, expires_at); a password change or a sign-in on the account does not touch RecoveryLink rows.
The holder opens /accounts/recover/<token>/ and sets a new password (scratch test test_recovery_link_survives_a_password_change_and_sign_in: the link still set the password after person.set_password had run).
Separately, an administrator's standing session can mint up to ten links an hour with no reauthentication, so a borrowed browser session suffices.

Impact. Narrow window and a secret token are required, so a hardening gap rather than a hole: the person's own password change does not close a door an administrator opened for them.

Evidence.

recovery.py:91-96 `with transaction.atomic(): person = link.person; person.set_password(new_password); person.save(update_fields=["password"]); link.used_at = timezone.now(); link.save(update_fields=["used_at"])` — the only consumption; no signal on password change or login revokes live links. views.py:586-592 `_link` returns the row if `link.is_live`, which models.py:663 defines as used_at/revoked_at empty and not expired. server_views.py:344-385 PersonRecoveryView has StaffRequiredMixin only.

Verification (verdict: confirmed; severity after verification: Low). Keep the finding to the narrow gap: a live RecoveryLink is not revoked when the person changes their password or signs in, so a leaked token stays usable for the remainder of its hour. Remediation: revoke the person's live links on password_changed, password_reset and user_logged_in (or fold the password hash and last_login into the link's validity, as Django's reset tokens do). Drop step 4 ('minting needs no fresh proof'): that is the trusted administrator or a compromised browser, both outside the threat model. Line refs: recovery.py:46-66 and 81-96, views.py:576-624 (_link at 586-592), models.py:663-664 are correct.

Suggested remediation. Revoke live recovery links whenever the account's password changes (allauth's password_changed / password_reset signals and recovery.spend itself) and on a successful sign-in; consider requiring reauthentication to mint one and emailing the account when a link is created. Add a test that a password change kills a live link.

Found in the security audit of 2026-10-10 (`SECURITY-AUDIT-2026-10-10.md`), run against `main` at `c30851b` from 8 to 10 October 2026, as **PST-010**. Severity: **Low**. The text below is the audit's entry, unchanged apart from links to the other issues it names. The finding was produced by automated review and checked by a second automated pass; re-check it before acting on it. --- - **Severity:** Low - **Category:** credential-lifecycle - **Location:** `src/postulo/accounts/recovery.py` (46-66) - **Found by:** authentication and sessions - **Related:** PST-011 (#740) **Who can do it, and what they need.** Whoever ends up holding a recovery link after the person it was made for no longer needs it: a chat log, a screenshot, a shoulder surfer. Precondition: the link was issued in the last hour and not revoked by hand. **How it works.** 1. Administrator issues a link; the person recovers by another route or simply remembers their password and signs in, or changes it under Settings. 2. Nothing revokes the link: spend() only checks what _link() checked (used_at, revoked_at, expires_at); a password change or a sign-in on the account does not touch RecoveryLink rows. 3. The holder opens /accounts/recover/<token>/ and sets a new password (scratch test test_recovery_link_survives_a_password_change_and_sign_in: the link still set the password after person.set_password had run). 4. Separately, an administrator's standing session can mint up to ten links an hour with no reauthentication, so a borrowed browser session suffices. **Impact.** Narrow window and a secret token are required, so a hardening gap rather than a hole: the person's own password change does not close a door an administrator opened for them. **Evidence.** ```text recovery.py:91-96 `with transaction.atomic(): person = link.person; person.set_password(new_password); person.save(update_fields=["password"]); link.used_at = timezone.now(); link.save(update_fields=["used_at"])` — the only consumption; no signal on password change or login revokes live links. views.py:586-592 `_link` returns the row if `link.is_live`, which models.py:663 defines as used_at/revoked_at empty and not expired. server_views.py:344-385 PersonRecoveryView has StaffRequiredMixin only. ``` **Verification** (verdict: confirmed; severity after verification: Low). Keep the finding to the narrow gap: a live RecoveryLink is not revoked when the person changes their password or signs in, so a leaked token stays usable for the remainder of its hour. Remediation: revoke the person's live links on password_changed, password_reset and user_logged_in (or fold the password hash and last_login into the link's validity, as Django's reset tokens do). Drop step 4 ('minting needs no fresh proof'): that is the trusted administrator or a compromised browser, both outside the threat model. Line refs: recovery.py:46-66 and 81-96, views.py:576-624 (_link at 586-592), models.py:663-664 are correct. **Suggested remediation.** Revoke live recovery links whenever the account's password changes (allauth's password_changed / password_reset signals and recovery.spend itself) and on a successful sign-in; consider requiring reauthentication to mint one and emailing the account when a link is created. Add a test that a password change kills a live link.

tiagoagueda added the

authentication

bug

security

labels

2026-10-10 10:55:49 +00:00

tiagoagueda added this to the 0.6.0 milestone

2026-10-10 10:57:23 +00:00

tiagoagueda referenced this issue

2026-10-10 10:57:23 +00:00

PST-011: Administrator recovery links, demotion and account deletion need no fresh proof, and taking over a member's account is silent #740

tiagoagueda referenced this issue