Merge pull request #93 from untitaker/href_quoting

Fix all known URL-quoting related problems

tests/storage/__init__.py

@@ -39,7 +39,8 @@ class BaseStorageTests(object):
         return storage()
 
     def _create_bogus_item(self, item_template=None):
-        r = random.random()
+        # assert that special chars are handled correctly.
+        r = '{}@vdirsyncer'.format(random.random())
         item_template = item_template or self.item_template
         return Item(item_template.format(r=r))
 
@@ -84,8 +85,6 @@ class BaseStorageTests(object):
 
     def test_update_nonexisting(self, s):
         item = self._create_bogus_item()
-        with pytest.raises(exceptions.PreconditionFailed):
-            s.update(s._get_href(item), item, '"123"')
         with pytest.raises(exceptions.PreconditionFailed):
             s.update('huehue', item, '"123"')
 
@@ -108,8 +107,8 @@ class BaseStorageTests(object):
 
     def test_list(self, s):
         assert not list(s.list())
-        s.upload(self._create_bogus_item())
-        assert list(s.list())
+        href, etag = s.upload(self._create_bogus_item())
+        assert list(s.list()) == [(href, etag)]
 
     def test_has(self, s):
         assert not s.has('asd')

tests/storage/dav/servers/radicale/__init__.py

@@ -105,4 +105,4 @@ class ServerMixin(object):
             collection += self.storage_class.fileext
 
         return {'url': url, 'username': 'bob', 'password': 'bob',
-                'collection': collection}
+                'collection': collection, 'unsafe_href_chars': ''}

tests/storage/dav/test_main.py

@@ -20,7 +20,8 @@ from tests import EVENT_TEMPLATE, TASK_TEMPLATE, VCARD_TEMPLATE
 
 import vdirsyncer.exceptions as exceptions
 from vdirsyncer.storage.base import Item
-from vdirsyncer.storage.dav import CaldavStorage, CarddavStorage
+from vdirsyncer.storage.dav import CaldavStorage, CarddavStorage, \
+    _normalize_href
 
 from .. import StorageTests
 
@@ -215,3 +216,26 @@ class TestCaldavStorage(DavStorageTests):
 class TestCarddavStorage(DavStorageTests):
     storage_class = CarddavStorage
     item_template = VCARD_TEMPLATE
+
+
+@pytest.mark.parametrize('base,path', [
+    ('http://example.com/', ''),
+    ('http://example.com/L%C3%98/', '/L%C3%98'),
+    ('http://example.com/LØ/', '/L%C3%98'),
+])
+def test_normalize_href(base, path):
+    assert _normalize_href(base, 'asdf') == path + '/asdf'
+
+    assert _normalize_href(base, 'hahah') == path + '/hahah'
+
+    assert _normalize_href(base, 'whoops@vdirsyncer.vcf') == \
+        path + '/whoops@vdirsyncer.vcf'
+
+    assert _normalize_href(base, 'whoops%40vdirsyncer.vcf') == \
+        path + '/whoops@vdirsyncer.vcf'
+
+    assert _normalize_href(base, 'wh%C3%98ops@vdirsyncer.vcf') == \
+        path + '/wh%C3%98ops@vdirsyncer.vcf'
+
+    assert _normalize_href(base, 'whØops@vdirsyncer.vcf') == \
+        path + '/wh%C3%98ops@vdirsyncer.vcf'

tests/test_sync.py

@@ -23,7 +23,7 @@ def empty_storage(x):
 def test_irrelevant_status():
     a = MemoryStorage()
     b = MemoryStorage()
-    status = {'1': ('1.txt', 1234, '1.ics', 2345)}
+    status = {'1': ('1', 1234, '1.ics', 2345)}
     sync(a, b, status)
     assert not status
     assert empty_storage(a)
@@ -55,8 +55,8 @@ def test_missing_status_and_different_items():
         sync(a, b, status)
     assert not status
     sync(a, b, status, conflict_resolution='a wins')
-    assert_item_equals(item1, b.get('1.txt')[0])
-    assert_item_equals(item1, a.get('1.txt')[0])
+    assert_item_equals(item1, b.get('1')[0])
+    assert_item_equals(item1, a.get('1')[0])
 
 
 def test_upload_and_update():
@@ -67,22 +67,22 @@ def test_upload_and_update():
     item = Item(u'UID:1')  # new item 1 in a
     a.upload(item)
     sync(a, b, status)
-    assert_item_equals(b.get('1.txt')[0], item)
+    assert_item_equals(b.get('1')[0], item)
 
     item = Item(u'UID:1\nASDF:YES')  # update of item 1 in b
