fix: harden password change after code review

Count and audit a wrong current password found at change time, log the unclear outcome with its details and end the other sessions as far as possible, note a lockout threshold of 1 in the audit, skip the local hints when the fine-grained policy cannot be read, round the minimum age up, reset the form after back navigation, give error messages unique ids, and fix the German rejection text. Adds tests for the block length, the masked password fields and the new cases; the review findings are recorded in the plan and the deferred work.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FocPJ75XPma7TcuYEMy8zm

portal/locale/de/LC_MESSAGES/django.po

@@ -160,7 +160,7 @@ msgid "The current password is not correct."
 msgstr "Das aktuelle Passwort ist nicht korrekt."
 
 msgid "The new password does not meet the rules above, or it was used or changed too recently."
-msgstr "Das neue Passwort erfüllt die oben genannten Regeln nicht, wurde schon einmal verwendet oder kommt zu kurz nach der letzten Änderung."
+msgstr "Das neue Passwort erfüllt die oben genannten Regeln nicht, wurde schon einmal verwendet oder das Passwort wurde erst vor Kurzem geändert."
 
 msgid "Please fill in this field."
 msgstr "Bitte füllen Sie dieses Feld aus."

portal/profile/services.py

@@ -9,6 +9,7 @@ stored or put into an exception.
 from __future__ import annotations
 
 import logging
+import math
 from dataclasses import dataclass
 from datetime import timedelta
 from zoneinfo import ZoneInfo
@@ -62,14 +63,16 @@ class Changed:
 
 def describe_policy(policy: PasswordPolicy) -> list[str]:
     """The user's password rules as sentences in the active language."""
