Merge pull request #624 from Unrud/improveerrors

Small improvements for error handling

radicale/__init__.py

@@ -788,6 +788,8 @@ class Application:
 
             try:
                 items = list(vobject.readComponents(content or ""))
+                for item in items:
+                    storage.check_item(item)
             except Exception as e:
                 self.logger.warning(
                     "Bad PUT request on %r: %s", path, e, exc_info=True)
@@ -797,13 +799,23 @@ class Application:
             tag = tags.get(content_type)
 
             if write_whole_collection:
+                try:
                     new_item = self.Collection.create_collection(
                         path, items, {"tag": tag})
+                except ValueError as e:
+                    self.logger.warning(
+                        "Bad PUT request on %r: %s", path, e, exc_info=True)
+                    return BAD_REQUEST
             else:
                 if tag:
                     parent_item.set_meta({"tag": tag})
                 href = posixpath.basename(path.strip("/"))
+                try:
                     new_item = parent_item.upload(href, items[0])
+                except ValueError as e:
+                    self.logger.warning(
+                        "Bad PUT request on %r: %s", path, e, exc_info=True)
+                    return BAD_REQUEST
             headers = {"ETag": new_item.etag}
             return client.CREATED, headers, None
 

radicale/storage.py

@@ -27,7 +27,6 @@ entry.
 
 import binascii
 import contextlib
-import errno
 import json
 import os
 import pickle
@@ -115,6 +114,20 @@ def load(configuration, logger):
     return CollectionCopy
 
 
