Don't break on repeated identical files in .zip files (mozilla-services#1043)

* Don't break on repeated identical files in .zip files

Fixes #1042

* correct linting image

Author: Peter Bengtsson

.circleci/config.yml

@@ -40,7 +40,7 @@ jobs:
       - run:
           name: Run lint check
           command: |
-            docker-compose run test-ci lintcheck
+            docker-compose run linting-ci lintcheck
 
       - run:
           name: Linting check for frontend

Makefile

@@ -91,9 +91,9 @@ build-frontend:
 	docker-compose run -u 0 -e CI base ./bin/build_frontend.sh
 
 lintcheck: .env .docker-build
-	docker-compose run web lintcheck
+	docker-compose run linting lintcheck
 	docker-compose run frontend lint
 
 lintfix: .env .docker-build
-	docker-compose run web blackfix
+	docker-compose run linting blackfix
 	docker-compose run frontend lintfix

docker-compose.yml

@@ -48,6 +48,19 @@ services:
       - $PWD:/app
     command: web-dev
 
+  linting:
+    extends:
+      service: base
+    volumes:
+      - $PWD:/app
+    command: lintcheck
+
+  # Same as linting but without volume mounts for CI.
+  linting-ci:
+    extends:
+      service: base
+    command: lintcheck
+
   # Container specifically for running tests.
   test:
     extends:

docs/dev.rst

@@ -665,13 +665,13 @@ To check that all code is formatted correctly, run:
 
 .. code-block:: shell
 
-    $ docker-compose run web lintcheck
+    $ docker-compose run linting lintcheck
 
 If you have a bunch of formatting complaints you can automatically fix them all with:
 
 .. code-block:: shell
 
-    $ docker-compose run web blackfix
+    $ docker-compose run linting blackfix
 
 
 Debugging ``minio`` container

tecken/upload/utils.py

@@ -33,6 +33,11 @@ class UnrecognizedArchiveFileExtension(ValueError):
     to extract."""
 
 
+class DuplicateFileDifferentSize(ValueError):
+    """When a zip file contains two identically named files whose file size is
+    different."""
+
+
 def get_file_md5_hash(fn, blocksize=65536):
     hasher = hashlib.md5()
     with open(fn, "rb") as f:
@@ -53,12 +58,28 @@ def dump_and_extract(root_dir, file_buffer, name):
     if name.lower().endswith(".zip"):
         zf = zipfile.ZipFile(file_buffer)
         zf.extractall(root_dir)
+        namelist = zf.namelist()
+        # If there are repeated names in the namelist, dig deeper!
+        if len(set(namelist)) != len(namelist):
+            # It's only a problem any of the files within are of different size
+            sizes = {}
+            for info in zf.infolist():
+                if info.filename in sizes:
+                    if info.file_size != sizes[info.filename]:
+                        raise DuplicateFileDifferentSize(
+                            "The zipfile buffer contains two files both called "
+                            f"{info.filename} and they have difference sizes "
+                            "({} != {})".format(info.file_size, sizes[info.filename])
+                        )
+                sizes[info.filename] = info.file_size
+        namelist = set(namelist)
+
     else:
         raise UnrecognizedArchiveFileExtension(os.path.splitext(name)[1])
 
     return [
         FileMember(os.path.join(root_dir, filename), filename)
-        for filename in zf.namelist()
+        for filename in namelist
         if os.path.isfile(os.path.join(root_dir, filename))
     ]
 

tecken/upload/views.py

@@ -32,6 +32,7 @@
 from tecken.upload.utils import (
     dump_and_extract,
     UnrecognizedArchiveFileExtension,
+    DuplicateFileDifferentSize,
     upload_file_upload,
 )
 from tecken.symbolicate.tasks import invalidate_symbolicate_cache_task
@@ -212,6 +213,8 @@ def upload_archive(request, upload_dir):
         return http.JsonResponse(
             {"error": f'Unrecognized archive file extension "{exception}"'}, status=400
         )
+    except DuplicateFileDifferentSize as exception:
+        return http.JsonResponse({"error": str(exception)}, status=400)
     error = check_symbols_archive_file_listing(file_listing)
     if error:
         return http.JsonResponse({"error": error.strip()}, status=400)

tests/duplicated-different-size.zip

@@ -30,11 +30,16 @@
 from tecken.upload.tasks import update_uploads_created_task
 
 
-_here = os.path.dirname(__file__)
-ZIP_FILE = os.path.join(_here, "sample.zip")
-INVALID_ZIP_FILE = os.path.join(_here, "invalid.zip")
-INVALID_CHARACTERS_ZIP_FILE = os.path.join(_here, "invalid-characters.zip")
-ACTUALLY_NOT_ZIP_FILE = os.path.join(_here, "notazipdespiteitsname.zip")
+def _join(x):
+    return os.path.join(os.path.dirname(__file__), x)
+
+
+ZIP_FILE = _join("sample.zip")
+INVALID_ZIP_FILE = _join("invalid.zip")
+INVALID_CHARACTERS_ZIP_FILE = _join("invalid-characters.zip")
+ACTUALLY_NOT_ZIP_FILE = _join("notazipdespiteitsname.zip")
+DUPLICATED_SAME_SIZE_ZIP_FILE = _join("duplicated-same-size.zip")
+DUPLICATED_DIFFERENT_SIZE_ZIP_FILE = _join("duplicated-different-size.zip")
 
 
 class FakeUser:
@@ -67,6 +72,13 @@ def test_dump_and_extract(tmpdir):
     assert os.path.isfile(os.path.join(tmpdir, "build-symbols.txt"))
 
 
+def test_dump_and_extract_duplicate_name_same_size(tmpdir):
+    with open(DUPLICATED_SAME_SIZE_ZIP_FILE, "rb") as f:
+        file_listings = dump_and_extract(tmpdir, f, DUPLICATED_SAME_SIZE_ZIP_FILE)
+    # Even though the file contains 2 files.
+    assert len(file_listings) == 1
+
+
 def test_should_compressed_key(settings):
     settings.COMPRESS_EXTENSIONS = ["bar"]
     assert should_compressed_key("foo.bar")
@@ -1186,6 +1198,25 @@ def test_upload_client_bad_request(fakeuser, client, settings):
         assert response.json()["error"] == error_msg
 
 
+@pytest.mark.django_db
+def test_upload_duplicate_files_in_zip_different_name(fakeuser, client, settings):
+    url = reverse("upload:upload_archive")
+    token = Token.objects.create(user=fakeuser)
+    permission, = Permission.objects.filter(codename="upload_symbols")
+    token.permissions.add(permission)
+
+    # Upload a file whose members have some repeated name AND different size
+    with open(DUPLICATED_DIFFERENT_SIZE_ZIP_FILE, "rb") as f:
+        response = client.post(url, {"file.zip": f}, HTTP_AUTH_TOKEN=token.key)
+        assert response.status_code == 400
+        error_msg = (
+            "The zipfile buffer contains two files both called "
+            "foo.pdb/50FEB8DBDC024C66B42193D881CE80E12/ntdll.sym and they have "
+            "difference sizes (24 != 39)"
+        )
+        assert response.json()["error"] == error_msg
+
+
 @pytest.mark.django_db
 def test_upload_client_unrecognized_bucket(botomock, fakeuser, client):
     """The upload view raises an error if you try to upload into a bucket