File upload improvements (CTFd#2451)

* Calculate a files sha1sum on upload for future local change detection purposes
* Allow clients to control the location of an uploaded file
* Adds the optional path field to the Uploaders class to control where files get uploaded to
* Closes CTFd#1595

Author: ColdHeat

CTFd/api/v1/files.py

@@ -90,10 +90,25 @@ def post(self):
         # challenge_id
         # page_id
 
+        # Handle situation where users attempt to upload multiple files with a single location
+        if len(files) > 1 and request.form.get("location"):
+            return {
+                "success": False,
+                "errors": {
+                    "location": ["Location cannot be specified with multiple files"]
+                },
+            }, 400
+
         objs = []
         for f in files:
             # uploads.upload_file(file=f, chalid=req.get('challenge'))
-            obj = uploads.upload_file(file=f, **request.form.to_dict())
+            try:
+                obj = uploads.upload_file(file=f, **request.form.to_dict())
+            except ValueError as e:
+                return {
+                    "success": False,
+                    "errors": {"location": [str(e)]},
+                }, 400
             objs.append(obj)
 
         schema = FileSchema(many=True)

CTFd/models/__init__.py

@@ -288,6 +288,7 @@ class Files(db.Model):
     id = db.Column(db.Integer, primary_key=True)
     type = db.Column(db.String(80), default="standard")
     location = db.Column(db.Text)
+    sha1sum = db.Column(db.String(40))
 
     __mapper_args__ = {"polymorphic_identity": "standard", "polymorphic_on": type}
 

CTFd/utils/uploads/__init__.py

@@ -1,4 +1,6 @@
+import hashlib
 import shutil
+from pathlib import Path
 
 from CTFd.models import ChallengeFiles, Files, PageFiles, db
 from CTFd.utils import get_app_config
@@ -16,8 +18,23 @@ def upload_file(*args, **kwargs):
     challenge_id = kwargs.get("challenge_id") or kwargs.get("challenge")
     page_id = kwargs.get("page_id") or kwargs.get("page")
     file_type = kwargs.get("type", "standard")
-
-    model_args = {"type": file_type, "location": None}
+    location = kwargs.get("location")
+
+    # Validate location and default filename to uploaded file's name
+    parent = None
+    filename = file_obj.filename
+    if location:
+        path = Path(location)
+        if len(path.parts) != 2:
+            raise ValueError(
+                "Location must contain two parts, a directory and a filename"
+            )
+        # Allow location to override the directory and filename
+        parent = path.parts[0]
+        filename = path.parts[1]
+        location = parent + "/" + filename
+
+    model_args = {"type": file_type, "location": location}
 
     model = Files
     if file_type == "challenge":
@@ -28,16 +45,39 @@ def upload_file(*args, **kwargs):
         model_args["page_id"] = page_id
 
     uploader = get_uploader()
-    location = uploader.upload(file_obj=file_obj, filename=file_obj.filename)
+    location = uploader.upload(file_obj=file_obj, filename=filename, path=parent)
 
-    model_args["location"] = location
+    sha1sum = hash_file(fp=file_obj)
 
-    file_row = model(**model_args)
-    db.session.add(file_row)
-    db.session.commit()
+    model_args["location"] = location
+    model_args["sha1sum"] = sha1sum
+
+    existing_file = Files.query.filter_by(location=location).first()
+    if existing_file:
+        for k, v in model_args.items():
+            setattr(existing_file, k, v)
+        db.session.commit()
+        file_row = existing_file
+    else:
+        file_row = model(**model_args)
+        db.session.add(file_row)
+        db.session.commit()
     return file_row
 
 
+def hash_file(fp, algo="sha1"):
+    fp.seek(0)
+    if algo == "sha1":
+        h = hashlib.sha1()  # nosec
+        # https://stackoverflow.com/a/64730457
+        while chunk := fp.read(1024):
+            h.update(chunk)
+        fp.seek(0)
+        return h.hexdigest()
+    else:
+        raise NotImplementedError
+
+
 def delete_file(file_id):
     f = Files.query.filter_by(id=file_id).first_or_404()
 

CTFd/utils/uploads/uploaders.py

@@ -54,13 +54,20 @@ def store(self, fileobj, filename):
 
         return filename
 
-    def upload(self, file_obj, filename):
+    def upload(self, file_obj, filename, path=None):
         if len(filename) == 0:
             raise Exception("Empty filenames cannot be used")
 
+        # Sanitize directory name
+        if path:
+            path = secure_filename(path) or hexencode(os.urandom(16))
+            path = path.replace(".", "")
+        else:
+            path = hexencode(os.urandom(16))
+
+        # Sanitize file name
         filename = secure_filename(filename)
-        md5hash = hexencode(os.urandom(16))
-        file_path = posixpath.join(md5hash, filename)
+        file_path = posixpath.join(path, filename)
 
         return self.store(file_obj, file_path)
 
@@ -110,17 +117,25 @@ def store(self, fileobj, filename):
         self.s3.upload_fileobj(fileobj, self.bucket, filename)
         return filename
 
-    def upload(self, file_obj, filename):
+    def upload(self, file_obj, filename, path=None):
+        # Sanitize directory name
+        if path:
+            path = secure_filename(path) or hexencode(os.urandom(16))
+            path = path.replace(".", "")
+            # Sanitize path
+            path = filter(self._clean_filename, secure_filename(path).replace(" ", "_"))
+        else:
+            path = hexencode(os.urandom(16))
+
+        # Sanitize file name
         filename = filter(
             self._clean_filename, secure_filename(filename).replace(" ", "_")
         )
         filename = "".join(filename)
         if len(filename) <= 0:
             return False
 
-        md5hash = hexencode(os.urandom(16))
-
-        dst = md5hash + "/" + filename
+        dst = path + "/" + filename
         self.s3.upload_fileobj(file_obj, self.bucket, dst)
         return dst
 

migrations/versions/5c4996aeb2cb_add_sha1sum_field_to_files_require_.py

@@ -0,0 +1,23 @@
+"""Add sha1sum field to Files
+
+Revision ID: 5c4996aeb2cb
+Revises: 9e6f6578ca84
+Create Date: 2024-01-07 13:09:08.843903
+
+"""
+import sqlalchemy as sa
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "5c4996aeb2cb"
+down_revision = "9e6f6578ca84"
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    op.add_column("files", sa.Column("sha1sum", sa.String(length=40), nullable=True))
+
+
+def downgrade():
+    op.drop_column("files", "sha1sum")

tests/api/v1/test_files.py

@@ -2,6 +2,7 @@
 # -*- coding: utf-8 -*-
 
 import os
+import pathlib
 import shutil
 from io import BytesIO
 
@@ -75,6 +76,7 @@ def test_api_files_post_admin():
             )
             assert r.status_code == 200
             f = Files.query.filter_by(id=1).first()
