From 8d5f2ded4270da0047200a6251af76d23332b28b Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 12:52:29 +0200
Subject: [PATCH 01/13] Describe encoding of Etag

---
 radicale/storage.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index d102e6e..5885f6e 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -100,7 +100,11 @@ def load(configuration, logger):
 
 
 def get_etag(text):
-    """Etag from collection or item."""
+    """Etag from collection or item.
+
+    Encoded as quoted-string (see RFC 2616).
+
+    """
     etag = md5()
     etag.update(text.encode("utf-8"))
     return '"%s"' % etag.hexdigest()
@@ -205,6 +209,7 @@ class Item:
 
     @property
     def etag(self):
+        """Encoded as quoted-string (see RFC 2616)."""
         return get_etag(self.serialize())
 
 
@@ -262,6 +267,7 @@ class BaseCollection:
 
     @property
     def etag(self):
+        """Encoded as quoted-string (see RFC 2616)."""
         return get_etag(self.serialize())
 
     @classmethod

From a4a6a62643ec201dacc69c7a8d451af408667c45 Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 12:53:07 +0200
Subject: [PATCH 02/13] Duplicate code: Use is_safe_path_component

---
 radicale/storage.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 5885f6e..78d54b9 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -125,7 +125,7 @@ def sanitize_path(path):
     path = posixpath.normpath(path)
     new_path = "/"
     for part in path.split("/"):
-        if not part or part in (".", ".."):
+        if not is_safe_path_component(part):
             continue
         new_path = posixpath.join(new_path, part)
     trailing_slash = "" if new_path.endswith("/") else trailing_slash

From a12ef691291e04530785125af328776737effe3a Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 12:55:28 +0200
Subject: [PATCH 03/13] Secure is_safe_filesystem_path_component

On Windows 1/2 would be a safe filesystem path component, but it's not safe to pass it to path_to_filesystem.
Currently only the get method can be called with a href like that and it checked for that.
This just moves the check into the is_safe_filesystem_path_component function.
---
 radicale/storage.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 78d54b9..8fb5bd2 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -142,7 +142,8 @@ def is_safe_path_component(path):
 
 
 def is_safe_filesystem_path_component(path):
-    """Check if path is a single component of a filesystem path.
+    """Check if path is a single component of a local and posix filesystem
+       path.
 
     Check that the path is safe to join too.
 
@@ -150,7 +151,8 @@ def is_safe_filesystem_path_component(path):
     return (
         path and not os.path.splitdrive(path)[0] and
         not os.path.split(path)[0] and path not in (os.curdir, os.pardir) and
-        not path.startswith(".") and not path.endswith("~"))
+        not path.startswith(".") and not path.endswith("~") and
+        is_safe_path_component(path))
 
 
 def path_to_filesystem(root, *paths):
@@ -628,7 +630,7 @@ class Collection(BaseCollection):
     def get(self, href):
         if not href:
             return None
-        href = href.strip("{}").replace("/", "_")
+        href = href.strip("{}")
         if not is_safe_filesystem_path_component(href):
             self.logger.debug(
                 "Can't translate name safely to filesystem: %s", href)

From 77e9ca1252081fc63309099259cb7d08278cdb9c Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:06:09 +0200
Subject: [PATCH 04/13] Remove EtagMismatchError

Etags are not checked in storage anymore and this is unused.
---
 radicale/storage.py | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 8fb5bd2..4685ea6 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -193,12 +193,6 @@ class ComponentNotFoundError(ValueError):
         super().__init__(message)
 
 
-class EtagMismatchError(ValueError):
-    def __init__(self, etag1, etag2):
-        message = "ETags don't match: %s != %s" % (etag1, etag2)
-        super().__init__(message)
-
-
 class Item:
     def __init__(self, collection, item, href, last_modified=None):
         self.collection = collection

From dc501d5dc50360b2cc3bc2e6cae16b89d50aa04b Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:08:12 +0200
Subject: [PATCH 05/13] Refactor/Duplicate code: Extract _fsync method

