Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure new passwords are distinct #468

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions hushline/forms.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
import re
from hmac import compare_digest as bytes_are_equal
from typing import Callable

from flask_wtf import FlaskForm
from wtforms import Field, Form, StringField
from wtforms.validators import DataRequired, Length, ValidationError


class WrongPassword(ValueError):
pass


class ComplexPassword:
def __init__(self, message: str | None = None) -> None:
if not message:
Expand All @@ -25,5 +31,15 @@ def __call__(self, form: Form, field: Field) -> None:
raise ValidationError(self.message)


def is_valid_password_swap(
*, check_password: Callable[[str], bool], old_password: str, new_password: str
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love passing a callable here, is the goal to keep all password validation logic in one part of the code?

) -> bool:
# since the passwords can be of different lengths, the equality test must occur *iif*
# the correctness test passes, since timing differences leak length information
if check_password(old_password):
return not bytes_are_equal(old_password.encode(), new_password.encode())
raise WrongPassword
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raising an error isn't really consistent with the existing code style. I am open to refactoring the flow control to be error-based, but I'd prefer that as a separate PR.



class TwoFactorForm(FlaskForm):
verification_code = StringField("2FA Code", validators=[DataRequired(), Length(min=6, max=6)])
60 changes: 34 additions & 26 deletions hushline/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

from .crypto import is_valid_pgp_key
from .db import db
from .forms import ComplexPassword, TwoFactorForm
from .forms import ComplexPassword, TwoFactorForm, WrongPassword, is_valid_password_swap
from .model import Message, SecondaryUsername, User
from .utils import require_2fa

Expand Down Expand Up @@ -176,12 +176,20 @@ def index() -> str | Response: # noqa: PLR0911, PLR0912

# Handle Change Password Form Submission
Copy link
Contributor Author

@rmlibre rmlibre Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this section needed if the change_password route function exists? I tried removing this section to see if the tests would pass, and they do. I'm not sure if that's because this isn't needed, or just because it isn't tested.

Probably better to refactor these areas so that there's only one implementation of each flow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it looks like only changes to display name, username, directory visibility, and bio are handled via the /settings route, while 2fa, password changes, smtp settings, pgp settings, account deletion, verified user and admin user settings are handled by dedicated routes.

if change_password_form.validate_on_submit():
if user.check_password(change_password_form.old_password.data):
user.password_hash = change_password_form.new_password.data
db.session.commit()
flash("👍 Password changed successfully.")
else:
flash("⛔️ Incorrect old password.")
try:
if is_valid_password_swap(
check_password=user.check_password,
old_password=change_password_form.old_password.data,
new_password=change_password_form.new_password.data,
):
user.password_hash = change_password_form.new_password.data
db.session.commit()
flash("👍 Password successfully changed. Please log in again.", "success")
# Redirect to the login page for re-authentication
return redirect(url_for("login"))
flash("⛔️ New password is invalid.", "error")
except WrongPassword:
flash("⛔️ Incorrect old password.", "error")
return redirect(url_for("settings"))

# Check if user is admin and add admin-specific data
Expand Down Expand Up @@ -254,25 +262,25 @@ def change_password() -> str | Response:
flash("User not found.", "error")
return redirect(url_for("login"))

change_password_form = ChangePasswordForm(request.form)
if not change_password_form.validate_on_submit():
flash("New password is invalid.")
return redirect(url_for("settings.index"))

# Verify the old password
if not user.check_password(change_password_form.old_password.data):
flash("Incorrect old password.", "error")
return redirect(url_for("settings.index"))

# Set the new password
user.password_hash = change_password_form.new_password.data
db.session.commit()
session.clear() # Clears the session, logging the user out
flash(
"👍 Password successfully changed. Please log in again.",
"success",
)
return redirect(url_for("login")) # Redirect to the login page for re-authentication
# Validate the password swap
try:
change_password_form = ChangePasswordForm(request.form)
if change_password_form.validate_on_submit() and is_valid_password_swap(
check_password=user.check_password,
old_password=change_password_form.old_password.data,
new_password=change_password_form.new_password.data,
):
# Set the new password
user.password_hash = change_password_form.new_password.data
db.session.commit()
session.clear() # Clears the session, logging the user out
flash("👍 Password successfully changed. Please log in again.", "success")
# Redirect to the login page for re-authentication
return redirect(url_for("login"))
flash("⛔️ New password is invalid.", "error")
except WrongPassword:
flash("⛔️ Incorrect old password.", "error")
return redirect(url_for("settings.index"))

@bp.route("/change-username", methods=["POST"])
@require_2fa
Expand Down
14 changes: 14 additions & 0 deletions tests/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,20 @@ def test_change_password(client: FlaskClient) -> None:
assert logged_in_user is not None
assert user.id == logged_in_user.id

# Submit POST request to verify new password must be distinct from the previous
response = client.post(
"/settings/change-password",
data={
"old_password": original_password,
"new_password": original_password,
},
follow_redirects=True,
)
assert response.status_code == 200
assert "settings" in response.request.url
assert b"New password is invalid" in response.data
assert b"Password successfully changed" not in response.data

# Submit POST request to change the username & verify update was successful
response = client.post(
"/settings/change-password",
Expand Down
Loading