Verify user,group/org images upload type, enforce extension

Author: amercader

CHANGELOG.rst

@@ -9,6 +9,21 @@ Changelog
 
 .. towncrier release notes start
 
+v.2.12.0 (Not yet released)
+===========================
+
+Migration notes
+---------------
+
+* Going forward, if both ``ckan.upload.[type].mimetypes`` and
+  ``ckan.upload.[type].types`` are empty, no uploads will be allowed
+  for this object type (e.g. ``user`` or ``group``). It previoulsy
+  meant that all file types were allowed. To keep the old behaviour use
+  the string ``*`` as value in both options (this is dangerous and
+  **not** recommended).
+
+
+
 v.2.11.1 2024-12-11
 ===================
 

ckan/config/config_declaration.yaml

@@ -1328,25 +1328,36 @@ groups:
         type: list
         default: image
         example: image text
-        description: File types allowed to upload as user's avatar. No restrictions applied when empty
+        description: File types allowed to upload as user's avatar. If empty and :ref:`ckan.upload.user.mimetypes`
+          is also empty, no uploads are allowed. To allow any kind of file upload, use the ``*`` string in both
+          options (this is dangerous and **not** recommended). Note also that ``text/svg`` can contain embedded
+          javascript code so it only should be used in trusted environments.
 
       - key: ckan.upload.user.mimetypes
         type: list
         default: image/png image/gif image/jpeg
-        example: image/png text/svg
-        description: File MIMETypes allowed to upload as user's avatar. No restrictions applied when empty
+        example: image/png
+        description: File MIMETypes allowed to upload as user's avatar. If empty and :ref:`ckan.upload.user.types`
+          is also empty, no uploads are allowed. To allow any kind of file upload, use the ``*`` string in both
+          options (this is dangerous and **not** recommended).
 
       - key: ckan.upload.group.types
         type: list
         default: image
         example: image text
-        description: File types allowed to upload as group image. No restrictions applied when empty
+        description: File types allowed to upload as group or organization image. If empty
+          and :ref:`ckan.upload.group.mimetypes` is also empty, no uploads are allowed. To allow any kind of
+          file upload, use the ``*`` string in both options (this is dangerous and **not** recommended).
 
       - key: ckan.upload.group.mimetypes
         type: list
         default: image/png image/gif image/jpeg
-        example: image/png text/svg
-        description: File MIMETypes allowed to upload as group image. No restrictions applied when empty
+        example: image/png
+        description: File MIMEtypes allowed to upload as group or organization image. If empty
+          and :ref:`ckan.upload.group.types` is also empty, no uploads are allowed. To allow any kind of
+          file upload, use the ``*`` string in both options (this is dangerous and **not** recommended).
+          Note also that ``text/svg`` can contain embedded javascript code so it only should be used in
+          trusted environments.
 
   - annotation: Webassets Settings
     options:

ckan/lib/uploader.py

@@ -7,6 +7,7 @@
 import logging
 import magic
 import mimetypes
+from pathlib import Path
 from typing import Any, IO, Optional, Union
 from urllib.parse import urlparse
 
