From 0ec0f604b763c298c1ff72886c7ff910b5fa4f2d Mon Sep 17 00:00:00 2001 From: gnacho Date: Sun, 18 Oct 2026 10:55:57 +0200 Subject: [PATCH] fix: reject calendar objects with several UIDs and bodies with several vCards --- cmd/server/dav_integration_test.go | 143 +++++++++++++++++++++++++++++ internal/dav/objects.go | 24 ++++- 2 files changed, 165 insertions(+), 2 deletions(-) diff --git a/cmd/server/dav_integration_test.go b/cmd/server/dav_integration_test.go index 82a6ede..f5b0e9a 100644 --- a/cmd/server/dav_integration_test.go +++ b/cmd/server/dav_integration_test.go @@ -393,6 +393,149 @@ func TestPutRejectsMismatchedUID(t *testing.T) { } } +// twoUIDsICS carries two unrelated events, in the order that used to slip +// through: validateItem kept the last UID it saw, so a body whose final UID +// matched the resource name was accepted and the first event became a +// component no client could address on its own. +const twoUIDsICS = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//davcloud//test//EN\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:e1\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260102T100000Z\r\n" + + "END:VEVENT\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:e2\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260103T100000Z\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + +// noUIDICS holds a second component without UID, which used to be skipped by +// the loop: the object was stored and one of its components was unaddressable. +const noUIDICS = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//davcloud//test//EN\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:e1\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260102T100000Z\r\n" + + "END:VEVENT\r\n" + + "BEGIN:VEVENT\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260103T100000Z\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + +// recurringWithOverrideICS is the legitimate multi-component body: a master +// event plus one occurrence override, both with the same UID. +const recurringWithOverrideICS = "BEGIN:VCALENDAR\r\n" + + "VERSION:2.0\r\n" + + "PRODID:-//davcloud//test//EN\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:serie-1\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260102T100000Z\r\n" + + "DTEND:20260102T110000Z\r\n" + + "RRULE:FREQ=DAILY;COUNT=3\r\n" + + "SUMMARY:serie\r\n" + + "END:VEVENT\r\n" + + "BEGIN:VEVENT\r\n" + + "UID:serie-1\r\n" + + "RECURRENCE-ID:20260104T100000Z\r\n" + + "DTSTAMP:20260101T000000Z\r\n" + + "DTSTART:20260104T120000Z\r\n" + + "DTEND:20260104T130000Z\r\n" + + "SUMMARY:serie movida\r\n" + + "END:VEVENT\r\n" + + "END:VCALENDAR\r\n" + +// TestPutRejectsMultipleUIDs: a calendar object resource holds one component +// plus, for a recurring event, overrides sharing its UID (RFC 4791). Two +// unrelated events under a single href share one ETag, so a client cannot +// move, edit or delete them individually, and sync breaks. Radicale refuses +// these bodies too. +func TestPutRejectsMultipleUIDs(t *testing.T) { + for _, tc := range []struct { + name string + url string + body string + want string + }{ + {"two different UIDs", "/caldav/alice/def-calendar/e2.ics", twoUIDsICS, "different UIDs"}, + {"a component without UID", "/caldav/alice/def-calendar/e1.ics", noUIDICS, "without UID"}, + } { + t.Run(tc.name, func(t *testing.T) { + srv, _ := newTestServer(t) + url := srv.URL + tc.url + resp, body := do(t, "PUT", url, tc.body, map[string]string{ + "Content-Type": "text/calendar", + }) + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("PUT = %d, want 400: %s", resp.StatusCode, body) + } + if !strings.Contains(body, tc.want) { + t.Errorf("refusal does not mention %q: %s", tc.want, body) + } + resp, _ = do(t, "GET", url, "", nil) + if resp.StatusCode != http.StatusNotFound { + t.Errorf("GET after the rejected PUT = %d, want 404 (nothing may be written)", resp.StatusCode) + } + }) + } +} + +// TestPutAcceptsRecurringEventWithOverride keeps the legitimate case working: +// overrides that share the master UID are part of the same resource. +func TestPutAcceptsRecurringEventWithOverride(t *testing.T) { + srv, _ := newTestServer(t) + url := srv.URL + "/caldav/alice/def-calendar/serie-1.ics" + resp, body := do(t, "PUT", url, recurringWithOverrideICS, map[string]string{ + "Content-Type": "text/calendar", + }) + if resp.StatusCode != http.StatusCreated { + t.Fatalf("PUT of a master event plus a same-UID override = %d, want 201: %s", resp.StatusCode, body) + } + resp, body = do(t, "GET", url, "", nil) + if resp.StatusCode != http.StatusOK { + t.Fatalf("GET = %d, want 200", resp.StatusCode) + } + if !strings.Contains(body, "RECURRENCE-ID:20260104T100000Z") { + t.Errorf("GET lost the override: %s", body) + } +} + +// TestPutRejectsMultipleVCards applies the same rule to an address book: one +// vCard per resource. The body below holds two cards and its first card +// matches the resource name, which is the shape that used to be stored. +func TestPutRejectsMultipleVCards(t *testing.T) { + srv, _ := newTestServer(t) + url := srv.URL + "/carddav/alice/def-addressbook/c1.vcf" + twoCards := testVCF + strings.Replace(testVCF, "UID:c1", "UID:c2", 1) + + resp, body := do(t, "PUT", url, twoCards, map[string]string{ + "Content-Type": "text/vcard", + }) + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("PUT of two vCards = %d, want 400: %s", resp.StatusCode, body) + } + if !strings.Contains(body, "more than one vCard") { + t.Errorf("refusal does not mention the second card: %s", body) + } + resp, _ = do(t, "GET", url, "", nil) + if resp.StatusCode != http.StatusNotFound { + t.Errorf("GET after the rejected PUT = %d, want 404 (nothing may be written)", resp.StatusCode) + } + + resp, body = do(t, "PUT", url, testVCF, map[string]string{ + "Content-Type": "text/vcard", + }) + if resp.StatusCode != http.StatusCreated { + t.Errorf("PUT of a single vCard = %d, want 201: %s", resp.StatusCode, body) + } +} + // TestPutAcceptsLaxContentType documents that PUT validates the content, not // the Content-Type header (issue #5): a parseable stream is accepted even // with a lax media type, an unparseable one is rejected. diff --git a/internal/dav/objects.go b/internal/dav/objects.go index 49abe75..d4e3b60 100644 --- a/internal/dav/objects.go +++ b/internal/dav/objects.go @@ -60,19 +60,39 @@ func (h *Handler) validateItem(name string, raw []byte) (uid string, err error) if err != nil { return "", fmt.Errorf("dav: the request body is not a valid iCalendar stream") } + // RFC 4791: a calendar object resource holds one component plus, for a + // recurring event, overrides that share its UID and carry + // RECURRENCE-ID. Several unrelated events under one href would share a + // single ETag, so no client could move, edit or delete them on their + // own, and a component without UID would be unaddressable. for _, comp := range cal.Children { switch comp.Name { case "VEVENT", "VTODO", "VJOURNAL": - if p := comp.Props.Get("UID"); p != nil { + p := comp.Props.Get("UID") + if p == nil || p.Value == "" { + return "", fmt.Errorf("dav: the calendar object has a %s without UID", comp.Name) + } + if uid == "" { uid = p.Value + continue + } + if uid != p.Value { + return "", fmt.Errorf("dav: the calendar object has components with different UIDs: %q and %q", uid, p.Value) } } } } else { - card, err := vcard.NewDecoder(bytes.NewReader(raw)).Decode() + dec := vcard.NewDecoder(bytes.NewReader(raw)) + card, err := dec.Decode() if err != nil { return "", fmt.Errorf("dav: the request body is not a valid vCard stream") } + // One vCard per resource, for the same reason as iCalendar. The + // decoder stops at the end of a card, so a second card that decodes + // means the body carries one no href could ever address. + if _, err := dec.Decode(); err == nil { + return "", fmt.Errorf("dav: the address object has more than one vCard") + } uid = card.Value("UID") } if uid == "" {