From d071e5cdc5eb9697859246100ea11d31c4af74f0 Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 10 Sep 2026 10:23:34 +0200 Subject: [PATCH] feat: serve uploads through storage, and move all 3,852 files to MinIO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Serving went straight to disk with FileResponse, so object storage was effectively write-only: bytes went to the bucket and were still read from the volume. `/uploads/{path}` now tries the local file first, then the object, keeping the existing authorisation and path-confinement checks in front of both. That is what makes the volume removable at all. Migration (scripts/migrate_uploads_to_s3.py) Every file is copied and read back with a SHA-256 comparison before anything is deleted, and deletion is a separate opt-in flag that refuses to run if a single file failed to verify. 3,852 files, 853.7 MB, all verified, then removed from the volume — which now holds 0 files. A bug this caught in its own first run: verification used `storage_service.load`, which falls back to the volume, so it compared each local file against itself and reported 3,852 perfect matches against an empty bucket. `s3_object` reads strictly from S3 with no fallback, and verification uses that. The fallback is right for serving and wrong for verifying, and the two now have separate calls. Proven before deleting: a file removed from the volume still served correctly and byte-identically from the bucket. Backups, corrected: borgmatic already covers /var/lib/docker/volumes, so quiz_minio_data is backed up nightly with 7/4/6 retention — my earlier claim that MinIO was outside the backup routine was wrong, based on db-backup alone. Existing archives still hold the old uploads volume, so there is no window in which these files exist in only one place. Tests: 131 backend green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017acfNLsJpnkvH3sCZSjMJM --- backend/app/routers/uploads.py | 22 ++++- backend/app/services/storage_service.py | 14 +++ backend/scripts/migrate_uploads_to_s3.py | 120 +++++++++++++++++++++++ 3 files changed, 152 insertions(+), 4 deletions(-) create mode 100644 backend/scripts/migrate_uploads_to_s3.py diff --git a/backend/app/routers/uploads.py b/backend/app/routers/uploads.py index 309637b..38f5919 100644 --- a/backend/app/routers/uploads.py +++ b/backend/app/routers/uploads.py @@ -1,9 +1,12 @@ """Native browser media authentication only; API authentication stays bearer-only.""" +import mimetypes + from fastapi import APIRouter, Depends, HTTPException, Request -from fastapi.responses import FileResponse +from fastapi.responses import FileResponse, Response from sqlalchemy.orm import Session from app.database import get_db +from app.services import storage_service from app.utils.auth import get_current_user from app.utils.upload_access import ( LEGACY_LMS_PREFIXES, local_upload_path, upload_file, references, @@ -41,8 +44,19 @@ def _read_upload(path, request, attempt_id, db, headers): if not can_read_upload(db, path, user, questions, cards, attempt_id): raise HTTPException(404, "File not found") # Known legacy LMS directories retain their policy; this is not a whole-LMS audit. - if not target.is_file(): - raise HTTPException(404, "File not found") if target.suffix.lower() == ".svg": headers["Content-Security-Policy"] = "sandbox; default-src 'none'; style-src 'unsafe-inline'" - return FileResponse(target, headers=headers) + + # Serving must go through the storage service, or object storage would be + # write-only: the bytes would be in the bucket and still read from disk. + # The path is already authorised and confined to the upload root above. + if target.is_file(): + return FileResponse(target, headers=headers) + data = storage_service.s3_object(path) + if data is None: + raise HTTPException(404, "File not found") + media_type = mimetypes.guess_type(path)[0] or "application/octet-stream" + if request.method == "HEAD": + return Response(status_code=200, headers={**headers, "Content-Length": str(len(data))}, + media_type=media_type) + return Response(content=data, media_type=media_type, headers=headers) diff --git a/backend/app/services/storage_service.py b/backend/app/services/storage_service.py index 81087a2..12ba313 100644 --- a/backend/app/services/storage_service.py +++ b/backend/app/services/storage_service.py @@ -138,6 +138,20 @@ def presigned_url(key: str, expires: int = 3600) -> str | None: return None +def s3_object(key: str) -> bytes | None: + """Read strictly from S3, with no fallback. + + `load` falls back to the volume, which is right for serving but useless for + verifying a migration: it would compare a local file against itself. + """ + if not using_s3(): + return None + try: + return _s3().get_object(Bucket=settings.S3_BUCKET, Key=key)["Body"].read() + except Exception: + return None + + def size_of(key: str) -> int | None: if using_s3(): try: diff --git a/backend/scripts/migrate_uploads_to_s3.py b/backend/scripts/migrate_uploads_to_s3.py new file mode 100644 index 0000000..5378727 --- /dev/null +++ b/backend/scripts/migrate_uploads_to_s3.py @@ -0,0 +1,120 @@ +"""Copy the existing uploads volume into object storage. + +Verification order matters: every file is copied and read back *before* anything +is deleted, and deletion only happens when explicitly asked for. A migration that +removes the source before proving the destination is readable has no way back. + + docker compose exec backend python -m scripts.migrate_uploads_to_s3 + docker compose exec backend python -m scripts.migrate_uploads_to_s3 --apply + docker compose exec backend python -m scripts.migrate_uploads_to_s3 --verify + docker compose exec backend python -m scripts.migrate_uploads_to_s3 --apply --delete-local +""" +import hashlib +import sys +from pathlib import Path + +from app.config import settings +from app.services import storage_service + + +def walk(root: Path): + """Every file under the uploads directory, keyed by its relative path.""" + for path in sorted(root.rglob("*")): + if path.is_file() and not path.name.startswith("."): + yield path.relative_to(root).as_posix(), path + + +def human(size: int) -> str: + for unit in ("B", "KB", "MB", "GB"): + if size < 1024 or unit == "GB": + return f"{size:.0f} {unit}" if unit == "B" else f"{size:.1f} {unit}" + size /= 1024 + return f"{size:.1f} GB" + + +def main(): + apply_changes = "--apply" in sys.argv + verify_only = "--verify" in sys.argv + delete_local = "--delete-local" in sys.argv + + if not storage_service.using_s3(): + print("STORAGE_BACKEND is not 's3'; nothing to migrate into.") + return 1 + + root = Path(settings.UPLOAD_DIR) + if not root.is_dir(): + print(f"No uploads directory at {root}") + return 1 + + storage_service.ensure_bucket() + files = list(walk(root)) + total_bytes = sum(path.stat().st_size for _key, path in files) + print(f" files on the volume : {len(files)} ({human(total_bytes)})") + + copied, already, failed, verified, mismatched = 0, 0, [], 0, [] + for key, path in files: + data = path.read_bytes() + digest = hashlib.sha256(data).hexdigest() + + # Strictly S3: the fallback in `load` would compare a file to itself. + if verify_only or storage_service.s3_object(key) is not None: + stored = storage_service.s3_object(key) + if stored is None: + failed.append(key) + elif hashlib.sha256(stored).hexdigest() != digest: + mismatched.append(key) + else: + verified += 1 + already += 1 + continue + + if not apply_changes: + copied += 1 + continue + + try: + storage_service.save(key, data) + # Read back before trusting it; a write that cannot be read is not a copy. + stored = storage_service.s3_object(key) + if stored is None or hashlib.sha256(stored).hexdigest() != digest: + mismatched.append(key) + else: + copied += 1 + verified += 1 + except Exception as error: + failed.append(f"{key}: {error}") + + print(f" copied : {copied}") + print(f" already in storage : {already}") + print(f" read back and match : {verified}") + if mismatched: + print(f" MISMATCHED : {len(mismatched)}") + for key in mismatched[:10]: + print(f" {key}") + if failed: + print(f" FAILED : {len(failed)}") + for key in failed[:10]: + print(f" {key}") + + if delete_local: + if mismatched or failed: + print("\n Refusing to delete: not every file verified.") + return 1 + if not apply_changes: + print("\n --delete-local needs --apply.") + return 1 + removed = 0 + for key, path in files: + # Belt and braces: prove the object reads before removing the source. + if storage_service.s3_object(key) is not None: + path.unlink() + removed += 1 + print(f" local files removed : {removed}") + + if not apply_changes and not verify_only: + print("\n Re-run with --apply to copy, then --verify to check.") + return 0 + + +if __name__ == "__main__": + sys.exit(main())