-    b.update('1.txt', item, b.get('1.txt')[1])
+    b.update('1', item, b.get('1')[1])
     sync(a, b, status)
-    assert_item_equals(a.get('1.txt')[0], item)
+    assert_item_equals(a.get('1')[0], item)
 
     item2 = Item(u'UID:2')  # new item 2 in b
     b.upload(item2)
     sync(a, b, status)
-    assert_item_equals(a.get('2.txt')[0], item2)
+    assert_item_equals(a.get('2')[0], item2)
 
     item2 = Item(u'UID:2\nASDF:YES')  # update of item 2 in a
-    a.update('2.txt', item2, a.get('2.txt')[1])
+    a.update('2', item2, a.get('2')[1])
     sync(a, b, status)
-    assert_item_equals(b.get('2.txt')[0], item2)
+    assert_item_equals(b.get('2')[0], item2)
 
 
 def test_deletion():
@@ -94,16 +94,16 @@ def test_deletion():
     a.upload(item)
     a.upload(Item(u'UID:2'))
     sync(a, b, status)
-    b.delete('1.txt', b.get('1.txt')[1])
+    b.delete('1', b.get('1')[1])
     sync(a, b, status)
-    assert not a.has('1.txt') and not b.has('1.txt')
+    assert not a.has('1') and not b.has('1')
 
     a.upload(item)
     sync(a, b, status)
-    assert a.has('1.txt') and b.has('1.txt')
-    a.delete('1.txt', a.get('1.txt')[1])
+    assert a.has('1') and b.has('1')
+    a.delete('1', a.get('1')[1])
     sync(a, b, status)
-    assert not a.has('1.txt') and not b.has('1.txt')
+    assert not a.has('1') and not b.has('1')
 
 
 def test_already_synced():
@@ -113,7 +113,7 @@ def test_already_synced():
     a.upload(item)
     b.upload(item)
     status = {
-        '1': ('1.txt', a.get('1.txt')[1], '1.txt', b.get('1.txt')[1])
+        '1': ('1', a.get('1')[1], '1', b.get('1')[1])
     }
     old_status = dict(status)
     a.update = b.update = a.upload = b.upload = \
@@ -122,7 +122,7 @@ def test_already_synced():
     for i in (1, 2):
         sync(a, b, status)
         assert status == old_status
-        assert a.has('1.txt') and b.has('1.txt')
+        assert a.has('1') and b.has('1')
 
 
 @pytest.mark.parametrize('winning_storage', 'ab')

vdirsyncer/storage/base.py

@@ -65,9 +65,6 @@ class Storage(object):
         '''
         raise NotImplementedError()
 
-    def _get_href(self, item):
-        return item.ident + self.fileext
-
     def __repr__(self):
         return '<{}(**{})>'.format(
             self.__class__.__name__,

vdirsyncer/storage/dav.py

@@ -22,6 +22,21 @@ dav_logger = log.get(__name__)
 CALDAV_DT_FORMAT = '%Y%m%dT%H%M%SZ'
 
 
+def _normalize_href(base, href, decoding_rounds=1):
+    '''Normalize the href to be a path only relative to hostname and
+    schema.'''
+    if not href:
+        raise ValueError(href)
+    x = utils.urlparse.urljoin(base, href)
+    x = utils.urlparse.urlsplit(x).path
+
+    for i in range(decoding_rounds):
+        x = utils.compat.urlunquote(x)
+
+    x = utils.compat.urlquote(x, '/@')
+    return x
+
+
 class Discover(object):
 
     xml_home = None
@@ -240,6 +255,8 @@ class DavStorage(Storage):
         ``guess``. If you know yours, consider setting it explicitly for
         performance.
     :param useragent: Default ``vdirsyncer``.
+    :param unsafe_href_chars: Replace the given characters when generating
+        hrefs. Defaults to ``'@'``.
     '''
 
     # the file extension of items. Useful for testing against radicale.
@@ -259,7 +276,8 @@ class DavStorage(Storage):
     _repr_attributes = ('username', 'url')
 
     def __init__(self, url, username='', password='', collection=None,
-                 verify=True, auth=None, useragent=USERAGENT, **kwargs):
+                 verify=True, auth=None, useragent=USERAGENT,
+                 unsafe_href_chars='@', **kwargs):
         super(DavStorage, self).__init__(**kwargs)
 
         url = url.rstrip('/') + '/'