+            assert f.sha1sum == "9032bbc224ed8b39183cb93b9a7447727ce67f9d"
             os.remove(os.path.join(app.config["UPLOAD_FOLDER"] + "/" + f.location))
     destroy_ctfd(app)
 
@@ -137,3 +139,104 @@ def test_api_file_delete_admin():
             shutil.rmtree(os.path.dirname(path), ignore_errors=True)
 
     destroy_ctfd(app)
+
+
+def test_api_file_custom_location():
+    """
+    Test file uploading with custom location
+    """
+    app = create_ctfd()
+    with app.app_context():
+        with login_as_user(app, name="admin") as client:
+            with client.session_transaction() as sess:
+                nonce = sess.get("nonce")
+            r = client.post(
+                "/api/v1/files",
+                content_type="multipart/form-data",
+                data={
+                    "file": (BytesIO(b"test file content"), "test.txt"),
+                    "location": "testing/asdf.txt",
+                    "nonce": nonce,
+                },
+            )
+            assert r.status_code == 200
+            f = Files.query.filter_by(id=1).first()
+            assert f.sha1sum == "9032bbc224ed8b39183cb93b9a7447727ce67f9d"
+            assert f.location == "testing/asdf.txt"
+            r = client.get("/files/" + f.location)
+            assert r.get_data(as_text=True) == "test file content"
+
+            r = client.get("/api/v1/files/1")
+            response = r.get_json()
+            assert (
+                response["data"]["sha1sum"]
+                == "9032bbc224ed8b39183cb93b9a7447727ce67f9d"
+            )
+            assert response["data"]["location"] == "testing/asdf.txt"
+
+            # Test deletion
+            r = client.delete("/api/v1/files/1", json="")
+            assert r.status_code == 200
+            assert Files.query.count() == 0
+
+            target = pathlib.Path(app.config["UPLOAD_FOLDER"]) / f.location
+            assert target.exists() is False
+
+            # Test invalid locations
+            invalid_paths = [
+                "testing/prefix/asdf.txt",
+                "/testing/asdf.txt",
+                "asdf.txt",
+            ]
+            for path in invalid_paths:
+                r = client.post(
+                    "/api/v1/files",
+                    content_type="multipart/form-data",
+                    data={
+                        "file": (BytesIO(b"test file content"), "test.txt"),
+                        "location": path,
+                        "nonce": nonce,
+                    },
+                )
+                assert r.status_code == 400
+    destroy_ctfd(app)
+
+
+def test_api_file_overwrite_by_location():
+    """
+    Test file overwriting with a specific location
+    """
+    app = create_ctfd()
+    with app.app_context():
+        with login_as_user(app, name="admin") as client:
+            with client.session_transaction() as sess:
+                nonce = sess.get("nonce")
+            r = client.post(
+                "/api/v1/files",
+                content_type="multipart/form-data",
+                data={
+                    "file": (BytesIO(b"test file content"), "test.txt"),
+                    "location": "testing/asdf.txt",
+                    "nonce": nonce,
+                },
+            )
+            assert r.status_code == 200
+            f = Files.query.filter_by(id=1).first()
+            r = client.get("/files/" + f.location)
+            assert r.get_data(as_text=True) == "test file content"
+
+            r = client.post(
+                "/api/v1/files",
+                content_type="multipart/form-data",
+                data={
+                    "file": (BytesIO(b"testing new uploaded file content"), "test.txt"),
+                    "location": "testing/asdf.txt",
+                    "nonce": nonce,
+                },
+            )
+            assert r.status_code == 200
+            f = Files.query.filter_by(id=1).first()
+            r = client.get("/files/" + f.location)
+            assert f.sha1sum == "0ee7eb85ac0b8d8ae03f3080589157cde553b13f"
+            assert r.get_data(as_text=True) == "testing new uploaded file content"
+    destroy_ctfd(app)