diff --git a/app/controllers/login.py b/app/controllers/login.py index 265918f..5cff3a2 100644 --- a/app/controllers/login.py +++ b/app/controllers/login.py @@ -19,6 +19,7 @@ def clear_main_auth(): """Clear main site auth session keys only.""" session.pop('name', None) session.pop('email', None) + session.pop('session_version', None) session.pop('login_attempt', None) @@ -87,6 +88,7 @@ def login_post(): clear_main_auth() session['email'] = user['email'] session['name'] = user['name'] + session['session_version'] = user.get('session_version', 0) flash('Login successful!', FLASH_SUCCESS) # Retrieve and clear the redirect from session diff --git a/app/controllers/password.py b/app/controllers/password.py index c4025d9..4cd1278 100644 --- a/app/controllers/password.py +++ b/app/controllers/password.py @@ -60,7 +60,8 @@ def change_password_post(): return 'Could not hash new password', 500 try: - update_user_password(username, hashed_new_password) + # keep *this* browser logged in; other sessions die with the bump + session['session_version'] = update_user_password(username, hashed_new_password) except Exception as e: print(f"Error updating user password: {e}") return 'Could not update password', 500 diff --git a/app/models/user.py b/app/models/user.py index c502e80..1311920 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -153,6 +153,7 @@ def user_create(name, email, password): 'name': name, 'email': email, 'password': password, + 'session_version': 0, 'visible': True, 'deleted': False } @@ -161,12 +162,19 @@ def user_create(name, email, password): def update_user_password(username, hashed_password): - """Update user password. + """Update user password and bump session_version. + + Bumping session_version invalidates every existing signed cookie session + for this user. Callers that want to keep the current browser logged in + must refresh session['session_version'] after this returns. Args: username: The username hashed_password: The new hashed password + Returns: + The new session_version value + Raises: ValueError: If username or password is empty ErrNoResult: If no user found @@ -178,14 +186,21 @@ def update_user_password(username, hashed_password): collection = get_collection('user') - result = collection.update_one( + # yes, $inc on a missing field starts at 0 then becomes 1. old accounts work. + result = collection.find_one_and_update( {'name': username}, - {'$set': {'password': hashed_password}} + { + '$set': {'password': hashed_password}, + '$inc': {'session_version': 1}, + }, + return_document=True, ) - if result.matched_count == 0: + if result is None: raise ErrNoResult("No user found with the provided username") + return result.get('session_version', 1) + def user_get_unread_notifications(username): """Get the count of unread notifications for a user. diff --git a/app/services/session.py b/app/services/session.py index a4c8c90..6c13b12 100644 --- a/app/services/session.py +++ b/app/services/session.py @@ -5,11 +5,36 @@ from flask import session as flask_session +SESSION_VERSION_KEY = 'session_version' + + def init_session(app, config): """Initialize session configuration.""" app.config['SESSION_COOKIE_NAME'] = config.get('CookieName', 'crackmesone') app.config['SECRET_KEY'] = config.get('SecretKey', 'change-me-in-production') - # Session options can be extended here + + @app.before_request + def validate_session_version(): + """Drop auth cookies that predate a password change/reset.""" + username = flask_session.get('name') + if not username: + return + + from app.models.errors import ErrNoResult, ErrUnavailable + from app.models.user import user_by_name + + try: + user = user_by_name(username) + except ErrNoResult: + clear_session() + return + except ErrUnavailable: + # don't log people out because Mongo hiccuped + return + + expected = user.get('session_version', 0) + if flask_session.get(SESSION_VERSION_KEY, 0) != expected: + clear_session() def get_session(): @@ -21,6 +46,7 @@ def clear_session(): """Clear main site auth session keys only.""" flask_session.pop('name', None) flask_session.pop('email', None) + flask_session.pop(SESSION_VERSION_KEY, None) flask_session.pop('login_attempt', None) diff --git a/review/routes.py b/review/routes.py index c0784b5..d339e22 100644 --- a/review/routes.py +++ b/review/routes.py @@ -1079,7 +1079,10 @@ def reset_user_password(user_email): result = g_crackmesone_db.user.update_one( {"email": {"$regex": f"^{escaped}$", "$options": "i"}}, - {"$set": {"password": hashed.decode('utf-8')}} + { + "$set": {"password": hashed.decode('utf-8')}, + "$inc": {"session_version": 1}, + }, ) if result.modified_count != 1: diff --git a/tests/conftest.py b/tests/conftest.py index 1d7b8b0..d17b6e0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -114,6 +114,7 @@ def _authenticate(client, user): with client.session_transaction() as session: session['name'] = user['name'] session['email'] = user['email'] + session['session_version'] = user.get('session_version', 0) return client diff --git a/tests/test_models_extended.py b/tests/test_models_extended.py index bb8b962..0df4572 100644 --- a/tests/test_models_extended.py +++ b/tests/test_models_extended.py @@ -149,7 +149,9 @@ def test_user_visibility_counts_password_and_notification_helpers(app, db, alice user_decrement_unread_notifications('alice', 20) assert user_get_unread_notifications('alice') == 0 update_user_password('alice', 'new-hash') - assert db.user.find_one({'name': 'alice'})['password'] == 'new-hash' + alice_after = db.user.find_one({'name': 'alice'}) + assert alice_after['password'] == 'new-hash' + assert alice_after['session_version'] == 1 def test_expired_password_reset_cleanup(app, db): diff --git a/tests/test_password_reset.py b/tests/test_password_reset.py index b37691a..30ad78f 100644 --- a/tests/test_password_reset.py +++ b/tests/test_password_reset.py @@ -53,6 +53,34 @@ def test_valid_reset_token_changes_password_and_is_single_use(client, db, alice) assert client.get(f'/reset-password/{token}').status_code == 302 +def test_password_reset_invalidates_existing_sessions(app, db, alice): + """Stolen cookies should stop working after a successful reset.""" + victim = app.test_client() + with victim.session_transaction() as session: + session['name'] = alice['name'] + session['email'] = alice['email'] + session['session_version'] = alice.get('session_version', 0) + + assert victim.get('/change-password').status_code == 200 + + token = create_reset_token('alice@example.test') + attacker = app.test_client() + with patch('app.controllers.password_reset.notify_password_reset_complete'): + reset = attacker.post(f'/reset-password/{token}', data={ + 'new_password': 'replacement-password', + 'new_password_verify': 'replacement-password', + }) + assert reset.status_code == 302 + assert db.user.find_one({'name': 'alice'})['session_version'] == 1 + + denied = victim.get('/change-password') + assert denied.status_code == 302 + assert denied.location == '/' + with victim.session_transaction() as session: + assert 'name' not in session + assert 'session_version' not in session + + def test_expired_reset_token_is_rejected(client, db, alice): db.password_reset_tokens.insert_one({ 'email': 'alice@example.test', diff --git a/tests/test_routes.py b/tests/test_routes.py index 9c442c5..3d24131 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -46,6 +46,7 @@ def test_login_with_valid_synthetic_user(client, alice): with client.session_transaction() as session: assert session['name'] == 'alice' assert session['email'] == 'alice@example.test' + assert session['session_version'] == 0 def test_login_with_wrong_password_does_not_authenticate(client, alice):