From 80c58aa5014ca8812d6eeea8df629ea3d647e060 Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Tue, 29 Sep 2026 13:22:47 +0200 Subject: [PATCH] Make password reset tokens single-use Bind each token to a random nonce stored on the user. Requesting a reset rotates the nonce, revoking older links; any password change clears it, so a link works once and dies on a logged-in password change. Co-Authored-By: Claude Opus 5.5 --- deployment/community/.env.template | 2 + deployment/enterprise/.env.template | 2 + server/mergin/auth/app.py | 58 ++++++++++-- server/mergin/auth/config.py | 3 + server/mergin/auth/controller.py | 26 ++++-- server/mergin/auth/listeners.py | 3 +- server/mergin/auth/models.py | 4 + server/mergin/tests/test_audit_events.py | 22 ++++- server/mergin/tests/test_auth.py | 91 ++++++++++++++++--- .../c4e7b1d9a2f3_add_password_reset_nonce.py | 28 ++++++ 10 files changed, 204 insertions(+), 35 deletions(-) create mode 100644 server/migrations/community/c4e7b1d9a2f3_add_password_reset_nonce.py diff --git a/deployment/community/.env.template b/deployment/community/.env.template index e786e52f..55842c36 100644 --- a/deployment/community/.env.template +++ b/deployment/community/.env.template @@ -57,6 +57,8 @@ SECRET_KEY=fixme #BEARER_TOKEN_EXPIRATION=3600 * 12 # in seconds +#PASSWORD_RESET_TOKEN_EXPIRATION=600 # in seconds + #SECURITY_BEARER_SALT=NODEFAULT SECURITY_BEARER_SALT=fixme diff --git a/deployment/enterprise/.env.template b/deployment/enterprise/.env.template index a11aa575..1efb6e6a 100644 --- a/deployment/enterprise/.env.template +++ b/deployment/enterprise/.env.template @@ -62,6 +62,8 @@ SECRET_KEY=fixme #BEARER_TOKEN_EXPIRATION=3600 * 12 # in seconds +#PASSWORD_RESET_TOKEN_EXPIRATION=600 # in seconds + #SECURITY_BEARER_SALT=NODEFAULT SECURITY_BEARER_SALT=fixme diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py index 638ec2ff..643de9b9 100644 --- a/server/mergin/auth/app.py +++ b/server/mergin/auth/app.py @@ -4,6 +4,7 @@ import functools import logging +import secrets from typing import Optional from blinker import signal from flask import current_app, render_template, Flask @@ -159,15 +160,30 @@ def send_confirmation_email(app, user, url, template, header, **kwargs): Send confirmation email from selected template with customizable email subject and confirmation URL. Optional kwargs are passed to render_template method if needed for particular template. """ - from ..celery import send_email_async + token = generate_confirmation_token( + app, user.email, app.config["SECURITY_EMAIL_SALT"] + ) + _send_token_email(app, user, f"{url}/{token}", template, header, **kwargs) + - salt = ( - app.config["SECURITY_EMAIL_SALT"] - if url == "confirm-email" - else app.config["SECURITY_PASSWORD_SALT"] +def send_password_reset_email(app: Flask, user: User) -> None: + """Issue a new single-use password reset token (revoking any previous one) and email it.""" + from ..app import db + + token = generate_password_reset_token(app, user) + db.session.commit() + _send_token_email( + app, + user, + f"change-password/{token}", + "email/password_reset.html", + "Password reset", ) - token = generate_confirmation_token(app, user.email, salt) - confirm_url = f"{url}/{token}" + + +def _send_token_email(app, user, confirm_url, template, header, **kwargs): + from ..celery import send_email_async + html = render_template( template, subject=header, confirm_url=confirm_url, user=user, **kwargs ) @@ -180,6 +196,34 @@ def send_confirmation_email(app, user, url, template, header, **kwargs): send_email_async.delay(**email_data) +def generate_password_reset_token(app: Flask, user: User) -> str: + """Sign a reset token bound to a fresh nonce stored on the user. + + Rotating the nonce revokes previously issued tokens; any password change clears it. + """ + user.password_reset_nonce = secrets.token_urlsafe(32) + serializer = URLSafeTimedSerializer(app.config["SECRET_KEY"]) + payload = {"email": user.email, "nonce": user.password_reset_nonce} + return serializer.dumps(payload, salt=app.config["SECURITY_PASSWORD_SALT"]) + + +def confirm_password_reset_token(token: str) -> Optional[dict]: + """Return a signed, unexpired reset token payload, or None. The nonce still has to be checked against the user.""" + serializer = URLSafeTimedSerializer(current_app.config["SECRET_KEY"]) + try: + payload = serializer.loads( + token, + salt=current_app.config["SECURITY_PASSWORD_SALT"], + max_age=current_app.config["PASSWORD_RESET_TOKEN_EXPIRATION"], + ) + except BadData: + return None + # tokens issued before nonces were introduced carried only the email + if not isinstance(payload, dict): + return None + return payload + + def generate_unlock_token(app: Flask, user: User) -> str: """Sign a token binding the current lock episode (email + locked_until) to the user.""" serializer = URLSafeTimedSerializer(app.config["SECRET_KEY"]) diff --git a/server/mergin/auth/config.py b/server/mergin/auth/config.py index 9b9c5c50..a4f34447 100644 --- a/server/mergin/auth/config.py +++ b/server/mergin/auth/config.py @@ -13,6 +13,9 @@ class Configuration(object): BEARER_TOKEN_EXPIRATION = config( "BEARER_TOKEN_EXPIRATION", default=3600 * 12, cast=int ) # in seconds + PASSWORD_RESET_TOKEN_EXPIRATION = config( + "PASSWORD_RESET_TOKEN_EXPIRATION", default=600, cast=int + ) # in seconds ACCOUNT_EXPIRATION = config("ACCOUNT_EXPIRATION", default=5, cast=int) # in days BCRYPT_LOG_ROUNDS = config("BCRYPT_LOG_ROUNDS", default=12, cast=int) # Comma-separated "attempts:seconds" pairs, e.g. "5:300,10:3600" diff --git a/server/mergin/auth/controller.py b/server/mergin/auth/controller.py index 9994a65a..38bc7865 100644 --- a/server/mergin/auth/controller.py +++ b/server/mergin/auth/controller.py @@ -16,7 +16,9 @@ auth_required, authenticate, send_confirmation_email, + send_password_reset_email, confirm_token, + confirm_password_reset_token, generate_confirmation_token, confirm_unlock_token, user_created, @@ -380,19 +382,13 @@ def password_reset(): # pylint: disable=W0613,W0612 target_email=form.email.data.strip(), ) if user and user.active and user.can_edit_profile: - send_confirmation_email( - current_app, - user, - "change-password", - "email/password_reset.html", - "Password reset", - ) + send_password_reset_email(current_app, user) return "", 200 def confirm_new_password(token): # pylint: disable=W0613,W0612 - email = confirm_token(token, salt=current_app.config["SECURITY_PASSWORD_SALT"]) - if not email: + payload = confirm_password_reset_token(token) + if not payload: emit( AuthEventType.USER_PASSWORD_RESET_FAILED, **request_context(), @@ -402,6 +398,7 @@ def confirm_new_password(token): # pylint: disable=W0613,W0612 ) abort(400, "Invalid token") + email = payload["email"] user = User.query.filter_by(email=email).first() if not user: emit( @@ -412,6 +409,16 @@ def confirm_new_password(token): # pylint: disable=W0613,W0612 reason="user_not_found", ) abort(404) + # token was already used, superseded by a newer one or revoked by a password change + if user.password_reset_nonce != payload["nonce"]: + emit( + AuthEventType.USER_PASSWORD_RESET_FAILED, + **request_context(), + target_user_id=user.id, + target_email=user.email, + reason="token_revoked", + ) + abort(400, "Invalid token") if not user.active: emit( AuthEventType.USER_PASSWORD_RESET_FAILED, @@ -433,6 +440,7 @@ def confirm_new_password(token): # pylint: disable=W0613,W0612 form = UserPasswordForm.from_json(request.json) if form.validate(): + # also clears the reset nonce, making the token single-use user.assign_password(form.password.data) user.reset_lockout() db.session.add(user) diff --git a/server/mergin/auth/listeners.py b/server/mergin/auth/listeners.py index bd1444d6..6f4d3172 100644 --- a/server/mergin/auth/listeners.py +++ b/server/mergin/auth/listeners.py @@ -13,7 +13,7 @@ from .models import User # Fields excluded from user.updated audit events: -# - sensitive values that must never appear in logs (passwd) +# - sensitive values that must never appear in logs (passwd, password_reset_nonce) # - high-frequency operational fields (last_signed_in, registration_date) # - lifecycle state fields covered by dedicated events (active, inactive_since) # - is_admin covered by the dedicated user.admin_panel_access.changed event @@ -22,6 +22,7 @@ _EXCLUDED_FROM_USER_UPDATED = frozenset( { "passwd", + "password_reset_nonce", "last_signed_in", "registration_date", "active", diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index e160dde5..96decb37 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -51,6 +51,8 @@ class User(db.Model): ) last_signed_in = db.Column(db.DateTime(), nullable=True) locked_until = db.Column(db.DateTime(), nullable=True) + # nonce of the only valid password reset token, cleared on any password change + password_reset_nonce = db.Column(db.String(64), nullable=True) receive_notifications = db.Column( db.Boolean, default=True, nullable=False, index=True ) @@ -89,6 +91,7 @@ def assign_password(self, password): if password else None ) + self.password_reset_nonce = None def needs_rehash(self): """Return True if the stored hash was generated with a different cost factor than configured.""" @@ -267,6 +270,7 @@ def anonymize(self): self.username = del_str self.email = None self.passwd = None + self.password_reset_nonce = None self.first_name = None self.last_name = None db.session.commit() diff --git a/server/mergin/tests/test_audit_events.py b/server/mergin/tests/test_audit_events.py index 4c6b9f13..eb4a5044 100644 --- a/server/mergin/tests/test_audit_events.py +++ b/server/mergin/tests/test_audit_events.py @@ -12,7 +12,10 @@ from flask import current_app from ..app import db -from ..auth.app import generate_confirmation_token, generate_unlock_token +from ..auth.app import ( + generate_password_reset_token, + generate_unlock_token, +) from ..auth.events import AuthEventType from ..auth.models import User from ..sync.events import SyncEventType @@ -82,9 +85,10 @@ def test_user_password_changed(client, audit_capture): def test_user_password_reset(app, client, audit_capture): user = User.query.filter_by(username=DEFAULT_USER[0]).first() - token = generate_confirmation_token( - app, user.email, app.config["SECURITY_PASSWORD_SALT"] - ) + token = generate_password_reset_token(app, user) + db.session.commit() + # the nonce is a secret - storing it must not emit user.updated + assert len(audit_capture.of_type(AuthEventType.USER_UPDATED)) == 0 client.post( f"/app/auth/reset-password/{token}", @@ -94,6 +98,16 @@ def test_user_password_reset(app, client, audit_capture): e = audit_capture.one(AuthEventType.USER_PASSWORD_RESET_COMPLETED) assert e.target_user_id == user.id assert e.metadata["target_email"] == user.email + assert len(audit_capture.of_type(AuthEventType.USER_UPDATED)) == 0 + + # reusing the token is rejected + client.post( + f"/app/auth/reset-password/{token}", + json={"password": "NewPass#456", "confirm": "NewPass#456"}, + ) + e = audit_capture.one(AuthEventType.USER_PASSWORD_RESET_FAILED) + assert e.target_user_id == user.id + assert e.metadata["reason"] == "token_revoked" def test_user_created(audit_capture): diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index 6bb3d934..815da3c1 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -17,6 +17,7 @@ from ..auth.app import ( generate_confirmation_token, confirm_token, + generate_password_reset_token, generate_unlock_token, ) from ..auth.models import User, LoginHistory @@ -578,9 +579,8 @@ def test_confirm_email(app, client): def test_confirm_password(app, client): user = User.query.filter_by(username="mergin").first() - token = generate_confirmation_token( - app, user.email, app.config["SECURITY_PASSWORD_SALT"] - ) + token = generate_password_reset_token(app, user) + db.session.commit() form_data = {"password": "ilovemergin#0", "confirm": "ilovemergin#0"} @@ -607,8 +607,8 @@ def test_confirm_password(app, client): resp = client.post( url_for( "/.mergin_auth_controller_confirm_new_password", - token=generate_confirmation_token( - app, "tests@mergin.com", app.config["SECURITY_PASSWORD_SALT"] + token=generate_password_reset_token( + app, SimpleNamespace(email="tests@mergin.com") ), ), data=json.dumps(form_data), @@ -622,14 +622,10 @@ def test_confirm_password(app, client): ) user.active = False db.session.add(user) + token = generate_password_reset_token(app, user) db.session.commit() resp = client.post( - url_for( - "/.mergin_auth_controller_confirm_new_password", - token=generate_confirmation_token( - app, "tests@mergin.com", app.config["SECURITY_PASSWORD_SALT"] - ), - ) + url_for("/.mergin_auth_controller_confirm_new_password", token=token) ) assert resp.status_code == 400 @@ -648,9 +644,8 @@ def test_confirm_password_clears_lockout(send_email_mock, app, client): ) assert user.is_locked_out() - token = generate_confirmation_token( - app, user.email, app.config["SECURITY_PASSWORD_SALT"] - ) + token = generate_password_reset_token(app, user) + db.session.commit() resp = client.post( url_for("/.mergin_auth_controller_confirm_new_password", token=token), data=json.dumps({"password": "newpass#1", "confirm": "newpass#1"}), @@ -678,6 +673,74 @@ def test_confirm_password_clears_lockout(send_email_mock, app, client): assert not user.is_locked_out() +def _confirm_new_password(client, token, password="newpass#1"): + return client.post( + url_for("/.mergin_auth_controller_confirm_new_password", token=token), + data=json.dumps({"password": password, "confirm": password}), + headers=json_headers, + ) + + +@patch("mergin.celery.send_email_async.apply_async") +def test_reset_password_token_single_use(send_email_mock, app, client): + """Reset link is one-off, only the latest link is valid and a regular password change revokes it.""" + user = add_user("resetuser", "oldpassword") + + # used token cannot be reused + token = generate_password_reset_token(app, user) + db.session.commit() + assert _confirm_new_password(client, token).status_code == 200 + assert user.password_reset_nonce is None + assert _confirm_new_password(client, token, "another#1").status_code == 400 + assert user.check_password("newpass#1") + + # requesting a new link revokes the older one + old_token = generate_password_reset_token(app, user) + db.session.commit() + resp = client.post( + url_for("/.mergin_auth_controller_password_reset"), + json={"email": user.email}, + ) + assert resp.status_code == 200 + assert send_email_mock.call_count == 1 + email_html = send_email_mock.call_args.args[1]["html"] + assert old_token not in email_html + assert _confirm_new_password(client, old_token).status_code == 400 + new_token = email_html.split("change-password/")[1].split('"')[0] + assert _confirm_new_password(client, new_token, "latest#1").status_code == 200 + + # regular password change revokes outstanding link + token = generate_password_reset_token(app, user) + db.session.commit() + login(client, user.username, "latest#1") + resp = client.post( + url_for("/.mergin_auth_controller_change_password"), + json={ + "old_password": "latest#1", + "password": "changed#1", + "confirm": "changed#1", + }, + ) + assert resp.status_code == 200 + assert _confirm_new_password(client, token).status_code == 400 + assert user.check_password("changed#1") + + +def test_reset_password_token_expired_or_legacy(app, client): + user = add_user("resetuser", "oldpassword") + token = generate_password_reset_token(app, user) + db.session.commit() + with patch.dict(app.config, {"PASSWORD_RESET_TOKEN_EXPIRATION": -1}): + assert _confirm_new_password(client, token).status_code == 400 + + # token format from before nonces were introduced (email only) is rejected + legacy_token = generate_confirmation_token( + app, user.email, app.config["SECURITY_PASSWORD_SALT"] + ) + assert _confirm_new_password(client, legacy_token).status_code == 400 + assert user.check_password("oldpassword") + + # reset password tests: success, no email, not-existing user (200 - masked) test_reset_data = [ ({"email": "mergin@mergin.com"}, 200), diff --git a/server/migrations/community/c4e7b1d9a2f3_add_password_reset_nonce.py b/server/migrations/community/c4e7b1d9a2f3_add_password_reset_nonce.py new file mode 100644 index 00000000..48e77cd1 --- /dev/null +++ b/server/migrations/community/c4e7b1d9a2f3_add_password_reset_nonce.py @@ -0,0 +1,28 @@ +"""Add password_reset_nonce to user table + +Revision ID: c4e7b1d9a2f3 +Revises: 7a095270b252 +Create Date: 2026-09-29 00:00:00.000000 + +""" + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = "c4e7b1d9a2f3" +down_revision = "7a095270b252" +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column( + "user", + sa.Column("password_reset_nonce", sa.String(64), nullable=True), + ) + + +def downgrade(): + op.drop_column("user", "password_reset_nonce")