Repository: plone/plone.restapi (language: python)

Issue:
It's possible to set an invalid short name when adding content
If an `id` is specified while creating content via the [FolderPost](https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/content/add.py) endpoint, it is used without validating that it meets the criteria for a valid id. For example, it's possible to add an item with an id containing spaces.

What I would expect: either the id should be normalized (this is what already happens if it is set while editing the item later), or trying to add an item with an invalid id should return a 400 response with a helpful error message.

Note: This might be considered a backwards incompatible change, since someone calling the API may be assuming that the item has the id that was specified, and not a normalized form of it. But, it could also be considered a bugfix, because it never should have been possible to add items with an invalid id.

Thoughts on how to handle this, @tisto?


Discussion / hints:
Some time ago we faced an issue adding a content with `layout` as an id, and found that Plone was doing some id checks, somewhere, sometimes. See https://github.com/plone/Products.CMFPlone/issues/1931 and https://github.com/plone/Products.CMFPlone/pull/1932.

I think we should have one single place in CMFPlone where the id validation is done, and then use that validator here in p.restapi too.
@erral There is already a way to get a valid id that takes into account those checks, which is an INameChooser adapter on the container. The restapi endpoint for adding content already uses it when the API client did not explicitly specify an id: https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/content/utils.py#L93

But, that is getting skipped when the client specified an id: https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/content/add.py#L93

It's an easy fix to use the name chooser there too. The question is, will the change in behavior cause any problems?
@davisagli if I recall correctly we added the id option mainly for migrations, so that a migration algorithm is able to set an ID. You are correct that we should validate this id no matter what.

Adding a check for a valid ID on the passed id param is not a breaking change IMO. We are just preventing the system from ending up with an invalid id/path.
I agree with Timo, I think we should do the same as Classic UI does. If we don't allow spaces in id in Classic we should neither allow that in p.restapi
+1 for Timo's pov.

Fix patch:
diff --git a/news/1487.bugfix b/news/1487.bugfix
new file mode 100644
index 0000000000..3f71368baa
--- /dev/null
+++ b/news/1487.bugfix
@@ -0,0 +1,3 @@
+When an `id` is specified explicitly in the content POST endpoint,
+return a 400 error response if it is invalid or unavailable.
+[davisagli]
\ No newline at end of file
diff --git a/src/plone/restapi/services/content/add.py b/src/plone/restapi/services/content/add.py
index cadedb991f..11bc6742b7 100644
--- a/src/plone/restapi/services/content/add.py
+++ b/src/plone/restapi/services/content/add.py
@@ -90,7 +90,11 @@ def reply(self):
         if not getattr(deserializer, "notifies_create", False):
             notify(ObjectCreatedEvent(obj))
 
