Skip to content

Commit 8c61a66

Browse files
committed
refactor: accept generic <error>/<responsedescription> without server fingerprints
_parse_response hardcoded Stalwart's "No resources found" responsedescription text and purelymail's {https://purelymail.com}does-not-exist error child to recognise 404s in multistatus replies. Both <error> and <responsedescription> are optional, server-defined children of <response> per RFC 4918, so the content assertions were never warranted and adding the next server's 404 shape meant editing the generic parser. Accept either element generically. A genuinely novel tag still hits error.weirdness(). The check_404 debug guard is now None-safe since a server may legally send these elements without a response-level status. Addresses §6.1 of docs/design/FULL_CODE_REVIEW_2026-06.md. prompt: for point 6.1 in the code review document, perhaps the solution is to fix so that any <error> or <responsedescription> is accepted, without any hardcoded server fingerprints (though this will fail once a third server throws in a <errortext> or something like that) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> AI Prompts: claude-sonnet-4-6: PR #686. Please look through it and describe what it does. claude-sonnet-4-6: for point 6.1 in the code review document (docs/design/FULL_CODE_REVIEW_2026-06.md), perhaps the solutoin is to fix so that any <error> or <repsonsedescription> is accepted, without any hardcoded server fingerprints. (though this will fail once a third server throws in a <errortext> or something like that)
1 parent aea46ac commit 8c61a66

3 files changed

Lines changed: 74 additions & 22 deletions

File tree

caldav/response.py

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -488,28 +488,24 @@ def _parse_response(self, response: _Element) -> tuple[str, list[_Element], Any
488488
href = _normalize_href(elem.text or "")
489489
elif elem.tag == dav.PropStat.tag:
490490
propstats.append(elem)
491-
elif elem.tag == "{DAV:}responsedescription":
492-
## This happens with Stalwart on a 404.
493-
## This code is mostly moot, but in debug
494-
## mode I want to be sure we do not toss away any data
495-
error.assert_(elem.text == "No resources found")
496-
check_404 = True
497-
elif elem.tag == "{DAV:}error":
498-
## This happens with purelymail on a 404.
499-
## This code is mostly moot, but in debug
500-
## mode I want to be sure we do not toss away any data
501-
children = elem.getchildren()
502-
error.assert_(len(children) == 1)
503-
error.assert_(children[0].tag == "{https://purelymail.com}does-not-exist")
491+
elif elem.tag in ("{DAV:}responsedescription", "{DAV:}error"):
492+
## Both are optional children of <response> per RFC 4918
493+
## and carry server-defined content. We've seen them on
494+
## 404s (Stalwart sends <responsedescription>No resources
495+
## found</responsedescription>, purelymail sends
496+
## <error><…:does-not-exist/></error>), so accept any such
497+
## element generically rather than fingerprinting servers.
504498
check_404 = True
505499
else:
506-
## i.e. purelymail may contain one more tag, <error>...</error>
507-
## This is probably not a breach of the standard. It may
508-
## probably be ignored. But it's something we may want to
509-
## know.
500+
## A tag we don't recognise at all (e.g. a server inventing
501+
## an <errortext> element). Not necessarily a standards
502+
## breach and probably ignorable, but worth surfacing.
510503
error.weirdness("unexpected element found in response", elem)
511504
error.assert_(href)
512-
if check_404:
505+
if check_404 and status:
506+
## We've only ever observed <error>/<responsedescription> on
507+
## 404s; flag it in debug mode if a server pairs them with some
508+
## other status so we notice and revisit this handling.
513509
error.assert_("404" in status)
514510
return (cast(str, href), propstats, status)
515511

docs/design/FULL_CODE_REVIEW_2026-06.md

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -454,11 +454,20 @@ producing drift bugs.
454454
## 6. Altitude / design notes
455455

456456
1. **`response.py:464` — server fingerprints hardcoded in the generic
457-
parser.** purelymail's `{https://purelymail.com}does-not-exist` tag,
458-
Stalwart's "No resources found" string, and SOGo status notes live in the
459-
core multistatus path instead of going through the compatibility-hints
460-
feature matrix. Adding the next server's 404 shape means editing generic
457+
parser.** ✅ FIXED. purelymail's `{https://purelymail.com}does-not-exist`
458+
tag, Stalwart's "No resources found" string, and SOGo status notes lived in
459+
the core multistatus path instead of going through the compatibility-hints
460+
feature matrix. Adding the next server's 404 shape meant editing generic
461461
parsing — the exact inversion the hints mechanism exists to avoid.
462+
463+
**Resolution:** `<error>` and `<responsedescription>` are optional children
464+
of `<response>` per RFC 4918 with server-defined content, so no per-server
465+
config (nor content fingerprint) is warranted at all — `_parse_response`
466+
now accepts either element generically. A genuinely novel tag still hits
467+
`error.weirdness()`. The `check_404` debug guard was made `None`-safe (a
468+
server may legally send these without a response-level status). Tests:
469+
`test_parse_sync_collection_generic_responsedescription` /
470+
`test_parse_sync_collection_generic_error` in `tests/test_protocol.py`.
462471
2. **`vcal.fix()` is a regex-rewriting layer applied to every inbound
463472
object.** §2.1–§2.4 show the current fixups are individually broken in
464473
four different ways; the module's own TODOs flag the approach. Worth

tests/test_protocol.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,53 @@ def test_parse_sync_collection_response(self):
208208
assert result.deleted[0] == "/cal/deleted.ics"
209209
assert result.sync_token == "new-token"
210210

211+
def test_parse_sync_collection_generic_responsedescription(self):
212+
"""A 404 <response> may carry an arbitrary <responsedescription>.
213+
214+
Per RFC 4918 <responsedescription> is an optional child of
215+
<response>; its text is server-defined. We must not hardcode
216+
any particular server's wording (e.g. Stalwart's "No resources
217+
found").
218+
"""
219+
xml = b"""<?xml version="1.0"?>
220+
<D:multistatus xmlns:D="DAV:" xmlns:C="urn:ietf:params:xml:ns:caldav">
221+
<D:response>
222+
<D:href>/cal/gone.ics</D:href>
223+
<D:status>HTTP/1.1 404 Not Found</D:status>
224+
<D:responsedescription>The thing you asked for is not here anymore</D:responsedescription>
225+
</D:response>
226+
<D:sync-token>tok</D:sync-token>
227+
</D:multistatus>"""
228+
229+
result = DAVResponse.from_bytes(xml).parse_sync_collection()
230+
231+
assert result.deleted == ["/cal/gone.ics"]
232+
assert result.changed == []
233+
234+
def test_parse_sync_collection_generic_error(self):
235+
"""A 404 <response> may carry an arbitrary <error> element.
236+
237+
Per RFC 4918 <error> is an optional child of <response> and its
238+
children are server-defined. We must not hardcode any
239+
particular server's error condition (e.g. purelymail's
240+
{https://purelymail.com}does-not-exist).
241+
"""
242+
xml = b"""<?xml version="1.0"?>
243+
<D:multistatus xmlns:D="DAV:" xmlns:C="urn:ietf:params:xml:ns:caldav"
244+
xmlns:X="https://example.com/ns">
245+
<D:response>
246+
<D:href>/cal/gone.ics</D:href>
247+
<D:status>HTTP/1.1 404 Not Found</D:status>
248+
<D:error><X:resource-must-be-null/></D:error>
249+
</D:response>
250+
<D:sync-token>tok</D:sync-token>
251+
</D:multistatus>"""
252+
253+
result = DAVResponse.from_bytes(xml).parse_sync_collection()
254+
255+
assert result.deleted == ["/cal/gone.ics"]
256+
assert result.changed == []
257+
211258
def test_parse_complex_properties(self):
212259
"""Parse complex properties like supported-calendar-component-set."""
213260
xml = b"""<?xml version="1.0"?>

0 commit comments

Comments
 (0)