From 5660f70753d3403db078832cda275ed05f1d77f2 Mon Sep 17 00:00:00 2001 From: Unrud Date: Tue, 6 Jun 2017 20:00:59 +0200 Subject: [PATCH 1/6] Rename deleted item in test The backend may derive the item name from the UID (e.g. "event1.ics" from the UID "event1"). --- radicale/tests/test_base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/radicale/tests/test_base.py b/radicale/tests/test_base.py index 6591ea82..938c8008 100644 --- a/radicale/tests/test_base.py +++ b/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): From a6a3756e0512f4d1fc3023a97911068d1c7fe4fa Mon Sep 17 00:00:00 2001 From: Unrud Date: Tue, 6 Jun 2017 20:01:02 +0200 Subject: [PATCH 2/6] Check error code of PUT request Detect errors early --- radicale/tests/test_base.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/radicale/tests/test_base.py b/radicale/tests/test_base.py index 938c8008..45e9e01f 100644 --- a/radicale/tests/test_base.py +++ b/radicale/tests/test_base.py @@ -263,8 +263,9 @@ class BaseRequestsMixIn: filters_text = "".join( "%s" % 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) From bea855cb80e7033384ed3b0e2c2302a39ce406d3 Mon Sep 17 00:00:00 2001 From: Unrud Date: Tue, 6 Jun 2017 20:01:04 +0200 Subject: [PATCH 3/6] Improve file names used when uploading whole collection * Use 64 bit random sequence (extremely low chance of collisions) * Improve error message in case of collisions * Add file extension to names --- radicale/storage.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/radicale/storage.py b/radicale/storage.py index 3c3258a5..ba54ac9b 100644 --- a/radicale/storage.py +++ b/radicale/storage.py @@ -27,7 +27,6 @@ entry. import binascii import contextlib -import errno import json import os import pickle @@ -541,13 +540,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): - file_name = hex(getrandbits(32))[2:] + for _ in range(1000): + 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 +690,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) From e47747d4d4805d27430b96258d58f99515f09245 Mon Sep 17 00:00:00 2001 From: Unrud Date: Tue, 6 Jun 2017 20:01:06 +0200 Subject: [PATCH 4/6] return None instead of False if UID field is missing --- radicale/storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/radicale/storage.py b/radicale/storage.py index ba54ac9b..e258879e 100644 --- a/radicale/storage.py +++ b/radicale/storage.py @@ -148,7 +148,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): From fe97741f0806409d3bd62cbfee23f2855cd7752a Mon Sep 17 00:00:00 2001 From: Unrud Date: Tue, 6 Jun 2017 20:01:07 +0200 Subject: [PATCH 5/6] Better reporting of errors in PUT requests --- radicale/__init__.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/radicale/__init__.py b/radicale/__init__.py index 5d3def1d..c5bc34e0 100644 --- a/radicale/__init__.py +++ b/radicale/__init__.py @@ -797,13 +797,23 @@ class Application: tag = tags.get(content_type) if write_whole_collection: - new_item = self.Collection.create_collection( - path, items, {"tag": tag}) + 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("/")) - new_item = parent_item.upload(href, items[0]) + 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 From 2860c664d036752be14c7e5982ba5371c46d5595 Mon Sep 17 00:00:00 2001 From: Unrud Date: Tue, 6 Jun 2017 20:01:09 +0200 Subject: [PATCH 6/6] Check that vobject_item have a UID --- radicale/__init__.py | 2 ++ radicale/storage.py | 23 +++++++++++++++++++++-- radicale/tests/static/todo2.ics | 1 + radicale/tests/static/todo3.ics | 1 + radicale/tests/static/todo4.ics | 1 + radicale/tests/static/todo5.ics | 1 + radicale/tests/static/todo6.ics | 1 + radicale/tests/static/todo7.ics | 1 + radicale/tests/static/todo8.ics | 2 +- 9 files changed, 30 insertions(+), 3 deletions(-) diff --git a/radicale/__init__.py b/radicale/__init__.py index c5bc34e0..ae53c893 100644 --- a/radicale/__init__.py +++ b/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) diff --git a/radicale/storage.py b/radicale/storage.py index e258879e..711e7fc3 100644 --- a/radicale/storage.py +++ b/radicale/storage.py @@ -114,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) @@ -986,8 +1000,13 @@ class Collection(BaseCollection): cinput_hash = cetag = ctext = ctag = cstart = cend = None vobject_item = None if input_hash != cinput_hash: - vobject_item = Item(self, href=href, - text=btext.decode(self.encoding)).item + 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() diff --git a/radicale/tests/static/todo2.ics b/radicale/tests/static/todo2.ics index 0642ef6c..32274f7c 100644 --- a/radicale/tests/static/todo2.ics +++ b/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 diff --git a/radicale/tests/static/todo3.ics b/radicale/tests/static/todo3.ics index fb29bc15..f9252fd7 100644 --- a/radicale/tests/static/todo3.ics +++ b/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 diff --git a/radicale/tests/static/todo4.ics b/radicale/tests/static/todo4.ics index eeecb96a..1c651dcf 100644 --- a/radicale/tests/static/todo4.ics +++ b/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 diff --git a/radicale/tests/static/todo5.ics b/radicale/tests/static/todo5.ics index ae6a629b..29c307fa 100644 --- a/radicale/tests/static/todo5.ics +++ b/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 diff --git a/radicale/tests/static/todo6.ics b/radicale/tests/static/todo6.ics index db9b4b56..805b4cf2 100644 --- a/radicale/tests/static/todo6.ics +++ b/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 diff --git a/radicale/tests/static/todo7.ics b/radicale/tests/static/todo7.ics index 1d44c3a4..f94b271d 100644 --- a/radicale/tests/static/todo7.ics +++ b/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 diff --git a/radicale/tests/static/todo8.ics b/radicale/tests/static/todo8.ics index 8005238f..27d49628 100644 --- a/radicale/tests/static/todo8.ics +++ b/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