diff --git a/hushline/settings/__init__.py b/hushline/settings/__init__.py index 834610fd..d8918151 100644 --- a/hushline/settings/__init__.py +++ b/hushline/settings/__init__.py @@ -1,6 +1,7 @@ import asyncio import base64 import io +from hmac import compare_digest as bytes_are_equal import aiohttp import pyotp @@ -366,22 +367,25 @@ def change_password() -> str | Response: change_password_form = ChangePasswordForm(request.form) if not change_password_form.validate_on_submit(): - flash("New password is invalid.") + flash("⛔️ Invalid form data. Please try again.", "error") return redirect(url_for("settings.index")) - if not change_password_form.old_password.data or not user.check_password( - change_password_form.old_password.data + if not user.check_password(change_password_form.old_password.data): + flash("⛔️ Incorrect old password.", "error") + return redirect(url_for("settings.index")) + + # SECURITY: only check equality after successful old password check + if bytes_are_equal( + change_password_form.old_password.data.encode(), + change_password_form.new_password.data.encode(), ): - flash("Incorrect old password.", "error") + flash("⛔️ Cannot choose a repeat password.", "error") return redirect(url_for("settings.index")) user.password_hash = change_password_form.new_password.data db.session.commit() session.clear() - flash( - "👍 Password successfully changed. Please log in again.", - "success", - ) + flash("👍 Password successfully changed. Please log in again.", "success") return redirect(url_for("login")) @bp.route("/enable-2fa", methods=["GET", "POST"]) diff --git a/tests/test_settings.py b/tests/test_settings.py index 8bed9770..81a56d47 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -60,53 +60,56 @@ def test_change_username(client: FlaskClient, user: User) -> None: @pytest.mark.usefixtures("_authenticated_user") def test_change_password(client: FlaskClient, user: User, user_password: str) -> None: - new_password = user_password + "xxx" - assert len(original_password_hash := user.password_hash) > 32 assert original_password_hash.startswith("$scrypt$") assert user_password not in original_password_hash - response = client.post( - url_for("settings.change_password"), - data={ - "old_password": user_password, - "new_password": new_password, - }, - follow_redirects=True, - ) - assert response.status_code == 200 - assert "Password successfully changed. Please log in again." in response.text - assert "/login" in response.request.url - assert len(new_password_hash := user.password_hash) > 32 - assert new_password_hash.startswith("$scrypt$") - assert original_password_hash not in new_password_hash - assert user_password not in new_password_hash - assert new_password not in new_password_hash + url = url_for("settings.change_password") + for new_password in [user_password, "", "aB!!", "aB3!", (33 * "aB3!")[:129], 5 * "aB3!"]: + data = dict(old_password=user_password, new_password=new_password) + response = client.post(url, data=data, follow_redirects=True) + if ( + user_password != new_password + and 17 < len(user_password) < 129 + and 17 < len(new_password) < 129 + ): + assert response.status_code == 200, data + assert "Password successfully changed. Please log in again." in response.text, data + assert "/login" in response.request.url, data + assert len(new_password_hash := user.password_hash) > 32, data + assert new_password_hash.startswith("$scrypt$"), data + assert original_password_hash not in new_password_hash, data + assert user_password not in new_password_hash, data + assert new_password not in new_password_hash, data + elif user_password == new_password: + assert "Cannot choose a repeat password." in response.text, data + assert "/settings" in response.request.url, data + assert original_password_hash == user.password_hash, data + else: + assert "Invalid form data. Please try again." in response.text, data + assert "/settings" in response.request.url, data + assert original_password_hash == user.password_hash, data + + assert original_password_hash != user.password_hash # TODO simulate a log out? # Attempt to log in with the registered user's old password - response = client.post( - url_for("login"), - data={"username": user.primary_username.username, "password": user_password}, - follow_redirects=True, - ) - assert response.status_code == 200 - assert "Invalid username or password" in response.text - assert "/login" in response.request.url + data = dict(username=user.primary_username.username, password=user_password) + response = client.post(url_for("login"), data=data, follow_redirects=True) + assert response.status_code == 200, data + assert "Invalid username or password" in response.text, data + assert "/login" in response.request.url, data # TODO simulate a log out? # Attempt to log in with the registered user's new password - response = client.post( - url_for("login"), - data={"username": user.primary_username.username, "password": new_password}, - follow_redirects=True, - ) - assert response.status_code == 200 - assert "Empty Inbox" in response.text - assert "Invalid username or password" not in response.text - assert "/inbox" in response.request.url + data = dict(username=user.primary_username.username, password=new_password) + response = client.post(url_for("login"), data=data, follow_redirects=True) + assert response.status_code == 200, data + assert "Empty Inbox" in response.text, data + assert "Invalid username or password" not in response.text, data + assert "/inbox" in response.request.url, data @pytest.mark.usefixtures("_authenticated_user")