From 1349c30f792392b4c67510362839e04f750e3b63 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Mon, 19 Oct 2026 16:48:30 +0200 Subject: [PATCH 1/2] feat: send If-None-Match: * on no_overwrite saves save(no_overwrite=True) used to search for the UID and then PUT unconditionally, leaving a window in which another client could create the object first. By default the PUT now carries If-None-Match: *, so the server itself refuses an overwrite at that URL (RFC 2616 section 14.26, required of CalDAV servers by RFC 4791 section 2; its section 5.3.2 asks clients to send it). The UID search stays, as it also finds the UID under another URL, unless the new save.no-uid-conflict hint says the server refuses that too; the server profiles get its measured values. Where the new save.if-none-match hint is unsupported, the header is not sent and the old search-then-PUT is used. A 200 reply to a PUT now counts as success. Prompt: Please get a subagent to do the needful on the caldav side for supporting the `If-None-Match: *` [I had a feeling of deja-vu here, so I also asked it to verify that there were no issues on this or forgotten work in some forgotten branch]. Followup-Prompt: (on the review finding that the hint defaults 'full' make unprobed servers get the wildcard headers) Check the RFC. If the `If-Match: *`-behaviour is mandated by the RFC, then keep the default as full ... if not, then set the default unknown ... Followup-Prompt: (on this commit's message) Please rewrite the commit message. Drop information about what servers supports and doesn't support things, just write the default behaviour and how the behaviour can be changed through the server compatbility (sic) hints. Followup-Prompt: (on the diff) This is a BREAKING CHANGE if all of the following conditions are met: * `save(no_overwrite=True)` or `save(no_create=True)` is in use * server implementation deviates from the RFC * server is set up without specifying `features` (or using a hand-specified feature list that wasn't updated) This should be reflected in the changelog, eventually. Followup-Prompt: (on the review document, round 2) Back to the drawing board. `If-None-Match` is required by RFC 9110 released in 2022. I think it's a stretch saying that a CalDAV server that doesn't support RFC 9110 is "broken". Hence, the default should be moved to "unknown", and this will not be a breaking change anymore. [superseded by the Followup-Prompt asking to verify the RFCs: they were checked, and the default stays full] Followup-Prompt: 1) there was some confusion on the RFCs. In the commit message, an RFC from 2022 was referenced. It's fully acceptable for a CalDAV-compliant server to not honor that RFC. Then more research was done, and the latest session decided that a CalDAV server not honoring If-Match and If-None-Match is broken. The latest finding should be verified. If it stays, then "full" should be the default support level, but the commit message has to reference the correct RFC(s). 2) the commit messages was (sic) not good, they were too confusing I think, and a bit too long. To shorten it a bit, I asked for the lists of servers supporting and not supporting the RFC to be dropped. Please look through again and come up with good commit messages explaining the old behaviour, the default new behaviour, and the new behaviour when it's known that the server is not honoring the RFC> (sic) 3) The changelog should not repeat the commit message, it should not give all the details, it suffices to say that those parameters have "improved handling on RFC-compliant servers". And that the change is breaking on non-compliant servers unless the `features` is correctly configured. And then a list of the known non-compliant servers. Followup-Prompt: (on the review finding that the breaking change is not marked feat! or with a BREAKING CHANGE footer) Remark that this is a *potentially* breaking change. We will not bump the version number to 4.0 for this, and the changelog should be clear on this: it's the server and not the client that is broken, and the correct workaround is to tell the client through the configureation (sic) that the server is broken. Followup-Prompt: Was caldav-server-tester updated with those things yet? We need to run the caldav-server-tester and grade all the servers. I will fire up the podman containers. Followup-Prompt: Run them towards the test config under [directory containing test server passwords] Followup-Prompt: ok [agreeing to record the measured save.no-uid-conflict values in the server profiles, with a refusal lacking a recognised precondition graded 'ungraceful'] Assisted-By: Claude Opus 5.5 (1M context) Reviewed-by: Tobias Brox --- caldav/calendarobjectresource.py | 147 +++++++++--- caldav/compatibility_hints.py | 54 ++++- caldav/lib/error.py | 13 + .../+no-overwrite-if-none-match.changed.md | 1 + changes/+put-200-ok.fixed.md | 1 + changes/+wildcard-preconditions.breaking.md | 3 + docs/design/FEATURE_COMPLETE_ROADMAP.md | 2 +- tests/test_caldav.py | 20 ++ tests/test_caldav_unit.py | 227 +++++++++++++++++- 9 files changed, 429 insertions(+), 39 deletions(-) create mode 100644 changes/+no-overwrite-if-none-match.changed.md create mode 100644 changes/+put-200-ok.fixed.md create mode 100644 changes/+wildcard-preconditions.breaking.md diff --git a/caldav/calendarobjectresource.py b/caldav/calendarobjectresource.py index 306741f3..985506c8 100644 --- a/caldav/calendarobjectresource.py +++ b/caldav/calendarobjectresource.py @@ -1140,28 +1140,36 @@ def _find_id_path(self, id=None, path=None) -> None: self.url = URL.objectify(path) def _put( - self, retry_on_failure=True, precondition: Precondition = "auto" + self, + retry_on_failure=True, + precondition: Precondition = "auto", + if_none_match: bool = False, ) -> "None | Coroutine[Any, Any, None]": ## TODO: quite much overlapping with _async_put, should consolidate ## TODO: this is low-level http-communication - shouldn't it be in the davclient file rather than in calendarobjectresource.py? - ## SECURITY TODO: we should probably have a check here to verify that no such object exists already - headers = self._precondition_headers(precondition) | ICALH + headers = self._put_headers(precondition, if_none_match) if self.is_async_client: - return self._async_put(headers, retry_on_failure, precondition) + return self._async_put(headers, retry_on_failure, precondition, if_none_match) try: r = self.client.put(self.url, self.data, headers) except error.AuthorizationError as e: - self._raise_uid_conflict(e) + self._raise_uid_conflict(e, if_none_match) raise - return self._post_put(r, retry_on_failure, precondition) + return self._post_put(r, retry_on_failure, precondition, if_none_match) - async def _async_put(self, headers, retry_on_failure=True, precondition: Precondition = "auto"): + async def _async_put( + self, + headers, + retry_on_failure=True, + precondition: Precondition = "auto", + if_none_match: bool = False, + ): try: r = await self.client.put(str(self.url), str(self.data), headers | ICALH) except error.AuthorizationError as e: - self._raise_uid_conflict(e) + self._raise_uid_conflict(e, if_none_match) raise - result = self._post_put(r, retry_on_failure, precondition) + result = self._post_put(r, retry_on_failure, precondition, if_none_match) if result is not None: # _post_put returned a retry coroutine (self._put(False) for async client) await result @@ -1194,6 +1202,17 @@ def _update_tag_props(self, r) -> None: else: self.props.pop(cdav.ScheduleTag.tag, None) + def _put_headers(self, precondition: Precondition, if_none_match: bool) -> dict[str, str]: + """Headers for the PUT of a save(). + + With ``if_none_match`` (``save(no_overwrite=True)``) only + ``If-None-Match: *`` is sent (RFC 9110 section 13.1.2): the PUT + must not overwrite whatever is there, so an If-Match for a + cached version would contradict it.""" + if if_none_match: + return {"if-none-match": "*"} | ICALH + return self._precondition_headers(precondition) | ICALH + @staticmethod def _check_precondition(precondition: Precondition) -> None: if precondition not in ("auto", "etag"): @@ -1271,25 +1290,36 @@ def delete( return self._delete(headers) @staticmethod - def _raise_uid_conflict(e: error.DAVError) -> None: + def _raise_uid_conflict(e: error.DAVError, no_overwrite: bool = False) -> None: """Raise UidConflictError if ``e`` names a UID precondition. Where ``e`` is what earlier versions raised (AuthorizationError for Cyrus' 403, which the client raises before ``_post_put`` sees it, or an ETag/Schedule-Tag mismatch), the UidConflictError is that too, - so existing handlers keep catching it (issue #756).""" + so existing handlers keep catching it (issue #756). On a + ``save(no_overwrite=True)`` it is a ConsistencyError, which is what + no_overwrite raised when its UID search found the object.""" if error.UID_CONFLICT_PRECONDITIONS.intersection(e.preconditions): - cls = error.UID_CONFLICT_ERRORS.get(type(e), error.UidConflictError) + if no_overwrite: + cls = error.UID_CONFLICT_CONSISTENCY_ERRORS.get( + type(e), error.UidConflictConsistencyError + ) + else: + cls = error.UID_CONFLICT_ERRORS.get(type(e), error.UidConflictError) raise cls(e.url, e.reason, preconditions=e.preconditions, body=e.body) from e def _post_put( - self, r, retry_on_failure, precondition: Precondition = "auto" + self, + r, + retry_on_failure, + precondition: Precondition = "auto", + if_none_match: bool = False, ) -> "None | Coroutine[Any, Any, None]": if r.status >= 400: ## The UID is taken (RFC 4791 section 5.3.2.1): not something a ## retry would fix (issue #756). A 412 on a conditional save ## used to raise the mismatch error, so it still is one. - headers = self._precondition_headers(precondition) + headers = {} if if_none_match else self._precondition_headers(precondition) cls = error.PutError if r.status == 412 and headers: cls = ( @@ -1297,7 +1327,15 @@ def _post_put( if "if-schedule-tag-match" in headers else error.ETagMismatchError ) - self._raise_uid_conflict(error.error_from_response(cls, r)) + self._raise_uid_conflict(error.error_from_response(cls, r), if_none_match) + if r.status == 412 and if_none_match: + ## If-None-Match: * failed - there is an object at this URL + raise error.error_from_response( + error.ConsistencyError, + r, + f"no_overwrite flag was set, but object already exists at {self.url} " + f"({error.errmsg(r)})", + ) if r.status == 412: headers = self._precondition_headers(precondition) if headers: @@ -1305,7 +1343,11 @@ def _post_put( raise error.error_from_response(error.PutError, r) elif r.status == 302: self.url = URL.objectify(r.headers.get("location")) - elif r.status not in (204, 201): + elif r.status not in (200, 201, 204): + ## A retry would resend If-None-Match: *, which an object this + ## very PUT may have stored (a 5xx after writing) refuses. + if if_none_match: + retry_on_failure = False if retry_on_failure: try: import vobject # noqa: F401 @@ -1315,27 +1357,35 @@ def _post_put( ## Accessing vobject_instance may "clean" the object. ## See https://github.com/python-caldav/caldav/issues/43 self.get_vobject_instance() - return self._put(False, precondition) + return self._put(False, precondition, if_none_match) else: raise error.error_from_response(error.PutError, r) self._update_tag_props(r) return None def _create( - self, id=None, path=None, retry_on_failure=True, precondition: Precondition = "auto" + self, + id=None, + path=None, + retry_on_failure=True, + precondition: Precondition = "auto", + if_none_match: bool = False, ) -> "None | Coroutine[Any, Any, None]": ## TODO: Find a better method name self._find_id_path(id=id, path=path) if self.is_async_client: - return self._async_create(retry_on_failure, precondition) - self._put(retry_on_failure, precondition) + return self._async_create(retry_on_failure, precondition, if_none_match) + self._put(retry_on_failure, precondition, if_none_match) return None async def _async_create( - self, retry_on_failure=True, precondition: Precondition = "auto" + self, + retry_on_failure=True, + precondition: Precondition = "auto", + if_none_match: bool = False, ) -> None: """Async implementation of _create.""" - await self._put(retry_on_failure, precondition) + await self._put(retry_on_failure, precondition, if_none_match) def _generate_url(self): ## See https://github.com/python-caldav/caldav/issues/143 for the rationale behind double-quoting slashes @@ -1525,8 +1575,8 @@ def save( """ - # TODO: the overwrite/no-overwrite logic can be handled server-side for - # servers that adheres to the RFC: "If-Match: *" and "If-None-Match: *" + ## TODO: no_create could likewise send "If-Match: *", but too many + ## servers get that wrong (see save-load.mutable.if-match-wildcard) ## Checked before anything is sent self._check_precondition(precondition) @@ -1569,7 +1619,10 @@ def get_self(): return None return None - if no_overwrite or no_create: + if_none_match, search = self._no_overwrite_plan( + no_overwrite, no_create, only_this_recurrence, all_recurrences + ) + if search: uid = self.id or self.icalendar_component.get("uid") existing = get_self() self._validate_save_constraints(existing, uid, no_overwrite, no_create) @@ -1606,9 +1659,42 @@ def get_self(): self._maybe_increment_sequence(increase_seqno) path = self.url.path if self.url else None - self._create(id=self.id, path=path, precondition=precondition) + self._create(id=self.id, path=path, precondition=precondition, if_none_match=if_none_match) return self + def _no_overwrite_plan( + self, no_overwrite, no_create, only_this_recurrence, all_recurrences + ) -> tuple[bool, bool]: + """How save() enforces no_overwrite and no_create. + + Returns (send If-None-Match: *, search for the UID first). + + If-None-Match: * (RFC 9110 section 13.1.2) makes the server + refuse to overwrite the URL in the same request, so there is no + window between the check and the PUT. It is not sent where the + server is known to overwrite anyway (save.if-none-match). It + only guards the URL, though: the UID search also finds the same + UID under another resource name. That is left to the server + only where it is known to refuse it (save.no-uid-conflict), and + never for a recurrence instance, which is merged into its master + and PUT over it. + """ + if not no_overwrite: + return False, no_create + features = self.client.features if self.client is not None else None + if_none_match = features is None or features.is_supported("save.if-none-match") + merges_recurrence = ( + only_this_recurrence is not False or all_recurrences + ) and "RECURRENCE-ID" in self.icalendar_component + search = ( + no_create + or not if_none_match + or merges_recurrence + or features is None + or not features.is_supported("save.no-uid-conflict") + ) + return if_none_match, search + def _validate_save_constraints(self, existing, uid, no_overwrite, no_create): """Raise ConsistencyError if no_overwrite/no_create constraints are violated.""" from caldav.lib import error @@ -1731,7 +1817,10 @@ async def get_self(): return None return None - if no_overwrite or no_create: + if_none_match, search = self._no_overwrite_plan( + no_overwrite, no_create, only_this_recurrence, all_recurrences + ) + if search: uid = self.id or self.icalendar_component.get("uid") existing = await get_self() self._validate_save_constraints(existing, uid, no_overwrite, no_create) @@ -1762,7 +1851,9 @@ async def get_self(): self._maybe_increment_sequence(increase_seqno) path = self.url.path if self.url else None - await self._create(id=self.id, path=path, precondition=precondition) + await self._create( + id=self.id, path=path, precondition=precondition, if_none_match=if_none_match + ) return self def is_loaded(self): diff --git a/caldav/compatibility_hints.py b/caldav/compatibility_hints.py index 70e89fac..fd128e83 100644 --- a/caldav/compatibility_hints.py +++ b/caldav/compatibility_hints.py @@ -510,7 +510,7 @@ class FeatureSet: "default": {"support": "full"}, }, "save-load.mutable.if-match-wildcard": { - "description": "An overwrite carrying If-Match: * is accepted, and a PUT carrying it to a missing object is refused. RFC 9110 section 13.1.1 has '*' match any current representation, so it means 'overwrite, but only if the object exists'. When 'unsupported', the server answers 412 Precondition Failed even though the object is there (Zimbra, Radicale - both compare '*' as a literal etag). When 'broken', a missing object is created: the condition is backwards (Bedework, 412 on an existing object) or ignored (SOGo). If-None-Match: * (section 13.1.2) has no feature of its own; the behaviour text notes where it overwrites or refuses to create. The library sends neither itself.", + "description": "An overwrite carrying If-Match: * is accepted, and a PUT carrying it to a missing object is refused. RFC 9110 section 13.1.1 has '*' match any current representation, so it means 'overwrite, but only if the object exists'. When 'unsupported', the server answers 412 Precondition Failed even though the object is there (Zimbra, Radicale - both compare '*' as a literal etag). When 'broken', a missing object is created: the condition is backwards (Bedework, 412 on an existing object) or ignored (SOGo). If-None-Match: * (section 13.1.2) is save.if-none-match. The library does not send If-Match: * itself.", "default": {"support": "full"}, "links": ["https://datatracker.ietf.org/doc/html/rfc9110#section-13.1.1"], }, @@ -792,6 +792,16 @@ class FeatureSet: "default": {"support": "full"}, "links": ["https://datatracker.ietf.org/doc/html/rfc9110#section-8.8.3"], }, + "save.if-none-match": { + "description": "A PUT carrying If-None-Match: * to an existing object is refused with 412 Precondition Failed, and creates a missing object. RFC 9110 section 13.1.2 has '*' hold only when the resource has no current representation, so it means 'create, but never overwrite'; RFC 4791 section 5.3.2 tells CalDAV clients to send it when creating. save(no_overwrite=True) sends it, so the server enforces 'do not overwrite' in the same request rather than after a search. When 'unsupported', the server overwrites the existing object (SOGo ignores the header; Zimbra compares '*' as a literal etag, so the condition always holds) - the library then does not send it, and relies on the UID search before the PUT, as before. The default is 'full': RFC 4791 section 2 requires a CalDAV server to support ETags as RFC 2616 defines them, and RFC 2616 section 14.26 (now RFC 9110 section 13.1.2) says the server MUST NOT perform the method when '*' is given and the object exists.", + "default": {"support": "full"}, + "links": ["https://datatracker.ietf.org/doc/html/rfc4791#section-5.3.2", "https://datatracker.ietf.org/doc/html/rfc4791#section-2", "https://datatracker.ietf.org/doc/html/rfc2616#section-14.26", "https://datatracker.ietf.org/doc/html/rfc9110#section-13.1.2"], + }, + "save.no-uid-conflict": { + "description": "A PUT of an object whose UID another object resource in the same calendar already holds is refused (RFC 4791 section 5.3.2.1, precondition CALDAV:no-uid-conflict; the library raises UidConflictError). The RFC requires it, but whether a server does is only known where it was observed, hence the default 'unknown'. 'ungraceful': refused, but without a precondition the library recognises, so it cannot tell the refusal from any other failed PUT. When 'full' and save.if-none-match holds, save(no_overwrite=True) skips the UID search before the PUT: If-None-Match: * covers the same URL, and this covers the same UID under another URL.", + "default": {"support": "unknown"}, + "links": ["https://datatracker.ietf.org/doc/html/rfc4791#section-5.3.2.1"], + }, "save.duplicate-uid": {}, "save.duplicate-uid.cross-calendar": { "description": "Server allows events with the same UID to exist in different calendars and treats them as separate entities. Support can be 'full' (allowed), 'ungraceful' (rejected with error), or 'unsupported' (silently ignored or moved). Behaviour 'silently-ignored' means the duplicate is not saved but no error is thrown. Behaviour 'moved-instead-of-copied' means the event is moved from the original calendar to the new calendar (Zimbra behavior)" @@ -1435,6 +1445,9 @@ def compare(self, observed): "support": "broken", "behaviour": "the reply is cut at DAV:limit without a 507, and the token skips the rest", }, + ## A second object with a UID already in the calendar is refused with 412 + ## CALDAV:no-uid-conflict (testNoOverwriteSameUidOtherUrl, 2026-10-19). + "save.no-uid-conflict": {"support": "full"}, } ## This seems to work as of version 3.5.4 of Radicale. @@ -1455,6 +1468,9 @@ def compare(self, observed): "support": "unsupported", "behaviour": "If-Match: * is compared as a literal etag: an overwrite of an existing object is refused with 412", }, + ## A second object with a UID already in the calendar is refused with 409 + ## CALDAV:no-uid-conflict (testNoOverwriteSameUidOtherUrl on 3.8.3, 2026-10-19). + "save.no-uid-conflict": {"support": "full"}, "search.recurrences.includes-implicit.todo.pending": {"support": "fragile", "behaviour": "inconsistent results between runs"}, "search.recurrences.expanded.todo": {"support": "unsupported"}, "search.recurrences.expanded.exception": {"support": "full"}, @@ -1473,6 +1489,8 @@ def compare(self, observed): ## matrix and test runs are rate limits, including how often a user is allowed ## to create a new calendar. This may break test runs badly. nextcloud = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "full"}, 'auto-connect.url': { 'basepath': '/remote.php/dav', }, @@ -1564,6 +1582,8 @@ def compare(self, observed): ## Zimbra is not very good at it's caldav support zimbra = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "full"}, 'auto-connect.url': {'basepath': '/dav/'}, ## Genuinely returns matching objects for a comp-type-less query that carries ## a time-range (verified: the event is returned, not just "no error"). @@ -1588,6 +1608,7 @@ def compare(self, observed): ## If-None-Match: * always does. Measured 2026-09-15 against the docker ## image and, repeatedly, an external Zimbra. 'save-load.mutable.if-match-wildcard': {'support': 'unsupported', 'behaviour': 'If-None-Match: * overwrote an existing object'}, + 'save.if-none-match': {'support': 'unsupported', 'behaviour': 'If-None-Match: * overwrote an existing object'}, ## Zimbra DOES apply a display name set at creation (the name sticks, so ## set-displayname is 'full') - but it couples the display name to the ## calendar URL. MKCALENDAR lands the calendar at the requested cal_id path; @@ -1682,6 +1703,8 @@ def compare(self, observed): ## 5.x, hence the version-stamped name - a plain `bedework` would be claiming ## far more than we have measured. bedework_3_10_3 = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "ungraceful", "behaviour": "refused with a bare 412 when the PUT carries If-None-Match: *"}, ## If tests are yielding unexpected results, try to increase this: 'search-cache': {'behaviour': 'delay', 'delay': 3}, 'scheduling.calendar-user-address-set': {'support': 'full'}, @@ -1746,6 +1769,8 @@ def compare(self, observed): ## deletes calendars, its sync-token and text search behave differently - so ## nothing is inherited from that profile. bedework_5_0_0 = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "ungraceful", "behaviour": "refused with a bare 412 when the PUT carries If-None-Match: *"}, ## Writes are asynchronous: a read-back issued immediately after a PUT may ## 404 or hand back the pre-write copy. That is what separated the two ## measurement runs - save-load.mutable came out "broken" (modification not @@ -1887,6 +1912,8 @@ def compare(self, observed): } baikal = { ## version 0.10.1 + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "unsupported", "behaviour": "a second object with the same UID was stored (201)"}, # Baikal (sabre/dav) delivers iTIP notifications to the attendee inbox AND auto-schedules # into their calendar. "scheduling.schedule-tag": False, @@ -1919,6 +1946,8 @@ def compare(self, observed): } cyrus = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "full", "behaviour": "refused with 403 CALDAV:unique-scheduling-object-resource"}, "search.comp-type.optional": {"support": "full"}, "search.recurrences.includes-implicit.infinite-scope": False, "search.time-range.alarm": {"support": "ungraceful"}, @@ -2022,6 +2051,8 @@ def compare(self, observed): ## See the synology profile above: Synology Calendar ships a modified DAViCal, ## so the two profiles should be kept in sync where the fork has not diverged. davical = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "unsupported", "behaviour": "a second object with the same UID was stored (201)"}, ## A fresh account comes without a calendar (observed on the docker test server) "get-current-user-principal.has-calendar": {"value": False}, # Disable HTTP/2 multiplexing - davical doesn't support it well and niquests @@ -2077,6 +2108,8 @@ def compare(self, observed): } sogo = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "ungraceful", "behaviour": "refused with 409, without the CALDAV:no-uid-conflict precondition"}, "scheduling.schedule-tag": False, "scheduling.mailbox.inbox-delivery": False, ## SOGo rejects the calendar-color property with an error (left at the @@ -2135,6 +2168,7 @@ def compare(self, observed): "support": "broken", "behaviour": "If-Match: * is ignored: it created a missing object (201); If-None-Match: * overwrote an existing object", }, + "save.if-none-match": {"support": "unsupported", "behaviour": "If-None-Match: * is ignored: it overwrote an existing object"}, ## Measured 2026-09-15. "create-calendar.with-supported-component-types": { "support": "unsupported", @@ -2181,6 +2215,8 @@ def compare(self, observed): #] robur = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "unsupported", "behaviour": "a second object with the same UID was stored (201)"}, "auto-connect.url": { 'domain': 'calendar.robur.coop', 'basepath': '/principals/', # TODO: this seems fishy @@ -2216,6 +2252,8 @@ def compare(self, observed): } posteo = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "unsupported", "behaviour": "a second object with the same UID was stored (201)"}, 'auto-connect.url': { 'scheme': 'https', 'domain': 'posteo.de:8443', @@ -2266,6 +2304,8 @@ def compare(self, observed): ## Davis uses sabre/dav (same backend as Baikal), so hints are similar. ## TODO: consolidate, make a sabredav dict and let davis/baikal build on it davis = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "unsupported", "behaviour": "a second object with the same UID was stored (201)"}, ## A fresh account comes without a calendar (observed on the docker test server) "get-current-user-principal.has-calendar": {"value": False}, # Davis uses sabre/dav (same backend as Baikal): delivers iTIP notifications to the @@ -2296,6 +2336,8 @@ def compare(self, observed): ## cannot be changed. The pre-provisioned "tasks" calendar supports VTODO only. ## VJOURNAL is not supported at all. ccs = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "full"}, ## Probed 2026-10-13: DAV:limit is refused with a 507 and ## number-of-matches-within-limits ("Report limit not supported"), as ## RFC 6578 section 3.7 says @@ -2351,6 +2393,8 @@ def compare(self, observed): ## CalDAV served at /dav/cal// over HTTP on port 8080. ## Feature support mostly unknown until tested; starting with empty hints. stalwart = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "full"}, ## Probed 2026-10-11. calendaring-sync found that it also answers a token ## whose history is gone with a partial report and no error. "sync-token.invalid-token": {"support": "broken", "behaviour": "'http://example.invalid/never-issued': 207, no error; '0000000000000000000000000000000000000000': 207, no error"}, @@ -2445,6 +2489,8 @@ def compare(self, observed): ## Lots of transient problems with purelymail purelymail = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "ungraceful", "behaviour": "refused with 403, with no-uid-conflict in the CardDAV namespace"}, ## Purelymail claims that the search indexes are "lazily" populated, ## so search works some minutes after the event was created/edited. ## A plain listing reflects a new, edited, completed or deleted object at @@ -2483,6 +2529,8 @@ def compare(self, observed): } gmx = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "full"}, 'auto-connect.url': { 'scheme': 'https', 'domain': 'caldav.gmx.net', @@ -2562,6 +2610,8 @@ def compare(self, observed): ## OX App Suite CalDAV served at /caldav/ (Apache proxies to /servlet/dav/caldav on port 8009). ## The Docker image must be built locally before use (see tests/docker-test-servers/ox/build.sh). ox = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "full"}, ## Probed 2026-10-11 "sync-token.invalid-token": {"support": "broken", "behaviour": "'0000000000000000000000000000000000000000': 207, no error"}, ## Renaming a calendar after creation via PROPPATCH is not supported, but @@ -2692,6 +2742,8 @@ def compare(self, observed): ## Runs SabreDAV 4.3.1. Profiled 2026-06-15 against a freshly created ## dedicated calendar; save-load and most search features work well. infomaniak = { + ## Measured with caldav-server-tester, 2026-10-19. + "save.no-uid-conflict": {"support": "unsupported", "behaviour": "a second object with the same UID was stored (201)"}, ## Calendar collections are created and deleted asynchronously: MKCALENDAR ## and DELETE return before the change takes effect, ~8s and ~6s measured ## 2026-10-05. A PUT into a calendar created moments before 404s, so the diff --git a/caldav/lib/error.py b/caldav/lib/error.py index 586f9f56..823b93fc 100644 --- a/caldav/lib/error.py +++ b/caldav/lib/error.py @@ -280,12 +280,25 @@ class UidConflictAuthorizationError(UidConflictError, AuthorizationError): """A 403 UID conflict (Cyrus).""" +class UidConflictConsistencyError(UidConflictError, ConsistencyError): + """A UID conflict on ``save(no_overwrite=True)``. Its UID search, which + may now be left to the server, raised ConsistencyError for this.""" + + +class UidConflictConsistencyAuthorizationError(UidConflictConsistencyError, AuthorizationError): + """A 403 UID conflict (Cyrus) where a ConsistencyError is expected.""" + + ## The UidConflictError to raise in place of what earlier versions raised UID_CONFLICT_ERRORS: dict[type[DAVError], type[UidConflictError]] = { AuthorizationError: UidConflictAuthorizationError, ETagMismatchError: UidConflictETagMismatchError, ScheduleTagMismatchError: UidConflictScheduleTagMismatchError, } +## The same, where a ConsistencyError is expected too +UID_CONFLICT_CONSISTENCY_ERRORS: dict[type[DAVError], type[UidConflictConsistencyError]] = { + AuthorizationError: UidConflictConsistencyAuthorizationError, +} class RateLimitError(DAVError): diff --git a/changes/+no-overwrite-if-none-match.changed.md b/changes/+no-overwrite-if-none-match.changed.md new file mode 100644 index 00000000..0e90f643 --- /dev/null +++ b/changes/+no-overwrite-if-none-match.changed.md @@ -0,0 +1 @@ +`save(no_overwrite=True)` (and `add_event(..., no_overwrite=True)` and friends) has improved handling on RFC-compliant servers: the server refuses an overwrite at the same URL in the same request as the write, so another client can no longer slip in between the check and the PUT there. A refusal still raises `ConsistencyError`. diff --git a/changes/+put-200-ok.fixed.md b/changes/+put-200-ok.fixed.md new file mode 100644 index 00000000..7a0b3af4 --- /dev/null +++ b/changes/+put-200-ok.fixed.md @@ -0,0 +1 @@ +A `200 OK` reply to the PUT of `save()` is now taken as success (RFC 9110 section 9.3.4 allows it for an overwrite). Before, it was retried, and the retry raised `PutError` when the server answered `200` again. diff --git a/changes/+wildcard-preconditions.breaking.md b/changes/+wildcard-preconditions.breaking.md new file mode 100644 index 00000000..37a68f76 --- /dev/null +++ b/changes/+wildcard-preconditions.breaking.md @@ -0,0 +1,3 @@ +Potentially breaking, on servers that are themselves broken: `save(no_overwrite=True)` now relies on the server honouring `If-None-Match: *`, which every CalDAV server is required to do. Against a server that ignores it, the save may overwrite an existing object. The fix is to tell the client that the server is broken: configure it with the server's `features` (e.g. `features: sogo`), or mark `save.if-none-match` unsupported in a hand-written feature list. Servers known to ignore it: SOGo, Zimbra. + +A subclass overriding `_create()` or `_put()` must accept their new keyword argument. diff --git a/docs/design/FEATURE_COMPLETE_ROADMAP.md b/docs/design/FEATURE_COMPLETE_ROADMAP.md index 25749a68..f680533f 100644 --- a/docs/design/FEATURE_COMPLETE_ROADMAP.md +++ b/docs/design/FEATURE_COMPLETE_ROADMAP.md @@ -309,7 +309,7 @@ change notifications. Requested by the proposal authors themselves. **Tasks:** - [x] Robust ETag-based collision detection — **done in v3.2.0**: the ETag from PUT/GET responses is cached in `self.props` and `ETagMismatchError` is raised on 412 -- [ ] Proper `If-Match` / `If-None-Match` header usage — half done: `If-Match` is sent when an ETag is cached (and `If-Schedule-Tag-Match` takes precedence when a Schedule-Tag is) on save, and on delete with `delete(if_match=True)` (https://github.com/python-caldav/caldav/issues/740), but `If-None-Match` is not used for create-only semantics +- [x] Proper `If-Match` / `If-None-Match` header usage — `If-Match` is sent when an ETag is cached (and `If-Schedule-Tag-Match` takes precedence when a Schedule-Tag is) on save, and on delete with `delete(if_match=True)` (https://github.com/python-caldav/caldav/issues/740); `save(no_overwrite=True)` sends `If-None-Match: *` (not on servers where `save.if-none-match` is unsupported), and leaves out its UID search where `save.no-uid-conflict` is full. `no_create` still searches rather than sending `If-Match: *`, which too many servers get wrong (`save-load.mutable.if-match-wildcard`) - [ ] Handle UID vs path name mismatches - [ ] Race condition mitigation - [ ] Clear error messages for conflicts diff --git a/tests/test_caldav.py b/tests/test_caldav.py index 1b1cc074..baba7411 100644 --- a/tests/test_caldav.py +++ b/tests/test_caldav.py @@ -2435,6 +2435,26 @@ def testFoldedLongUID(self): with pytest.raises(error.ConsistencyError): loaded.save(no_overwrite=True) + def testNoOverwriteSameUidOtherUrl(self): + """ + save(no_overwrite=True) refuses an object whose UID is already in + the calendar under another resource name, not only one at the + same URL. Where save.no-uid-conflict is full the library leaves + that to the server (CALDAV:no-uid-conflict, RFC 4791 section + 5.3.2.1) and skips its own UID search. + """ + self.skip_unless_support("save-load.event") + c = self._fixCalendar() + ev1_now = near_now_ics(ev1) + c.add_event(ev1_now) + other = Event(client=self.caldav, parent=c, data=ev1_now, url=c.url.join("other-name.ics")) + with pytest.raises(error.ConsistencyError) as exc_info: + other.save(no_overwrite=True) + if self.is_supported("save.no-uid-conflict") and self.is_supported("save.if-none-match"): + ## refused by the server, not by the library's UID search + assert isinstance(exc_info.value, error.UidConflictError) + assert len(c.get_events()) == 1 + def testObjectBySyncToken(self): """ Support for sync-collection reports, ref https://github.com/python-caldav/caldav/issues/87. diff --git a/tests/test_caldav_unit.py b/tests/test_caldav_unit.py index f0fcf788..e1343fa9 100755 --- a/tests/test_caldav_unit.py +++ b/tests/test_caldav_unit.py @@ -2641,8 +2641,10 @@ def test_save_none_falls_through_to_put_when_master_missing(self): """save(only_this_recurrence=None) calls _create() rather than raising when master absent.""" event, _ = self._make_orphan_event() created = [] - event._create = lambda id=None, path=None, retry_on_failure=True, precondition="auto": ( - created.append(True) + event._create = ( + lambda id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( + created.append(True) + ) ) event.save(only_this_recurrence=None) assert created, "_create() must be called (PUT as-is) when master not found" @@ -2651,8 +2653,10 @@ def test_save_false_bypasses_master_lookup(self): """save(only_this_recurrence=False) skips the master lookup entirely and PUTs directly.""" event, mock_calendar = self._make_orphan_event() created = [] - event._create = lambda id=None, path=None, retry_on_failure=True, precondition="auto": ( - created.append(True) + event._create = ( + lambda id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( + created.append(True) + ) ) event.save(only_this_recurrence=False) mock_calendar.get_event_by_uid.assert_not_called() @@ -2667,7 +2671,7 @@ def test_add_object_orphan_does_not_raise_notfound(self): created = [] original_create = CalendarObjectResource._create CalendarObjectResource._create = ( - lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto": ( + lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( created.append(True) ) ) @@ -2696,7 +2700,7 @@ def test_save_orphan_found_by_uid_does_not_recurse(self): with mock.patch.object( CalendarObjectResource, "_create", - lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto": ( + lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( created.append(self_) ), ): @@ -2728,7 +2732,12 @@ async def get_event_by_uid(uid): created = [] async def fake_create( - self_, id=None, path=None, retry_on_failure=True, precondition="auto" + self_, + id=None, + path=None, + retry_on_failure=True, + precondition="auto", + if_none_match=False, ): created.append(self_) @@ -3215,7 +3224,7 @@ def _put_sync(func): with mock.patch.object( CalendarObjectResource, "_create", - lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto": ( + lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( put.append(self_) ), ): @@ -3230,7 +3239,12 @@ def _put_async(coro_func): put = [] async def fake_create( - self_, id=None, path=None, retry_on_failure=True, precondition="auto" + self_, + id=None, + path=None, + retry_on_failure=True, + precondition="auto", + if_none_match=False, ): put.append(self_) @@ -7481,3 +7495,198 @@ def test_principal_search_failure_keeps_preconditions(self, base) -> None: assert exc_info.value.preconditions == [MKCALENDAR_TAG] ## the message format of earlier versions is kept assert " - \n' + + +class TestNoOverwriteIfNoneMatch: + """save(no_overwrite=True) sends If-None-Match: * (RFC 9110 section + 13.1.2), so the server refuses an overwrite of the same URL in the + same request. The UID search before the PUT is only skipped where + the server is known to refuse the UID under another URL as well + (CALDAV:no-uid-conflict, RFC 4791 section 5.3.2.1).""" + + URL = "http://cal.example.com/dav/user/mycal/" + + @pytest.fixture(params=["sync", "async"]) + def base(self, request): + if request.param == "sync": + return DAVClient + from caldav.async_davclient import AsyncDAVClient + + return AsyncDAVClient + + def _client(self, base, put_status=201, put_body=b"", features=None, exc=None): + sent = [] + client = base(url="http://cal.example.com/dav/", features=features) + + def request(url, method="GET", body_=None, headers=None): + method = method.upper() + sent.append((method, {k.lower(): v for k, v in (headers or {}).items()})) + if method == "PUT": + if exc is not None: + raise exc + status, body = put_status, put_body + else: + status, body = 207, EMPTY_MULTISTATUS + resp = mock.MagicMock() + resp.status_code = status + resp.reason = "whatever" + resp.headers = {"Content-Type": "application/xml"} if body else {} + resp.content = body + resp.text = body.decode() + return DAVResponse(resp, client) + + if base is DAVClient: + client.request = lambda url, method="GET", body="", headers=None: request( + url, method, body, headers + ) + else: + + async def async_request(url, method="GET", body="", headers=None): + return request(url, method, body, headers) + + client.request = async_request + return client, sent + + def _event(self, client, data=ev1, etag=None): + cal = Calendar(client, url=self.URL) + event = Event(client, data=data, parent=cal) + if etag: + event.url = URL(self.URL + "ev1.ics") + event.props[dav.GetEtag.tag] = etag + return event + + _run = staticmethod(TestPreconditionsOnOtherStatuses._run) + + NO_UID_CONFLICT = {"save.no-uid-conflict": {"support": "full"}} + + def test_search_kept_by_default(self, base) -> None: + """Whether the server refuses the UID under another URL is not + known, so the search stays; the PUT is guarded all the same.""" + client, sent = self._client(base) + self._run(self._event(client).save(no_overwrite=True)) + assert [m for m, _ in sent] == ["REPORT", "PUT"] + assert sent[-1][1]["if-none-match"] == "*" + + def test_search_skipped_where_no_uid_conflict_holds(self, base) -> None: + client, sent = self._client(base, features=self.NO_UID_CONFLICT) + self._run(self._event(client).save(no_overwrite=True)) + assert [m for m, _ in sent] == ["PUT"] + assert sent[-1][1]["if-none-match"] == "*" + + def test_plain_save_sends_no_if_none_match(self, base) -> None: + client, sent = self._client(base, features=self.NO_UID_CONFLICT) + self._run(self._event(client).save()) + assert "if-none-match" not in sent[-1][1] + + def test_412_raises_consistency_error(self, base) -> None: + client, sent = self._client(base, put_status=412, features=self.NO_UID_CONFLICT) + with pytest.raises(error.ConsistencyError): + self._run(self._event(client).save(no_overwrite=True)) + assert [m for m, _ in sent] == ["PUT"] + + def test_loaded_object_sends_only_if_none_match(self, base) -> None: + """If-Match with the cached ETag would contradict If-None-Match: *.""" + client, sent = self._client(base, put_status=412, features=self.NO_UID_CONFLICT) + event = self._event(client, etag='"etag-a"') + with pytest.raises(error.ConsistencyError): + self._run(event.save(no_overwrite=True)) + assert sent[-1][1]["if-none-match"] == "*" + assert "if-match" not in sent[-1][1] + assert "if-schedule-tag-match" not in sent[-1][1] + + @pytest.mark.parametrize("status", [409, 412]) + def test_uid_conflict_is_also_consistency_error(self, base, status) -> None: + """The same UID under another URL: before, the search found it and + ConsistencyError was raised, so handlers for that keep working.""" + client, sent = self._client( + base, put_status=status, put_body=NO_UID_CONFLICT_BODY, features=self.NO_UID_CONFLICT + ) + with pytest.raises(error.UidConflictError) as exc_info: + self._run(self._event(client).save(no_overwrite=True)) + assert isinstance(exc_info.value, error.ConsistencyError) + assert exc_info.value.preconditions == ["{urn:ietf:params:xml:ns:caldav}no-uid-conflict"] + assert [m for m, _ in sent] == ["PUT"] + + def test_403_uid_conflict_is_also_consistency_error(self, base) -> None: + tag = "{urn:ietf:params:xml:ns:caldav}unique-scheduling-object-resource" + exc = error.AuthorizationError( + url=self.URL + "ev1.ics", + reason="Forbidden", + preconditions=[tag], + body=UNIQUE_SCHEDULING_BODY, + ) + client, _ = self._client(base, features=self.NO_UID_CONFLICT, exc=exc) + with pytest.raises(error.UidConflictError) as exc_info: + self._run(self._event(client).save(no_overwrite=True)) + assert isinstance(exc_info.value, error.ConsistencyError) + ## ... and still what earlier versions raised (issue #756) + assert isinstance(exc_info.value, error.AuthorizationError) + + def test_server_ignoring_if_none_match_keeps_search(self, base) -> None: + """Where If-None-Match: * is known to overwrite (Zimbra, SOGo) the + old search-then-PUT is kept, and the header is not sent.""" + features = { + "save.if-none-match": {"support": "unsupported"}, + "save.no-uid-conflict": {"support": "full"}, + } + client, sent = self._client(base, features=features) + self._run(self._event(client).save(no_overwrite=True)) + assert [m for m, _ in sent] == ["REPORT", "PUT"] + assert "if-none-match" not in sent[-1][1] + + def test_recurrence_instance_keeps_search(self, base) -> None: + """A recurrence instance is merged into its master, which the PUT + then overwrites - only the search can refuse that.""" + client, sent = self._client(base, features=self.NO_UID_CONFLICT) + instance = ev1.replace("DTSTART", "RECURRENCE-ID:20060714T170000Z\nDTSTART", 1) + self._run( + self._event(client, data=instance).save(no_overwrite=True, only_this_recurrence=None) + ) + assert [m for m, _ in sent][0] == "REPORT" + assert sent[-1][1]["if-none-match"] == "*" + + @pytest.mark.parametrize("server", ["zimbra", "sogo"]) + def test_hints_of_servers_overwriting(self, server) -> None: + from caldav import compatibility_hints + from caldav.compatibility_hints import FeatureSet + + fs = FeatureSet(getattr(compatibility_hints, server)) + assert not fs.is_supported("save.if-none-match") + + def test_if_none_match_default_full_no_uid_conflict_default_unknown(self) -> None: + from caldav.compatibility_hints import FeatureSet + + fs = FeatureSet() + assert fs.is_supported("save.if-none-match") + assert not fs.is_supported("save.no-uid-conflict") + + def test_200_is_success_without_retry(self, base) -> None: + """A 200 is a successful PUT (RFC 9110 section 9.3.4). Retrying it + would resend If-None-Match: *, which the object the first PUT + stored then refuses: the caller's own save would read as a + conflict.""" + client, sent = self._client(base, put_status=200, features=self.NO_UID_CONFLICT) + self._run(self._event(client).save(no_overwrite=True)) + assert [m for m, _ in sent] == ["PUT"] + + def test_plain_save_200_is_success_without_retry(self, base) -> None: + client, sent = self._client(base, put_status=200, features=self.NO_UID_CONFLICT) + self._run(self._event(client).save()) + assert [m for m, _ in sent] == ["PUT"] + + def test_failure_not_retried_with_wildcard(self, base) -> None: + """A 5xx may come after the object was written, so a retry with + If-None-Match: * could be refused by the caller's own object.""" + client, sent = self._client(base, put_status=500, features=self.NO_UID_CONFLICT) + with pytest.raises(error.PutError): + self._run(self._event(client).save(no_overwrite=True)) + assert [m for m, _ in sent] == ["PUT"] + + def test_plain_save_failure_still_retried(self, base) -> None: + client, sent = self._client(base, put_status=500, features=self.NO_UID_CONFLICT) + with pytest.raises(error.PutError): + self._run(self._event(client).save()) + assert [m for m, _ in sent] == ["PUT", "PUT"] From b4bdcb7436e7a96dd4173f53685bc8247be92a4e Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Mon, 19 Oct 2026 21:31:22 +0200 Subject: [PATCH 2/2] feat: send If-Match: * on no_create saves save(no_create=True) used to search for the UID before the PUT, with the same race window as no_overwrite had. By default the PUT now carries If-Match: * instead, so the server itself refuses to create a missing object (RFC 2616 section 14.24). This checks the URL rather than the UID: an object stored under another name is now refused instead of found. A cached strong ETag or a Schedule-Tag is sent instead of the wildcard, and keeps the search, since a 412 to it cannot tell a deleted object from a changed one; so does a recurrence instance, which is merged into its master. Where the save-load.mutable.if-match-wildcard hint is not supported, the old search-then-PUT is used. Prompt: If I remember correct, there was some similar header to force through the other variant (no_create)? Followup-Prompts: [See the previous commit] Assisted-By: Claude Opus 5.5 (1M context) Reviewed-by: Tobias Brox --- caldav/calendarobjectresource.py | 188 +++++++++++------- caldav/compatibility_hints.py | 6 +- caldav/lib/error.py | 5 +- .../+no-overwrite-if-none-match.changed.md | 2 +- changes/+wildcard-preconditions.breaking.md | 7 +- docs/design/FEATURE_COMPLETE_ROADMAP.md | 2 +- tests/test_caldav_unit.py | 167 ++++++++++++++-- 7 files changed, 280 insertions(+), 97 deletions(-) diff --git a/caldav/calendarobjectresource.py b/caldav/calendarobjectresource.py index 985506c8..afef62e8 100644 --- a/caldav/calendarobjectresource.py +++ b/caldav/calendarobjectresource.py @@ -54,6 +54,8 @@ ## Which tag save() and delete(if_match=True) send as precondition. ## https://github.com/python-caldav/caldav/issues/762 Precondition = Literal["auto", "etag"] +## The "*" header save(no_overwrite=True) or save(no_create=True) sends +Wildcard = Literal["if-none-match", "if-match"] def _quote_uid(uid: str, features: Any = None) -> str: @@ -1143,33 +1145,33 @@ def _put( self, retry_on_failure=True, precondition: Precondition = "auto", - if_none_match: bool = False, + wildcard: Wildcard | None = None, ) -> "None | Coroutine[Any, Any, None]": ## TODO: quite much overlapping with _async_put, should consolidate ## TODO: this is low-level http-communication - shouldn't it be in the davclient file rather than in calendarobjectresource.py? - headers = self._put_headers(precondition, if_none_match) + headers = self._put_headers(precondition, wildcard) if self.is_async_client: - return self._async_put(headers, retry_on_failure, precondition, if_none_match) + return self._async_put(headers, retry_on_failure, precondition, wildcard) try: r = self.client.put(self.url, self.data, headers) except error.AuthorizationError as e: - self._raise_uid_conflict(e, if_none_match) + self._raise_uid_conflict(e, wildcard is not None) raise - return self._post_put(r, retry_on_failure, precondition, if_none_match) + return self._post_put(r, retry_on_failure, precondition, wildcard) async def _async_put( self, headers, retry_on_failure=True, precondition: Precondition = "auto", - if_none_match: bool = False, + wildcard: Wildcard | None = None, ): try: r = await self.client.put(str(self.url), str(self.data), headers | ICALH) except error.AuthorizationError as e: - self._raise_uid_conflict(e, if_none_match) + self._raise_uid_conflict(e, wildcard is not None) raise - result = self._post_put(r, retry_on_failure, precondition, if_none_match) + result = self._post_put(r, retry_on_failure, precondition, wildcard) if result is not None: # _post_put returned a retry coroutine (self._put(False) for async client) await result @@ -1202,15 +1204,17 @@ def _update_tag_props(self, r) -> None: else: self.props.pop(cdav.ScheduleTag.tag, None) - def _put_headers(self, precondition: Precondition, if_none_match: bool) -> dict[str, str]: + def _put_headers(self, precondition: Precondition, wildcard: Wildcard | None) -> dict[str, str]: """Headers for the PUT of a save(). - With ``if_none_match`` (``save(no_overwrite=True)``) only - ``If-None-Match: *`` is sent (RFC 9110 section 13.1.2): the PUT - must not overwrite whatever is there, so an If-Match for a - cached version would contradict it.""" - if if_none_match: - return {"if-none-match": "*"} | ICALH + With a ``wildcard`` (see ``_save_plan()``) only that header is + sent, with the value ``*``. For ``If-None-Match: *`` + (``save(no_overwrite=True)``, RFC 9110 section 13.1.2) an + If-Match for a cached version would contradict it; ``If-Match: *`` + (``no_create``, section 13.1.1) is only planned where no version + is cached.""" + if wildcard: + return {wildcard: "*"} | ICALH return self._precondition_headers(precondition) | ICALH @staticmethod @@ -1290,17 +1294,19 @@ def delete( return self._delete(headers) @staticmethod - def _raise_uid_conflict(e: error.DAVError, no_overwrite: bool = False) -> None: + def _raise_uid_conflict(e: error.DAVError, consistency: bool = False) -> None: """Raise UidConflictError if ``e`` names a UID precondition. Where ``e`` is what earlier versions raised (AuthorizationError for Cyrus' 403, which the client raises before ``_post_put`` sees it, or an ETag/Schedule-Tag mismatch), the UidConflictError is that too, - so existing handlers keep catching it (issue #756). On a - ``save(no_overwrite=True)`` it is a ConsistencyError, which is what - no_overwrite raised when its UID search found the object.""" + so existing handlers keep catching it (issue #756). With + ``consistency`` - a ``save(no_overwrite=True)`` or + ``save(no_create=True)`` that sent a wildcard instead of searching + for the UID - it is a ConsistencyError, which is what the search + raised.""" if error.UID_CONFLICT_PRECONDITIONS.intersection(e.preconditions): - if no_overwrite: + if consistency: cls = error.UID_CONFLICT_CONSISTENCY_ERRORS.get( type(e), error.UidConflictConsistencyError ) @@ -1313,13 +1319,13 @@ def _post_put( r, retry_on_failure, precondition: Precondition = "auto", - if_none_match: bool = False, + wildcard: Wildcard | None = None, ) -> "None | Coroutine[Any, Any, None]": if r.status >= 400: ## The UID is taken (RFC 4791 section 5.3.2.1): not something a ## retry would fix (issue #756). A 412 on a conditional save ## used to raise the mismatch error, so it still is one. - headers = {} if if_none_match else self._precondition_headers(precondition) + headers = {} if wildcard else self._precondition_headers(precondition) cls = error.PutError if r.status == 412 and headers: cls = ( @@ -1327,8 +1333,8 @@ def _post_put( if "if-schedule-tag-match" in headers else error.ETagMismatchError ) - self._raise_uid_conflict(error.error_from_response(cls, r), if_none_match) - if r.status == 412 and if_none_match: + self._raise_uid_conflict(error.error_from_response(cls, r), wildcard is not None) + if r.status == 412 and wildcard == "if-none-match": ## If-None-Match: * failed - there is an object at this URL raise error.error_from_response( error.ConsistencyError, @@ -1336,6 +1342,13 @@ def _post_put( f"no_overwrite flag was set, but object already exists at {self.url} " f"({error.errmsg(r)})", ) + if r.status == 412 and wildcard == "if-match": + ## If-Match: * failed - there is no object at this URL + raise error.error_from_response( + error.ConsistencyError, + r, + f"no_create flag was set, but there is no object at {self.url} ({error.errmsg(r)})", + ) if r.status == 412: headers = self._precondition_headers(precondition) if headers: @@ -1344,9 +1357,10 @@ def _post_put( elif r.status == 302: self.url = URL.objectify(r.headers.get("location")) elif r.status not in (200, 201, 204): - ## A retry would resend If-None-Match: *, which an object this - ## very PUT may have stored (a 5xx after writing) refuses. - if if_none_match: + ## No retry with a wildcard: an object this very PUT may have + ## stored (a 5xx after writing) would refuse If-None-Match: *, + ## and the failure is reported as it was for If-Match: * too. + if wildcard: retry_on_failure = False if retry_on_failure: try: @@ -1357,7 +1371,7 @@ def _post_put( ## Accessing vobject_instance may "clean" the object. ## See https://github.com/python-caldav/caldav/issues/43 self.get_vobject_instance() - return self._put(False, precondition, if_none_match) + return self._put(False, precondition, wildcard) else: raise error.error_from_response(error.PutError, r) self._update_tag_props(r) @@ -1369,23 +1383,23 @@ def _create( path=None, retry_on_failure=True, precondition: Precondition = "auto", - if_none_match: bool = False, + wildcard: Wildcard | None = None, ) -> "None | Coroutine[Any, Any, None]": ## TODO: Find a better method name self._find_id_path(id=id, path=path) if self.is_async_client: - return self._async_create(retry_on_failure, precondition, if_none_match) - self._put(retry_on_failure, precondition, if_none_match) + return self._async_create(retry_on_failure, precondition, wildcard) + self._put(retry_on_failure, precondition, wildcard) return None async def _async_create( self, retry_on_failure=True, precondition: Precondition = "auto", - if_none_match: bool = False, + wildcard: Wildcard | None = None, ) -> None: """Async implementation of _create.""" - await self._put(retry_on_failure, precondition, if_none_match) + await self._put(retry_on_failure, precondition, wildcard) def _generate_url(self): ## See https://github.com/python-caldav/caldav/issues/143 for the rationale behind double-quoting slashes @@ -1515,7 +1529,11 @@ def save( """Save the object, can be used for creation and update. no_overwrite and no_create will check if the object exists. - Those two are mutually exclusive. Some servers don't support + Those two are mutually exclusive. Where the server handles it, + the PUT carries ``If-None-Match: *`` or ``If-Match: *`` so the + server enforces the flag in the same request, otherwise the UID + is searched for first; either way a violation raises + ``ConsistencyError``. Some servers don't support searching for an object uid without explicitly specifying what kind of object it should be, hence obj_type can be passed. obj_type is only used in conjunction with no_overwrite and @@ -1575,9 +1593,6 @@ def save( """ - ## TODO: no_create could likewise send "If-Match: *", but too many - ## servers get that wrong (see save-load.mutable.if-match-wildcard) - ## Checked before anything is sent self._check_precondition(precondition) @@ -1619,8 +1634,8 @@ def get_self(): return None return None - if_none_match, search = self._no_overwrite_plan( - no_overwrite, no_create, only_this_recurrence, all_recurrences + wildcard, search = self._save_plan( + no_overwrite, no_create, only_this_recurrence, all_recurrences, precondition ) if search: uid = self.id or self.icalendar_component.get("uid") @@ -1659,41 +1674,70 @@ def get_self(): self._maybe_increment_sequence(increase_seqno) path = self.url.path if self.url else None - self._create(id=self.id, path=path, precondition=precondition, if_none_match=if_none_match) + self._create(id=self.id, path=path, precondition=precondition, wildcard=wildcard) return self - def _no_overwrite_plan( - self, no_overwrite, no_create, only_this_recurrence, all_recurrences - ) -> tuple[bool, bool]: + def _save_plan( + self, + no_overwrite: bool, + no_create: bool, + only_this_recurrence: bool | None, + all_recurrences: bool, + precondition: Precondition, + ) -> tuple[Wildcard | None, bool]: """How save() enforces no_overwrite and no_create. - Returns (send If-None-Match: *, search for the UID first). - - If-None-Match: * (RFC 9110 section 13.1.2) makes the server - refuse to overwrite the URL in the same request, so there is no - window between the check and the PUT. It is not sent where the - server is known to overwrite anyway (save.if-none-match). It - only guards the URL, though: the UID search also finds the same - UID under another resource name. That is left to the server - only where it is known to refuse it (save.no-uid-conflict), and - never for a recurrence instance, which is merged into its master - and PUT over it. - """ - if not no_overwrite: - return False, no_create + Returns (the header to send with the value ``*``, if any; whether + to search for the UID first). + + A wildcard precondition makes the server enforce the flag in the + same request as the PUT, so there is no window between a check + and the write. It is only sent where the server is not known to + get it wrong. A recurrence instance is merged into its master, + which another save() then PUTs, so it always searches. + + no_overwrite: If-None-Match: * (RFC 9110 section 13.1.2), unless + save.if-none-match says the server overwrites anyway. It only + guards the URL: the search also finds the same UID under another + resource name, so it is left out only where the server is known + to refuse that (save.no-uid-conflict). + + no_create: If-Match: * (section 13.1.1), unless + save-load.mutable.if-match-wildcard says the server gets it + wrong. It replaces the search. Not where a strong ETag or a + Schedule-Tag is cached: that precondition is sent as before + (stricter than ``*``, and two If-Match headers cannot be sent), + and the search is kept, since a 412 to it cannot tell a deleted + object (ConsistencyError) from a changed one (ETagMismatchError). + + Both flags at once cannot hold; the search refuses either way. + """ + if not (no_overwrite or no_create): + return None, False + if no_overwrite and no_create: + return None, True features = self.client.features if self.client is not None else None - if_none_match = features is None or features.is_supported("save.if-none-match") - merges_recurrence = ( - only_this_recurrence is not False or all_recurrences - ) and "RECURRENCE-ID" in self.icalendar_component - search = ( - no_create - or not if_none_match - or merges_recurrence - or features is None - or not features.is_supported("save.no-uid-conflict") + + def supported(feature: str) -> bool: + return features is not None and features.is_supported(feature) + + merges_recurrence = bool( + (only_this_recurrence is not False or all_recurrences) + and "RECURRENCE-ID" in self.icalendar_component ) - return if_none_match, search + if no_overwrite: + if not (features is None or supported("save.if-none-match")): + return None, True + return "if-none-match", merges_recurrence or not supported("save.no-uid-conflict") + if self._precondition_headers(precondition) or not ( + features is None or supported("save-load.mutable.if-match-wildcard") + ): + return None, True + if not (self.id or self.icalendar_component.get("uid")): + from caldav.lib import error + + raise error.ConsistencyError("no_create flag was set, but no ID given") + return "if-match", merges_recurrence def _validate_save_constraints(self, existing, uid, no_overwrite, no_create): """Raise ConsistencyError if no_overwrite/no_create constraints are violated.""" @@ -1817,8 +1861,8 @@ async def get_self(): return None return None - if_none_match, search = self._no_overwrite_plan( - no_overwrite, no_create, only_this_recurrence, all_recurrences + wildcard, search = self._save_plan( + no_overwrite, no_create, only_this_recurrence, all_recurrences, precondition ) if search: uid = self.id or self.icalendar_component.get("uid") @@ -1851,9 +1895,7 @@ async def get_self(): self._maybe_increment_sequence(increase_seqno) path = self.url.path if self.url else None - await self._create( - id=self.id, path=path, precondition=precondition, if_none_match=if_none_match - ) + await self._create(id=self.id, path=path, precondition=precondition, wildcard=wildcard) return self def is_loaded(self): diff --git a/caldav/compatibility_hints.py b/caldav/compatibility_hints.py index fd128e83..59770c57 100644 --- a/caldav/compatibility_hints.py +++ b/caldav/compatibility_hints.py @@ -510,9 +510,9 @@ class FeatureSet: "default": {"support": "full"}, }, "save-load.mutable.if-match-wildcard": { - "description": "An overwrite carrying If-Match: * is accepted, and a PUT carrying it to a missing object is refused. RFC 9110 section 13.1.1 has '*' match any current representation, so it means 'overwrite, but only if the object exists'. When 'unsupported', the server answers 412 Precondition Failed even though the object is there (Zimbra, Radicale - both compare '*' as a literal etag). When 'broken', a missing object is created: the condition is backwards (Bedework, 412 on an existing object) or ignored (SOGo). If-None-Match: * (section 13.1.2) is save.if-none-match. The library does not send If-Match: * itself.", + "description": "An overwrite carrying If-Match: * is accepted, and a PUT carrying it to a missing object is refused. RFC 9110 section 13.1.1 has '*' match any current representation, so it means 'overwrite, but only if the object exists'. When 'unsupported', the server answers 412 Precondition Failed even though the object is there (Zimbra, Radicale - both compare '*' as a literal etag). When 'broken', a missing object is created: the condition is backwards (Bedework, 412 on an existing object) or ignored (SOGo). If-None-Match: * (section 13.1.2) is save.if-none-match. save(no_create=True) sends If-Match: * where this is supported ('full' or 'quirk') and no strong ETag or Schedule-Tag is cached, so the server refuses to create in the same request; elsewhere it searches for the UID before the PUT. The default is 'full': RFC 4791 section 2 requires a CalDAV server to support ETags as RFC 2616 defines them, and RFC 2616 section 14.24 (now RFC 9110 section 13.1.1) says the server MUST NOT perform the method when '*' is given and no object exists.", "default": {"support": "full"}, - "links": ["https://datatracker.ietf.org/doc/html/rfc9110#section-13.1.1"], + "links": ["https://datatracker.ietf.org/doc/html/rfc4791#section-2", "https://datatracker.ietf.org/doc/html/rfc2616#section-14.24", "https://datatracker.ietf.org/doc/html/rfc9110#section-13.1.1"], }, "search": { "description": "calendar MUST support searching for objects using the REPORT method, as specified in RFC4791, section 7", @@ -793,7 +793,7 @@ class FeatureSet: "links": ["https://datatracker.ietf.org/doc/html/rfc9110#section-8.8.3"], }, "save.if-none-match": { - "description": "A PUT carrying If-None-Match: * to an existing object is refused with 412 Precondition Failed, and creates a missing object. RFC 9110 section 13.1.2 has '*' hold only when the resource has no current representation, so it means 'create, but never overwrite'; RFC 4791 section 5.3.2 tells CalDAV clients to send it when creating. save(no_overwrite=True) sends it, so the server enforces 'do not overwrite' in the same request rather than after a search. When 'unsupported', the server overwrites the existing object (SOGo ignores the header; Zimbra compares '*' as a literal etag, so the condition always holds) - the library then does not send it, and relies on the UID search before the PUT, as before. The default is 'full': RFC 4791 section 2 requires a CalDAV server to support ETags as RFC 2616 defines them, and RFC 2616 section 14.26 (now RFC 9110 section 13.1.2) says the server MUST NOT perform the method when '*' is given and the object exists.", + "description": "A PUT carrying If-None-Match: * to an existing object is refused with 412 Precondition Failed, and creates a missing object. RFC 9110 section 13.1.2 has '*' hold only when the resource has no current representation, so it means 'create, but never overwrite'; RFC 4791 section 5.3.2 tells CalDAV clients to send it when creating. save(no_overwrite=True) sends it, so the server enforces 'do not overwrite' in the same request rather than after a search. When 'unsupported', the server overwrites the existing object (SOGo ignores the header; Zimbra compares '*' as a literal etag, so the condition always holds) - the library then does not send it, nor anywhere else this is not supported ('full' or 'quirk'), and relies on the UID search before the PUT, as before. The default is 'full': RFC 4791 section 2 requires a CalDAV server to support ETags as RFC 2616 defines them, and RFC 2616 section 14.26 (now RFC 9110 section 13.1.2) says the server MUST NOT perform the method when '*' is given and the object exists.", "default": {"support": "full"}, "links": ["https://datatracker.ietf.org/doc/html/rfc4791#section-5.3.2", "https://datatracker.ietf.org/doc/html/rfc4791#section-2", "https://datatracker.ietf.org/doc/html/rfc2616#section-14.26", "https://datatracker.ietf.org/doc/html/rfc9110#section-13.1.2"], }, diff --git a/caldav/lib/error.py b/caldav/lib/error.py index 823b93fc..1a2c4964 100644 --- a/caldav/lib/error.py +++ b/caldav/lib/error.py @@ -281,8 +281,9 @@ class UidConflictAuthorizationError(UidConflictError, AuthorizationError): class UidConflictConsistencyError(UidConflictError, ConsistencyError): - """A UID conflict on ``save(no_overwrite=True)``. Its UID search, which - may now be left to the server, raised ConsistencyError for this.""" + """A UID conflict on ``save(no_overwrite=True)`` or + ``save(no_create=True)``. Their UID search, which may now be left to + the server, raised ConsistencyError for this.""" class UidConflictConsistencyAuthorizationError(UidConflictConsistencyError, AuthorizationError): diff --git a/changes/+no-overwrite-if-none-match.changed.md b/changes/+no-overwrite-if-none-match.changed.md index 0e90f643..d0ec644e 100644 --- a/changes/+no-overwrite-if-none-match.changed.md +++ b/changes/+no-overwrite-if-none-match.changed.md @@ -1 +1 @@ -`save(no_overwrite=True)` (and `add_event(..., no_overwrite=True)` and friends) has improved handling on RFC-compliant servers: the server refuses an overwrite at the same URL in the same request as the write, so another client can no longer slip in between the check and the PUT there. A refusal still raises `ConsistencyError`. +`save(no_overwrite=True)` and `save(no_create=True)` (and `add_event(..., no_overwrite=True)` and friends) have improved handling on RFC-compliant servers: the server enforces the flag at the same URL in the same request as the write, so another client can no longer slip in between the check and the PUT there. A refusal still raises `ConsistencyError`. diff --git a/changes/+wildcard-preconditions.breaking.md b/changes/+wildcard-preconditions.breaking.md index 37a68f76..d15b7ef3 100644 --- a/changes/+wildcard-preconditions.breaking.md +++ b/changes/+wildcard-preconditions.breaking.md @@ -1,3 +1,6 @@ -Potentially breaking, on servers that are themselves broken: `save(no_overwrite=True)` now relies on the server honouring `If-None-Match: *`, which every CalDAV server is required to do. Against a server that ignores it, the save may overwrite an existing object. The fix is to tell the client that the server is broken: configure it with the server's `features` (e.g. `features: sogo`), or mark `save.if-none-match` unsupported in a hand-written feature list. Servers known to ignore it: SOGo, Zimbra. +Potentially breaking, on servers that are themselves broken: `save(no_overwrite=True)` and `save(no_create=True)` now rely on the server honouring `If-None-Match: *` and `If-Match: *`, which every CalDAV server is required to do. Against a server that does not, an existing object may be overwritten, a missing one created, or a legitimate update refused. The fix is to tell the client that the server is broken: configure it with the server's `features` (e.g. `features: sogo`), or mark `save.if-none-match` and `save-load.mutable.if-match-wildcard` correctly in a hand-written feature list. Servers known not to honour them, as of this release: -A subclass overriding `_create()` or `_put()` must accept their new keyword argument. +- `If-None-Match: *`: SOGo, Zimbra +- `If-Match: *`: Bedework, GMX, iCloud, Posteo, Radicale, SOGo, Yahoo, Zimbra + +A subclass overriding `_create()` or `_put()` must accept their new `wildcard=` keyword. diff --git a/docs/design/FEATURE_COMPLETE_ROADMAP.md b/docs/design/FEATURE_COMPLETE_ROADMAP.md index f680533f..014b2aee 100644 --- a/docs/design/FEATURE_COMPLETE_ROADMAP.md +++ b/docs/design/FEATURE_COMPLETE_ROADMAP.md @@ -309,7 +309,7 @@ change notifications. Requested by the proposal authors themselves. **Tasks:** - [x] Robust ETag-based collision detection — **done in v3.2.0**: the ETag from PUT/GET responses is cached in `self.props` and `ETagMismatchError` is raised on 412 -- [x] Proper `If-Match` / `If-None-Match` header usage — `If-Match` is sent when an ETag is cached (and `If-Schedule-Tag-Match` takes precedence when a Schedule-Tag is) on save, and on delete with `delete(if_match=True)` (https://github.com/python-caldav/caldav/issues/740); `save(no_overwrite=True)` sends `If-None-Match: *` (not on servers where `save.if-none-match` is unsupported), and leaves out its UID search where `save.no-uid-conflict` is full. `no_create` still searches rather than sending `If-Match: *`, which too many servers get wrong (`save-load.mutable.if-match-wildcard`) +- [x] Proper `If-Match` / `If-None-Match` header usage — `If-Match` is sent when an ETag is cached (and `If-Schedule-Tag-Match` takes precedence when a Schedule-Tag is) on save, and on delete with `delete(if_match=True)` (https://github.com/python-caldav/caldav/issues/740); `save(no_overwrite=True)` sends `If-None-Match: *` (not on servers where `save.if-none-match` is unsupported), and leaves out its UID search where `save.no-uid-conflict` is full. `save(no_create=True)` sends `If-Match: *` instead of searching, where `save-load.mutable.if-match-wildcard` is full and no ETag or Schedule-Tag is cached - [ ] Handle UID vs path name mismatches - [ ] Race condition mitigation - [ ] Clear error messages for conflicts diff --git a/tests/test_caldav_unit.py b/tests/test_caldav_unit.py index e1343fa9..b0951d1a 100755 --- a/tests/test_caldav_unit.py +++ b/tests/test_caldav_unit.py @@ -2642,7 +2642,7 @@ def test_save_none_falls_through_to_put_when_master_missing(self): event, _ = self._make_orphan_event() created = [] event._create = ( - lambda id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( + lambda id=None, path=None, retry_on_failure=True, precondition="auto", wildcard=None: ( created.append(True) ) ) @@ -2654,7 +2654,7 @@ def test_save_false_bypasses_master_lookup(self): event, mock_calendar = self._make_orphan_event() created = [] event._create = ( - lambda id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( + lambda id=None, path=None, retry_on_failure=True, precondition="auto", wildcard=None: ( created.append(True) ) ) @@ -2671,7 +2671,7 @@ def test_add_object_orphan_does_not_raise_notfound(self): created = [] original_create = CalendarObjectResource._create CalendarObjectResource._create = ( - lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( + lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto", wildcard=None: ( created.append(True) ) ) @@ -2700,7 +2700,7 @@ def test_save_orphan_found_by_uid_does_not_recurse(self): with mock.patch.object( CalendarObjectResource, "_create", - lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( + lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto", wildcard=None: ( created.append(self_) ), ): @@ -2737,7 +2737,7 @@ async def fake_create( path=None, retry_on_failure=True, precondition="auto", - if_none_match=False, + wildcard=None, ): created.append(self_) @@ -3224,7 +3224,7 @@ def _put_sync(func): with mock.patch.object( CalendarObjectResource, "_create", - lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto", if_none_match=False: ( + lambda self_, id=None, path=None, retry_on_failure=True, precondition="auto", wildcard=None: ( put.append(self_) ), ): @@ -3244,7 +3244,7 @@ async def fake_create( path=None, retry_on_failure=True, precondition="auto", - if_none_match=False, + wildcard=None, ): put.append(self_) @@ -7500,12 +7500,9 @@ def test_principal_search_failure_keeps_preconditions(self, base) -> None: EMPTY_MULTISTATUS = b'\n' -class TestNoOverwriteIfNoneMatch: - """save(no_overwrite=True) sends If-None-Match: * (RFC 9110 section - 13.1.2), so the server refuses an overwrite of the same URL in the - same request. The UID search before the PUT is only skipped where - the server is known to refuse the UID under another URL as well - (CALDAV:no-uid-conflict, RFC 4791 section 5.3.2.1).""" +class _SaveRequests: + """Records the requests of a save() against a mocked server, sync + and async.""" URL = "http://cal.example.com/dav/user/mycal/" @@ -7517,7 +7514,9 @@ def base(self, request): return AsyncDAVClient - def _client(self, base, put_status=201, put_body=b"", features=None, exc=None): + def _client( + self, base, put_status=201, put_body=b"", features=None, exc=None, report=EMPTY_MULTISTATUS + ): sent = [] client = base(url="http://cal.example.com/dav/", features=features) @@ -7529,7 +7528,7 @@ def request(url, method="GET", body_=None, headers=None): raise exc status, body = put_status, put_body else: - status, body = 207, EMPTY_MULTISTATUS + status, body = 207, report resp = mock.MagicMock() resp.status_code = status resp.reason = "whatever" @@ -7560,6 +7559,14 @@ def _event(self, client, data=ev1, etag=None): _run = staticmethod(TestPreconditionsOnOtherStatuses._run) + +class TestNoOverwriteIfNoneMatch(_SaveRequests): + """save(no_overwrite=True) sends If-None-Match: * (RFC 9110 section + 13.1.2), so the server refuses an overwrite of the same URL in the + same request. The UID search before the PUT is only skipped where + the server is known to refuse the UID under another URL as well + (CALDAV:no-uid-conflict, RFC 4791 section 5.3.2.1).""" + NO_UID_CONFLICT = {"save.no-uid-conflict": {"support": "full"}} def test_search_kept_by_default(self, base) -> None: @@ -7690,3 +7697,133 @@ def test_plain_save_failure_still_retried(self, base) -> None: with pytest.raises(error.PutError): self._run(self._event(client).save()) assert [m for m, _ in sent] == ["PUT", "PUT"] + + +EV1_MULTISTATUS = ( + '\n' + '' + "/dav/user/mycal/ev1.ics" + '"etag-a"' + + ev1 + + "HTTP/1.1 200 OK" + "" +).encode() + + +class TestNoCreateIfMatch(_SaveRequests): + """save(no_create=True) sends If-Match: * (RFC 9110 section 13.1.1), + so the server refuses to create a missing object in the same request, + where save-load.mutable.if-match-wildcard is full. Elsewhere, and + when a cached ETag or Schedule-Tag already makes the PUT + conditional, the UID search before the PUT is kept.""" + + def test_if_match_star_replaces_search(self, base) -> None: + client, sent = self._client(base, put_status=204) + self._run(self._event(client).save(no_create=True)) + assert [m for m, _ in sent] == ["PUT"] + assert sent[-1][1]["if-match"] == "*" + assert "if-none-match" not in sent[-1][1] + + def test_412_raises_consistency_error(self, base) -> None: + """As the search did when the object was missing.""" + client, sent = self._client(base, put_status=412) + with pytest.raises(error.ConsistencyError) as exc_info: + self._run(self._event(client).save(no_create=True)) + assert not isinstance(exc_info.value, error.ETagMismatchError) + assert "there is no object at" in str(exc_info.value) + assert [m for m, _ in sent] == ["PUT"] + + @pytest.mark.parametrize("status", [409, 412]) + def test_uid_conflict_is_also_consistency_error(self, base, status) -> None: + client, sent = self._client(base, put_status=status, put_body=NO_UID_CONFLICT_BODY) + with pytest.raises(error.UidConflictError) as exc_info: + self._run(self._event(client).save(no_create=True)) + assert isinstance(exc_info.value, error.ConsistencyError) + assert [m for m, _ in sent] == ["PUT"] + + def test_failure_not_retried_with_wildcard(self, base) -> None: + client, sent = self._client(base, put_status=500) + with pytest.raises(error.PutError): + self._run(self._event(client).save(no_create=True)) + assert [m for m, _ in sent] == ["PUT"] + + def test_plain_save_sends_no_if_match_star(self, base) -> None: + client, sent = self._client(base) + self._run(self._event(client).save()) + assert "if-match" not in sent[-1][1] + + def test_cached_etag_wins_and_search_kept(self, base) -> None: + """The cached ETag is stricter than '*' and is sent alone; a 412 + to it cannot tell a deleted object from a changed one, so the + search still decides 'does not exist'.""" + client, sent = self._client(base, put_status=204, report=EV1_MULTISTATUS) + self._run(self._event(client, etag='"etag-a"').save(no_create=True)) + assert [m for m, _ in sent] == ["REPORT", "PUT"] + assert sent[-1][1]["if-match"] == '"etag-a"' + + def test_cached_schedule_tag_wins_and_search_kept(self, base) -> None: + """As with an ETag: If-Schedule-Tag-Match is sent, not '*'.""" + client, sent = self._client(base, put_status=204, report=EV1_MULTISTATUS) + event = self._event(client) + event.props[cdav.ScheduleTag.tag] = '"stag-a"' + self._run(event.save(no_create=True)) + assert [m for m, _ in sent] == ["REPORT", "PUT"] + assert sent[-1][1]["if-schedule-tag-match"] == '"stag-a"' + assert "if-match" not in sent[-1][1] + + def test_cached_etag_missing_object_found_by_search(self, base) -> None: + client, sent = self._client(base) + with pytest.raises(error.ConsistencyError): + self._run(self._event(client, etag='"etag-a"').save(no_create=True)) + assert "PUT" not in [m for m, _ in sent] + + def test_weak_etag_gets_if_match_star(self, base) -> None: + """A weak ETag is never sent (If-Match compares strongly).""" + client, sent = self._client(base, put_status=204) + self._run(self._event(client, etag='W/"etag-a"').save(no_create=True)) + assert [m for m, _ in sent] == ["PUT"] + assert sent[-1][1]["if-match"] == "*" + + @pytest.mark.parametrize("support", ["unsupported", "broken"]) + def test_server_mishandling_if_match_star_keeps_search(self, base, support) -> None: + """Radicale and Zimbra refuse '*' on an existing object, Bedework + and SOGo create a missing one: the search-then-PUT stays.""" + features = {"save-load.mutable.if-match-wildcard": {"support": support}} + client, sent = self._client(base, put_status=204, features=features, report=EV1_MULTISTATUS) + self._run(self._event(client).save(no_create=True)) + assert [m for m, _ in sent] == ["REPORT", "PUT"] + assert "if-match" not in sent[-1][1] + + def test_no_uid_raises_before_sending(self, base) -> None: + client, sent = self._client(base) + event = self._event( + client, data=ev1.replace("UID:20010712T182145Z-123401@example.com\n", "") + ) + with pytest.raises(error.ConsistencyError): + self._run(event.save(no_create=True)) + assert sent == [] + + def test_no_create_and_no_overwrite_never_send_both(self, base) -> None: + client, sent = self._client(base, features={"save.no-uid-conflict": {"support": "full"}}) + with pytest.raises(error.ConsistencyError): + self._run(self._event(client).save(no_create=True, no_overwrite=True)) + assert "PUT" not in [m for m, _ in sent] + + def test_recurrence_instance_keeps_search(self, base) -> None: + """A recurrence instance is merged into its master, which is PUT + by another save() - only the search can refuse a missing one.""" + client, sent = self._client(base) + instance = ev1.replace("DTSTART", "RECURRENCE-ID:20060714T170000Z\nDTSTART", 1) + with pytest.raises(error.ConsistencyError): + self._run( + self._event(client, data=instance).save(no_create=True, only_this_recurrence=None) + ) + assert [m for m, _ in sent] == ["REPORT"] + + @pytest.mark.parametrize("server", ["radicale", "zimbra", "bedework_5_0_0", "sogo"]) + def test_hints_of_servers_mishandling_if_match_star(self, server) -> None: + from caldav import compatibility_hints + from caldav.compatibility_hints import FeatureSet + + fs = FeatureSet(getattr(compatibility_hints, server)) + assert not fs.is_supported("save-load.mutable.if-match-wildcard")