-    rules = [
-        ngettext(
-            "At least %(n)s character",
-            "At least %(n)s characters",
-            policy.min_length,
+    rules = []
+    if policy.min_length:
+        rules.append(
+            ngettext(
+                "At least %(n)s character",
+                "At least %(n)s characters",
+                policy.min_length,
+            )
+            % {"n": policy.min_length}
         )
-        % {"n": policy.min_length}
-    ]
     if policy.complexity:
         rules.append(
             _(
@@ -104,13 +107,13 @@ def describe_policy(policy: PasswordPolicy) -> list[str]:
 
 def _min_age_rule(age: timedelta) -> str:
     if age >= timedelta(days=1):
-        days = round(age.total_seconds() / 86400)
+        days = math.ceil(age.total_seconds() / 86400)  # round up: too early is refused
         return ngettext(
             "A new password can be changed again after %(n)s day",
             "A new password can be changed again after %(n)s days",
             days,
         ) % {"n": days}
-    hours = max(1, round(age.total_seconds() / 3600))
+    hours = max(1, math.ceil(age.total_seconds() / 3600))
     return ngettext(
         "A new password can be changed again after %(n)s hour",
         "A new password can be changed again after %(n)s hours",
@@ -133,7 +136,13 @@ def _character_groups(password: str) -> int:
 
 
 def local_hints(new: str, policy: PasswordPolicy) -> list[str]:
-    """Rule keys the portal can check itself; the directory stays the authority."""
+    """Rule keys the portal can check itself; the directory stays the authority.
+
+    Nothing is claimed when a fine-grained policy applies but could not be read: the domain
+    values may be stricter or looser than the rules that really apply.
+    """
+    if policy.fine_grained and not policy.fine_grained_readable:
+        return []
     reasons = []
     if len(new) < policy.min_length:
         reasons.append("too_short")
@@ -161,6 +170,13 @@ def pause_seconds(policy: PasswordPolicy) -> int:
 # -- the change -----------------------------------------
 
 
+def _threshold_note(policy: PasswordPolicy | None) -> dict:
+    """Audit note for a lockout threshold of 1, which the portal cannot protect against."""
+    if policy is not None and policy.lockout_threshold == 1:
+        return {"lockout_protection": "none"}
+    return {}
+
+
 def _record_failure(user: SessionUser, ip: str, reason: str, **detail) -> None:
     audit.record_failure(
         user.key, "password.change_failed", ip=ip, detail={"reason": reason, **detail}
@@ -192,7 +208,7 @@ def change_password(
                 max_failures=failure_limit(policy),
                 pause_seconds=pause_seconds(policy),
             )
-            _record_failure(user, ip, "wrong_current")
+            _record_failure(user, ip, "wrong_current", **_threshold_note(policy))
             raise WrongCurrentPassword(policy)
         account = directory.find_user(user.sam)
     except DirectoryUnavailable:
@@ -205,12 +221,16 @@ def change_password(
         raise PasswordNotAccepted(reasons, policy)
 
     changed_in_directory = False
+    step = "directory"
     try:
         with transaction.atomic():
             directory.change_password(user.sam, current, new)
             changed_in_directory = True
+            step = "end_sessions"
             ended = end_sessions(user, except_session_key=session_key)
+            step = "queue_mail"
             mail_queued = _queue_mail(account)
+            step = "audit"
             audit.record(
                 user.key,
                 "password.changed",
@@ -218,10 +238,19 @@ def change_password(
                 detail={
                     "other_sessions_ended": ended,
                     "mail": "queued" if mail_queued else "skipped",
+                    **_threshold_note(policy),
                 },
             )
     except PasswordRejected as exc:
         if exc.reason == "wrong_current":  # the password changed between the check and the change
+            throttle.hit(
+                ENDPOINT,
+                user=user.key,
+                ip=ip or None,
+                max_failures=failure_limit(policy),
+                pause_seconds=pause_seconds(policy),
+            )
+            _record_failure(user, ip, "wrong_current", **_threshold_note(policy))
             raise WrongCurrentPassword(policy) from exc
         _record_failure(user, ip, "policy")
         raise PasswordNotAccepted([], policy) from exc
@@ -231,7 +260,7 @@ def change_password(
     except Exception as exc:
         if changed_in_directory:
             # AD-4: the directory changed but the portal's part did not commit.
-            logger.error("Password changed in the directory but the portal did not record it")
+            _after_unclear_outcome(user, ip, session_key, step, exc)
             raise OutcomeUnknown("The password was changed but not recorded") from exc
         raise
 
@@ -239,6 +268,34 @@ def change_password(
     return Changed(mail_queued=mail_queued, other_sessions_ended=ended)
 
 
+def _after_unclear_outcome(
+    user: SessionUser, ip: str, session_key: str | None, step: str, exc: Exception
+) -> None:
+    """The directory has the new password but the portal's part rolled back.
+
+    Log it with the change details (AD-4, never the exception text, which could hold input),
+    then do what can still be done: end the other sessions and write a failure entry. The
+    database may be the very thing that failed, so each attempt stands alone.
+    """
+    logger.error(
+        "Password changed in the directory but the portal did not record it: "
+        "user=%s ip=%s time=%s step=%s error=%s",
+        user.key,
+        ip,
+        timezone.now().isoformat(),
+        step,
+        type(exc).__name__,
+    )
+    try:
+        end_sessions(user, except_session_key=session_key)
+    except Exception:
+        logger.error("The other sessions of %s could not be ended", user.key)
+    try:
+        _record_failure(user, ip, "unclear", step=step)
+    except Exception:
+        logger.error("The failure entry for %s could not be written", user.key)
+
+
 def _queue_mail(account) -> bool:
     """Queue the confirmation e-mail inside the caller's transaction; False if there is none."""
     address = (account.mail if account else "") or ""

portal/static/portal/shell.js

@@ -64,6 +64,19 @@
     }, 0);
   });
 
+  // A page restored from the back/forward cache must not come back stuck in the sending state.
+  window.addEventListener("pageshow", function (event) {
+    if (!event.persisted) {
+      return;
+    }
+    document.querySelectorAll("form.busy").forEach(function (form) {
+      form.classList.remove("busy");
+      form.querySelectorAll("button[type=submit]").forEach(function (button) {
+        button.disabled = false;
+      });
+    });
+  });
+
   var focusTarget = document.querySelector("[data-focus-on-load]");
   if (focusTarget) {
     focusTarget.focus();

portal/templates/profile/_field.html

@@ -1,7 +1,7 @@
 {% load i18n %}
 <div class="field{% if field.errors %} has-error{% endif %}">
   <label for="{{ field.id_for_label }}">{{ label }}</label>
-  <input type="password" name="{{ field.html_name }}" id="{{ field.id_for_label }}" autocomplete="{{ autocomplete }}" maxlength="256" required{% if field.errors %} aria-invalid="true"{% endif %} aria-describedby="{{ field.id_for_label }}-hint{% if field.errors %} {{ field.id_for_label }}-error{% endif %}">
+  <input type="password" name="{{ field.html_name }}" id="{{ field.id_for_label }}" autocomplete="{{ autocomplete }}" maxlength="{{ field.field.max_length }}" required{% if field.errors %} aria-invalid="true"{% endif %} aria-describedby="{{ field.id_for_label }}-hint{% if field.errors %} {{ field.id_for_label }}-error{% endif %}">
   {% if hint %}<p class="hint" id="{{ field.id_for_label }}-hint">{{ hint }}</p>{% else %}<span id="{{ field.id_for_label }}-hint"></span>{% endif %}
-  {% for error in field.errors %}<p class="error" id="{{ field.id_for_label }}-error">{{ error }}</p>{% endfor %}
+  {% if field.errors %}<div id="{{ field.id_for_label }}-error">{% for error in field.errors %}<p class="error">{{ error }}</p>{% endfor %}</div>{% endif %}
 </div>

portal/tests/test_password_change.py

@@ -6,6 +6,8 @@ from datetime import timedelta
 
 import pytest
 from django.test import Client
+from django.utils import timezone
+from django.views.debug import SafeExceptionReporterFilter
 
 from portal.accounts.models import ThrottleCounter, UserSession
 from portal.adapters.directory import DirectoryUnavailable, PasswordPolicy, PasswordRejected
@@ -238,6 +240,13 @@ def test_errors_are_next_to_the_field_and_in_a_summary(people, adapters):
     assert 'class="error-summary"' in html and "The password was not changed." in html
     assert 'href="#id_confirm_password"' in html and 'id="id_confirm_password-error"' in html
     assert 'aria-invalid="true"' in html and "data-focus-on-load" in html
+    assert 'maxlength="256"' in html
+
+
+def test_two_errors_on_one_field_share_one_description(people, adapters):
+    html = change(signed_in(adapters), new="a").content.decode()  # too short and too few groups
+    assert html.count('id="id_new_password-error"') == 1
+    assert html.count('class="error"') >= 2
 
 
 # -- wrong current password and the limit -----------------------------------------
@@ -272,11 +281,21 @@ def test_the_limit_stays_below_the_directory_lockout_threshold(people, adapters)
     assert "password.throttled" in audit_actions()
 
 
-def test_the_pause_is_over_after_the_lockout_window(people, adapters):
+def test_the_block_lasts_as_long_as_the_lockout_window(people, adapters):
     client = signed_in(adapters)
     for _ in range(4):
         change(client, current=WRONG)
-    ThrottleCounter.objects.update(blocked_until=None, count=0)  # the pause is over
+    # the fake domain forgets failed attempts after 30 minutes: the pause must not end sooner
+    for subject in ("user:alice", "ip:127.0.0.1"):
+        left = ThrottleCounter.objects.get(subject=subject).blocked_until - timezone.now()
+        assert timedelta(minutes=29) < left <= timedelta(minutes=30), subject
+
+
+def test_a_user_can_try_again_once_the_block_is_cleared(people, adapters):
+    client = signed_in(adapters)
+    for _ in range(4):
+        change(client, current=WRONG)
+    ThrottleCounter.objects.update(blocked_until=None, count=0)  # as if the pause had run out
     assert change(client).status_code == 302
 
 
@@ -296,6 +315,18 @@ def test_the_limit_for_other_thresholds(people):
     assert services.pause_seconds(policy(0, None)) == 15 * 60
 
 
+def test_a_lockout_threshold_of_one_is_noted_in_the_audit(people, adapters):
+    adapters.directory.policy = PasswordPolicy(
+        8, True, 0, timedelta(0), None, lockout_threshold=1, lockout_window=timedelta(minutes=30)
+    )
+    change(signed_in(adapters), current=WRONG)
+    entry = AuditEntry.objects.get(action="password.change_failed")
+    assert entry.detail == {"reason": "wrong_current", "lockout_protection": "none"}
+    ThrottleCounter.objects.all().delete()
+    change(signed_in(adapters))
+    assert AuditEntry.objects.get(action="password.changed").detail["lockout_protection"] == "none"
+
+
 def test_the_limit_is_per_ip_too(people, adapters):
     client = signed_in(adapters)
     for _ in range(4):
@@ -322,6 +353,16 @@ def test_rules_the_portal_can_name_are_named_without_asking_the_directory(
     assert AuditEntry.objects.get(action="password.change_failed").detail["reason"] == "policy"
 
 
+def test_without_a_readable_policy_the_portal_leaves_the_rules_to_the_directory(people, adapters):
+    adapters.directory.policy = PasswordPolicy(
+        8, True, 0, timedelta(0), None, fine_grained=True, fine_grained_readable=False
+    )
+    spy = Spy(adapters.directory)
+    response = change(signed_in(adapters), new="Ab1")
+    assert spy.count("change_password") == 1  # not refused by domain values that may not apply
+    assert "too short" not in response.content.decode()
+
+
 def test_a_rule_failure_does_not_count_as_a_wrong_password(people, adapters):
     client = signed_in(adapters)
     for _ in range(8):
@@ -351,6 +392,10 @@ def test_the_password_changed_by_someone_else_in_between_counts_as_wrong(
     monkeypatch.setattr(adapters.directory, "change_password", raced)
     response = change(signed_in(adapters))
     assert response.status_code == 400 and "not correct" in response.content.decode()
+    assert ThrottleCounter.objects.get(subject="user:alice").count == 1
+    entry = AuditEntry.objects.get(action="password.change_failed")
+    assert entry.detail == {"reason": "wrong_current"}
+    assert "password.changed" not in audit_actions()
 
 
 # -- failures after the first directory call -----------------------------------------
@@ -393,14 +438,34 @@ def test_a_failure_after_the_directory_changed_is_reported_as_unclear(
         response = change(client)
     assert response.status_code == 500 and "Result unclear" in response.content.decode()
     assert adapters.directory.verify_password("alice", NEW)  # the directory did change
-    assert other.get("/").status_code == 200  # the portal's part rolled back
-    assert "password.changed" not in audit_actions()
-    assert "changed in the directory but the portal did not record it" in caplog.text
+    assert client.get("/").status_code == 200  # this session stays
+    assert other.get("/").status_code == 302  # the other one is ended as far as possible
+    assert "password.changed" not in audit_actions() and OutboxMail.objects.count() == 0
+    entry = AuditEntry.objects.get(action="password.change_failed")
+    assert entry.detail == {"reason": "unclear", "step": "queue_mail"}
+    record = next(r for r in caplog.records if "did not record it" in r.getMessage())
+    assert record.levelno == logging.ERROR
+    assert "user=alice" in record.getMessage() and "step=queue_mail" in record.getMessage()
+    assert "RuntimeError" in record.getMessage() and "database trouble" not in caplog.text
 
 
 # -- no secrets anywhere -----------------------------------------
 
 
+def test_the_password_fields_are_masked_in_error_reports(people, adapters, monkeypatch):
+    def crash(*args, **kwargs):
+        raise RuntimeError("crash")
+
+    monkeypatch.setattr(services, "change_password", crash)
+    client = signed_in(adapters)
+    client.raise_request_exception = False
+    response = change(client)
+    assert response.status_code == 500
+    shown = SafeExceptionReporterFilter().get_post_parameters(response.wsgi_request)
+    for name in ("current_password", "new_password", "confirm_password"):
+        assert shown[name] == SafeExceptionReporterFilter.cleansed_substitute, name
+
+
 def test_no_password_appears_in_logs_audit_session_database_or_pages(people, adapters, caplog):
     client = signed_in(adapters)
     other = signed_in(adapters)
@@ -423,6 +488,13 @@ def test_no_password_appears_in_logs_audit_session_database_or_pages(people, ada
 # -- the rules in words -----------------------------------------
 
 
+def test_the_minimum_age_is_rounded_up_and_a_minimum_length_of_zero_is_not_listed():
+    policy = PasswordPolicy(0, False, 0, timedelta(days=1, hours=10), None)
+    assert services.describe_policy(policy) == ["A new password can be changed again after 2 days"]
+    soon = PasswordPolicy(0, False, 0, timedelta(minutes=90), None)
+    assert services.describe_policy(soon) == ["A new password can be changed again after 2 hours"]
+
+
 def test_the_rules_are_written_out_for_each_part_of_the_policy():
     policy = PasswordPolicy(1, False, 1, timedelta(hours=1), None)
     assert services.describe_policy(policy) == [