+def check_item(vobject_item):
+    """Check vobject items for common errors."""
+    if vobject_item.name == "VCALENDAR":
+        for component in vobject_item.components():
+            if (component.name in ("VTODO", "VEVENT", "VJOURNAL") and
+                    not get_uid(component)):
+                raise ValueError("UID in %s is missing" % component.name)
+    elif vobject_item.name == "VCARD":
+        if not get_uid(vobject_item):
+            raise ValueError("UID in VCARD is missing")
+    else:
+        raise ValueError("Unknown item type: %r" % vobject_item.name)
+
+
 def scandir(path, only_dirs=False, only_files=False):
     """Iterator for directory elements. (For compatibility with Python < 3.5)
 
@@ -149,7 +162,7 @@ def get_etag(text):
 
 def get_uid(item):
     """UID value of an item if defined."""
-    return hasattr(item, "uid") and item.uid.value
+    return (hasattr(item, "uid") or None) and item.uid.value
 
 
 def sanitize_path(path):
@@ -541,13 +554,14 @@ class Collection(BaseCollection):
         self._sync_directory(directory)
 
     @staticmethod
-    def _find_available_file_name(exists_fn):
+    def _find_available_file_name(exists_fn, suffix=""):
         # Prevent infinite loop
-        for _ in range(10000):
+        for _ in range(1000):
-            file_name = hex(getrandbits(32))[2:]
+            file_name = "%016x" % getrandbits(64) + suffix
             if not exists_fn(file_name):
                 return file_name
-        raise FileExistsError(errno.EEXIST, "No usable file name found")
+        # something is wrong with the PRNG
+        raise RuntimeError("No unique random sequence found")
 
     @classmethod
     def _fsync(cls, fd):
@@ -690,15 +704,19 @@ class Collection(BaseCollection):
                         new_collection = vobject.iCalendar()
                         for item in items:
                             new_collection.add(item)
+                        # href must comply to is_safe_filesystem_path_component
+                        # and no file name collisions must exist between hrefs
                         href = self._find_available_file_name(
-                            vobject_items.get)
+                            vobject_items.get, suffix=".ics")
                         vobject_items[href] = new_collection
                     self.upload_all_nonatomic(vobject_items)
                 elif props.get("tag") == "VCARD":
                     vobject_items = {}
                     for card in collection:
+                        # href must comply to is_safe_filesystem_path_component
+                        # and no file name collisions must exist between hrefs
                         href = self._find_available_file_name(
-                            vobject_items.get)
+                            vobject_items.get, suffix=".vcf")
                         vobject_items[href] = card
                     self.upload_all_nonatomic(vobject_items)
 
@@ -982,8 +1000,13 @@ class Collection(BaseCollection):
             cinput_hash = cetag = ctext = ctag = cstart = cend = None
         vobject_item = None
         if input_hash != cinput_hash:
+            try:
                 vobject_item = Item(self, href=href,
                                     text=btext.decode(self.encoding)).item
+                check_item(vobject_item)
+            except Exception as e:
+                raise RuntimeError("Failed to parse item %r from %r: %s" %
+                                   (href, self.path, e)) from e
             # Serialize the object again, to normalize the text representation.
             # The storage may have been edited externally.
             ctext = vobject_item.serialize()

radicale/tests/static/todo2.ics

@@ -23,5 +23,6 @@ BEGIN:VTODO
 DTSTART;TZID=Europe/Paris:20130901T180000
 DUE;TZID=Europe/Paris:20130903T180000
 RRULE:FREQ=MONTHLY
+UID:todo2
 END:VTODO
 END:VCALENDAR

radicale/tests/static/todo3.ics

@@ -21,5 +21,6 @@ END:STANDARD
 END:VTIMEZONE
 BEGIN:VTODO
 DTSTART;TZID=Europe/Paris:20130901T180000
+UID:todo3
 END:VTODO
 END:VCALENDAR

radicale/tests/static/todo4.ics

@@ -21,5 +21,6 @@ END:STANDARD
 END:VTIMEZONE
 BEGIN:VTODO
 DUE;TZID=Europe/Paris:20130901T180000
+UID:todo4
 END:VTODO
 END:VCALENDAR

radicale/tests/static/todo5.ics

@@ -22,5 +22,6 @@ END:VTIMEZONE
 BEGIN:VTODO
 CREATED;TZID=Europe/Paris:20130903T180000
 COMPLETED;TZID=Europe/Paris:20130920T180000
+UID:todo5
 END:VTODO
 END:VCALENDAR

radicale/tests/static/todo6.ics

@@ -21,5 +21,6 @@ END:STANDARD
 END:VTIMEZONE
 BEGIN:VTODO
 COMPLETED;TZID=Europe/Paris:20130920T180000
+UID:todo6
 END:VTODO
 END:VCALENDAR

radicale/tests/static/todo7.ics

@@ -21,5 +21,6 @@ END:STANDARD
 END:VTIMEZONE
 BEGIN:VTODO
 CREATED;TZID=Europe/Paris:20130803T180000
+UID:todo7
 END:VTODO
 END:VCALENDAR

radicale/tests/static/todo8.ics

@@ -20,6 +20,6 @@ RRULE:FREQ=YEARLY;BYDAY=-1SU;BYMONTH=10
 END:STANDARD
 END:VTIMEZONE
 BEGIN:VTODO
-
+UID:todo8
 END:VTODO
 END:VCALENDAR

radicale/tests/test_base.py

@@ -133,13 +133,13 @@ class BaseRequestsMixIn:
         status, headers, answer = self.request("PUT", "/calendar.ics/", event)
         assert status == 201
         status, headers, answer = self.request(
-            "PUT", "/calendar.ics/event1.ics", event)
+            "PUT", "/calendar.ics/test_event.ics", event)
         assert status == 201
         # Overwrite
         status, headers, answer = self.request("PUT", "/calendar.ics/", event)
         assert status == 201
         status, headers, answer = self.request(
-            "GET", "/calendar.ics/event1.ics")
+            "GET", "/calendar.ics/test_event.ics")
         assert status == 404
 
     def test_delete(self):
@@ -263,8 +263,9 @@ class BaseRequestsMixIn:
         filters_text = "".join(
             "<C:filter>%s</C:filter>" % filter_ for filter_ in filters)
         self.request("MKCOL", "/calendar.ics/")
-        self.request(
+        status, _, _ = self.request(
             "PUT", "/calendar.ics/", "BEGIN:VCALENDAR\r\nEND:VCALENDAR")
+        assert status == 201
         for i in range(items):
             filename = "{}{}.ics".format(kind, i + 1)
             event = get_file_content(filename)