@@ -159,10 +160,14 @@ def update_data_dict(self, data_dict: dict[str, Any], url_field: str,
                 self.filename = str(datetime.datetime.utcnow()) + self.filename
                 self.filename = munge.munge_filename_legacy(self.filename)
                 self.filepath = os.path.join(self.storage_path, self.filename)
-                data_dict[url_field] = self.filename
                 self.upload_file = _get_underlying_file(
                     self.upload_field_storage)
                 self.tmp_filepath = self.filepath + '~'
+
+                self.verify_type()
+
+                data_dict[url_field] = self.filename
+
         # keep the file if there has been no change
         elif self.old_filename and not self.old_filename.startswith('http'):
             if not self.clear:
@@ -177,7 +182,6 @@ def upload(self, max_size: int = 2) -> None:
         anything unless the request is actually good.
         max_size is size in MB maximum of the file'''
 
-        self.verify_type()
 
         if self.filename:
             assert self.upload_file and self.filepath
@@ -202,29 +206,66 @@ def upload(self, max_size: int = 2) -> None:
                 pass
 
     def verify_type(self):
-        if not self.filename or not self.upload_file:
+
+        if not self.upload_file:
             return
 
-        mimetypes = config.get(
+        allowed_mimetypes = config.get(
             f"ckan.upload.{self.object_type}.mimetypes")
-        types = config.get(f"ckan.upload.{self.object_type}.types")
-        if not mimetypes and not types:
-            return
+        allowed_types = config.get(f"ckan.upload.{self.object_type}.types")
+        if not allowed_mimetypes and not allowed_types:
+            raise logic.ValidationError(
+                {
+                    self.file_field: [f"No uploads allowed for object type {self.object_type}"]
+                }
+            )
+
+        # Check that the declared types in the request are supported
+        declared_mimetype_from_filename = mimetypes.guess_type(
+            self.upload_field_storage.filename
+        )[0]
+        declared_content_type = self.upload_field_storage.content_type
+        for declared_mimetype in (
+            declared_mimetype_from_filename,
+            declared_content_type,
+        ):
+            if (
+                declared_mimetype
+                and allowed_mimetypes
+                and allowed_mimetypes[0] != "*"
+                and declared_mimetype not in allowed_mimetypes
+            ):
+                raise logic.ValidationError(
+                    {
+                        self.file_field: [
+                            f"Unsupported upload type: {declared_mimetype}"
+                        ]
+                    }
+                )
+
+        # Check that the actual type guessed from the contents is supported
+        # (2KB required for detecting xlsx mimetype)
+        content = self.upload_file.read(2048)
+        guessed_mimetype = magic.from_buffer(content, mime=True)
 
-        # 2KB required for detecting xlsx mimetype
-        actual = magic.from_buffer(self.upload_file.read(2048), mime=True)
         self.upload_file.seek(0, os.SEEK_SET)
+
         err: ErrorDict = {
-            self.file_field: [f"Unsupported upload type: {actual}"]
+            self.file_field: [f"Unsupported upload type: {guessed_mimetype}"]
         }
 
-        if mimetypes and actual not in mimetypes:
+        if allowed_mimetypes and allowed_mimetypes[0] != "*" and guessed_mimetype not in allowed_mimetypes:
             raise logic.ValidationError(err)
 
-        type_ = actual.split("/")[0]
-        if types and type_ not in types:
+        type_ = guessed_mimetype.split("/")[0]
+        if allowed_types and allowed_types[0] != "*" and type_ not in allowed_types:
             raise logic.ValidationError(err)
 
+        preferred_extension = mimetypes.guess_extension(guessed_mimetype)
+        if preferred_extension:
+            self.filename = str(Path(self.filename).with_suffix(preferred_extension))
+            self.filepath = str(Path(self.filepath).with_suffix(preferred_extension))
+
 
 class ResourceUpload(object):
     mimetype: Optional[str]

ckan/tests/cli/test_clean.py

@@ -11,8 +11,8 @@ def test_output_if_there_are_not_invalid_users(self, cli):
         result = cli.invoke(ckan, ["clean", "users"])
         assert "No users were found with invalid images." in result.output
 
-    @pytest.mark.ckan_config("ckan.upload.user.mimetypes", "")
-    @pytest.mark.ckan_config("ckan.upload.user.types", "")
+    @pytest.mark.ckan_config("ckan.upload.user.mimetypes", "*")
+    @pytest.mark.ckan_config("ckan.upload.user.types", "*")
     def test_confirm_dialog_if_no_force(
         self, cli, monkeypatch, create_with_upload, faker, ckan_config
     ):
@@ -54,8 +54,8 @@ def test_confirm_dialog_if_no_force(
         users = call_action("user_list")
         assert len(users) == 2
 
-    @pytest.mark.ckan_config("ckan.upload.user.mimetypes", "")
-    @pytest.mark.ckan_config("ckan.upload.user.types", "")
+    @pytest.mark.ckan_config("ckan.upload.user.mimetypes", "*")
+    @pytest.mark.ckan_config("ckan.upload.user.types", "*")
     def test_correct_users_are_deleted(
         self, cli, monkeypatch, create_with_upload, faker, ckan_config
     ):

ckan/tests/lib/test_uploader.py

@@ -90,7 +90,7 @@ def test_group_upload(self, monkeypatch, tmpdir, make_app, ckan_config, faker):
                  u'upload': FileStorage(
                      BytesIO(faker.image()),
                      filename=u'logo.png',
-                     content_type=u'PNG'
+                     content_type=u'image/png'
                  ),
                  u'name': u'test-group-upload'}
         group_upload = Upload(u'group')

ckan/tests/logic/action/test_create.py

@@ -2226,6 +2226,47 @@ def test_upload_non_picture_with_png_extension(
                 logic.ValidationError, match="Unsupported upload type"):
             create_with_upload("hello world", "file.png", **params)
 
+    @pytest.mark.ckan_config("ckan.upload.user.mimetypes", "")
+    @pytest.mark.ckan_config("ckan.upload.user.types", "")
+    def test_uploads_not_allowed_when_empty_mimetypes_and_types(
+            self, create_with_upload, faker):
+        params = {
+            "name": faker.user_name(),
+            "email": faker.email(),
+            "password": "12345678",
+            "action": "user_create",
+            "upload_field_name": "image_upload",
+        }
+        with pytest.raises(
+                logic.ValidationError, match="No uploads allowed for object type"):
+            create_with_upload("hello world", "file.png", **params)
+
+    @pytest.mark.ckan_config("ckan.upload.user.mimetypes", "*")
+    @pytest.mark.ckan_config("ckan.upload.user.types", "image")
+    def test_upload_all_types_allowed_needs_both_options(self, create_with_upload, faker):
+        params = {
+            "name": faker.user_name(),
+            "email": faker.email(),
+            "password": "12345678",
+            "action": "user_create",
+            "upload_field_name": "image_upload",
+        }
+        with pytest.raises(
+                logic.ValidationError, match="Unsupported upload type"):
+            assert create_with_upload(faker.json(), "file.json", **params)
+
+    @pytest.mark.ckan_config("ckan.upload.user.mimetypes", "*")
+    @pytest.mark.ckan_config("ckan.upload.user.types", "*")
+    def test_upload_all_types_allowed(self, create_with_upload, faker):
+        params = {
+            "name": faker.user_name(),
+            "email": faker.email(),
+            "password": "12345678",
+            "action": "user_create",
+            "upload_field_name": "image_upload",
+        }
+        assert create_with_upload(faker.json(), "file.json", **params)
+
     @pytest.mark.ckan_config("ckan.upload.user.types", "image")
     def test_upload_picture(self, create_with_upload, faker):
         params = {
@@ -2237,6 +2278,20 @@ def test_upload_picture(self, create_with_upload, faker):
         }
         assert create_with_upload(faker.image(), "file.png", **params)
 
+    @pytest.mark.ckan_config("ckan.upload.user.types", "image")
+    def test_upload_picture_extension_enforced(self, create_with_upload, faker):
+        params = {
+            "name": faker.user_name(),
+            "email": faker.email(),
+            "password": "12345678",
+            "action": "user_create",
+            "upload_field_name": "image_upload",
+        }
+        user = create_with_upload(faker.image(image_format="jpeg"), "file.png", **params)
+
+        assert user["image_url"].endswith(".jpg")
+        assert user["image_display_url"].endswith(".jpg")
+
 
 class TestVocabularyCreate(object):
     @pytest.mark.usefixtures("non_clean_db")