diff --git a/README.md b/README.md index 1b753d6..8f053ad 100644 --- a/README.md +++ b/README.md @@ -228,6 +228,39 @@ Alternatively, an existing admin can add new reviewers through the web interface | Reset user passwords | No | Yes | | Manage reviewer accounts | No | Yes | +#### Cleaning review files left by previous account deletions + +Account deletion removes both pending uploads and approved archives, including +private source archives and attached solutions. A filesystem error stops the +operation and reports an error so it can be retried after fixing permissions. +Deletion is not transactional: items already removed are not rolled back. + +Older versions could delete a crackme's database record while leaving its file +in `tmp/crackme`. This inflated the dashboard count and produced +`uuid not found in db` errors in the review queue. No database migration is needed. + +Use the application virtual environment and the same `config/config.json` as +the running site. From the project root, preview the affected files: + +```bash +python script/cleanup_orphaned_pending_files.py +``` + +Pause uploads (for example, stop the application service and allow in-flight +requests to finish), then apply the cleanup: + +```bash +python script/cleanup_orphaned_pending_files.py --apply +``` + +Restart the application with the updated code. The script moves only files +without a corresponding database record into a new private directory under +`private/orphaned_submissions/`; it prints that directory and preserves relative +paths for recovery. It never changes database documents or published archives. +By default, files newer than five minutes are skipped to avoid uploads awaiting +their database insert. With uploads stopped, add `--min-age-seconds 0` to include +recent orphans. `--root` and `--config` support non-default deployment paths. + ## Contributing Interested in contributing? Please read [CONTRIBUTING.md](CONTRIBUTING.md) for guidelines. **Note:** You must contact the maintainer before starting any work. diff --git a/review/routes.py b/review/routes.py index c0784b5..4b5f561 100644 --- a/review/routes.py +++ b/review/routes.py @@ -202,6 +202,27 @@ def delete_source_archive(hexid): pass +def _delete_submission_files(item_type, hexid): + """Remove pending and published files before deleting their DB record. + + Missing files are harmless (including markdown-only solutions). Other + filesystem errors must stop deletion so the account can be retried. + """ + hexid = str(ObjectId(hexid)) + paths = [ + os.path.join(get_tmp_dir(item_type), hexid), + os.path.join(get_static_dir(item_type), f'{hexid}.zip'), + ] + if item_type == 'crackme': + paths.append(os.path.join(get_source_dir(), hexid)) + + for path in paths: + try: + os.remove(path) + except FileNotFoundError: + pass + + def find_pending_file(item_type, hexid): """ Find a pending submission file by its hexid. @@ -889,10 +910,10 @@ def approve_pending_solution(hexid): def delete_approved_solution(solution_uuid): """ - Delete an already-approved solution. + Delete a solution, including pending submissions during account deletion. - Removes the solution from database, deletes the zip file, - and decrements the crackme's solution count. + Removes pending/published files and the database record. Only approved + solutions contribute to the crackme's public solution count. Args: solution_uuid: Solution's hex ID @@ -911,22 +932,13 @@ def delete_approved_solution(solution_uuid): return "Solution not found in database" crackme_id = solution.get("crackmeid") - zip_path = os.path.join( - get_static_dir('solution'), - f"{solution_uuid}.zip" - ) - - # Delete zip file (if exists) - try: - os.remove(zip_path) - except Exception: - pass + _delete_submission_files('solution', solution_uuid) # Delete from database g_crackmesone_db.solution.delete_one({"_id": ObjectId(solution_uuid)}) - # Decrement crackme solution count (always, as long as solution was in database) - if crackme_id: + # Legacy approved solutions may not have an explicit visibility field. + if crackme_id and solution.get('visible', True): try: g_crackmesone_db.crackme.update_one( {"_id": crackme_id}, @@ -940,9 +952,10 @@ def delete_approved_solution(solution_uuid): def delete_approved_crackme(crackme_uuid): """ - Delete an already-approved crackme with cascade deletion. + Delete a pending or approved crackme with cascade deletion. - Deletes the crackme, all its solutions, comments, and ratings. + Deletes pending/published files, private source, the crackme, and its + solutions, comments, ratings and solve records. Args: crackme_uuid: Crackme's hex ID @@ -953,6 +966,7 @@ def delete_approved_crackme(crackme_uuid): if not ObjectId.is_valid(crackme_uuid): return "Invalid crackme uuid" + crackme_uuid = str(ObjectId(crackme_uuid)) crackme = g_crackmesone_db.crackme.find_one({ "_id": ObjectId(crackme_uuid) }) @@ -960,23 +974,14 @@ def delete_approved_crackme(crackme_uuid): if not crackme: return "Crackme uuid not found" + _delete_submission_files('crackme', crackme_uuid) + # Cascade delete related data deleted = _cascade_delete_crackme_data( ObjectId(crackme_uuid), crackme_uuid ) - # Delete zip file - try: - os.remove(os.path.join( - get_static_dir('crackme'), - f"{crackme_uuid}.zip" - )) - except Exception: - pass - - delete_source_archive(crackme_uuid) - # Delete crackme document g_crackmesone_db.crackme.delete_one({"_id": ObjectId(crackme_uuid)}) @@ -1012,11 +1017,9 @@ def _cascade_delete_crackme_data(crackme_id, crackme_hexid): # Delete solutions for solution in g_crackmesone_db.solution.find({'crackmeid': crackme_id}): - try: - delete_approved_solution(str(solution["_id"])) - deleted['solutions'] += 1 - except Exception as e: - print(f"Warning: Failed to delete solution {solution['_id']}: {e}") + # Do not orphan a solution if its files could not be removed. + delete_approved_solution(str(solution["_id"])) + deleted['solutions'] += 1 # Delete comments result = g_crackmesone_db.comment.delete_many({ diff --git a/script/cleanup_orphaned_pending_files.py b/script/cleanup_orphaned_pending_files.py new file mode 100644 index 0000000..c332e52 --- /dev/null +++ b/script/cleanup_orphaned_pending_files.py @@ -0,0 +1,102 @@ +#!/usr/bin/env python3 +"""Quarantine old review files whose database records no longer exist. + +Run from the project root with the application's virtual environment: + python script/cleanup_orphaned_pending_files.py + python script/cleanup_orphaned_pending_files.py --apply + +The default is a dry run. No database documents or published archives are +modified. Recent files are skipped because uploads write files before inserting +their database records. Pause uploads while applying a cleanup; use +--min-age-seconds 0 only with uploads paused and in-flight requests finished. +""" + +import argparse +import json +from pathlib import Path +import shutil +import stat +import tempfile +import time + +from bson import ObjectId +from pymongo import MongoClient + + +def cleanup_orphaned_files(db, root, apply=False, min_age_seconds=300): + """Return affected relative paths and the optional quarantine directory.""" + if min_age_seconds < 0: + raise ValueError('min_age_seconds must be non-negative') + root = Path(root).resolve() + cutoff = time.time() - min_age_seconds + affected = [] + quarantine = None + + for relative_dir, collection_name in ( + ('tmp/crackme', 'crackme'), + ('tmp/solution', 'solution'), + ('private/crackme_source', 'crackme'), + ): + directory = root / relative_dir + if not directory.exists(): + continue + for path in sorted(directory.iterdir()): + if not ObjectId.is_valid(path.name): + continue + try: + metadata = path.lstat() + except FileNotFoundError: + continue + # Never follow symlinks or process directories, including ones with + # valid ObjectId names. Leave new/in-progress uploads alone. + if not stat.S_ISREG(metadata.st_mode) or metadata.st_mtime > cutoff: + continue + # Query the configured database for each file. Connection/query + # errors propagate; they must never be interpreted as "no record". + if db[collection_name].find_one( + {'_id': ObjectId(path.name)}, {'_id': 1}) is not None: + continue + + relative = path.relative_to(root) + if apply: + if quarantine is None: + parent = root / 'private/orphaned_submissions' + parent.mkdir(parents=True, exist_ok=True, mode=0o700) + quarantine = Path(tempfile.mkdtemp(prefix='cleanup-', dir=parent)) + print(f'Quarantine directory: {quarantine}') + destination = quarantine / relative + destination.parent.mkdir(parents=True, exist_ok=True) + shutil.move(str(path), str(destination)) + affected.append(relative) + print(f'{"Quarantined" if apply else "Would quarantine"}: {relative}') + + return affected, quarantine + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('--apply', action='store_true', help='Move orphaned files out of the queue') + parser.add_argument('--root', type=Path, default=Path(__file__).resolve().parents[1]) + parser.add_argument('--config', type=Path, help='Defaults to ROOT/config/config.json') + parser.add_argument('--min-age-seconds', type=int, default=300, + help='Skip newer files (default: 300 seconds)') + args = parser.parse_args(argv) + if args.min_age_seconds < 0: + parser.error('--min-age-seconds must be non-negative') + config_path = args.config or args.root / 'config/config.json' + with config_path.open() as handle: + config = json.load(handle)['Database'] + + with MongoClient(config.get('URL', 'mongodb://127.0.0.1:27017'), + serverSelectionTimeoutMS=10000) as client: + client.admin.command('ping') + database = client[config.get('Name', 'crackmesone')] + affected, _ = cleanup_orphaned_files( + database, args.root, args.apply, args.min_age_seconds) + print(f'{len(affected)} orphaned file(s) {"quarantined" if args.apply else "found"}.') + if not args.apply: + print('Dry run: no changes made. Use --apply to quarantine these files.') + + +if __name__ == '__main__': + main() diff --git a/tests/test_pending_deletion_cleanup.py b/tests/test_pending_deletion_cleanup.py new file mode 100644 index 0000000..ccc5fd1 --- /dev/null +++ b/tests/test_pending_deletion_cleanup.py @@ -0,0 +1,157 @@ +"""Regressions for files left in the review queue after account deletion.""" + +from datetime import datetime, timezone +from pathlib import Path + +from bson import ObjectId +import pytest + + +@pytest.fixture +def submission_root(monkeypatch, tmp_path): + from review import routes + + monkeypatch.setattr(routes, 'CRACKMESONE_DIR', str(tmp_path)) + monkeypatch.setattr(routes, 'log_reviewer_operation', lambda *a, **kw: None) + for directory in ( + 'tmp/crackme', 'tmp/solution', 'static/crackme', 'static/solution', + 'private/crackme_source', + ): + (tmp_path / directory).mkdir(parents=True) + return tmp_path + + +def _crackme(db, author, root, visible=False): + oid = ObjectId() + doc = { + '_id': oid, 'hexid': str(oid), 'name': f'Submission {oid}', + 'author': author, 'created_at': datetime.now(timezone.utc), + 'visible': visible, 'deleted': False, + } + db.crackme.insert_one(doc) + path = root / (f'static/crackme/{oid}.zip' if visible + else f'tmp/crackme/{oid}') + path.write_bytes(b'submission') + (root / 'private/crackme_source' / str(oid)).write_bytes(b'source') + return doc + + +def test_deleting_flood_account_cleans_queue_and_preserves_other_users( + app, db, alice, bob, submission_root, monkeypatch): + from review import routes + + root = submission_root + owned = [_crackme(db, 'alice', root) for _ in range(48)] + approved = _crackme(db, 'alice', root, visible=True) + other = _crackme(db, 'bob', root) + monkeypatch.setitem(routes.users, 'cleanup-admin', {'is_admin': True}) + client = app.test_client() + with client.session_transaction() as session: + session[routes.REVIEWER_SESSION_KEY] = 'cleanup-admin' + session[routes.REVIEWER_ADMIN_KEY] = True + session[routes.REVIEWER_CSRF_KEY] = 'cleanup-csrf' + + assert routes.count_pending_items('crackme') == 49 + response = client.post('/review/deleteuser', data={ + 'action': 'confirm_delete', 'confirmed_email': alice['email'], + 'csrf_token': 'cleanup-csrf', + }) + + assert response.status_code == 200 + assert b'User account deletion successful!' in response.data + assert db.user.find_one({'name': 'alice'}) is None + assert db.user.find_one({'name': 'bob'}) is not None + assert db.crackme.count_documents({'author': 'alice'}) == 0 + assert routes.count_pending_items('crackme') == 1 + queue, error = routes.get_pending_crackmes() + assert error is None + assert [item['crackme_uuid'] for item in queue] == [other['hexid']] + assert b'uuid not found in db' not in client.get('/review/reviewcrackme').data + for doc in owned + [approved]: + assert not (root / 'tmp/crackme' / doc['hexid']).exists() + assert not (root / 'private/crackme_source' / doc['hexid']).exists() + assert not (root / 'static/crackme' / f"{approved['hexid']}.zip").exists() + assert (root / 'tmp/crackme' / other['hexid']).read_bytes() == b'submission' + assert (root / 'private/crackme_source' / other['hexid']).exists() + + +@pytest.mark.parametrize('visible,attachment', [(False, True), (False, False), (True, True)]) +def test_account_deletion_removes_solutions_without_corrupting_public_count( + db, alice, bob, sample_crackme, submission_root, visible, attachment): + from review import routes + + oid = ObjectId() + db.solution.insert_one({ + '_id': oid, 'hexid': str(oid), 'author': 'bob', + 'crackmeid': sample_crackme['_id'], 'visible': visible, + 'has_attachment': attachment, + }) + db.crackme.update_one({'_id': sample_crackme['_id']}, + {'$set': {'nbsolutions': 2}}) + pending = submission_root / 'tmp/solution' / str(oid) + archive = submission_root / 'static/solution' / f'{oid}.zip' + if attachment: + (archive if visible else pending).write_bytes(b'solution') + + assert 'deletion successful' in routes.delete_user_account(bob['email']) + assert db.solution.find_one({'_id': oid}) is None + assert not pending.exists() + assert not archive.exists() + assert db.crackme.find_one({'_id': sample_crackme['_id']})['nbsolutions'] == ( + 1 if visible else 2 + ) + + +def test_crackme_cascade_removes_another_authors_pending_solution( + db, alice, bob, submission_root): + from review import routes + + crackme = _crackme(db, 'alice', submission_root) + oid = ObjectId() + db.solution.insert_one({ + '_id': oid, 'hexid': str(oid), 'author': 'bob', + 'crackmeid': crackme['_id'], 'visible': False, + }) + pending = submission_root / 'tmp/solution' / str(oid) + pending.write_bytes(b'solution') + + assert 'deletion successful' in routes.delete_user_account(alice['email']) + assert not pending.exists() + assert db.solution.find_one({'_id': oid}) is None + assert db.user.find_one({'name': 'bob'}) is not None + + +@pytest.mark.parametrize('blocked', ['crackme', 'solution', 'source']) +def test_file_deletion_failure_preserves_account_and_related_documents( + db, alice, bob, submission_root, monkeypatch, blocked): + from review import routes + + crackme = _crackme(db, 'alice', submission_root) + oid = ObjectId() + db.solution.insert_one({ + '_id': oid, 'hexid': str(oid), 'author': 'bob', + 'crackmeid': crackme['_id'], 'visible': False, + }) + solution_path = submission_root / 'tmp/solution' / str(oid) + solution_path.write_bytes(b'solution') + blocked_path = { + 'crackme': submission_root / 'tmp/crackme' / crackme['hexid'], + 'solution': solution_path, + 'source': submission_root / 'private/crackme_source' / crackme['hexid'], + }[blocked] + remove = routes.os.remove + + def refuse_file(path): + if Path(path) == blocked_path: + raise PermissionError('test file is not writable') + return remove(path) + + monkeypatch.setattr(routes.os, 'remove', refuse_file) + result = routes.delete_user_account(alice['email']) + + assert result.startswith('Error during deletion:') + assert 'test file is not writable' in result + assert db.user.find_one({'name': 'alice'}) is not None + assert db.crackme.find_one({'_id': crackme['_id']}) is not None + assert db.solution.find_one({'_id': oid}) is not None + assert blocked_path.exists() diff --git a/tests/test_pending_file_cleanup_script.py b/tests/test_pending_file_cleanup_script.py new file mode 100644 index 0000000..ab18dc6 --- /dev/null +++ b/tests/test_pending_file_cleanup_script.py @@ -0,0 +1,73 @@ +"""Coverage of the narrowly scoped, reversible orphan cleanup utility.""" + +import os +import time + +from bson import ObjectId +from pymongo.errors import ConnectionFailure +import pytest + +from script.cleanup_orphaned_pending_files import cleanup_orphaned_files + + +def _file(root, directory, name=None, old=True): + path = root / directory / (name or str(ObjectId())) + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b'original upload') + if old: + timestamp = time.time() - 3600 + os.utime(path, (timestamp, timestamp)) + return path + + +def test_cleanup_quarantines_only_old_orphans_and_is_repeatable(db, tmp_path): + orphan = _file(tmp_path, 'tmp/crackme') + source = _file(tmp_path, 'private/crackme_source', orphan.name) + solution = _file(tmp_path, 'tmp/solution') + source_only = _file(tmp_path, 'private/crackme_source') + orphans = [orphan, source, solution, source_only] + preserved = [ + _file(tmp_path, 'tmp/crackme', old=False), + _file(tmp_path, 'tmp/solution', 'notes.txt'), + _file(tmp_path, 'static/crackme', str(ObjectId()) + '.zip'), + ] + for kind in ('crackme', 'solution'): + for visible in (True, False): + valid = _file(tmp_path, f'tmp/{kind}') + db[kind].insert_one({'_id': ObjectId(valid.name), 'visible': visible}) + preserved.append(valid) + directory = tmp_path / 'tmp/crackme' / str(ObjectId()) + directory.mkdir() + link = tmp_path / 'tmp/crackme' / str(ObjectId()) + link.symlink_to(orphan) + + affected, quarantine = cleanup_orphaned_files(db, tmp_path) + assert set(affected) == {p.relative_to(tmp_path) for p in orphans} + assert quarantine is None + assert not (tmp_path / 'private/orphaned_submissions').exists() + assert all(p.exists() for p in orphans + preserved) + + affected, quarantine = cleanup_orphaned_files(db, tmp_path, apply=True) + assert set(affected) == {p.relative_to(tmp_path) for p in orphans} + assert quarantine.is_relative_to(tmp_path / 'private/orphaned_submissions') + assert all(not p.exists() for p in orphans) + assert all((quarantine / p.relative_to(tmp_path)).read_bytes() == b'original upload' + for p in orphans) + assert all(p.read_bytes() == b'original upload' for p in preserved) + assert directory.is_dir() + assert link.is_symlink() + assert db.crackme.count_documents({}) == db.solution.count_documents({}) == 2 + assert cleanup_orphaned_files(db, tmp_path, apply=True) == ([], None) + + +def test_cleanup_does_not_move_files_when_database_query_fails(db, tmp_path, monkeypatch): + orphan = _file(tmp_path, 'tmp/crackme') + + def unavailable(*args, **kwargs): + raise ConnectionFailure('database unavailable') + + monkeypatch.setattr(db.crackme, 'find_one', unavailable) + with pytest.raises(ConnectionFailure): + cleanup_orphaned_files(db, tmp_path, apply=True) + assert orphan.exists() + assert not (tmp_path / 'private/orphaned_submissions').exists()