---
 radicale/storage.py | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 4685ea6..0b0d570 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -394,11 +394,7 @@ class Collection(BaseCollection):
             delete=False, prefix=".Radicale.tmp-", newline=newline)
         try:
             yield tmp
-            if self.configuration.getboolean("storage", "fsync"):
-                if os.name == "posix" and hasattr(fcntl, "F_FULLFSYNC"):
-                    fcntl.fcntl(tmp.fileno(), fcntl.F_FULLFSYNC)
-                else:
-                    os.fsync(tmp.fileno())
+            self._fsync(tmp.fileno())
             tmp.close()
             os.replace(tmp.name, path)
         except:
@@ -416,6 +412,14 @@ class Collection(BaseCollection):
                 return file_name
         raise FileExistsError(errno.EEXIST, "No usable file name found")
 
+    @classmethod
+    def _fsync(cls, fd):
+        if cls.configuration.getboolean("storage", "fsync"):
+            if os.name == "posix" and hasattr(fcntl, "F_FULLFSYNC"):
+                fcntl.fcntl(fd, fcntl.F_FULLFSYNC)
+            else:
+                os.fsync(fd)
+
     @classmethod
     def _sync_directory(cls, path):
         """Sync directory to disk.
@@ -428,10 +432,7 @@ class Collection(BaseCollection):
         if os.name == "posix":
             fd = os.open(path, 0)
             try:
-                if hasattr(fcntl, "F_FULLFSYNC"):
-                    fcntl.fcntl(fd, fcntl.F_FULLFSYNC)
-                else:
-                    os.fsync(fd)
+                cls._fsync(fd)
             finally:
                 os.close(fd)
 
@@ -586,15 +587,9 @@ class Collection(BaseCollection):
             path = path_to_filesystem(self._filesystem_path, href)
             fs.append(open(path, "w", encoding=self.encoding, newline=""))
             fs[-1].write(item.serialize())
-        fsync_fn = lambda fd: None
-        if self.configuration.getboolean("storage", "fsync"):
-            if os.name == "posix" and hasattr(fcntl, "F_FULLFSYNC"):
-                fsync_fn = lambda fd: fcntl.fcntl(fd, fcntl.F_FULLFSYNC)
-            else:
-                fsync_fn = os.fsync
         # sync everything at once because it's slightly faster.
         for f in fs:
-            fsync_fn(f.fileno())
+            self._fsync(f.fileno())
             f.close()
         self._sync_directory(self._filesystem_path)
 

From 5dbf9df8765a17814c0e47fab1b188d570e6a376 Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:09:10 +0200
Subject: [PATCH 06/13] Add missing checks for safe fileystem components

Currently it's not possible to exploit these.
---
 radicale/storage.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/radicale/storage.py b/radicale/storage.py
index 0b0d570..95293ab 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -584,6 +584,8 @@ class Collection(BaseCollection):
         """
         fs = []
         for href, item in vobject_items.items():
+            if not is_safe_filesystem_path_component(href):
+                raise UnsafePathError(href)
             path = path_to_filesystem(self._filesystem_path, href)
             fs.append(open(path, "w", encoding=self.encoding, newline=""))
             fs[-1].write(item.serialize())
@@ -595,6 +597,8 @@ class Collection(BaseCollection):
 
     @classmethod
     def move(cls, item, to_collection, to_href):
+        if not is_safe_filesystem_path_component(to_href):
+            raise UnsafePathError(to_href)
         os.replace(
             path_to_filesystem(item.collection._filesystem_path, item.href),
             path_to_filesystem(to_collection._filesystem_path, to_href))

From e7d8b4816cff4d79cd7806935ca07ce56a1bb6af Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:12:55 +0200
Subject: [PATCH 07/13] Duplicate code: Use list and get methods

---
 radicale/storage.py | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 95293ab..0dfa34c 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -698,17 +698,9 @@ class Collection(BaseCollection):
         return time.strftime("%a, %d %b %Y %H:%M:%S GMT", time.gmtime(last))
 
     def serialize(self):