@@ -268,6 +286,7 @@ class DavStorage(Storage):
         self.session = DavSession(url, username, password, verify, auth,
                                   useragent, dav_header=self.dav_header)
         self.collection = collection
+        self.unsafe_href_chars = unsafe_href_chars
 
         # defined for _repr_attributes
         self.username = username
@@ -288,17 +307,14 @@ class DavStorage(Storage):
             s.displayname = c['displayname']
             yield s
 
-    def _normalize_href(self, href):
-        '''Normalize the href to be a path only relative to hostname and
-        schema.'''
-        if not href:
-            raise ValueError(href)
-        x = utils.urlparse.urljoin(self.session.url, href)
-        return utils.compat.urlunquote_plus(utils.urlparse.urlsplit(x).path)
+    def _normalize_href(self, *args, **kwargs):
+        return _normalize_href(self.session.url, *args, **kwargs)
 
     def _get_href(self, item):
-        href = utils.compat.urlunquote_plus(item.ident) + self.fileext
-        return self._normalize_href(href)
+        href = item.ident
+        for char in self.unsafe_href_chars:
+            href = item.ident.replace(char, '_')
+        return self._normalize_href(href + self.fileext)
 
     def get(self, href):
         ((actual_href, item, etag),) = self.get_multi([href])
@@ -613,7 +629,10 @@ class CarddavStorage(DavStorage):
             if 'vcard' not in content_type.text.lower():
                 continue
 
-            href = self._normalize_href(element.find('{DAV:}href').text)
+            # Decode twice because ownCloud encodes twice.
+            # See https://github.com/owncloud/contacts/issues/581
+            href = self._normalize_href(element.find('{DAV:}href').text,
+                                        decoding_rounds=2)
             etag = prop.find('{DAV:}getetag').text
 
             if href in hrefs:
@@ -623,4 +642,4 @@ class CarddavStorage(DavStorage):
                 continue
 
             hrefs.add(href)
-            yield self._normalize_href(href), etag
+            yield href, etag

vdirsyncer/storage/filesystem.py

@@ -61,6 +61,9 @@ class FilesystemStorage(Storage):
     def _get_filepath(self, href):
         return os.path.join(self.path, href)
 
+    def _get_href(self, item):
+        return item.ident + self.fileext
+
     def list(self):
         for fname in os.listdir(self.path):
             fpath = os.path.join(self.path, fname)

vdirsyncer/storage/http.py

@@ -110,9 +110,8 @@ class HttpStorage(Storage):
 
         for item in split_collection(r.text):
             item = Item(item)
-            href = self._get_href(item)
             etag = item.hash
-            self._items[href] = item, etag
+            self._items[item.ident] = item, etag
 
         return ((href, etag) for href, (item, etag) in iteritems(self._items))
 

vdirsyncer/storage/memory.py

@@ -41,7 +41,7 @@ class MemoryStorage(Storage):
         return href in self.items
 
     def upload(self, item):
-        href = self._get_href(item)
+        href = item.ident
         if href in self.items:
             raise exceptions.AlreadyExistingError(item)
         etag = _get_etag()

vdirsyncer/storage/singlefile.py

@@ -96,9 +96,8 @@ class SingleFileStorage(Storage):
 
         for item in split_collection(text):
             item = Item(item)
-            href = self._get_href(item)
             etag = item.hash
-            self._items[href] = item, etag
+            self._items[item.ident] = item, etag
 
         return ((href, etag) for href, (item, etag) in iteritems(self._items))
 
@@ -112,7 +111,7 @@ class SingleFileStorage(Storage):
             raise exceptions.NotFoundError(href)
 
     def upload(self, item):
-        href = self._get_href(item)
+        href = item.ident
         self.list()
         if href in self._items:
             raise exceptions.AlreadyExistingError(href)

vdirsyncer/utils/compat.py

@@ -15,15 +15,15 @@ PY2 = sys.version_info[0] == 2
 if PY2:  # pragma: no cover
     import urlparse
     from urllib import \
-         quote_plus as urlquote_plus, \
-         unquote_plus as urlunquote_plus
+         quote as urlquote, \
+         unquote as urlunquote
     text_type = unicode  # flake8: noqa
     iteritems = lambda x: x.iteritems()
     itervalues = lambda x: x.itervalues()
 else:  # pragma: no cover
     import urllib.parse as urlparse
-    urlquote_plus = urlparse.quote_plus
-    urlunquote_plus = urlparse.unquote_plus
+    urlquote = urlparse.quote
+    urlunquote = urlparse.unquote
     text_type = str
     iteritems = lambda x: x.items()
     itervalues = lambda x: x.values()