-        obj = add(self.context, obj, rename=not bool(id_))
+        try:
+            obj = add(self.context, obj, rename=not bool(id_))
+        except ValueError as e:
+            self.request.response.setStatus(400)
+            return dict(error=dict(type="ValueError", message=str(e)))
 
         # Link translation given the translation_of property
         if (
diff --git a/src/plone/restapi/services/content/utils.py b/src/plone/restapi/services/content/utils.py
index beb23b98ef..34881eb8d5 100644
--- a/src/plone/restapi/services/content/utils.py
+++ b/src/plone/restapi/services/content/utils.py
@@ -70,19 +70,19 @@ def create(container, type_, id_=None, title=None):
 
 def add(container, obj, rename=True):
     """Add an object to a container."""
-    id_ = getattr(aq_base(obj), "id", None)
+    old_id = getattr(aq_base(obj), "id", None)
 
     # Archetypes objects are already created in a container thus we just fire
     # the notification events and rename the object if necessary.
     if base_hasattr(obj, "_at_rename_after_creation"):
-        notify(ObjectAddedEvent(obj, container, id_))
+        notify(ObjectAddedEvent(obj, container, old_id))
         notifyContainerModified(container)
         if obj._at_rename_after_creation and rename:
             obj._renameAfterCreation(check_auto_id=True)
         return obj
     else:
+        chooser = INameChooser(container)
         if rename:
-            chooser = INameChooser(container)
             # INameFromTitle adaptable objects should not get a name
             # suggestion. NameChooser would prefer the given name instead of
             # the one provided by the INameFromTitle adapter.
@@ -90,8 +90,12 @@ def add(container, obj, rename=True):
             name_from_title = INameFromTitle(obj, None)
             if name_from_title is None:
                 suggestion = obj.Title()
-            id_ = chooser.chooseName(suggestion, obj)
-            obj.id = id_
+        else:
+            suggestion = old_id
+        id_ = chooser.chooseName(suggestion, obj)
+        if not rename and id_ != old_id:
+            raise ValueError(f"id is invalid or already used: {old_id}")
+        obj.id = id_
         new_id = container._setObject(id_, obj)
         # _setObject triggers ObjectAddedEvent which can end up triggering a
         # content rule to move the item to a different container. In this case


Test patch:
diff --git a/src/plone/restapi/tests/test_content_post.py b/src/plone/restapi/tests/test_content_post.py
index 7958821c81..57d38ef2a6 100644
--- a/src/plone/restapi/tests/test_content_post.py
+++ b/src/plone/restapi/tests/test_content_post.py
@@ -139,7 +139,27 @@ def test_post_without_id_creates_id_from_filename(self):
         transaction.begin()
         self.assertIn("test.txt", self.portal.folder1)
 
-    def test_post_with_id_already_in_use_returns_400(self):
+    def test_post_with_explicit_file_id_allows_dots(self):
+        response = requests.post(
+            self.portal.folder1.absolute_url(),
+            headers={"Accept": "application/json"},
+            auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD),
+            json={
+                "@type": "File",
+                "title": "My File",
+                "file": {
+                    "filename": "test.txt",
+                    "data": "Spam and Eggs",
+                    "content_type": "text/plain",
+                },
+                "id": "custom.txt",
+            },
+        )
+        self.assertEqual(201, response.status_code)
+        transaction.begin()
+        self.assertIn("custom.txt", self.portal.folder1)
+
+    def test_post_with_id_already_in_use_returns_validation_error(self):
         self.portal.folder1.invokeFactory("Document", "mydocument")
         transaction.commit()
         response = requests.post(
@@ -149,6 +169,49 @@ def test_post_with_id_already_in_use_returns_400(self):
             json={"@type": "Document", "id": "mydocument", "title": "My Document"},
         )
         self.assertEqual(400, response.status_code)
+        self.assertEqual(
+            "id is invalid or already used: mydocument",
+            response.json().get("error").get("message"),
+        )
+
+    def test_post_with_invalid_id_chars_returns_validation_error(self):
+        response = requests.post(
+            self.portal.folder1.absolute_url(),
+            headers={"Accept": "application/json"},
+            auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD),
+            json={"@type": "Document", "id": "my document", "title": "My Document"},
+        )
+        self.assertEqual(400, response.status_code)
+        self.assertEqual(
+            "id is invalid or already used: my document",
+            response.json().get("error").get("message"),
+        )
+
+    def test_post_with_reserved_id_returns_validation_error(self):
+        # Some short names are reserved to avoid colliding with field names, etc
+        response = requests.post(
+            self.portal.folder1.absolute_url(),
+            headers={"Accept": "application/json"},
+            auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD),
+            json={"@type": "Document", "id": "layout", "title": "My Document"},
+        )
+        self.assertEqual(400, response.status_code)
+        self.assertEqual(
+            "id is invalid or already used: layout",
+            response.json().get("error").get("message"),
+        )
+
+    def test_post_with_empty_id_creates_id_from_title(self):
+        # spaces in id get replaced with dashes
+        response = requests.post(
+            self.portal.folder1.absolute_url(),
+            headers={"Accept": "application/json"},
+            auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD),
+            json={"@type": "Document", "id": "", "title": "My Document"},
+        )
+        self.assertEqual(201, response.status_code)
+        transaction.begin()
+        self.assertIn("my-document", self.portal.folder1)
 
     def test_post_to_folder_returns_401_unauthorized(self):
         response = requests.post(