Merge pull request #929 from pimutils/fix-918

Fix 918

tests/storage/__init__.py

@@ -282,8 +282,6 @@ class StorageTests:
     async def test_specialchars(
         self, monkeypatch, requires_collections, get_storage_args, get_item
     ):
-        if getattr(self, "dav_server", "") == "radicale":
-            pytest.skip("Radicale is fundamentally broken.")
         if getattr(self, "dav_server", "") in ("icloud", "fastmail"):
             pytest.skip("iCloud and FastMail reject this name.")
 
@@ -312,6 +310,26 @@ class StorageTests:
         if self.storage_class.storage_name.endswith("dav"):
             assert urlquote(uid, "/@:") in href
 
+    @pytest.mark.asyncio
+    async def test_newline_in_uid(
+        self, monkeypatch, requires_collections, get_storage_args, get_item
+    ):
+        monkeypatch.setattr("vdirsyncer.utils.generate_href", lambda x: x)
+
+        uid = "UID:20210609T084907Z-@synaps-web-54fddfdf7-7kcfm%0A.ics"
+
+        s = self.storage_class(**await get_storage_args())
+        item = get_item(uid=uid)
+
+        href, etag = await s.upload(item)
+        item2, etag2 = await s.get(href)
+        if etag is not None:
+            assert etag2 == etag
+            assert_item_equals(item2, item)
+
+        ((_, etag3),) = await aiostream.stream.list(s.list())
+        assert etag2 == etag3
+
     @pytest.mark.asyncio
     async def test_empty_metadata(self, requires_metadata, s):
         if getattr(self, "dav_server", ""):

tests/storage/dav/test_main.py

@@ -2,6 +2,7 @@ import pytest
 
 from vdirsyncer.storage.dav import _BAD_XML_CHARS
 from vdirsyncer.storage.dav import _merge_xml
+from vdirsyncer.storage.dav import _normalize_href
 from vdirsyncer.storage.dav import _parse_xml
 
 
@@ -44,3 +45,13 @@ def test_xml_specialchars(char):
 
     if char in _BAD_XML_CHARS:
         assert x.text == "yes\nhello"
+
+
+@pytest.mark.parametrize(
+    "href",
+    [
+        "/dav/calendars/user/testuser/123/UID%253A20210609T084907Z-@synaps-web-54fddfdf7-7kcfm%250A.ics",  # noqa: E501
+    ],
+)
+def test_normalize_href(href):
+    assert href == _normalize_href("https://example.com", href)

vdirsyncer/storage/dav.py

@@ -29,25 +29,6 @@ dav_logger = logging.getLogger(__name__)
 CALDAV_DT_FORMAT = "%Y%m%dT%H%M%SZ"
 
 
-def _generate_path_reserved_chars():
-    for x in "/?#[]!$&'()*+,;":
-        x = urlparse.quote(x, "")
-        yield x.upper()
-        yield x.lower()
-
-
-_path_reserved_chars = frozenset(_generate_path_reserved_chars())
-del _generate_path_reserved_chars
-
-
-def _contains_quoted_reserved_chars(x):
-    for y in _path_reserved_chars:
-        if y in x:
-            dav_logger.debug(f"Unsafe character: {y!r}")
-            return True
-    return False
-
-
 async def _assert_multistatus_success(r):
     # Xandikos returns a multistatus on PUT.
     try:
@@ -65,8 +46,7 @@ async def _assert_multistatus_success(r):
 
 
 def _normalize_href(base, href):
-    """Normalize the href to be a path only relative to hostname and
-    schema."""
+    """Normalize the href to be a path only relative to hostname and schema."""
     orig_href = href
     if not href:
         raise ValueError(href)
@@ -74,17 +54,10 @@ def _normalize_href(base, href):
     x = urlparse.urljoin(base, href)
     x = urlparse.urlsplit(x).path
 
-    # Encoding issues:
-    # - https://github.com/owncloud/contacts/issues/581
-    # - https://github.com/Kozea/Radicale/issues/298
-    old_x = None
-    while old_x is None or x != old_x:
-        if _contains_quoted_reserved_chars(x):
-            break
-        old_x = x
-        x = urlparse.unquote(x)
-
-    x = urlparse.quote(x, "/@%:")
+    # We unquote and quote again, but want to make sure we
+    # keep around the "@" character.
+    x = urlparse.unquote(x)
+    x = urlparse.quote(x, "/@")
 
     if orig_href == x:
         dav_logger.debug(f"Already normalized: {x!r}")