From 2d9e29c20cdb6ea8a9cd4f735d86695c31495b35 Mon Sep 17 00:00:00 2001 From: Xusheng Date: Sun, 26 Jul 2026 09:04:20 -0400 Subject: [PATCH 1/2] Let users and reviewers rename crackmes; users change username/email (#176) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Usernames and crackme names were used as mutable cross-collection keys, so they could never be changed without orphaning content. This keeps the denormalized copies (they make listings query-cheap) but makes them mutable, cascading every change across all referencing collections. - Rename crackme: crackme_update now accepts `name` and cascades the copies on comment/solution/label_request via cascade_crackme_name. Owner edit form and the reviewer edit tool both expose the name field. - Rename username: user_rename cascades across the 8 username-referencing collections (USERNAME_REFERENCES). References are updated first and the user document last, so a mid-cascade failure can't lock the user out. - Change email: user_change_email updates the user doc, refreshes the copy on a pending deletion request, and drops stale reset tokens. - New /settings page for changing username/email, each gated by current-password and full registration-style uniqueness cross-checks (name/email can't collide with any other account's name or email). Reviewer identities (reviewed_by) are deliberately not cascaded — reviewers authenticate via review/users.json, a separate namespace. Pending crackme/writeup review queues read the author live from the DB, so a rename between submit and approval stays consistent. Known limitation: usernames/crackme names embedded in historical notification text keep their old literal value (functional references all stay correct). Co-Authored-By: Claude Opus 4.8 (1M context) --- app/controllers/__init__.py | 2 + app/controllers/account_settings.py | 188 ++++++++++++++ app/controllers/crackme.py | 28 +- app/models/crackme.py | 50 +++- app/models/user.py | 123 +++++++++ review/routes.py | 23 +- review/templates/reviewer/editcrackme.html | 4 +- templates/crackme/edit.html | 10 +- templates/user/read.html | 3 +- templates/user/settings.html | 62 +++++ tests/test_account_settings.py | 285 +++++++++++++++++++++ tests/test_comments_and_crackmes.py | 8 +- tests/test_labels.py | 2 +- tests/test_reviewer_remaining.py | 43 ++++ 14 files changed, 813 insertions(+), 18 deletions(-) create mode 100644 app/controllers/account_settings.py create mode 100644 templates/user/settings.html create mode 100644 tests/test_account_settings.py diff --git a/app/controllers/__init__.py b/app/controllers/__init__.py index 690bfbd..7be45c9 100644 --- a/app/controllers/__init__.py +++ b/app/controllers/__init__.py @@ -19,6 +19,7 @@ def register_blueprints(app): from app.controllers.rating import rating_bp from app.controllers.rules import rules_bp from app.controllers.password import password_bp + from app.controllers.account_settings import account_settings_bp from app.controllers.password_reset import password_reset_bp from app.controllers.account_deletion import account_deletion_bp from app.controllers.static import static_bp @@ -38,6 +39,7 @@ def register_blueprints(app): app.register_blueprint(rating_bp) app.register_blueprint(rules_bp) app.register_blueprint(password_bp) + app.register_blueprint(account_settings_bp) app.register_blueprint(password_reset_bp) app.register_blueprint(account_deletion_bp) app.register_blueprint(static_bp) diff --git a/app/controllers/account_settings.py b/app/controllers/account_settings.py new file mode 100644 index 0000000..b9ec2a9 --- /dev/null +++ b/app/controllers/account_settings.py @@ -0,0 +1,188 @@ +""" +Account settings controller - change username and email. + +Usernames and emails are denormalized across many collections, so both changes +route through dedicated model helpers (``user_rename`` / ``user_change_email``) +that cascade the update. A change is only applied after the new value is proven +free (not used by any other account, as either a name or an email) and the +current password is confirmed. +""" + +from flask import Blueprint, render_template, request, redirect, flash, session +from app.models.user import ( + user_by_name, user_by_mail, user_rename, user_change_email +) +from app.models.errors import ErrNoResult +from app.services.passhash import match_string +from app.services.limiter import limit +from app.services.view import FLASH_ERROR, FLASH_SUCCESS, authorized_chars_only +from app.controllers.decorators import login_required + +account_settings_bp = Blueprint('account_settings', __name__) + + +def _name_available(candidate, current_id): + """True if ``candidate`` is free to use as a username. + + Mirrors registration's cross-checks: it must not be another user's name and + must not be any user's email. A match on the current user's own account + (e.g. a case-only change) is allowed. + """ + try: + existing = user_by_name(candidate) + if existing.get('_id') != current_id: + return False + except ErrNoResult: + pass + + try: + user_by_mail(candidate) + return False # Taken as some user's email. + except ErrNoResult: + pass + + return True + + +def _email_available(candidate, current_id): + """True if ``candidate`` is free to use as an email. + + Must not be another user's email and must not be any user's username. + """ + try: + existing = user_by_mail(candidate) + if existing.get('_id') != current_id: + return False + except ErrNoResult: + pass + + try: + user_by_name(candidate) + return False # Taken as some user's username. + except ErrNoResult: + pass + + return True + + +@account_settings_bp.route('/settings', methods=['GET']) +@login_required +def settings_get(): + """Display the account settings page.""" + username = session.get('name') + try: + user = user_by_name(username) + except ErrNoResult: + # Session references a user that no longer exists; force re-login. + return redirect('/logout') + + return render_template('user/settings.html', + current_username=user['name'], + current_email=user['email']) + + +@account_settings_bp.route('/settings/username', methods=['POST']) +@login_required +@limit("5 per hour", key_func=lambda: session.get('name')) +def change_username_post(): + """Handle a username change, cascading it across all references.""" + username = session.get('name') + new_name = request.form.get('name', '').strip() + password = request.form.get('current_password', '') + + try: + user = user_by_name(username) + except ErrNoResult: + return redirect('/logout') + + if not new_name: + flash('Username cannot be empty.', FLASH_ERROR) + return redirect('/settings') + + if not authorized_chars_only(new_name): + flash('Username contains disallowed characters.', FLASH_ERROR) + return redirect('/settings') + + if not match_string(user['password'], password): + flash('Current password is incorrect.', FLASH_ERROR) + return redirect('/settings') + + if new_name == user['name']: + flash('That is already your username.', FLASH_ERROR) + return redirect('/settings') + + try: + available = _name_available(new_name, user.get('_id')) + except Exception as e: + print(f"Username availability check error: {e}") + flash('An error occurred. Please try again later.', FLASH_ERROR) + return redirect('/settings') + + if not available: + flash(f'The username "{new_name}" is not available.', FLASH_ERROR) + return redirect('/settings') + + try: + user_rename(user['name'], new_name) + except Exception as e: + print(f"Username change error: {e}") + flash('An error occurred. Please try again later.', FLASH_ERROR) + return redirect('/settings') + + # Keep the session in step with the renamed account. + session['name'] = new_name + flash('Username updated successfully!', FLASH_SUCCESS) + return redirect('/settings') + + +@account_settings_bp.route('/settings/email', methods=['POST']) +@login_required +@limit("5 per hour", key_func=lambda: session.get('name')) +def change_email_post(): + """Handle an email change, refreshing denormalized copies.""" + username = session.get('name') + new_email = request.form.get('email', '').strip().lower() + password = request.form.get('current_password', '') + + try: + user = user_by_name(username) + except ErrNoResult: + return redirect('/logout') + + if not new_email: + flash('Email cannot be empty.', FLASH_ERROR) + return redirect('/settings') + + if not authorized_chars_only(new_email): + flash('Email contains disallowed characters.', FLASH_ERROR) + return redirect('/settings') + + if not match_string(user['password'], password): + flash('Current password is incorrect.', FLASH_ERROR) + return redirect('/settings') + + if new_email == (user.get('email') or '').lower(): + flash('That is already your email.', FLASH_ERROR) + return redirect('/settings') + + try: + available = _email_available(new_email, user.get('_id')) + except Exception as e: + print(f"Email availability check error: {e}") + flash('An error occurred. Please try again later.', FLASH_ERROR) + return redirect('/settings') + + if not available: + flash(f'The email "{new_email}" is not available.', FLASH_ERROR) + return redirect('/settings') + + try: + user_change_email(user['name'], new_email) + except Exception as e: + print(f"Email change error: {e}") + flash('An error occurred. Please try again later.', FLASH_ERROR) + return redirect('/settings') + + session['email'] = new_email + flash('Email updated successfully!', FLASH_SUCCESS) + return redirect('/settings') diff --git a/app/controllers/crackme.py b/app/controllers/crackme.py index 4675007..7cc8e21 100644 --- a/app/controllers/crackme.py +++ b/app/controllers/crackme.py @@ -11,7 +11,7 @@ crackme_by_hexid, last_crackmes, crackme_create_prepare, crackme_insert, crackme_delete_by_hexid, crackme_by_user_and_name, crackme_update_difficulty, crackme_update_quality, crackme_increment_downloads, - crackme_update + crackme_update, crackme_name_taken_by_user ) from app.models.solution import solutions_by_crackme from app.models.comment import comments_by_crackme @@ -158,12 +158,19 @@ def upload_crackme_post(): flash(f'Field missing: {missing}', FLASH_ERROR) return render_template('crackme/create.html', label_groups=get_label_groups()) - name = bleach.clean(request.form.get('name', '')) + # Strip so the stored name matches what the edit form later submits back + # (the edit path also strips); otherwise a later no-op edit would look like + # a rename and fire a spurious cascade. + name = bleach.clean(request.form.get('name', '')).strip() info = bleach.clean(request.form.get('info', '')) lang = bleach.clean(request.form.get('lang', '')) arch = bleach.clean(request.form.get('arch', '')) platform = request.form.get('platform', '') difficulty = request.form.get('difficulty', '') + + if not name: + flash('Field missing: name', FLASH_ERROR) + return render_template('crackme/create.html', label_groups=get_label_groups()) # Keep only values from the controlled vocabulary. Labels are not mandatory # (a crackme with no matching technique may legitimately have none). labels = normalize_labels(request.form.getlist('labels')) @@ -336,14 +343,27 @@ def edit_crackme_post(hexid): flash('You can only edit your own crackmes.', FLASH_ERROR) return redirect(f'/crackme/{hexid}') - # Get form data (name is not editable) + # Get form data + name = bleach.clean(request.form.get('name', '')).strip() info = bleach.clean(request.form.get('info', '')) lang = bleach.clean(request.form.get('lang', '')) arch = bleach.clean(request.form.get('arch', '')) platform = request.form.get('platform', '') + if not name: + flash('Crackme name cannot be empty.', FLASH_ERROR) + return redirect(f'/crackme/{hexid}/edit') + + # Renaming must not collide with another of the user's own crackmes, so that + # crackme_by_user_and_name lookups stay unambiguous. + if name != crackme.get('name') and \ + crackme_name_taken_by_user(username, name, exclude_hexid=hexid): + flash('You already have another crackme with that name.', FLASH_ERROR) + return redirect(f'/crackme/{hexid}/edit') + # Update the crackme updates = { + 'name': name, 'info': info, 'lang': lang, 'arch': arch, @@ -362,7 +382,7 @@ def edit_crackme_post(hexid): if changes: # Send notification to the author about the edit try: - notification_add(username, f"Your crackme '{html_escape(crackme.get('name'))}' has been updated.") + notification_add(username, f"Your crackme '{html_escape(name)}' has been updated.") except Exception as e: print(f"Notification error: {e}") diff --git a/app/models/crackme.py b/app/models/crackme.py index 3780db0..cd9f3df 100644 --- a/app/models/crackme.py +++ b/app/models/crackme.py @@ -342,6 +342,46 @@ def random_crackmes(count=50): return list(collection.aggregate(pipeline)) +# Collections that keep a denormalized copy of a crackme's name, as +# (collection, hexid-match-field, name-field). Renaming a crackme must refresh +# every one of these so listings (e.g. a user's writeups, the label-request +# review queue) don't show the stale old name. +CRACKME_NAME_REFERENCES = ( + ('comment', 'crackmehexid', 'crackmename'), + ('solution', 'crackmehexid', 'crackmename'), + ('label_request', 'crackme_hexid', 'crackme_name'), +) + + +def cascade_crackme_name(hexid, new_name): + """Update every denormalized copy of a renamed crackme's name. + + Shared by the user-facing edit flow (via :func:`crackme_update`) and the + reviewer edit tool so both keep the copies in sync. + """ + for collection_name, match_field, name_field in CRACKME_NAME_REFERENCES: + get_collection(collection_name).update_many( + {match_field: hexid}, + {'$set': {name_field: new_name}} + ) + + +def crackme_name_taken_by_user(username, name, exclude_hexid=None): + """Return True if the user already has a (different) crackme with this name. + + Used to keep ``crackme_by_user_and_name`` lookups unambiguous when a user + renames one of their crackmes. Matches across all visibility states. + """ + if not check_connection(): + raise ErrUnavailable("Database is unavailable") + + collection = get_collection('crackme') + query = {'author': username, 'name': name} + if exclude_hexid: + query['hexid'] = {'$ne': exclude_hexid} + return collection.find_one(query, {'_id': 1}) is not None + + def crackme_update(hexid, updates): """Update a crackme's fields. @@ -351,12 +391,16 @@ def crackme_update(hexid, updates): Returns: Dictionary with old values of changed fields, or None if crackme not found + + When ``name`` changes, the denormalized copies of it on comments, solutions + and label requests are refreshed too (see :func:`_cascade_crackme_name`). """ if not check_connection(): raise ErrUnavailable("Database is unavailable") - # Only allow updating these fields (name is excluded to avoid database issues) - allowed_fields = {'info', 'lang', 'arch', 'platform', 'labels'} + # Fields a user may edit. ``name`` is now editable; any change to it cascades + # to the denormalized copies below. + allowed_fields = {'name', 'info', 'lang', 'arch', 'platform', 'labels'} filtered_updates = {k: v for k, v in updates.items() if k in allowed_fields} if not filtered_updates: @@ -381,6 +425,8 @@ def crackme_update(hexid, updates): {'hexid': hexid}, {'$set': filtered_updates} ) + if 'name' in changes: + cascade_crackme_name(hexid, changes['name']['new']) return changes diff --git a/app/models/user.py b/app/models/user.py index e84350f..5673b03 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -139,6 +139,129 @@ def user_create(name, email, password): collection.insert_one(user) +# Collections (and the field on each) that store a username as a reference to a +# user. The username is denormalized onto all of these, so renaming a user must +# cascade to every one of them or the user would silently lose ownership of their +# crackmes, comments, solutions, ratings, notifications, and pending requests. +# +# Reviewer identities (e.g. ``label_request.reviewed_by`` and +# ``account_deletion_request.reviewed_by``) are deliberately NOT included: +# reviewers authenticate through review/users.json, a namespace separate from the +# user collection, so their names are never main-site usernames. +USERNAME_REFERENCES = ( + ('crackme', 'author'), + ('comment', 'author'), + ('solution', 'author'), + ('rating_difficulty', 'author'), + ('rating_quality', 'author'), + ('notifications', 'user'), + ('label_request', 'requester'), + ('account_deletion_request', 'requester'), +) + + +def user_rename(old_name, new_name): + """Rename a user and update every stored reference to their old username. + + Because the username is denormalized across many collections (see + :data:`USERNAME_REFERENCES`), a rename must touch all of them so nothing + orphans — including content still awaiting review (a pending crackme or + writeup keeps working because the reviewer queue reads ``author`` from the + database at review time). + + Callers must validate that ``new_name`` is available first (see the + availability checks in the account-settings controller). ``old_name`` must + be the user's exact stored name. + + Returns: + The number of user documents renamed (1 on success). + + Raises: + ErrNoResult: If no user matches ``old_name``. + ErrUnavailable: If the database is unavailable. + """ + if not check_connection(): + raise ErrUnavailable("Database is unavailable") + + if old_name == new_name: + return 0 + + user_collection = get_collection('user') + if user_collection.find_one({'name': old_name}, {'_id': 1}) is None: + raise ErrNoResult("No user found with the provided username") + + # MongoDB has no cheap way to do this atomically without a replica-set + # transaction, so ordering is the safety net: cascade the references FIRST + # and flip the user document LAST. If a reference update fails partway, the + # user document still carries the old name, so the caller's session keeps + # resolving (no lockout) and re-running the rename converges. Renaming the + # user document first would risk exactly that lockout. + for collection_name, field in USERNAME_REFERENCES: + get_collection(collection_name).update_many( + {field: old_name}, + {'$set': {field: new_name}} + ) + + result = user_collection.update_one( + {'name': old_name}, + {'$set': {'name': new_name}} + ) + + return result.modified_count + + +def user_change_email(username, new_email): + """Change a user's email and refresh denormalized copies of it. + + The email also lives on any pending account-deletion request (kept there so + the confirmation email survives the user document being deleted), and it + keys password-reset tokens. Both are handled here: the deletion-request copy + is updated, and any reset tokens for the old address are dropped since the + user no longer controls it. + + Args: + username: The account's exact stored username. + new_email: The new email address (stored lower-cased, matching + registration). + + Returns: + The previous email address, or None if it was unset. + + Raises: + ErrNoResult: If no user matches ``username``. + ErrUnavailable: If the database is unavailable. + """ + if not check_connection(): + raise ErrUnavailable("Database is unavailable") + + new_email = new_email.lower() + user_collection = get_collection('user') + user = user_collection.find_one({'name': username}, {'email': 1}) + if not user: + raise ErrNoResult("No user found with the provided username") + + old_email = user.get('email') + if old_email == new_email: + return old_email + + user_collection.update_one({'name': username}, {'$set': {'email': new_email}}) + + # Keep the denormalized copy on any pending deletion request fresh. + get_collection('account_deletion_request').update_many( + {'requester': username}, + {'$set': {'email': new_email}} + ) + + # Reset tokens for the old address now point somewhere the user no longer + # owns; discard them so they can't be used against the wrong mailbox. + if old_email: + get_collection('password_reset_tokens').delete_many( + {'email': old_email.lower()} + ) + + return old_email + + def update_user_password(username, hashed_password): """Update user password. diff --git a/review/routes.py b/review/routes.py index eab0686..ddf6e9b 100644 --- a/review/routes.py +++ b/review/routes.py @@ -45,6 +45,7 @@ from app.services.crypto import get_obfuscation_salt from app.services.view import is_valid_hexid from app.services.labels import get_label_groups, normalize_labels +from app.models.crackme import cascade_crackme_name, crackme_name_taken_by_user from app.models.label_request import ( label_requests_pending, count_pending_label_requests, label_request_by_hexid, label_request_set_status, STATUS_APPROVED, STATUS_REJECTED, @@ -2071,7 +2072,8 @@ def editcrackme(current_user): if request.method == 'POST' and crackme_uuid: validate_csrf_token() - # Get form data (name is not editable) + # Get form data + name = request.form.get('name', '').strip() info = request.form.get('info', '').strip() lang = request.form.get('lang', '') arch = request.form.get('arch', '') @@ -2087,9 +2089,18 @@ def editcrackme(current_user): if not crackme_obj: error = "Crackme not found" + elif not name: + error = "Crackme name cannot be empty" + elif name != crackme_obj.get('name') and crackme_name_taken_by_user( + crackme_obj.get('author'), name, exclude_hexid=crackme_obj['hexid']): + # Keep crackme_by_user_and_name lookups unambiguous. + error = "The author already has another crackme with that name" else: - # Track changes (name is not editable) + # Track changes changes = [] + name_changed = crackme_obj.get('name') != name + if name_changed: + changes.append(f"name: '{crackme_obj.get('name')}' -> '{name}'") if crackme_obj.get('info') != info: changes.append("description updated") if crackme_obj.get('lang') != lang: @@ -2101,10 +2112,11 @@ def editcrackme(current_user): if sorted(crackme_obj.get('labels', [])) != sorted(labels): changes.append(f"labels: {crackme_obj.get('labels', [])} -> {labels}") - # Update the crackme (name excluded) + # Update the crackme g_crackmesone_db.crackme.update_one( {"_id": ObjectId(crackme_uuid)}, {"$set": { + "name": name, "info": info, "lang": lang, "arch": arch, @@ -2113,6 +2125,11 @@ def editcrackme(current_user): }} ) + # A renamed crackme has denormalized name copies on comments, + # solutions and label requests that must be refreshed too. + if name_changed: + cascade_crackme_name(crackme_obj['hexid'], name) + # Handle file replacement file_replaced = False if 'file' in request.files: diff --git a/review/templates/reviewer/editcrackme.html b/review/templates/reviewer/editcrackme.html index 0919b9e..0a603e0 100644 --- a/review/templates/reviewer/editcrackme.html +++ b/review/templates/reviewer/editcrackme.html @@ -42,8 +42,8 @@

Edit crackme: "{{ crackme.name }}" by {{ crackme.author }}

- - + +
diff --git a/templates/crackme/edit.html b/templates/crackme/edit.html index 6974bc2..d0ed4f3 100644 --- a/templates/crackme/edit.html +++ b/templates/crackme/edit.html @@ -6,11 +6,19 @@

Edit crackme: {{ crackme.name }}

-

Update the details of your crackme. Note: You cannot change the name or uploaded file.

+

Update the details of your crackme. Note: You cannot change the uploaded file.

+
+
+ +
+
+ +
+
diff --git a/templates/user/read.html b/templates/user/read.html index ebb31cd..e1458ed 100644 --- a/templates/user/read.html +++ b/templates/user/read.html @@ -173,7 +173,8 @@

Comments

{% if viewingOwnPage %}
- Change Password + Account Settings + Change Password Delete Account
{% endif %} diff --git a/templates/user/settings.html b/templates/user/settings.html new file mode 100644 index 0000000..fd7e041 --- /dev/null +++ b/templates/user/settings.html @@ -0,0 +1,62 @@ +{% extends "base.html" %} +{% block title %}Account Settings{% endblock %} +{% block page_title %}Account Settings{% endblock %} +{% block head %}{% endblock %} +{% block content %} + +
+
+
+

Change Username

+

Your username identifies you across your crackmes, writeups and comments. Changing it updates all of them.

+ + +
+
+ +
+
+ +
+
+
+
+ +
+
+ +
+
+ + + +
+ +

Change Email

+

Your email is used for password resets and account notifications. It is never shown publicly.

+
+ +
+
+ +
+
+ +
+
+
+
+ +
+
+ +
+
+ +
+
+
+
+ +{% endblock %} +{% block foot %}{% endblock %} diff --git a/tests/test_account_settings.py b/tests/test_account_settings.py new file mode 100644 index 0000000..fbfbd1f --- /dev/null +++ b/tests/test_account_settings.py @@ -0,0 +1,285 @@ +"""Tests for renaming users/crackmes and changing emails. + +Covers both the cascade model helpers and the account-settings / crackme-edit +controllers, including the "user renames while a writeup is pending review" +scenario. +""" + +from datetime import datetime, timezone + +import pytest +from bson import ObjectId + +from app.models.errors import ErrNoResult +from app.models.user import user_rename, user_change_email, user_by_name + + +def _seed_alice_content(db, author='alice'): + """Insert one document of every kind that references a username.""" + cid = ObjectId() + hexid = str(cid) + db.crackme.insert_one({ + '_id': cid, 'hexid': hexid, 'name': 'C1', 'author': author, + 'visible': True, 'deleted': False, + }) + db.comment.insert_one({ + 'author': author, 'crackmehexid': hexid, 'crackmename': 'C1', + 'info': 'nice', 'visible': True, 'created_at': datetime.now(timezone.utc), + }) + db.solution.insert_one({ + '_id': ObjectId(), 'hexid': str(ObjectId()), 'author': author, + 'crackmeid': cid, 'crackmehexid': hexid, 'crackmename': 'C1', + 'visible': False, 'deleted': False, 'info': 'sol', + }) + db.rating_difficulty.insert_one({'author': author, 'crackmehexid': hexid, 'rating': 3}) + db.rating_quality.insert_one({'author': author, 'crackmehexid': hexid, 'rating': 4}) + db.notifications.insert_one({'user': author, 'text': 'hi', 'seen': False}) + db.label_request.insert_one({ + 'requester': author, 'crackme_hexid': hexid, 'crackme_name': 'C1', + 'status': 'pending', + }) + db.account_deletion_request.insert_one({ + 'requester': author, 'email': 'alice@example.test', 'status': 'pending', + }) + return hexid + + +# --------------------------------------------------------------------------- # +# Model: username rename cascade +# --------------------------------------------------------------------------- # + +def test_user_rename_cascades_every_reference(app, db, alice): + _seed_alice_content(db, 'alice') + + user_rename('alice', 'alice_renamed') + + # The user document itself is renamed... + assert user_by_name('alice_renamed')['email'] == 'alice@example.test' + with pytest.raises(ErrNoResult): + user_by_name('alice') + + # ...and no reference to the old name survives anywhere. + assert db.crackme.count_documents({'author': 'alice'}) == 0 + assert db.comment.count_documents({'author': 'alice'}) == 0 + assert db.solution.count_documents({'author': 'alice'}) == 0 + assert db.rating_difficulty.count_documents({'author': 'alice'}) == 0 + assert db.rating_quality.count_documents({'author': 'alice'}) == 0 + assert db.notifications.count_documents({'user': 'alice'}) == 0 + assert db.label_request.count_documents({'requester': 'alice'}) == 0 + assert db.account_deletion_request.count_documents({'requester': 'alice'}) == 0 + + # Every reference now points at the new name. + assert db.crackme.count_documents({'author': 'alice_renamed'}) == 1 + assert db.comment.count_documents({'author': 'alice_renamed'}) == 1 + assert db.solution.count_documents({'author': 'alice_renamed'}) == 1 + assert db.rating_difficulty.count_documents({'author': 'alice_renamed'}) == 1 + assert db.rating_quality.count_documents({'author': 'alice_renamed'}) == 1 + assert db.notifications.count_documents({'user': 'alice_renamed'}) == 1 + assert db.label_request.count_documents({'requester': 'alice_renamed'}) == 1 + assert db.account_deletion_request.count_documents({'requester': 'alice_renamed'}) == 1 + + +def test_user_rename_missing_user_raises(app, db): + with pytest.raises(ErrNoResult): + user_rename('nobody', 'somebody') + + +def test_user_rename_failure_leaves_user_on_old_name(app, db, alice, monkeypatch): + """A mid-cascade failure must not rename the user doc (no session lockout).""" + import app.models.user as user_model + _seed_alice_content(db, 'alice') + real_get = user_model.get_collection + + def boom(name): + if name == 'solution': # fail partway through the cascade + raise RuntimeError('db blip') + return real_get(name) + + monkeypatch.setattr(user_model, 'get_collection', boom) + + with pytest.raises(RuntimeError): + user_rename('alice', 'alice_new') + + monkeypatch.undo() + # The user document still resolves under the old name, so the session stays + # valid and the rename can simply be retried. + assert user_by_name('alice')['email'] == 'alice@example.test' + with pytest.raises(ErrNoResult): + user_by_name('alice_new') + + +def test_user_rename_leaves_other_users_untouched(app, db, alice, bob): + db.crackme.insert_one({ + '_id': ObjectId(), 'hexid': str(ObjectId()), 'name': 'B', 'author': 'bob', + 'visible': True, 'deleted': False, + }) + user_rename('alice', 'alice2') + assert db.crackme.count_documents({'author': 'bob'}) == 1 + + +# --------------------------------------------------------------------------- # +# Model: email change +# --------------------------------------------------------------------------- # + +def test_user_change_email_updates_copies_and_drops_tokens(app, db, alice): + db.account_deletion_request.insert_one({ + 'requester': 'alice', 'email': 'alice@example.test', 'status': 'pending', + }) + db.password_reset_tokens.insert_one({ + 'email': 'alice@example.test', 'token': 'tok', + }) + + old = user_change_email('alice', 'NewAlice@Example.test') + + assert old == 'alice@example.test' + assert user_by_name('alice')['email'] == 'newalice@example.test' + # Denormalized copy on the pending deletion request is refreshed... + assert db.account_deletion_request.find_one( + {'requester': 'alice'})['email'] == 'newalice@example.test' + # ...and stale reset tokens for the old address are gone. + assert db.password_reset_tokens.count_documents({'email': 'alice@example.test'}) == 0 + + +# --------------------------------------------------------------------------- # +# Model: crackme rename cascade +# --------------------------------------------------------------------------- # + +def test_crackme_rename_cascades_denormalized_name(app, db, sample_crackme): + from app.models.crackme import crackme_update + + hexid = sample_crackme['hexid'] + db.comment.insert_one({ + 'author': 'bob', 'crackmehexid': hexid, 'crackmename': 'Test Crackme', + 'info': 'c', 'visible': True, 'created_at': datetime.now(timezone.utc), + }) + db.solution.insert_one({ + '_id': ObjectId(), 'hexid': str(ObjectId()), 'author': 'bob', + 'crackmeid': sample_crackme['_id'], 'crackmehexid': hexid, + 'crackmename': 'Test Crackme', 'visible': True, 'deleted': False, + }) + db.label_request.insert_one({ + 'requester': 'bob', 'crackme_hexid': hexid, 'crackme_name': 'Test Crackme', + 'status': 'pending', + }) + + changes = crackme_update(hexid, {'name': 'Renamed Crackme'}) + + assert changes['name']['new'] == 'Renamed Crackme' + assert db.crackme.find_one({'hexid': hexid})['name'] == 'Renamed Crackme' + assert db.comment.find_one({'crackmehexid': hexid})['crackmename'] == 'Renamed Crackme' + assert db.solution.find_one({'crackmehexid': hexid})['crackmename'] == 'Renamed Crackme' + assert db.label_request.find_one({'crackme_hexid': hexid})['crackme_name'] == 'Renamed Crackme' + + +# --------------------------------------------------------------------------- # +# Controller: username change +# --------------------------------------------------------------------------- # + +def test_change_username_endpoint_renames_and_cascades(app, db, alice, alice_client): + hexid = _seed_alice_content(db, 'alice') + + resp = alice_client.post('/settings/username', data={ + 'name': 'alice_new', 'current_password': 'alice-password', + }) + assert resp.status_code == 302 + + assert user_by_name('alice_new')['email'] == 'alice@example.test' + assert db.crackme.find_one({'hexid': hexid})['author'] == 'alice_new' + # Session was updated to the new name. + with alice_client.session_transaction() as sess: + assert sess['name'] == 'alice_new' + + +def test_change_username_rejects_name_taken_by_other(app, db, alice, bob, alice_client): + resp = alice_client.post('/settings/username', data={ + 'name': 'bob', 'current_password': 'alice-password', + }) + assert resp.status_code == 302 + # alice keeps her name; bob is untouched. + assert user_by_name('alice')['email'] == 'alice@example.test' + assert user_by_name('bob')['email'] == 'bob@example.test' + + +def test_change_username_rejects_wrong_password(app, db, alice, alice_client): + resp = alice_client.post('/settings/username', data={ + 'name': 'alice_new', 'current_password': 'wrong', + }) + assert resp.status_code == 302 + with pytest.raises(ErrNoResult): + user_by_name('alice_new') + + +def test_pending_writeup_survives_username_change(app, db, alice, alice_client): + """A writeup submitted before a rename must remain owned and still pending.""" + cid = ObjectId() + sol_id = ObjectId() + db.crackme.insert_one({ + '_id': cid, 'hexid': str(cid), 'name': 'C', 'author': 'someoneelse', + 'visible': True, 'deleted': False, + }) + db.solution.insert_one({ + '_id': sol_id, 'hexid': str(sol_id), 'author': 'alice', 'crackmeid': cid, + 'crackmehexid': str(cid), 'crackmename': 'C', 'visible': False, + 'deleted': False, 'info': 'wip', + }) + + alice_client.post('/settings/username', data={ + 'name': 'alice2', 'current_password': 'alice-password', + }) + + sol = db.solution.find_one({'_id': sol_id}) + assert sol['author'] == 'alice2' # ownership preserved + assert sol['visible'] is False # still in the review queue + + +# --------------------------------------------------------------------------- # +# Controller: email change +# --------------------------------------------------------------------------- # + +def test_change_email_endpoint(app, db, alice, alice_client): + resp = alice_client.post('/settings/email', data={ + 'email': 'alice2@example.test', 'current_password': 'alice-password', + }) + assert resp.status_code == 302 + assert user_by_name('alice')['email'] == 'alice2@example.test' + with alice_client.session_transaction() as sess: + assert sess['email'] == 'alice2@example.test' + + +def test_change_email_rejects_taken(app, db, alice, bob, alice_client): + resp = alice_client.post('/settings/email', data={ + 'email': 'bob@example.test', 'current_password': 'alice-password', + }) + assert resp.status_code == 302 + assert user_by_name('alice')['email'] == 'alice@example.test' + + +# --------------------------------------------------------------------------- # +# Controller: crackme rename via edit form +# --------------------------------------------------------------------------- # + +def test_edit_crackme_renames_via_form(app, db, alice, alice_client, sample_crackme): + hexid = sample_crackme['hexid'] + db.solution.insert_one({ + '_id': ObjectId(), 'hexid': str(ObjectId()), 'author': 'bob', + 'crackmeid': sample_crackme['_id'], 'crackmehexid': hexid, + 'crackmename': 'Test Crackme', 'visible': True, 'deleted': False, + }) + + resp = alice_client.post(f'/crackme/{hexid}/edit', data={ + 'name': 'Brand New Name', 'info': 'updated', 'lang': 'C/C++', + 'arch': 'x86-64', 'platform': 'Linux', + }) + assert resp.status_code == 302 + assert db.crackme.find_one({'hexid': hexid})['name'] == 'Brand New Name' + assert db.solution.find_one({'crackmehexid': hexid})['crackmename'] == 'Brand New Name' + + +def test_edit_crackme_rejects_empty_name(app, db, alice, alice_client, sample_crackme): + hexid = sample_crackme['hexid'] + resp = alice_client.post(f'/crackme/{hexid}/edit', data={ + 'name': ' ', 'info': 'x', 'lang': 'C/C++', 'arch': 'x86-64', + 'platform': 'Linux', + }) + assert resp.status_code == 302 + assert db.crackme.find_one({'hexid': hexid})['name'] == 'Test Crackme' diff --git a/tests/test_comments_and_crackmes.py b/tests/test_comments_and_crackmes.py index 61d8348..728755d 100644 --- a/tests/test_comments_and_crackmes.py +++ b/tests/test_comments_and_crackmes.py @@ -103,12 +103,12 @@ def test_owner_can_edit_crackme_but_other_user_cannot( path = f"/crackme/{sample_crackme['hexid']}/edit" denied = bob_client.post(path, data={ - 'info': 'Unauthorized', 'lang': 'Python', 'arch': 'ARM', - 'platform': 'Windows', + 'name': 'Hijacked', 'info': 'Unauthorized', 'lang': 'Python', + 'arch': 'ARM', 'platform': 'Windows', }) allowed = alice_client.post(path, data={ - 'info': 'Updated information', 'lang': 'Rust', 'arch': 'ARM', - 'platform': 'Linux', + 'name': sample_crackme['name'], 'info': 'Updated information', + 'lang': 'Rust', 'arch': 'ARM', 'platform': 'Linux', }) assert denied.status_code == 302 diff --git a/tests/test_labels.py b/tests/test_labels.py index f2907e3..7f1ce14 100644 --- a/tests/test_labels.py +++ b/tests/test_labels.py @@ -459,7 +459,7 @@ def test_admin_editcrackme_shows_and_saves_labels(app, db, sample_crackme): assert b'value="Anti-debugging" data-label-class="1" checked' in page.data saved = client.post("/review/editcrackme", data={ - "crackme_uuid": hexid, + "crackme_uuid": hexid, "name": sample_crackme["name"], "info": sample_crackme["info"], "lang": sample_crackme["lang"], "arch": sample_crackme["arch"], "platform": sample_crackme["platform"], "labels": ["Packer", "bogus"], "csrf_token": "admin-csrf", diff --git a/tests/test_reviewer_remaining.py b/tests/test_reviewer_remaining.py index 572d2e1..108b71a 100644 --- a/tests/test_reviewer_remaining.py +++ b/tests/test_reviewer_remaining.py @@ -57,6 +57,7 @@ def test_admin_edits_crackme_metadata_and_notifies_author( response = client.post('/review/editcrackme', data={ 'csrf_token': 'workflow-csrf', 'crackme_uuid': sample_crackme['hexid'], + 'name': sample_crackme['name'], 'info': 'Reviewer-updated description', 'lang': 'Rust', 'arch': 'ARM', 'platform': 'Linux', 'notify_author': 'on', @@ -70,6 +71,47 @@ def test_admin_edits_crackme_metadata_and_notifies_author( _cleanup_admin() +def test_admin_renames_crackme_and_cascades(app, db, sample_crackme, monkeypatch): + from review import routes + + client = _admin_client(app) + monkeypatch.setattr(routes, 'log_reviewer_operation', lambda *a, **k: None) + hexid = sample_crackme['hexid'] + db.solution.insert_one({ + '_id': ObjectId(), 'hexid': str(ObjectId()), 'author': 'bob', + 'crackmeid': sample_crackme['_id'], 'crackmehexid': hexid, + 'crackmename': 'Test Crackme', 'visible': True, 'deleted': False, + }) + + response = client.post('/review/editcrackme', data={ + 'csrf_token': 'workflow-csrf', 'crackme_uuid': hexid, + 'name': 'Reviewer Renamed', 'info': sample_crackme['info'], + 'lang': sample_crackme['lang'], 'arch': sample_crackme['arch'], + 'platform': sample_crackme['platform'], + }) + + assert response.status_code == 200 + assert db.crackme.find_one({'hexid': hexid})['name'] == 'Reviewer Renamed' + assert db.solution.find_one({'crackmehexid': hexid})['crackmename'] == 'Reviewer Renamed' + _cleanup_admin() + + +def test_admin_edit_rejects_empty_crackme_name(app, db, sample_crackme, monkeypatch): + from review import routes + + client = _admin_client(app) + monkeypatch.setattr(routes, 'log_reviewer_operation', lambda *a, **k: None) + response = client.post('/review/editcrackme', data={ + 'csrf_token': 'workflow-csrf', 'crackme_uuid': sample_crackme['hexid'], + 'name': ' ', 'info': 'x', 'lang': 'Rust', 'arch': 'ARM', + 'platform': 'Linux', + }) + + assert response.status_code == 200 + assert db.crackme.find_one({'_id': sample_crackme['_id']})['name'] == 'Test Crackme' + _cleanup_admin() + + def test_admin_replaces_crackme_file(app, db, sample_crackme, monkeypatch, tmp_path): from review import routes @@ -82,6 +124,7 @@ def test_admin_replaces_crackme_file(app, db, sample_crackme, monkeypatch, tmp_p ) response = client.post('/review/editcrackme', data={ 'csrf_token': 'workflow-csrf', 'crackme_uuid': sample_crackme['hexid'], + 'name': sample_crackme['name'], 'info': sample_crackme['info'], 'lang': sample_crackme['lang'], 'arch': sample_crackme['arch'], 'platform': sample_crackme['platform'], 'file': (BytesIO(b'replacement'), 'replacement.bin'), From ca3733c939d215dc5b2b0514a908d9e15575f99b Mon Sep 17 00:00:00 2001 From: Xusheng Date: Sun, 26 Jul 2026 09:15:23 -0400 Subject: [PATCH 2/2] Resolve session identity by immutable id so renames don't desync other devices Sessions are client-side signed cookies, so a username change on one device can't invalidate the stale name cached in another device's cookie. Left as-is, that stale session would post content under a username that no longer exists and lose ownership of its own (renamed) content, and if the freed name were later re-registered the cookie would silently act as the new account. Fix: store the immutable user id in the session at login and re-resolve the user from it in a before_request hook, refreshing the cookie's name in place (or clearing auth if the account is gone). Keying on the id also makes name reuse a non-issue. No extra queries on page views: the lookup replaces the per-request unread-notifications query the context processor already ran, and it resolves by the indexed _id (a point-read) rather than the unindexed name/hexid fields. It adds one indexed point-read only to authenticated requests that render no template (POST-redirects, downloads). Tests: stale-session name refresh, comment-from-stale-session uses the current name (the desync case), and session cleared when the user is gone. Test user fixtures now carry _id/hexid like real (user_create'd) users. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/__init__.py | 64 +++++++++++++++++++++++++++++++--- app/controllers/login.py | 4 +++ app/services/session.py | 1 + tests/conftest.py | 8 +++++ tests/test_account_settings.py | 48 +++++++++++++++++++++++++ 5 files changed, 120 insertions(+), 5 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 461c546..c8065da 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -80,18 +80,72 @@ def create_app(config_path=None, config=None): # Exempt reviewer routes from main CSRF (reviewer has its own CSRF) csrf.exempt(reviewer_bp) + # Re-resolve the logged-in user from their immutable id on every request. + # Usernames are mutable (they can be changed in account settings), so the + # name stored in the signed cookie can go stale on other devices. Keying off + # the id lets us refresh the cookie's name in place — otherwise a stale + # session could post content under a username that no longer exists, or lose + # ownership of its own (now-renamed) content. If the account is gone, the + # auth keys are cleared so the request is treated as anonymous. + @app.before_request + def refresh_session_identity(): + from bson import ObjectId + from bson.errors import InvalidId + from flask import request, session, g + from app.models.user import user_by_name + from app.services.database import get_collection + from app.models.errors import ErrNoResult + + g.current_user = None + if request.endpoint == 'static' or not session.get('name'): + return + + try: + user = None + hexid = session.get('hexid') + if hexid: + # Resolve by the primary key (indexed point-read), not the + # unindexed hexid field, so this stays the cheapest query there is. + try: + user = get_collection('user').find_one({'_id': ObjectId(hexid)}) + except InvalidId: + user = None + else: + # Legacy session issued before ids were stored; resolve by name + # once and backfill the id so future requests are id-based. + try: + user = user_by_name(session['name']) + except ErrNoResult: + user = None + + if user is None: + # Account deleted (or the old name was freed and re-registered by + # someone else): drop our stale auth rather than impersonate. + session.pop('name', None) + session.pop('email', None) + session.pop('hexid', None) + else: + session['name'] = user['name'] + session['email'] = user.get('email', session.get('email')) + session['hexid'] = user.get('hexid') or str(user['_id']) + g.current_user = user + except Exception as e: + # A transient DB error shouldn't break the request; leave the + # session untouched and let downstream handlers cope. + print(f"Session identity refresh error: {e}") + # Register template context processors @app.context_processor def inject_globals(): - from flask import session - from app.models.user import user_get_unread_notifications + from flask import session, g from app.services.crackme_fields import ( DIFFICULTY_CHOICES, LANG_CHOICES, ARCH_CHOICES, PLATFORM_CHOICES) username = session.get('name', '') - unread_notifs = 0 - if username: - unread_notifs = user_get_unread_notifications(username) + # Reuse the user loaded in refresh_session_identity so this doesn't add a + # second per-request query. + current_user = getattr(g, 'current_user', None) + unread_notifs = current_user.get('unread_notifications', 0) if current_user else 0 return { 'BaseURI': '/', diff --git a/app/controllers/login.py b/app/controllers/login.py index 265918f..4b6f207 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('hexid', None) session.pop('login_attempt', None) @@ -87,6 +88,9 @@ def login_post(): clear_main_auth() session['email'] = user['email'] session['name'] = user['name'] + # Store the immutable id so the session survives a later username + # change (the name in the cookie is refreshed from this each request). + session['hexid'] = user.get('hexid') or str(user['_id']) flash('Login successful!', FLASH_SUCCESS) # Retrieve and clear the redirect from session diff --git a/app/services/session.py b/app/services/session.py index a4c8c90..a55485f 100644 --- a/app/services/session.py +++ b/app/services/session.py @@ -21,6 +21,7 @@ def clear_session(): """Clear main site auth session keys only.""" flask_session.pop('name', None) flask_session.pop('email', None) + flask_session.pop('hexid', None) flask_session.pop('login_attempt', None) diff --git a/tests/conftest.py b/tests/conftest.py index 1d7b8b0..c7c1d15 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -84,7 +84,11 @@ def runner(app): @pytest.fixture def alice(db): + from bson import ObjectId + obj_id = ObjectId() user = { + '_id': obj_id, + 'hexid': str(obj_id), 'name': 'alice', 'email': 'alice@example.test', 'password': hash_string('alice-password'), @@ -98,7 +102,11 @@ def alice(db): @pytest.fixture def bob(db): + from bson import ObjectId + obj_id = ObjectId() user = { + '_id': obj_id, + 'hexid': str(obj_id), 'name': 'bob', 'email': 'bob@example.test', 'password': hash_string('bob-password'), diff --git a/tests/test_account_settings.py b/tests/test_account_settings.py index fbfbd1f..fa97a92 100644 --- a/tests/test_account_settings.py +++ b/tests/test_account_settings.py @@ -232,6 +232,54 @@ def test_pending_writeup_survives_username_change(app, db, alice, alice_client): assert sol['visible'] is False # still in the review queue +# --------------------------------------------------------------------------- # +# Stale sessions on other devices self-heal from the immutable id +# --------------------------------------------------------------------------- # + +def _device_logged_in_as(app, user): + """A fresh client whose cookie carries the user's id (as login would set).""" + client = app.test_client() + with client.session_transaction() as sess: + sess['name'] = user['name'] + sess['email'] = user['email'] + sess['hexid'] = user['hexid'] + return client + + +def test_stale_session_name_refreshes_after_rename(app, db, alice): + # Device B is logged in as alice. + device_b = _device_logged_in_as(app, alice) + # Device A renames the account. + user_rename('alice', 'alice_renamed') + + device_b.get('/') # any request triggers the before_request refresh + + with device_b.session_transaction() as sess: + assert sess['name'] == 'alice_renamed' + + +def test_stale_session_posts_comment_under_current_name(app, db, alice, sample_crackme): + """The exact desync case: a comment from a stale session must NOT use the old name.""" + device_b = _device_logged_in_as(app, alice) + user_rename('alice', 'alice_renamed') + + device_b.post(f"/comment/{sample_crackme['hexid']}", data={'comment': 'hello'}) + + # No comment is authored under the stale 'alice'; it uses the healed name. + assert db.comment.count_documents({'author': 'alice'}) == 0 + assert db.comment.count_documents({'author': 'alice_renamed'}) == 1 + + +def test_session_cleared_when_user_no_longer_exists(app, db, alice): + device = _device_logged_in_as(app, alice) + db.user.delete_one({'_id': alice['_id']}) + + device.get('/') + + with device.session_transaction() as sess: + assert sess.get('name') is None + + # --------------------------------------------------------------------------- # # Controller: email change # --------------------------------------------------------------------------- #