-        if not os.path.exists(self._filesystem_path):
-            return None
         items = []
-        for href in os.listdir(self._filesystem_path):
-            if not is_safe_filesystem_path_component(href):
-                self.logger.debug("Skipping component: %s", href)
-                continue
-            path = os.path.join(self._filesystem_path, href)
-            if os.path.isfile(path):
-                with open(path, encoding=self.encoding, newline="") as fd:
-                    items.append(vobject.readOne(fd.read()))
+        for href in self.list():
+            items.append(self.get(href).item)
         if self.get_meta("tag") == "VCALENDAR":
             collection = vobject.iCalendar()
             for item in items:

From f5650df5f77d46b795da6b7071e1794aecb24211 Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:13:35 +0200
Subject: [PATCH 08/13] Remove checks for existence of collection

They are unnecessary since the discover methods stopped returning collections that actually don't exist.
---
 radicale/storage.py | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 0dfa34c..3d69fd6 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -607,12 +607,7 @@ class Collection(BaseCollection):
             cls._sync_directory(item.collection._filesystem_path)
 
     def list(self):
-        try:
-            hrefs = os.listdir(self._filesystem_path)
-        except IOError:
-            return
-
-        for href in hrefs:
+        for href in os.listdir(self._filesystem_path):
             if not is_safe_filesystem_path_component(href):
                 self.logger.debug("Skipping component: %s", href)
                 continue
@@ -653,18 +648,17 @@ class Collection(BaseCollection):
     def delete(self, href=None):
         if href is None:
             # Delete the collection
-            if os.path.isdir(self._filesystem_path):
-                parent_dir = os.path.dirname(self._filesystem_path)
-                try:
-                    os.rmdir(self._filesystem_path)
-                except OSError:
-                    with TemporaryDirectory(
-                            prefix=".Radicale.tmp-", dir=parent_dir) as tmp:
-                        os.rename(self._filesystem_path, os.path.join(
-                            tmp, os.path.basename(self._filesystem_path)))
-                        self._sync_directory(parent_dir)
-                else:
+            parent_dir = os.path.dirname(self._filesystem_path)
+            try:
+                os.rmdir(self._filesystem_path)
+            except OSError:
+                with TemporaryDirectory(
+                        prefix=".Radicale.tmp-", dir=parent_dir) as tmp:
+                    os.rename(self._filesystem_path, os.path.join(
+                        tmp, os.path.basename(self._filesystem_path)))
                     self._sync_directory(parent_dir)
+            else:
+                self._sync_directory(parent_dir)
         else:
             # Delete an item
             if not is_safe_filesystem_path_component(href):

From cd9f789294d5c6c2ba9b582ed63ee26eca9ee9a6 Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:14:51 +0200
Subject: [PATCH 09/13] Name variables for files f

fd sounds more like file descriptions.
prop doesn't sound like a file at all.
---
 radicale/storage.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 3d69fd6..9b4fd77 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -626,8 +626,8 @@ class Collection(BaseCollection):
         path = path_to_filesystem(self._filesystem_path, href)
         if not os.path.isfile(path):
             return None
-        with open(path, encoding=self.encoding, newline="") as fd:
-            text = fd.read()
+        with open(path, encoding=self.encoding, newline="") as f:
+            text = f.read()
         last_modified = time.strftime(
             "%a, %d %b %Y %H:%M:%S GMT",
             time.gmtime(os.path.getmtime(path)))
@@ -671,18 +671,18 @@ class Collection(BaseCollection):
 
     def get_meta(self, key):
         if os.path.exists(self._props_path):
-            with open(self._props_path, encoding=self.encoding) as prop:
-                return json.load(prop).get(key)
+            with open(self._props_path, encoding=self.encoding) as f:
+                return json.load(f).get(key)
 
     def set_meta(self, props):
         if os.path.exists(self._props_path):
-            with open(self._props_path, encoding=self.encoding) as prop:
-                old_props = json.load(prop)
+            with open(self._props_path, encoding=self.encoding) as f:
+                old_props = json.load(f)
                 old_props.update(props)
                 props = old_props
         props = {key: value for key, value in props.items() if value}
-        with self._atomic_write(self._props_path, "w+") as prop:
-            json.dump(props, prop)
+        with self._atomic_write(self._props_path, "w+") as f:
+            json.dump(props, f)
 
     @property
     def last_modified(self):

From de09f6689a22fe6b2845411520f1fe5c22c31877 Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:16:42 +0200
Subject: [PATCH 10/13] Only relevant files for last_modified calculation

Leftovers from failed transactions etc. should not change that property.
---
 radicale/storage.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 9b4fd77..f8f75d3 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -686,9 +686,12 @@ class Collection(BaseCollection):
 
     @property
     def last_modified(self):
-        last = max([os.path.getmtime(self._filesystem_path)] + [
-            os.path.getmtime(os.path.join(self._filesystem_path, filename))
-            for filename in os.listdir(self._filesystem_path)] or [0])
+        relevant_files = [self._filesystem_path] + [
+            path_to_filesystem(self._filesystem_path, href)
+            for href in self.list()]
+        if os.path.exists(self._props_path):
+            relevant_files.append(self._props_path)
+        last = max(map(os.path.getmtime, relevant_files))
         return time.strftime("%a, %d %b %Y %H:%M:%S GMT", time.gmtime(last))
 
     def serialize(self):

From 5ccfe16372a3eea7b182963a57ebd5be1d58a6e7 Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:21:57 +0200
Subject: [PATCH 11/13] Remove Collection.has

It's the same as BaseCollection.has
---
 radicale/storage.py | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index f8f75d3..8401561 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -633,9 +633,6 @@ class Collection(BaseCollection):
             time.gmtime(os.path.getmtime(path)))
         return Item(self, vobject.readOne(text), href, last_modified)
 
-    def has(self, href):
-        return self.get(href) is not None
-
     def upload(self, href, vobject_item):
         if not is_safe_filesystem_path_component(href):
             raise UnsafePathError(href)

From 6df54bf88ab67bb96d92971967d251b8267bf8ec Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:23:01 +0200
Subject: [PATCH 12/13] Log name of faulty component

If vobject can't parse a component it raises an exception, but the filename of that component is missing in the logs.
---
 radicale/storage.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 8401561..867b562 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -631,7 +631,12 @@ class Collection(BaseCollection):
         last_modified = time.strftime(
             "%a, %d %b %Y %H:%M:%S GMT",
             time.gmtime(os.path.getmtime(path)))
-        return Item(self, vobject.readOne(text), href, last_modified)
+        try:
+            item = vobject.readOne(text)
+        except Exception:
+            self.logger.error("Failed to parse component: %s", href)
+            raise
+        return Item(self, item, href, last_modified)
 
     def upload(self, href, vobject_item):
         if not is_safe_filesystem_path_component(href):

From 03fbb1e68ebb2dcc953826ed4542326e20fdf758 Mon Sep 17 00:00:00 2001
From: Unrud <unrud@openaliasbox.org>
Date: Sun, 4 Sep 2016 13:35:44 +0200
Subject: [PATCH 13/13] Don't strip {} in get method

If someone uploads a file that starts or ends with the chars {}, all REPORT requests on that collection will fail and it's impossible to delete the file.
---
 radicale/storage.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/radicale/storage.py b/radicale/storage.py
index 867b562..af2fe38 100644
--- a/radicale/storage.py
+++ b/radicale/storage.py
@@ -618,7 +618,6 @@ class Collection(BaseCollection):
     def get(self, href):
         if not href:
             return None
-        href = href.strip("{}")
         if not is_safe_filesystem_path_component(href):
             self.logger.debug(
                 "Can't translate name safely to filesystem: %s", href)