From d4a055b54927cf10df776f5ca6c4f24b89ea54fd Mon Sep 17 00:00:00 2001 From: Distronode <254259913+distronode-com@users.noreply.github.com> Date: Wed, 16 Sep 2026 20:29:50 -0400 Subject: [PATCH 1/2] caldav: list every calendar on the account, and read each one ticked Connecting a CalDAV account binds one collection, and ListCalendars reported only that one, so the calendar picker could never offer the other calendars in the account (#42, Synology Calendar). The selection model behind the picker was already provider-generic; CalDAV simply never listed anything for it to select. ListCalendars now repeats discovery from the bound collection with the stored credentials: current-user-principal (RFC 5397 defines it on every resource), then calendar-home-set, falling back to the collection's parent for a server that answers neither. The server URL typed at connect is not stored, and starting from the collection needs no migration. Every VEVENT-capable collection under the home is listed through the same helper connect now uses, named by its last path segment when it has no displayname, with the bound collection marked Primary and Writable read from DAV:current-user-privilege-set (all, write, write-content or bind; not reported counts as writable, which is what every CalDAV calendar was assumed to be before). If a listing leaves the bound collection out it is still offered, so what an account read before this change stays selectable. A listed collection on a different origin (scheme, host, port) from its calendar home is logged and skipped, at connect and when listing. A calendar id here is a URL that free/busy and bookings send the account's Basic credentials to for as long as it stays selected, so an href must not be able to aim them at another host. This changes connecting for one self-hosted setup: a server behind a reverse proxy that reports its internal scheme or host in every href used to connect and now cannot. Connect names both addresses and points at the proxy or base URL settings for that case, rather than saying no calendar was found. Free/busy now REPORTs every calendar selected for conflicts in each account, fail-open per calendar as before. An account that never saved a selection still reads only its bound collection, including one whose only row is the one migration 00049 seeded. Writes already followed the chosen destination; a test now pins that a booking lands there and that update and cancel act on the stored event URL. A provider whose calendar id says where credentials go can implement the new calendar.SelectionValidator, and SetAccountCalendars asks it to accept every id before its transaction opens. CalDAV does, against its own listing: an unlisted id is a 400 and nothing is saved, where the endpoint used to store any URL the client sent. Google and Microsoft do not implement it, so saving their selections stays a local write with no provider call, exactly as before. Their ids name calendars inside an API the account's token already scopes; changing their behaviour would buy nothing and would, for one, start refusing Google's "primary" alias. A failure to check maps to 502 and a rejected login to 409, as on the GET. For the latter, a CalDAV 401/403 now unwraps to calendar.ErrReauthRequired and IsReauthErr recognises that sentinel, so a revoked app password reads as "reconnect" rather than "unreachable". The connect form's authentication message is unchanged. The picker gains a short explanation of Check and Book, including why Book moves rather than unticks: one destination per user across all accounts, which this does not change. Every new request goes through the existing metadata-only dial guard; no new SSRF tier is added. Fixes #42 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 24 + docs/ARCHITECTURE.md | 27 ++ frontend/src/routes/calendar/+page.svelte | 7 +- internal/caldav/caldav.go | 21 +- internal/caldav/calendars.go | 126 +++++- internal/caldav/calendars_test.go | 519 ++++++++++++++++++++++ internal/caldav/discovery.go | 115 +++-- internal/caldav/freebusy.go | 9 +- internal/caldav/webdav.go | 109 ++++- internal/calendar/autherr.go | 6 +- internal/calendar/autherr_test.go | 1 + internal/calendar/calselect.go | 46 ++ internal/calendar/calselect_test.go | 153 +++++++ internal/handler/calendar.go | 17 + internal/handler/calendar_caldav_test.go | 116 +++++ 15 files changed, 1243 insertions(+), 53 deletions(-) create mode 100644 internal/caldav/calendars_test.go create mode 100644 internal/calendar/calselect_test.go create mode 100644 internal/handler/calendar_caldav_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f45a54ce..d8159800 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,30 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad ## [Unreleased] +### Fixed +- **A CalDAV account now offers every calendar in it, not only the one it connected with.** + Closes [#42](https://github.com/Calnode/calnode/issues/42). + + Connecting binds one collection, and the calendar picker could only ever show that one, so + an account with several calendars (the report was Synology Calendar) could be checked and + booked into through its default calendar alone. The picker now lists every calendar under + the account's calendar home that can hold events, marks read-only shares (from + `DAV:current-user-privilege-set`) so they can be checked but not booked into, and free/busy + reads every calendar ticked for conflicts. Accounts that never saved a selection keep + reading only the calendar they were bound to, and nothing needs reconnecting: the list is + rediscovered from the stored calendar URL. The picker also says what Check and Book mean, + and why Book moves rather than unticks. + + Two refusals come with it, both CalDAV-only. A calendar listed on a different origin from + its calendar home is skipped, because a CalDAV calendar id is the URL that later receives the + account's credentials. This changes connecting for one setup: a server behind a reverse proxy + that reports its internal scheme or host in every href used to connect and now does not, with + an error naming both addresses and pointing at the proxy or base URL settings. And saving a + selection that names a CalDAV calendar the server did not list is refused with a 400, where + the endpoint used to store whatever the client sent. Google and Microsoft selections save + exactly as before, with no call to the provider: their ids carry no credentials anywhere, so + there is nothing for the check to protect. + ## [0.9.0] - 2026-09-10 ### Added diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e91d5db6..f392d7bd 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -477,6 +477,24 @@ Calnode talks to calendars through a **provider abstraction**, not a single vend added via the `openid` scope. A blanket `404 MailboxNotEnabledForRESTAPI` on every `/me/*` call means the account has **no Exchange Online mailbox** (not an auth error). Graph errors log their response body to make this self-evident. +- *CalDAV* (`internal/caldav`): username + app password over Basic auth, connected through + `POST /v1/calendar/caldav/connect`, which walks `current-user-principal` → + `calendar-home-set` → a Depth 1 listing and binds the account's default collection + (`calendar_connections.calendar_id`). **A calendar id is the collection URL**, so the picker, + free/busy and write-back all address collections directly. `ListCalendars` repeats that walk + from the bound collection with the stored credentials (the server URL typed at connect is not + stored; a server that reports no principal there gets the collection's parent) and lists + every VEVENT-capable collection under the home, with `Writable` read from + `DAV:current-user-privilege-set` (`all`, `write`, `write-content` or `bind`; not reported + counts as writable). **A collection on a different origin (scheme, host, port) from the home + is skipped**, at connect and when listing, because a stored id is sent the account's + credentials on every read and write. When that leaves connect with no event calendar at all + (typically a reverse proxy whose upstream reports its internal scheme or host in hrefs), it + fails naming both addresses rather than with "no writable calendar found". CalDAV implements + `calendar.SelectionValidator` (below). Free/busy REPORTs each calendar ticked for conflicts + (`ConflictCalendarIDs`) and makes no discovery requests; an account that never saved a + selection reads only its bound collection. A 401/403 wraps `calendar.ErrReauthRequired`, so + a revoked app password shows as "needs reconnecting" in the picker. **Online-meeting links are provider-matched (`booking_handler.go`).** A `google_meet`/`teams` event type auto-mints a link **only when the primary host's @@ -538,6 +556,15 @@ Saving a sub-calendar destination also moves the account-level destination to th or the choice silently does nothing when the picked calendar lives in a different account from the current destination. +**Where a calendar id says where credentials go, a saved selection is checked first.** A +provider implementing `calendar.SelectionValidator` (CalDAV, whose id is a collection URL that +free/busy and bookings authenticate to) is asked to accept every id before +`SetAccountCalendars` opens its transaction. A refused id is `*UnknownCalendarError` (400 from +`PUT .../calendars`) and nothing is saved; a failure to check is `ErrCalendarList` (502) or a +reauth error (409), as on the GET. Google and Microsoft do not implement it: their ids are +names inside an API the account's token already scopes, so saving there stays a local write +with no provider call, and aliases such as Google's `"primary"` still save. + **`booking_hosts.external_calendar_id` records where each event actually went** (migration 00055). Reschedule and cancel use it rather than re-resolving the current destination - otherwise changing the destination orphans every existing booking: the provider 404s, the diff --git a/frontend/src/routes/calendar/+page.svelte b/frontend/src/routes/calendar/+page.svelte index 67fec4f5..0f50df1c 100644 --- a/frontend/src/routes/calendar/+page.svelte +++ b/frontend/src/routes/calendar/+page.svelte @@ -318,8 +318,11 @@

No calendars found for this account.

{:else}

- Tick the calendars to check for conflicts, and choose the one bookings are - written into. + Check counts busy time on a calendar, so those + times can't be booked. Book is the single calendar + new bookings are written into (one across all your accounts), so it can't be + unticked, only moved to another calendar. Read-only calendars can be checked but + not booked into.

diff --git a/internal/caldav/caldav.go b/internal/caldav/caldav.go index cf143e75..4600ca0d 100644 --- a/internal/caldav/caldav.go +++ b/internal/caldav/caldav.go @@ -171,7 +171,7 @@ func (c *Client) loadConn(ctx context.Context, userID string, checkConflicts, is // For CalDAV the "calendar id" IS the collection URL, so picking a different calendar // inside the account is just a different URL to PUT into. Only applies to the write - // target; conflict checking already reads every selected collection. + // target; conflict checking reads every selected collection (conflictConns). if isDestination == 1 { if subCal, ok, sErr := connstore.DestinationCalendarID(ctx, c.db, userID, "caldav", cn.username); sErr != nil { return conn{}, false, sErr @@ -182,9 +182,10 @@ func (c *Client) loadConn(ctx context.Context, userID string, checkConflicts, is return cn, true, nil } -// conflictConns returns every CalDAV connection for the user with check_conflicts = 1, so a -// user can connect several CalDAV accounts and have them all checked. Decrypt failures on one -// row are logged and skipped (fail-open). +// conflictConns returns one entry per calendar to read for conflicts, across every CalDAV +// connection the user has with check_conflicts = 1, so a user can connect several CalDAV +// accounts and tick several calendars in each. Entries of one account share its credentials and +// differ only in calURL. Decrypt failures on one row are logged and skipped (fail-open). func (c *Client) conflictConns(ctx context.Context, userID string) ([]conn, error) { rows, err := c.db.QueryContext(ctx, ` SELECT id, COALESCE(account_email,''), access_token_enc, calendar_id @@ -208,9 +209,11 @@ func (c *Client) conflictConns(ctx context.Context, userID string) ([]conn, erro return nil, err } // Resolve selection + decrypt after the cursor is closed (single-connection DB pool; the - // ConflictCalendarIDs query would deadlock against an open cursor). CalDAV binds one - // calendar per connection, so the sub-calendar picker is simply an on/off toggle: an empty - // result means the user deselected it. + // ConflictCalendarIDs query would deadlock against an open cursor). An account with a saved + // selection yields exactly the calendars ticked in it, and none when all were unticked. One + // that never saved a selection yields its bound collection, which is all any CalDAV + // connection read before its calendars could be listed. The ids are collection URLs, and + // SetAccountCalendars only saves ones this provider listed for the account. var conns []conn for _, d := range data { calIDs, err := calendar.ConflictCalendarIDs(ctx, c.db, "caldav", userID, d.username, d.calURL) @@ -225,7 +228,9 @@ func (c *Client) conflictConns(ctx context.Context, userID string) ([]conn, erro c.logger.Warn("caldav: skipping connection with bad credentials", "user_id", userID, "error", err) continue } - conns = append(conns, conn{id: d.id, username: d.username, password: string(pw), calURL: d.calURL}) + for _, calURL := range calIDs { + conns = append(conns, conn{id: d.id, username: d.username, password: string(pw), calURL: calURL}) + } } return conns, nil } diff --git a/internal/caldav/calendars.go b/internal/caldav/calendars.go index 3a55808f..1a6c011f 100644 --- a/internal/caldav/calendars.go +++ b/internal/caldav/calendars.go @@ -3,27 +3,129 @@ package caldav import ( "context" "database/sql" + "fmt" "github.com/calnode/calnode/internal/calendar" ) -// ListCalendars returns the connected CalDAV calendar for the account. Discovery currently -// binds one calendar per connection; enumerating multiple CalDAV collections is a future -// enhancement, so this reports the single bound calendar. +// ListCalendars lists every calendar in the account that can hold events: each VEVENT-capable +// collection under the account's calendar home, on the same origin as that home (see +// listCollections). The collection the connection is bound to is marked Primary; Writable comes +// from the server's DAV:current-user-privilege-set, and counts as true when the server does not +// report one. +// +// The server URL typed at connect time is not stored, so discovery restarts from the bound +// collection (see homeForCalendar) using the stored credentials. Nothing new is persisted, which +// is why this needs no migration and works for connections made before calendars were listed. func (c *Client) ListCalendars(ctx context.Context, userID, accountEmail string) ([]calendar.CalendarInfo, error) { - var calID string + cn, ok, err := c.accountConn(ctx, userID, accountEmail) + if err != nil || !ok { + return nil, err + } + home, err := c.homeForCalendar(ctx, cn) + if err != nil { + return nil, err + } + l, err := c.listCollections(ctx, home, cn.username, cn.password) + if err != nil { + return nil, err + } + + out := make([]calendar.CalendarInfo, 0, len(l.collections)+1) + boundListed := false + for _, col := range l.collections { + primary := col.url == cn.calURL + boundListed = boundListed || primary + out = append(out, calendar.CalendarInfo{ID: col.url, Name: col.name(), Primary: primary, Writable: col.writable}) + } + if !boundListed { + // The bound collection is what free/busy reads until a selection is saved, and it is + // already the URL this connection sends its credentials to, so it stays selectable even + // when the home listing leaves it out (a home found by the parent fallback, say). + // Nothing is known about it beyond what connect found, which is what this method + // reported before calendars were listed. + bound := calendar.CalendarInfo{ID: cn.calURL, Name: lastSegment(cn.calURL), Primary: true, Writable: true} + out = append([]calendar.CalendarInfo{bound}, out...) + } + return out, nil +} + +var _ calendar.SelectionValidator = (*Client)(nil) + +// ValidateSelection accepts a selection only when every calendar id is one ListCalendars offers +// for the account right now. A CalDAV calendar id is the collection URL that free/busy and +// booking writes send the account's credentials to, so an id the client made up would bypass +// both the connect-time URL check and the same-origin rule applied when listing. +func (c *Client) ValidateSelection(ctx context.Context, userID, accountEmail string, calendarIDs []string) error { + cals, err := c.ListCalendars(ctx, userID, accountEmail) + if err != nil { + return err + } + listed := make(map[string]bool, len(cals)) + for _, cal := range cals { + listed[cal.ID] = true + } + for _, id := range calendarIDs { + if !listed[id] { + return &calendar.UnknownCalendarError{CalendarID: id} + } + } + return nil +} + +// homeForCalendar finds the calendar home holding the connection's bound collection. +// DAV:current-user-principal is defined on every resource (RFC 5397), and the principal names +// its calendar-home-set, so this is the same walk connect makes, started from the collection +// instead of the server URL. A server that answers neither gets the collection's parent, which is +// the calendar home on the usual layout (.../calendars///). +// +// A failed principal or home lookup falls through to the parent rather than failing, so a bound +// collection that has since been deleted does not stop the user choosing another one. An +// authentication failure still surfaces: the parent listing is made with the same credentials. +func (c *Client) homeForCalendar(ctx context.Context, cn conn) (string, error) { + ms, reqURL, err := c.propfind(ctx, cn.calURL, cn.username, cn.password, "0", propCurrentUserPrincipal) + if err != nil { + c.logger.Warn("caldav: principal lookup failed, listing the bound calendar's parent", "calendar", cn.calURL, "error", err) + return parentCollection(cn.calURL), nil + } + var principal string + for _, r := range ms.Responses { + if h := r.okProp().CurrentUserPrincipal.Href; h != "" { + principal = resolveRef(reqURL, h) + break + } + } + if principal != "" { + home, err := c.calendarHome(ctx, principal, cn.username, cn.password) + if err != nil { + c.logger.Warn("caldav: calendar home lookup failed, listing the bound calendar's parent", "principal", principal, "error", err) + } else if home != "" { + return home, nil + } + } + return parentCollection(cn.calURL), nil +} + +// accountConn loads one account's connection (by account email) regardless of its +// check_conflicts / is_destination flags. ok=false when the user has no such account. +func (c *Client) accountConn(ctx context.Context, userID, accountEmail string) (conn, bool, error) { + var cn conn + var pwEnc string err := c.db.QueryRowContext(ctx, ` - SELECT calendar_id FROM calendar_connections + SELECT id, COALESCE(account_email,''), access_token_enc, calendar_id + FROM calendar_connections WHERE user_id = ? AND provider = 'caldav' AND COALESCE(account_email,'') = ? - LIMIT 1`, userID, accountEmail).Scan(&calID) + LIMIT 1`, userID, accountEmail).Scan(&cn.id, &cn.username, &pwEnc, &cn.calURL) if err == sql.ErrNoRows { - return nil, nil + return conn{}, false, nil } if err != nil { - return nil, err + return conn{}, false, fmt.Errorf("caldav: load account connection: %w", err) + } + pw, err := c.decrypt(pwEnc) + if err != nil { + return conn{}, false, fmt.Errorf("caldav: decrypt password: %w", err) } - // CalDAV exposes exactly the one collection the user configured, and they configured it - // precisely so Calnode could write there - so unlike Google/Microsoft there is no - // read-only case to detect here. - return []calendar.CalendarInfo{{ID: calID, Name: "Calendar", Primary: true, Writable: true}}, nil + cn.password = string(pw) + return cn, true, nil } diff --git a/internal/caldav/calendars_test.go b/internal/caldav/calendars_test.go new file mode 100644 index 00000000..8931ecb2 --- /dev/null +++ b/internal/caldav/calendars_test.go @@ -0,0 +1,519 @@ +package caldav + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "reflect" + "slices" + "sort" + "strings" + "sync" + "testing" + "time" + + "github.com/calnode/calnode/internal/calendar" +) + +// multiCalServer is one CalDAV account whose calendar home holds every kind of collection the +// calendar listing has to tell apart: +// +// /calendars/user/elsewhere/ an href on ANOTHER origin (a second server), listed first +// /calendars/user/personal/ "Personal", VEVENT, write privilege: the bound calendar +// /calendars/user/team/ no display name, VEVENT+VTODO, privileges not reported +// /calendars/user/tasks/ "Tasks", VTODO only +// /calendars/user/holidays/ "Holidays", VEVENT, read + write-properties only +// +// Every request is recorded, and any request reaching the other origin is counted. +type multiCalServer struct { + srv *httptest.Server + other *httptest.Server + + // noPrincipalOnCollections makes a PROPFIND for the principal on anything but "/" fail, + // as on a server that does not implement RFC 5397 on calendar collections. + noPrincipalOnCollections bool + + mu sync.Mutex + reqs []string // "METHOD /path" + events map[string]string // path -> stored iCalendar body + otherHits int +} + +func newMultiCalServer(t *testing.T) *multiCalServer { + t.Helper() + fs := &multiCalServer{events: map[string]string{}} + fs.other = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fs.mu.Lock() + fs.otherHits++ + fs.mu.Unlock() + w.WriteHeader(http.StatusMultiStatus) + io.WriteString(w, ``) + })) + t.Cleanup(fs.other.Close) + fs.srv = httptest.NewServer(http.HandlerFunc(fs.serve)) + t.Cleanup(fs.srv.Close) + return fs +} + +func (fs *multiCalServer) url(p string) string { return fs.srv.URL + p } + +func (fs *multiCalServer) reset() { + fs.mu.Lock() + defer fs.mu.Unlock() + fs.reqs = nil +} + +// paths returns the sorted paths of recorded requests with the given method. +func (fs *multiCalServer) paths(method string) []string { + fs.mu.Lock() + defer fs.mu.Unlock() + var out []string + for _, r := range fs.reqs { + if m, p, _ := strings.Cut(r, " "); m == method { + out = append(out, p) + } + } + sort.Strings(out) + return out +} + +func (fs *multiCalServer) hitsElsewhere() int { + fs.mu.Lock() + defer fs.mu.Unlock() + return fs.otherHits +} + +const msOpen = `` + +func okResponse(href, props string) string { + return `` + href + `HTTP/1.1 200 OK` + + props + `` +} + +func calendarProps(name, comps, privileges string) string { + p := `` + if name != "" { + p += `` + name + `` + } + p += `` + comps + `` + if privileges != "" { + p += `` + privileges + `` + } + return p +} + +const ( + vevent = `` + vtodo = `` + priv = `` +) + +// busyHour is the hour (UTC, 2026-06-25) of the one event each calendar returns, so a FreeBusy +// result says which calendars were read. +var busyHour = map[string]int{ + "/calendars/user/personal/": 9, + "/calendars/user/team/": 10, + "/calendars/user/holidays/": 11, +} + +func (fs *multiCalServer) serve(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + fs.mu.Lock() + fs.reqs = append(fs.reqs, r.Method+" "+r.URL.Path) + fs.mu.Unlock() + + multistatus := func(inner string) { + w.WriteHeader(http.StatusMultiStatus) + io.WriteString(w, msOpen+inner+``) + } + switch { + case r.Method == "PROPFIND" && strings.Contains(string(body), "current-user-principal"): + if fs.noPrincipalOnCollections && r.URL.Path != "/" { + w.WriteHeader(http.StatusNotFound) + return + } + multistatus(okResponse(r.URL.Path, `/principals/user/`)) + case r.Method == "PROPFIND" && strings.Contains(string(body), "calendar-home-set") && r.URL.Path == "/principals/user/": + multistatus(okResponse(r.URL.Path, `/calendars/user/`)) + case r.Method == "PROPFIND" && r.URL.Path == "/calendars/user/" && r.Header.Get("Depth") == "1": + multistatus( + okResponse("/calendars/user/", ``) + + okResponse(fs.other.URL+"/calendars/user/elsewhere/", + calendarProps("Elsewhere", vevent, fmt.Sprintf(priv, "read")+fmt.Sprintf(priv, "write"))) + + okResponse("/calendars/user/personal/", + calendarProps("Personal", vevent, fmt.Sprintf(priv, "read")+fmt.Sprintf(priv, "write"))) + + // Privileges reported under a 404 propstat, listed before the 200 one: not reported. + `/calendars/user/team/` + + `HTTP/1.1 404 Not Found` + + `HTTP/1.1 200 OK` + calendarProps("", vevent+vtodo, "") + `` + + `` + + okResponse("/calendars/user/tasks/", + calendarProps("Tasks", vtodo, fmt.Sprintf(priv, "all"))) + + okResponse("/calendars/user/holidays/", + calendarProps("Holidays", vevent, fmt.Sprintf(priv, "read")+fmt.Sprintf(priv, "write-properties"))), + ) + case r.Method == "REPORT": + hour, ok := busyHour[r.URL.Path] + if !ok { + w.WriteHeader(http.StatusNotFound) + return + } + multistatus(okResponse(r.URL.Path+"ev.ics", fmt.Sprintf(`BEGIN:VCALENDAR +BEGIN:VEVENT +UID:%[1]d@x +DTSTART:20260625T%02[1]d0000Z +DTEND:20260625T%02[2]d0000Z +END:VEVENT +END:VCALENDAR`, hour, hour+1))) + case r.Method == http.MethodPut: + fs.mu.Lock() + fs.events[r.URL.Path] = string(body) + fs.mu.Unlock() + w.WriteHeader(http.StatusCreated) + case r.Method == http.MethodGet: + fs.mu.Lock() + ev, ok := fs.events[r.URL.Path] + fs.mu.Unlock() + if !ok { + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("ETag", `"1"`) + io.WriteString(w, ev) + case r.Method == http.MethodDelete: + w.WriteHeader(http.StatusNoContent) + default: + w.WriteHeader(http.StatusNotFound) + } +} + +const multiCalAccount = "user@example.com" + +// connectMultiCal connects user u1 to a fresh multiCalServer and clears the request log. +func connectMultiCal(t *testing.T) (*Client, *multiCalServer) { + t.Helper() + c := newTestClient(t) + seedUser(t, c.db, "u1") + fs := newMultiCalServer(t) + _, bound, err := c.Connect(context.Background(), "u1", fs.srv.URL, multiCalAccount, "app-pw") + if err != nil { + t.Fatalf("Connect: %v", err) + } + if want := fs.url("/calendars/user/personal/"); bound != want { + t.Fatalf("connect bound %q, want %q (the first same-origin VEVENT calendar)", bound, want) + } + fs.reset() + return c, fs +} + +func selectCalendars(t *testing.T, c *Client, sels ...calendar.CalendarSelection) { + t.Helper() + svc := calendar.NewService(c.db) + svc.Register(c) + if err := svc.SetAccountCalendars(context.Background(), "u1", "caldav", multiCalAccount, sels); err != nil { + t.Fatalf("SetAccountCalendars: %v", err) + } +} + +func sel(id string, check, dest bool) calendar.CalendarSelection { + return calendar.CalendarSelection{CalendarInfo: calendar.CalendarInfo{ID: id}, CheckConflicts: check, IsDestination: dest} +} + +func busyHours(t *testing.T, c *Client) []int { + t.Helper() + day := time.Date(2026, 6, 25, 0, 0, 0, 0, time.UTC) + busy, err := c.FreeBusy(context.Background(), "u1", day, day.Add(24*time.Hour)) + if err != nil { + t.Fatalf("FreeBusy: %v", err) + } + var hours []int + for _, iv := range busy { + hours = append(hours, iv.Start.Hour()) + } + sort.Ints(hours) + return hours +} + +// Issue #42: a CalDAV account with several calendars only ever showed the one it was bound to. +func TestListCalendars_listsEveryEventCalendarOnTheAccount(t *testing.T) { + c, fs := connectMultiCal(t) + + got, err := c.ListCalendars(context.Background(), "u1", multiCalAccount) + if err != nil { + t.Fatalf("ListCalendars: %v", err) + } + want := []calendar.CalendarInfo{ + {ID: fs.url("/calendars/user/personal/"), Name: "Personal", Primary: true, Writable: true}, + // No display name: named by its path. Privileges not reported: writable. + {ID: fs.url("/calendars/user/team/"), Name: "team", Primary: false, Writable: true}, + // read + write-properties is a read-only share. + {ID: fs.url("/calendars/user/holidays/"), Name: "Holidays", Primary: false, Writable: false}, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("ListCalendars =\n %+v\nwant (no VTODO-only list, nothing from another origin)\n %+v", got, want) + } + // Discovery restarted from the stored collection, not a server URL that is never stored. + if p := fs.paths("PROPFIND"); !slices.Contains(p, "/calendars/user/personal/") { + t.Errorf("PROPFINDs = %v, want discovery to start at the bound collection", p) + } + if n := fs.hitsElsewhere(); n != 0 { + t.Errorf("%d request(s) reached the other origin; its collection must be skipped, not contacted", n) + } +} + +// A server that does not answer current-user-principal on a calendar collection still gets its +// calendars listed, from the bound collection's parent. +func TestListCalendars_fallsBackToTheBoundCalendarsParent(t *testing.T) { + c, fs := connectMultiCal(t) + fs.noPrincipalOnCollections = true + + got, err := c.ListCalendars(context.Background(), "u1", multiCalAccount) + if err != nil { + t.Fatalf("ListCalendars: %v", err) + } + if len(got) != 3 || !got[0].Primary { + t.Errorf("ListCalendars = %+v, want the same three calendars with the bound one primary", got) + } + if slices.Contains(fs.paths("PROPFIND"), "/principals/user/") { + t.Error("expected no principal walk when the collection reports no principal") + } +} + +// A stored app password the server now rejects is a reconnect, not an unreachable server. +func TestListCalendars_rejectedPasswordNeedsAReconnect(t *testing.T) { + c := newTestClient(t) + seedUser(t, c.db, "u1") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + if err := c.saveConnection(context.Background(), "u1", multiCalAccount, "revoked", srv.URL+"/calendars/user/personal/"); err != nil { + t.Fatalf("saveConnection: %v", err) + } + _, err := c.ListCalendars(context.Background(), "u1", multiCalAccount) + if !calendar.IsReauthErr(err) { + t.Errorf("err = %v, want one IsReauthErr recognises", err) + } + if err == nil || !strings.Contains(err.Error(), "authentication failed") { + t.Errorf("err = %v, want the connect form's authentication message kept", err) + } +} + +func TestFreeBusy_readsExactlyTheCalendarsSelectedForConflicts(t *testing.T) { + c, fs := connectMultiCal(t) + selectCalendars(t, c, + sel(fs.url("/calendars/user/personal/"), false, true), // unticked: never read + sel(fs.url("/calendars/user/team/"), true, false), + sel(fs.url("/calendars/user/holidays/"), true, false), // read-only is fine for conflicts + ) + fs.reset() + + if got, want := busyHours(t, c), []int{10, 11}; !reflect.DeepEqual(got, want) { + t.Errorf("busy hours = %v, want %v (team + holidays)", got, want) + } + if got, want := fs.paths("REPORT"), []string{"/calendars/user/holidays/", "/calendars/user/team/"}; !reflect.DeepEqual(got, want) { + t.Errorf("REPORTs = %v, want exactly %v", got, want) + } + if p := fs.paths("PROPFIND"); len(p) != 0 { + t.Errorf("free/busy made discovery requests %v; it must read stored selections only", p) + } +} + +// Connections that never saved a selection keep reading only the calendar they were bound to, +// whether they have no selection rows at all or the one row migration 00049 seeded. +func TestFreeBusy_withoutASavedSelectionReadsOnlyTheBoundCalendar(t *testing.T) { + for _, tc := range []struct { + name string + seed bool + }{{"no selection rows", false}, {"row seeded by migration 00049", true}} { + t.Run(tc.name, func(t *testing.T) { + c, fs := connectMultiCal(t) + if tc.seed { + if _, err := c.db.Exec(` + INSERT INTO connection_calendars (id, user_id, provider, account_email, calendar_id, name, check_conflicts, is_destination) + SELECT 'seeded', user_id, provider, account_email, calendar_id, '', check_conflicts, is_destination + FROM calendar_connections WHERE user_id = 'u1'`); err != nil { + t.Fatalf("seed: %v", err) + } + } + + if got, want := busyHours(t, c), []int{9}; !reflect.DeepEqual(got, want) { + t.Errorf("busy hours = %v, want %v (the bound calendar only)", got, want) + } + if got, want := fs.paths("REPORT"), []string{"/calendars/user/personal/"}; !reflect.DeepEqual(got, want) { + t.Errorf("REPORTs = %v, want exactly %v", got, want) + } + }) + } +} + +func TestCreateEvent_writesIntoTheChosenCalendar(t *testing.T) { + c, fs := connectMultiCal(t) + team := fs.url("/calendars/user/team/") + selectCalendars(t, c, + sel(fs.url("/calendars/user/personal/"), true, false), + sel(team, true, true), + ) + fs.reset() + + start := time.Date(2026, 6, 26, 15, 0, 0, 0, time.UTC) + eventID, _, calID, err := c.CreateEvent(context.Background(), "u1", calendar.CreateEventParams{ + Summary: "Intro call", Start: start, End: start.Add(time.Hour), + }) + if err != nil { + t.Fatalf("CreateEvent: %v", err) + } + puts := fs.paths(http.MethodPut) + if len(puts) != 1 || !strings.HasPrefix(puts[0], "/calendars/user/team/") { + t.Fatalf("PUTs = %v, want one into /calendars/user/team/", puts) + } + if eventID != fs.url(puts[0]) { + t.Errorf("eventID = %q, want the resource URL written %q", eventID, fs.url(puts[0])) + } + if calID != team { + t.Errorf("calendarID = %q, want %q recorded against the booking", calID, team) + } + + // The destination moves back to Personal. The booking's event still lives in Team, and + // moving or cancelling it must act on the stored resource URL. + selectCalendars(t, c, + sel(fs.url("/calendars/user/personal/"), true, true), + sel(team, true, false), + ) + fs.reset() + if err := c.UpdateEvent(context.Background(), "u1", calID, eventID, start.Add(time.Hour), start.Add(2*time.Hour)); err != nil { + t.Fatalf("UpdateEvent: %v", err) + } + if err := c.CancelEvent(context.Background(), "u1", calID, eventID); err != nil { + t.Fatalf("CancelEvent: %v", err) + } + for _, m := range []string{http.MethodGet, http.MethodPut, http.MethodDelete} { + if got := fs.paths(m); !reflect.DeepEqual(got, puts) { + t.Errorf("%s paths = %v, want %v (the stored event URL)", m, got, puts) + } + } +} + +// A calendar id is a URL that later receives the account's credentials, so a selection may only +// name calendars the server listed. Nothing is saved when any id is refused, and validating never +// contacts the refused URL. +func TestSetAccountCalendars_refusesACalendarTheServerDidNotList(t *testing.T) { + for _, tc := range []struct { + name string + id func(fs *multiCalServer) string + }{ + {"VTODO-only list", func(fs *multiCalServer) string { return fs.url("/calendars/user/tasks/") }}, + {"collection on another origin", func(fs *multiCalServer) string { return fs.other.URL + "/calendars/user/elsewhere/" }}, + {"URL the server never listed", func(fs *multiCalServer) string { return fs.url("/calendars/someone-else/private/") }}, + } { + t.Run(tc.name, func(t *testing.T) { + c, fs := connectMultiCal(t) + svc := calendar.NewService(c.db) + svc.Register(c) + id := tc.id(fs) + + err := svc.SetAccountCalendars(context.Background(), "u1", "caldav", multiCalAccount, + []calendar.CalendarSelection{sel(fs.url("/calendars/user/personal/"), true, false), sel(id, true, true)}) + var unknown *calendar.UnknownCalendarError + if !errors.As(err, &unknown) || unknown.CalendarID != id { + t.Fatalf("err = %v, want *UnknownCalendarError for %q", err, id) + } + var n int + if err := c.db.QueryRow(`SELECT COUNT(*) FROM connection_calendars WHERE user_id = 'u1'`).Scan(&n); err != nil { + t.Fatal(err) + } + if n != 0 { + t.Errorf("%d selection row(s) saved despite the refusal", n) + } + if n := fs.hitsElsewhere(); n != 0 { + t.Errorf("%d request(s) reached the other origin while validating", n) + } + }) + } +} + +// A self-hosted server behind a TLS-terminating reverse proxy can report its internal scheme or +// host in every href. Those calendars are skipped for origin, and connect must say that is why it +// found nothing, rather than "no calendar found", which points the operator at the wrong thing. +// A server that genuinely has no event calendar keeps the old message. +func TestConnect_explainsCalendarsListedOnlyAtAnotherAddress(t *testing.T) { + for _, tc := range []struct { + name string + hrefBase func(r *http.Request) string // prefix for each calendar href + comps string + want func(host string) string + }{ + { + name: "every event calendar on another origin", + hrefBase: func(r *http.Request) string { return "https://" + r.Host }, + comps: vevent, + want: func(host string) string { + return "the server lists its calendars at a different address (https://" + host + + ") than the one Calnode reached it at (http://" + host + "); check the server's reverse proxy or base URL settings" + }, + }, + { + name: "no event calendar at all", + hrefBase: func(*http.Request) string { return "" }, + comps: vtodo, + want: func(string) string { return "no writable calendar found on the server" }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + c := newTestClient(t) + seedUser(t, c.db, "u1") + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + w.WriteHeader(http.StatusMultiStatus) + switch { + case strings.Contains(string(body), "current-user-principal"): + io.WriteString(w, msOpen+okResponse(r.URL.Path, `/principals/user/`)+``) + case strings.Contains(string(body), "calendar-home-set"): + io.WriteString(w, msOpen+okResponse(r.URL.Path, `/calendars/user/`)+``) + default: + base := tc.hrefBase(r) + io.WriteString(w, msOpen+ + okResponse("/calendars/user/", ``)+ + okResponse(base+"/calendars/user/personal/", calendarProps("Personal", tc.comps, ""))+ + okResponse(base+"/calendars/user/work/", calendarProps("Work", tc.comps, ""))+ + // A same-origin tasks list must not stop the origin explanation. + okResponse("/calendars/user/tasks/", calendarProps("Tasks", vtodo, ""))+ + ``) + } + })) + defer srv.Close() + + _, _, err := c.Connect(context.Background(), "u1", srv.URL, multiCalAccount, "app-pw") + if err == nil { + t.Fatal("Connect succeeded, want an error") + } + if want := tc.want(strings.TrimPrefix(srv.URL, "http://")); !strings.Contains(err.Error(), want) { + t.Errorf("err = %q\nwant it to contain %q", err, want) + } + if n := countConns(t, c.db, "u1"); n != 0 { + t.Errorf("%d connection(s) saved by a failed connect", n) + } + }) + } +} + +func TestSameOrigin(t *testing.T) { + for _, tc := range []struct { + a, b string + want bool + }{ + {"https://dav.example.com/cal/", "https://dav.example.com:443/other/", true}, + {"http://dav.example.com/cal/", "http://DAV.example.com:80/", true}, + {"https://dav.example.com/cal/", "http://dav.example.com/cal/", false}, + {"https://dav.example.com/cal/", "https://dav.example.com:8443/cal/", false}, + {"https://dav.example.com/cal/", "https://evil.example.com/cal/", false}, + {"/relative/", "/relative/", false}, + } { + if got := sameOrigin(tc.a, tc.b); got != tc.want { + t.Errorf("sameOrigin(%q, %q) = %v, want %v", tc.a, tc.b, got, tc.want) + } + } +} diff --git a/internal/caldav/discovery.go b/internal/caldav/discovery.go index 874e46d5..a1c38840 100644 --- a/internal/caldav/discovery.go +++ b/internal/caldav/discovery.go @@ -24,7 +24,7 @@ const ( ` propCalendarCollections = ` -` +` ) // Connect validates the given CalDAV credentials by discovering the user's default calendar @@ -49,59 +49,124 @@ func (c *Client) Connect(ctx context.Context, userID, serverURL, username, passw } // discoverCalendar walks principal → calendar-home → calendar collections and returns the URL -// of the first VEVENT-capable calendar collection. +// of the collection the connection binds to: the account's default, which free/busy reads and +// bookings are written to until the user picks calendars of their own. func (c *Client) discoverCalendar(ctx context.Context, serverURL, username, password string) (string, error) { // 1. current-user-principal — try the base URL, then the RFC 5785 well-known path. - principal, base, err := c.findPrincipal(ctx, serverURL, username, password) + principal, _, err := c.findPrincipal(ctx, serverURL, username, password) if err != nil { return "", err } // 2. calendar-home-set on the principal. - ms, homeReqURL, err := c.propfind(ctx, principal, username, password, "0", propCalendarHomeSet) + home, err := c.calendarHome(ctx, principal, username, password) if err != nil { return "", err } - var home string - for _, r := range ms.Responses { - if h := r.okProp().CalendarHomeSet.Href; h != "" { - home = resolveRef(homeReqURL, h) - break - } - } if home == "" { // Some servers expose the calendar home at the principal itself. home = principal } - // 3. list calendar collections (Depth 1) and pick the first that supports VEVENT. - ms, homeReqURL, err = c.propfind(ctx, home, username, password, "1", propCalendarCollections) + // 3. list calendar collections (Depth 1) and pick the default among them. + l, err := c.listCollections(ctx, home, username, password) if err != nil { return "", err } - var firstCal string + cols := l.collections + if len(cols) == 0 && len(l.foreign) > 0 { + // Every event calendar was on another origin. That is the signature of a self-hosted + // server behind a reverse proxy that reports its internal scheme or host in hrefs, and + // "no calendar found" would send the operator looking in the wrong place. + return "", fmt.Errorf("caldav: the server lists its calendars at a different address (%s) than the one Calnode reached it at (%s); check the server's reverse proxy or base URL settings", + displayOrigin(l.foreign[0]), displayOrigin(l.homeURL)) + } + if len(cols) == 0 { + return "", fmt.Errorf("caldav: no writable calendar found on the server") + } + for _, col := range cols { + // Prefer a calendar literally named/pathed "calendar" as the default when present. + if strings.EqualFold(col.displayName, "Calendar") || strings.Contains(strings.ToLower(col.url), "/calendar") { + return col.url, nil + } + } + return cols[0].url, nil +} + +// calendarHome returns the calendar-home-set a principal reports (RFC 4791 §6.2.1), or "" when +// it reports none. +func (c *Client) calendarHome(ctx context.Context, principal, username, password string) (string, error) { + ms, reqURL, err := c.propfind(ctx, principal, username, password, "0", propCalendarHomeSet) + if err != nil { + return "", err + } + for _, r := range ms.Responses { + if h := r.okProp().CalendarHomeSet.Href; h != "" { + return resolveRef(reqURL, h), nil + } + } + return "", nil +} + +// collection is one calendar collection that can hold events, as listed under a calendar home. +type collection struct { + url string + displayName string // as reported; "" when the server gave none + writable bool +} + +// name is what the calendar picker shows: the display name, else the last path segment. +func (col collection) name() string { + if n := strings.TrimSpace(col.displayName); n != "" { + return n + } + return lastSegment(col.url) +} + +// listing is what one Depth 1 PROPFIND of a calendar home returned. +type listing struct { + homeURL string // the URL that answered, after any redirects + collections []collection // same-origin, VEVENT-capable, in server order + foreign []string // VEVENT-capable collections skipped for being on another origin +} + +// listCollections lists the VEVENT-capable calendar collections directly under a calendar home, +// in the order the server returns them. +// +// A collection whose URL is on a different origin (scheme, host, port) from the home URL that +// listed it is logged and skipped. Every URL returned here can be stored as a calendar id, and a +// stored id is then sent the account's Basic credentials on every free/busy read and booking +// write. The server already sees those credentials; an href must not be able to make them go to +// some other host for as long as the selection is saved. Skipped URLs are reported in foreign so +// connect can say why it found nothing. +func (c *Client) listCollections(ctx context.Context, home, username, password string) (listing, error) { + ms, homeReqURL, err := c.propfind(ctx, home, username, password, "1", propCalendarCollections) + if err != nil { + return listing{}, err + } + l := listing{homeURL: homeReqURL} + seen := map[string]bool{} for _, r := range ms.Responses { p := r.okProp() if p.ResourceType.Calendar == nil { continue // not a calendar collection (e.g. the home container itself) } if !supportsVEvent(p.SupportedComps) { - continue // e.g. a tasks/reminders or birthdays calendar + continue // e.g. a tasks/reminders list } u := resolveRef(homeReqURL, r.Href) - if firstCal == "" { - firstCal = u + if !sameOrigin(u, homeReqURL) { + c.logger.Warn("caldav: skipping a calendar listed on another origin", "home", homeReqURL, "calendar", u) + l.foreign = append(l.foreign, u) + continue } - // Prefer a calendar literally named/pathed "calendar" as the default when present. - if strings.EqualFold(p.DisplayName, "Calendar") || strings.Contains(strings.ToLower(u), "/calendar") { - return u, nil + if seen[u] { + continue // the picker keys rows by id, and the selection table is unique on it } + seen[u] = true + l.collections = append(l.collections, collection{url: u, displayName: p.DisplayName, writable: p.PrivilegeSet.canWrite()}) } - if firstCal == "" { - _ = base - return "", fmt.Errorf("caldav: no writable calendar found on the server") - } - return firstCal, nil + return l, nil } // findPrincipal resolves current-user-principal, trying the base URL first and then the diff --git a/internal/caldav/freebusy.go b/internal/caldav/freebusy.go index 186d1984..3677f6a4 100644 --- a/internal/caldav/freebusy.go +++ b/internal/caldav/freebusy.go @@ -30,10 +30,11 @@ const calendarQueryTmpl = ` ` -// FreeBusy returns the union of busy intervals across every CalDAV account the user has -// connected with check_conflicts = 1. Each account is queried with a CalDAV calendar-query -// REPORT; transparent and cancelled events don't block. Fail-open per account: a connection -// that errors is logged and skipped (a flaky calendar never blocks availability). +// FreeBusy returns the union of busy intervals across every calendar selected for conflicts in +// every CalDAV account the user has connected with check_conflicts = 1 (see conflictConns). Each +// calendar is queried with a CalDAV calendar-query REPORT; transparent and cancelled events +// don't block. Fail-open per calendar: one that errors is logged and skipped (a flaky calendar +// never blocks availability). func (c *Client) FreeBusy(ctx context.Context, userID string, from, to time.Time) ([]slots.Interval, error) { conns, err := c.conflictConns(ctx, userID) if err != nil { diff --git a/internal/caldav/webdav.go b/internal/caldav/webdav.go index de10cc9a..7c19c518 100644 --- a/internal/caldav/webdav.go +++ b/internal/caldav/webdav.go @@ -5,9 +5,13 @@ import ( "encoding/xml" "fmt" "io" + "net" "net/http" "net/url" + "path" "strings" + + "github.com/calnode/calnode/internal/calendar" ) // do issues one WebDAV request with HTTP Basic auth and returns the status, body, and any @@ -60,7 +64,7 @@ func (c *Client) propfind(ctx context.Context, rawURL, username, password, depth continue } if status == http.StatusUnauthorized || status == http.StatusForbidden { - return nil, cur, fmt.Errorf("caldav: authentication failed (status %d) — check the username and app password", status) + return nil, cur, &authError{status: status} } if status != http.StatusMultiStatus && status != http.StatusOK { return nil, cur, fmt.Errorf("caldav: PROPFIND %s returned status %d", cur, status) @@ -74,6 +78,18 @@ func (c *Client) propfind(ctx context.Context, rawURL, username, password, depth return nil, cur, fmt.Errorf("caldav: too many redirects resolving %s", rawURL) } +// authError is a 401/403 from the server. Its message is what the connect form shows. It also +// unwraps to calendar.ErrReauthRequired, because listing an existing account's calendars now +// talks to the server: an app password that has since been revoked must read as "reconnect +// this account", not as the server being unreachable. +type authError struct{ status int } + +func (e *authError) Error() string { + return fmt.Sprintf("caldav: authentication failed (status %d) — check the username and app password", e.status) +} + +func (e *authError) Unwrap() error { return calendar.ErrReauthRequired } + // resolveRef resolves a (possibly relative) href against a base request URL, returning an // absolute URL string. On parse failure it returns the ref unchanged. func resolveRef(base, ref string) string { @@ -88,6 +104,65 @@ func resolveRef(base, ref string) string { return b.ResolveReference(r).String() } +// sameOrigin reports whether two absolute URLs share a scheme, host and port. An omitted port +// equals the scheme's default, so https://h and https://h:443 are the same origin. A URL that +// does not parse, or has no scheme or host, matches nothing. +func sameOrigin(a, b string) bool { + oa, ob := origin(a), origin(b) + return oa != "" && oa == ob +} + +func origin(raw string) string { + u, err := url.Parse(raw) + if err != nil { + return "" + } + scheme, host := strings.ToLower(u.Scheme), strings.ToLower(u.Hostname()) + if scheme == "" || host == "" { + return "" + } + port := u.Port() + if port == "" { + switch scheme { + case "http": + port = "80" + case "https": + port = "443" + } + } + return scheme + "://" + net.JoinHostPort(host, port) +} + +// displayOrigin renders a URL's origin as an operator would type it (https://host, or +// http://host:5000), for error messages. +func displayOrigin(raw string) string { + u, err := url.Parse(raw) + if err != nil || u.Host == "" { + return raw + } + return u.Scheme + "://" + u.Host +} + +// lastSegment returns the final path segment of a collection URL ("work" for .../user/work/), +// used as a calendar's name when the server reports no display name. +func lastSegment(raw string) string { + u, err := url.Parse(raw) + if err != nil { + return raw + } + seg := path.Base(strings.TrimRight(u.Path, "/")) + if seg == "" || seg == "." || seg == "/" { + return raw + } + return seg +} + +// parentCollection returns the URL of the collection that contains raw (.../user/ for +// .../user/work/). +func parentCollection(raw string) string { + return resolveRef(strings.TrimRight(raw, "/"), "./") +} + // ----- WebDAV / CalDAV multistatus XML ----- type msMultistatus struct { @@ -112,6 +187,38 @@ type msProp struct { ResourceType resourceType `xml:"DAV: resourcetype"` SupportedComps supportedCompSet `xml:"urn:ietf:params:xml:ns:caldav supported-calendar-component-set"` CalendarData string `xml:"urn:ietf:params:xml:ns:caldav calendar-data"` + // nil when the server did not report the property (or reported it under a non-2xx status). + PrivilegeSet *privilegeSet `xml:"DAV: current-user-privilege-set"` +} + +// privilegeSet is DAV:current-user-privilege-set (RFC 3744 §5.4), the privileges the signed-in +// user holds on a resource. Only the ones that allow adding an event are decoded. +type privilegeSet struct { + Privileges []privilege `xml:"DAV: privilege"` +} + +type privilege struct { + All *struct{} `xml:"DAV: all"` + Write *struct{} `xml:"DAV: write"` + WriteContent *struct{} `xml:"DAV: write-content"` + Bind *struct{} `xml:"DAV: bind"` +} + +// canWrite reports whether events can be created in the collection. A set the server did not +// report counts as writable: that is what every CalDAV calendar was assumed to be before +// privileges were read, and a server that omits the property has given no reason to refuse. +// write-properties alone does not count, since servers grant it on read-only shares so the +// sharee can rename or recolour them. +func (s *privilegeSet) canWrite() bool { + if s == nil { + return true + } + for _, p := range s.Privileges { + if p.All != nil || p.Write != nil || p.WriteContent != nil || p.Bind != nil { + return true + } + } + return false } type hrefHolder struct { diff --git a/internal/calendar/autherr.go b/internal/calendar/autherr.go index 73b371f7..fa24e42f 100644 --- a/internal/calendar/autherr.go +++ b/internal/calendar/autherr.go @@ -16,11 +16,15 @@ var ErrReauthRequired = errors.New("calendar: reconnect required") // IsReauthErr reports whether err indicates the account's OAuth grant is no longer usable, as // opposed to a transient failure. It matches both a typed oauth2.RetrieveError and the string // form providers wrap it in (Google returns "invalid_grant"; Microsoft returns "invalid_grant" -// alongside an AADSTS code). +// alongside an AADSTS code), and anything wrapping ErrReauthRequired, which is how CalDAV +// reports a server that rejects the stored app password. func IsReauthErr(err error) bool { if err == nil { return false } + if errors.Is(err, ErrReauthRequired) { + return true + } var re *oauth2.RetrieveError if errors.As(err, &re) { switch re.ErrorCode { diff --git a/internal/calendar/autherr_test.go b/internal/calendar/autherr_test.go index 67ae7bf5..1195b27f 100644 --- a/internal/calendar/autherr_test.go +++ b/internal/calendar/autherr_test.go @@ -20,6 +20,7 @@ func TestIsReauthErr(t *testing.T) { errors.New(`oauth2: "invalid_grant" "AADSTS70000: Account security interrupt..."`)), true}, {"typed retrieve error", &oauth2.RetrieveError{ErrorCode: "invalid_grant"}, true}, {"typed interaction required", &oauth2.RetrieveError{ErrorCode: "interaction_required"}, true}, + {"wraps ErrReauthRequired", fmt.Errorf("caldav: list: %w", ErrReauthRequired), true}, {"transient network", errors.New("microsoft: list calendars call: dial tcp: i/o timeout"), false}, {"http 500", errors.New("gcal: list calendars status 500"), false}, } diff --git a/internal/calendar/calselect.go b/internal/calendar/calselect.go index d59cc330..96e2833d 100644 --- a/internal/calendar/calselect.go +++ b/internal/calendar/calselect.go @@ -3,6 +3,7 @@ package calendar import ( "context" "database/sql" + "errors" "fmt" "github.com/calnode/calnode/internal/uid" @@ -77,8 +78,37 @@ func (s *Service) AccountCalendars(ctx context.Context, userID, provider, accoun return out, nil } +// SelectionValidator is implemented by a provider whose calendar ids must be checked before a +// selection is saved, because the id itself says where the account's credentials are sent. +// CalDAV is one: its calendar id is a collection URL that free/busy and booking writes then +// authenticate to. A provider whose ids are opaque names inside an API it already authorises +// (Google, Microsoft) does not implement it, so saving there stays a local write. +// +// ValidateSelection returns *UnknownCalendarError for an id the provider does not offer for +// the account, and any other error when it could not check. +type SelectionValidator interface { + ValidateSelection(ctx context.Context, userID, accountEmail string, calendarIDs []string) error +} + +// ErrCalendarList wraps a provider's failure to check a selection against the account's +// calendars, so a caller can report "the provider could not be reached" rather than an +// internal error. +var ErrCalendarList = errors.New("calendar: could not list the account's calendars") + +// UnknownCalendarError is a saved selection naming a calendar the provider does not list for +// the account. +type UnknownCalendarError struct{ CalendarID string } + +func (e *UnknownCalendarError) Error() string { + return fmt.Sprintf("calendar %q is not one of this account's calendars", e.CalendarID) +} + // SetAccountCalendars replaces the saved selection for the connection's account. At most one // calendar across the whole user may be the destination (enforced by clearing others first). +// +// When the provider is a SelectionValidator, nothing is saved unless it accepts every calendar +// id: a refused id comes back as *UnknownCalendarError, and a failure to check is wrapped in +// ErrCalendarList. Other providers save the ids as given, as they always have. func (s *Service) SetAccountCalendars(ctx context.Context, userID, provider, accountEmail string, sels []CalendarSelection) error { ok, err := s.accountExists(ctx, userID, provider, accountEmail) if err != nil { @@ -88,6 +118,22 @@ func (s *Service) SetAccountCalendars(ctx context.Context, userID, provider, acc return sql.ErrNoRows } + // Checked before the transaction opens: the pool is a single connection, and a provider + // checking the ids reads the connection row itself. + if v, ok := s.providers[provider].(SelectionValidator); ok { + ids := make([]string, 0, len(sels)) + for _, c := range sels { + ids = append(ids, c.ID) + } + if err := v.ValidateSelection(ctx, userID, accountEmail, ids); err != nil { + var unknown *UnknownCalendarError + if errors.As(err, &unknown) { + return err + } + return fmt.Errorf("%w: %w", ErrCalendarList, err) + } + } + tx, err := s.db.BeginTx(ctx, nil) if err != nil { return err diff --git a/internal/calendar/calselect_test.go b/internal/calendar/calselect_test.go new file mode 100644 index 00000000..314278e4 --- /dev/null +++ b/internal/calendar/calselect_test.go @@ -0,0 +1,153 @@ +package calendar + +import ( + "context" + "database/sql" + "errors" + "fmt" + "slices" + "testing" +) + +// plainProvider is a provider that does not validate selections (Google and Microsoft today). +// Only Name is expected to be called; the embedded nil Provider panics loudly otherwise, and +// ListCalendars is counted so a test can prove saving never lists. +type plainProvider struct { + Provider + name string + listed int +} + +func (p *plainProvider) Name() string { return p.name } + +func (p *plainProvider) ListCalendars(context.Context, string, string) ([]CalendarInfo, error) { + p.listed++ + return nil, errors.New("plainProvider: ListCalendars should not be called when saving") +} + +// validatingProvider accepts only the ids it offers, or fails to check at all when err is set. +type validatingProvider struct { + plainProvider + offered []string + err error +} + +func (p *validatingProvider) ValidateSelection(_ context.Context, _, _ string, ids []string) error { + if p.err != nil { + return p.err + } + for _, id := range ids { + if !slices.Contains(p.offered, id) { + return &UnknownCalendarError{CalendarID: id} + } + } + return nil +} + +func selection(id string, check, dest bool) CalendarSelection { + return CalendarSelection{CalendarInfo: CalendarInfo{ID: id}, CheckConflicts: check, IsDestination: dest} +} + +func savedSelection(t *testing.T, db *sql.DB, account string) (checked []string, dest string) { + t.Helper() + rows, err := db.Query(`SELECT calendar_id, check_conflicts, is_destination FROM connection_calendars + WHERE user_id = 'u1' AND account_email = ? ORDER BY calendar_id`, account) + if err != nil { + t.Fatal(err) + } + defer rows.Close() + for rows.Next() { + var id string + var cc, d int + if err := rows.Scan(&id, &cc, &d); err != nil { + t.Fatal(err) + } + if cc != 0 { + checked = append(checked, id) + } + if d != 0 { + dest = id + } + } + return checked, dest +} + +// A provider that does not validate selections saves exactly what it is given, with no call +// to the provider: Google's "primary" alias, which its calendar list never returns, still saves. +func TestSetAccountCalendars_nonValidatingProviderSavesAsGiven(t *testing.T) { + db := newTestDB(t) + seedConn(t, db, "c1", "u1", "google", "work@x.test", 1, 1) + p := &plainProvider{name: "google"} + svc := NewService(db) + svc.Register(p) + + if err := svc.SetAccountCalendars(context.Background(), "u1", "google", "work@x.test", []CalendarSelection{ + selection("primary", true, true), + selection("not-in-any-list@x.test", true, false), + }); err != nil { + t.Fatalf("SetAccountCalendars: %v", err) + } + if p.listed != 0 { + t.Errorf("ListCalendars called %d time(s); saving must stay a local write for this provider", p.listed) + } + checked, dest := savedSelection(t, db, "work@x.test") + if want := []string{"not-in-any-list@x.test", "primary"}; !slices.Equal(checked, want) || dest != "primary" { + t.Errorf("saved checked=%v dest=%q, want checked=%v dest=primary", checked, dest, want) + } +} + +// A validating provider's refusal saves nothing, leaving the previous selection as it was. +func TestSetAccountCalendars_validatingProviderRefusalSavesNothing(t *testing.T) { + db := newTestDB(t) + seedConn(t, db, "c1", "u1", "caldav", "me@x.test", 1, 1) + p := &validatingProvider{plainProvider: plainProvider{name: "caldav"}, + offered: []string{"https://dav.x.test/cal/home/", "https://dav.x.test/cal/work/"}} + svc := NewService(db) + svc.Register(p) + ctx := context.Background() + + if err := svc.SetAccountCalendars(ctx, "u1", "caldav", "me@x.test", []CalendarSelection{ + selection("https://dav.x.test/cal/home/", true, false), + selection("https://dav.x.test/cal/work/", true, true), + }); err != nil { + t.Fatalf("offered selection: %v", err) + } + + err := svc.SetAccountCalendars(ctx, "u1", "caldav", "me@x.test", []CalendarSelection{ + selection("https://dav.x.test/cal/home/", false, true), + selection("https://elsewhere.test/steal/", true, false), + }) + var unknown *UnknownCalendarError + if !errors.As(err, &unknown) || unknown.CalendarID != "https://elsewhere.test/steal/" { + t.Fatalf("err = %v, want *UnknownCalendarError for https://elsewhere.test/steal/", err) + } + if errors.Is(err, ErrCalendarList) { + t.Errorf("err = %v; a refused id is not a listing failure", err) + } + checked, dest := savedSelection(t, db, "me@x.test") + if want := []string{"https://dav.x.test/cal/home/", "https://dav.x.test/cal/work/"}; !slices.Equal(checked, want) || dest != "https://dav.x.test/cal/work/" { + t.Errorf("after a refused save checked=%v dest=%q, want the earlier selection %v with dest work", checked, dest, want) + } +} + +// When a validating provider cannot check at all, nothing is saved and the error says so: a +// caller maps ErrCalendarList to "provider unreachable" and a rejected login to "reconnect". +func TestSetAccountCalendars_validationFailureIsErrCalendarList(t *testing.T) { + db := newTestDB(t) + seedConn(t, db, "c1", "u1", "caldav", "me@x.test", 1, 1) + svc := NewService(db) + svc.Register(&validatingProvider{plainProvider: plainProvider{name: "caldav"}, + err: fmt.Errorf("caldav: authentication failed: %w", ErrReauthRequired)}) + + err := svc.SetAccountCalendars(context.Background(), "u1", "caldav", "me@x.test", + []CalendarSelection{selection("https://dav.x.test/cal/home/", true, true)}) + if !errors.Is(err, ErrCalendarList) { + t.Errorf("err = %v, want it to wrap ErrCalendarList", err) + } + if !IsReauthErr(err) { + t.Errorf("err = %v, want the provider's reauth error still recognisable", err) + } + if checked, dest := savedSelection(t, db, "me@x.test"); len(checked) != 0 || dest != "" { + t.Errorf("saved checked=%v dest=%q although the selection could not be checked", checked, dest) + } +} diff --git a/internal/handler/calendar.go b/internal/handler/calendar.go index 2514781e..df5ac6e6 100644 --- a/internal/handler/calendar.go +++ b/internal/handler/calendar.go @@ -367,6 +367,23 @@ func (h *Handler) PutConnectionCalendars(w http.ResponseWriter, r *http.Request) h.writeError(w, http.StatusNotFound, "calendar connection not found") return } + // A provider that validates selections (CalDAV) checks every id against the account's + // current calendars (see SetAccountCalendars), so its listing failures reach this + // handler the same way they reach the GET. + var unknown *calendar.UnknownCalendarError + if errors.As(err, &unknown) { + h.writeError(w, http.StatusBadRequest, unknown.Error()+". Reload the list of calendars and save again.") + return + } + if calendar.IsReauthErr(err) { + h.writeError(w, http.StatusConflict, "This calendar needs reconnecting — disconnect it and connect again.") + return + } + if errors.Is(err, calendar.ErrCalendarList) { + h.logger.ErrorContext(r.Context(), "set connection calendars: list", "error", err) + h.writeError(w, http.StatusBadGateway, "could not reach the calendar provider") + return + } h.logger.ErrorContext(r.Context(), "set connection calendars", "error", err) h.writeError(w, http.StatusInternalServerError, "internal error") return diff --git a/internal/handler/calendar_caldav_test.go b/internal/handler/calendar_caldav_test.go new file mode 100644 index 00000000..4f2ada47 --- /dev/null +++ b/internal/handler/calendar_caldav_test.go @@ -0,0 +1,116 @@ +package handler_test + +import ( + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/calnode/calnode/internal/caldav" + "github.com/calnode/calnode/internal/calendar" + "github.com/calnode/calnode/internal/db" + "github.com/calnode/calnode/internal/handler" +) + +// twoCalendarDAV is a CalDAV server whose account holds two event calendars. +func twoCalendarDAV(t *testing.T) *httptest.Server { + t.Helper() + const ns = `` + cal := func(href, name string) string { + return `` + href + `HTTP/1.1 200 OK` + + `` + name + `` + + `` + + `` + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + w.WriteHeader(http.StatusMultiStatus) + switch { + case strings.Contains(string(body), "current-user-principal"): + io.WriteString(w, ns+``+r.URL.Path+`HTTP/1.1 200 OK/p/`) + case strings.Contains(string(body), "calendar-home-set"): + io.WriteString(w, ns+`/p/HTTP/1.1 200 OK/cal/`) + default: + io.WriteString(w, ns+cal("/cal/home/", "Home")+cal("/cal/work/", "Work")+``) + } + })) + t.Cleanup(srv.Close) + return srv +} + +// PUT /v1/calendar/connections/{id}/calendars must refuse a calendar the provider did not list, +// with a 400 that says what to do, and still accept one it did. +func TestPutConnectionCalendars_caldavRefusesAnUnlistedCalendar(t *testing.T) { + database, err := db.Open("sqlite://:memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + t.Cleanup(func() { database.Close() }) + if err := db.Migrate(database); err != nil { + t.Fatalf("db.Migrate: %v", err) + } + h := handler.New(database, slog.Default()) + cc, err := caldav.New(database, testGCalKeyHex) + if err != nil { + t.Fatalf("caldav.New: %v", err) + } + svc := calendar.NewService(database) + svc.Register(cc) + h.SetCalendar(svc) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/v1/setup", strings.NewReader(`{"name":"Cal User","email":"cal@example.com","timezone":"UTC"}`)) + req.Header.Set("Content-Type", "application/json") + h.Setup(rec, req) + var setup struct { + APIKey string `json:"api_key"` + } + if rec.Code != http.StatusCreated || json.Unmarshal(rec.Body.Bytes(), &setup) != nil { + t.Fatalf("setup: %d %s", rec.Code, rec.Body.String()) + } + + // Connect through the real handler and client: the loopback test server passes the + // metadata-only guard, which is the guard every listing and read goes through. + dav := twoCalendarDAV(t) + rec = httptest.NewRecorder() + h.RequireAuth(h.ConnectCalDAV)(rec, authReq(http.MethodPost, "/v1/calendar/caldav/connect", + `{"server_url":"`+dav.URL+`","username":"me@example.com","app_password":"pw"}`, setup.APIKey)) + if rec.Code != http.StatusOK { + t.Fatalf("connect: %d %s", rec.Code, rec.Body.String()) + } + + rec = httptest.NewRecorder() + h.RequireAuth(h.GetConnectionCalendars)(rec, authReq(http.MethodGet, + "/v1/calendar/connections/x/calendars?provider=caldav&account=me%40example.com", "", setup.APIKey)) + var listed struct { + Calendars []calendar.CalendarSelection `json:"calendars"` + } + if rec.Code != http.StatusOK || json.Unmarshal(rec.Body.Bytes(), &listed) != nil || len(listed.Calendars) != 2 { + t.Fatalf("list: %d %s, want both calendars", rec.Code, rec.Body.String()) + } + + put := func(calendarID string) *httptest.ResponseRecorder { + body, _ := json.Marshal(map[string]any{ + "provider": "caldav", "account_email": "me@example.com", + "calendars": []map[string]any{{"id": calendarID, "check_conflicts": true, "is_destination": true}}, + }) + rec := httptest.NewRecorder() + h.RequireAuth(h.PutConnectionCalendars)(rec, authReq(http.MethodPut, "/v1/calendar/connections/x/calendars", string(body), setup.APIKey)) + return rec + } + + rec = put("http://169.254.169.254/latest/meta-data/") + if rec.Code != http.StatusBadRequest { + t.Fatalf("unlisted calendar: status %d %s, want 400", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "is not one of this account's calendars") { + t.Errorf("unlisted calendar: body %s, want a message naming the problem", rec.Body.String()) + } + + if rec = put(listed.Calendars[1].ID); rec.Code != http.StatusNoContent { + t.Errorf("listed calendar: status %d %s, want 204", rec.Code, rec.Body.String()) + } +} From 294f99d86cdd1976f6e790d1ad5b0291fd4f5acd Mon Sep 17 00:00:00 2001 From: Distronode <254259913+distronode-com@users.noreply.github.com> Date: Wed, 16 Sep 2026 21:34:35 -0400 Subject: [PATCH 2/2] caldav: pin calendar listing to the connected calendar's origin The same-origin rule added for #42 compared each listed collection with the calendar home that listed it, but nothing held the home itself, or the principal before it, to the account. Listing starts from the stored collection URL and followed whatever current-user-principal and calendar-home-set hrefs the server returned, and propfind followed any redirect, sending the account's Basic credentials each time. A principal, home or redirect on another origin (a reverse proxy's absolute internal href, or a hostile server) was therefore sent those credentials on every picker load, and a home there made that origin's collections look local, so ValidateSelection accepted them as persistent free/busy and write targets. Raised by pullfrog on #54, and reproduced: against the previous commit the new test sees 3 to 6 credentialed requests reach a second server in each case, lists its calendar, and saves it. Listing is now pinned to the bound collection's origin: - homeForCalendar follows a principal or calendar home only when it is on that origin; otherwise it logs and lists the bound collection's parent, which is on that origin by construction and is the calendar home on the usual layout. - propfind takes an origin to pin to and refuses a redirect off it before sending anything, so a redirect cannot carry the credentials elsewhere. A refused redirect on the principal or home lookup falls back to the parent; one on the home listing fails the listing. - ListCalendars drops any collection not on that origin. With the two guards above this cannot trigger today; it keeps the rule true if a later change widens what the listing can reach. Connect-time discovery is deliberately unchanged and still follows hrefs and redirects anywhere. It starts from a URL the user typed, can legitimately cross hosts (iCloud's calendar home is on a per-account partition host), and its own collection origin check is unchanged. Tests: a principal href on a second origin, a calendar-home-set href on a second origin, and a home listing that 301s to a second origin. The second server is a complete CalDAV account with a writable calendar; each case asserts it receives zero requests, that nothing of it is listed, and that ValidateSelection and SetAccountCalendars refuse it. Disabling the principal pin, the home pin or the redirect refusal each fails exactly its own case. The ListCalendars filter is defence in depth: removing it alone fails nothing, because it can only act once one of the other guards has already been bypassed. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 +- docs/ARCHITECTURE.md | 7 ++ internal/caldav/calendars.go | 48 +++++++++--- internal/caldav/calendars_test.go | 119 +++++++++++++++++++++++++++--- internal/caldav/discovery.go | 18 ++--- internal/caldav/webdav.go | 15 +++- 6 files changed, 179 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8159800..cc1950eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,9 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad Two refusals come with it, both CalDAV-only. A calendar listed on a different origin from its calendar home is skipped, because a CalDAV calendar id is the URL that later receives the - account's credentials. This changes connecting for one setup: a server behind a reverse proxy + account's credentials; listing an existing account's calendars goes further and never sends a + request, or follows a principal, calendar home or redirect, off the origin of the calendar it + connected with. This changes connecting for one setup: a server behind a reverse proxy that reports its internal scheme or host in every href used to connect and now does not, with an error naming both addresses and pointing at the proxy or base URL settings. And saving a selection that names a CalDAV calendar the server did not list is refused with a 400, where diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index f392d7bd..3592bfae 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -495,6 +495,13 @@ Calnode talks to calendars through a **provider abstraction**, not a single vend (`ConflictCalendarIDs`) and makes no discovery requests; an account that never saved a selection reads only its bound collection. A 401/403 wraps `calendar.ErrReauthRequired`, so a revoked app password shows as "needs reconnecting" in the picker. + **Listing never leaves the bound collection's origin.** It runs on every picker load and + every saved selection, with the account's credentials, against hrefs the server chooses, so + `homeForCalendar` does not follow a principal or calendar home on another origin (it lists the + bound collection's parent instead), `propfind` is pinned to that origin and refuses a redirect + off it, and `ListCalendars` drops any collection off it. Connect-time discovery is not pinned: + it starts from a URL the user typed, and can legitimately cross hosts (iCloud's calendar home + is on a per-account partition host). **Online-meeting links are provider-matched (`booking_handler.go`).** A `google_meet`/`teams` event type auto-mints a link **only when the primary host's diff --git a/internal/caldav/calendars.go b/internal/caldav/calendars.go index 1a6c011f..1973e748 100644 --- a/internal/caldav/calendars.go +++ b/internal/caldav/calendars.go @@ -17,6 +17,12 @@ import ( // The server URL typed at connect time is not stored, so discovery restarts from the bound // collection (see homeForCalendar) using the stored credentials. Nothing new is persisted, which // is why this needs no migration and works for connections made before calendars were listed. +// +// Every request made here stays on the bound collection's origin, and so does every calendar +// offered: this runs on each picker load and each saved selection, with the account's +// credentials, against URLs the server chose. homeForCalendar will not follow a principal or +// home on another origin, propfind refuses a redirect off it, and a collection off it is dropped +// below even if a listing somehow reached one. func (c *Client) ListCalendars(ctx context.Context, userID, accountEmail string) ([]calendar.CalendarInfo, error) { cn, ok, err := c.accountConn(ctx, userID, accountEmail) if err != nil || !ok { @@ -26,7 +32,7 @@ func (c *Client) ListCalendars(ctx context.Context, userID, accountEmail string) if err != nil { return nil, err } - l, err := c.listCollections(ctx, home, cn.username, cn.password) + l, err := c.listCollections(ctx, cn.calURL, home, cn.username, cn.password) if err != nil { return nil, err } @@ -34,6 +40,10 @@ func (c *Client) ListCalendars(ctx context.Context, userID, accountEmail string) out := make([]calendar.CalendarInfo, 0, len(l.collections)+1) boundListed := false for _, col := range l.collections { + if !sameOrigin(col.url, cn.calURL) { + c.logger.Warn("caldav: skipping a calendar on another origin from the connected calendar", "connected", cn.calURL, "calendar", col.url) + continue + } primary := col.url == cn.calURL boundListed = boundListed || primary out = append(out, calendar.CalendarInfo{ID: col.url, Name: col.name(), Primary: primary, Writable: col.writable}) @@ -82,11 +92,18 @@ func (c *Client) ValidateSelection(ctx context.Context, userID, accountEmail str // A failed principal or home lookup falls through to the parent rather than failing, so a bound // collection that has since been deleted does not stop the user choosing another one. An // authentication failure still surfaces: the parent listing is made with the same credentials. +// +// A principal or home href on another origin from the bound collection is not followed, and +// neither is a redirect off it (propfind is pinned to the bound collection). Following either +// would send the account's credentials there on every picker load, and a home there would make +// that origin's collections look local to it. Both fall through to the parent, which is on the +// bound collection's origin by construction. func (c *Client) homeForCalendar(ctx context.Context, cn conn) (string, error) { - ms, reqURL, err := c.propfind(ctx, cn.calURL, cn.username, cn.password, "0", propCurrentUserPrincipal) + parent := parentCollection(cn.calURL) + ms, reqURL, err := c.propfind(ctx, cn.calURL, cn.calURL, cn.username, cn.password, "0", propCurrentUserPrincipal) if err != nil { c.logger.Warn("caldav: principal lookup failed, listing the bound calendar's parent", "calendar", cn.calURL, "error", err) - return parentCollection(cn.calURL), nil + return parent, nil } var principal string for _, r := range ms.Responses { @@ -95,15 +112,24 @@ func (c *Client) homeForCalendar(ctx context.Context, cn conn) (string, error) { break } } - if principal != "" { - home, err := c.calendarHome(ctx, principal, cn.username, cn.password) - if err != nil { - c.logger.Warn("caldav: calendar home lookup failed, listing the bound calendar's parent", "principal", principal, "error", err) - } else if home != "" { - return home, nil - } + if principal == "" { + return parent, nil + } + if !sameOrigin(principal, cn.calURL) { + c.logger.Warn("caldav: not following a principal on another origin, listing the bound calendar's parent", "calendar", cn.calURL, "principal", principal) + return parent, nil + } + home, err := c.calendarHome(ctx, cn.calURL, principal, cn.username, cn.password) + switch { + case err != nil: + c.logger.Warn("caldav: calendar home lookup failed, listing the bound calendar's parent", "principal", principal, "error", err) + case home == "": + case !sameOrigin(home, cn.calURL): + c.logger.Warn("caldav: not following a calendar home on another origin, listing the bound calendar's parent", "calendar", cn.calURL, "home", home) + default: + return home, nil } - return parentCollection(cn.calURL), nil + return parent, nil } // accountConn loads one account's connection (by account email) regardless of its diff --git a/internal/caldav/calendars_test.go b/internal/caldav/calendars_test.go index 8931ecb2..338e6566 100644 --- a/internal/caldav/calendars_test.go +++ b/internal/caldav/calendars_test.go @@ -27,16 +27,25 @@ import ( // /calendars/user/tasks/ "Tasks", VTODO only // /calendars/user/holidays/ "Holidays", VEVENT, read + write-properties only // -// Every request is recorded, and any request reaching the other origin is counted. +// Every request is recorded, and any request reaching the other origin is counted. The other +// origin is a complete CalDAV account of its own (principal, home, and one writable "Stolen" +// calendar), so a listing that wandered there would find something to offer. type multiCalServer struct { srv *httptest.Server other *httptest.Server - // noPrincipalOnCollections makes a PROPFIND for the principal on anything but "/" fail, - // as on a server that does not implement RFC 5397 on calendar collections. + mu sync.Mutex // guards everything below, including the switches set by tests + + // Switches for requests made after connect (a request to "/" is connect's own discovery). + // noPrincipalOnCollections makes a PROPFIND for the principal fail, as on a server that + // does not implement RFC 5397 on calendar collections. principalElsewhere and homeElsewhere + // answer with an href on the other origin, and homeRedirectsElsewhere makes the Depth 1 + // listing of the home a 301 to the other origin. noPrincipalOnCollections bool + principalElsewhere bool + homeElsewhere bool + homeRedirectsElsewhere bool - mu sync.Mutex reqs []string // "METHOD /path" events map[string]string // path -> stored iCalendar body otherHits int @@ -46,11 +55,19 @@ func newMultiCalServer(t *testing.T) *multiCalServer { t.Helper() fs := &multiCalServer{events: map[string]string{}} fs.other = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) fs.mu.Lock() fs.otherHits++ fs.mu.Unlock() w.WriteHeader(http.StatusMultiStatus) - io.WriteString(w, ``) + switch { + case strings.Contains(string(body), "current-user-principal"): + io.WriteString(w, msOpen+okResponse(r.URL.Path, `/principals/user/`)+``) + case strings.Contains(string(body), "calendar-home-set"): + io.WriteString(w, msOpen+okResponse(r.URL.Path, `/calendars/user/`)+``) + default: + io.WriteString(w, msOpen+okResponse("/calendars/user/stolen/", calendarProps("Stolen", vevent, fmt.Sprintf(priv, "all")))+``) + } })) t.Cleanup(fs.other.Close) fs.srv = httptest.NewServer(http.HandlerFunc(fs.serve)) @@ -60,6 +77,13 @@ func newMultiCalServer(t *testing.T) *multiCalServer { func (fs *multiCalServer) url(p string) string { return fs.srv.URL + p } +// set flips switches under the lock the handler reads them with. +func (fs *multiCalServer) set(f func(fs *multiCalServer)) { + fs.mu.Lock() + defer fs.mu.Unlock() + f(fs) +} + func (fs *multiCalServer) reset() { fs.mu.Lock() defer fs.mu.Unlock() @@ -123,7 +147,10 @@ func (fs *multiCalServer) serve(w http.ResponseWriter, r *http.Request) { body, _ := io.ReadAll(r.Body) fs.mu.Lock() fs.reqs = append(fs.reqs, r.Method+" "+r.URL.Path) + noPrincipal, principalElsewhere := fs.noPrincipalOnCollections, fs.principalElsewhere + homeElsewhere, homeRedirects := fs.homeElsewhere, fs.homeRedirectsElsewhere fs.mu.Unlock() + afterConnect := r.URL.Path != "/" multistatus := func(inner string) { w.WriteHeader(http.StatusMultiStatus) @@ -131,13 +158,24 @@ func (fs *multiCalServer) serve(w http.ResponseWriter, r *http.Request) { } switch { case r.Method == "PROPFIND" && strings.Contains(string(body), "current-user-principal"): - if fs.noPrincipalOnCollections && r.URL.Path != "/" { + if noPrincipal && afterConnect { w.WriteHeader(http.StatusNotFound) return } - multistatus(okResponse(r.URL.Path, `/principals/user/`)) + principal := "/principals/user/" + if principalElsewhere && afterConnect { + principal = fs.other.URL + principal + } + multistatus(okResponse(r.URL.Path, ``+principal+``)) case r.Method == "PROPFIND" && strings.Contains(string(body), "calendar-home-set") && r.URL.Path == "/principals/user/": - multistatus(okResponse(r.URL.Path, `/calendars/user/`)) + home := "/calendars/user/" + if homeElsewhere { + home = fs.other.URL + home + } + multistatus(okResponse(r.URL.Path, ``+home+``)) + case r.Method == "PROPFIND" && r.URL.Path == "/calendars/user/" && r.Header.Get("Depth") == "1" && homeRedirects: + w.Header().Set("Location", fs.other.URL+"/calendars/user/") + w.WriteHeader(http.StatusMovedPermanently) case r.Method == "PROPFIND" && r.URL.Path == "/calendars/user/" && r.Header.Get("Depth") == "1": multistatus( okResponse("/calendars/user/", ``) + @@ -268,7 +306,7 @@ func TestListCalendars_listsEveryEventCalendarOnTheAccount(t *testing.T) { // calendars listed, from the bound collection's parent. func TestListCalendars_fallsBackToTheBoundCalendarsParent(t *testing.T) { c, fs := connectMultiCal(t) - fs.noPrincipalOnCollections = true + fs.set(func(fs *multiCalServer) { fs.noPrincipalOnCollections = true }) got, err := c.ListCalendars(context.Background(), "u1", multiCalAccount) if err != nil { @@ -435,6 +473,69 @@ func TestSetAccountCalendars_refusesACalendarTheServerDidNotList(t *testing.T) { } } +// Listing runs on every picker load and every saved CalDAV selection, with the account's +// credentials, against hrefs and redirects the server chooses. A principal, a calendar home or a +// redirect on another origin must not receive a single request, and nothing there may be offered +// by ListCalendars or accepted by ValidateSelection. +func TestListCalendars_staysOnTheConnectedCalendarsOrigin(t *testing.T) { + for _, tc := range []struct { + name string + set func(fs *multiCalServer) + wantErr string // "" when the listing should still succeed from the bound calendar's parent + }{ + {"principal on another origin", func(fs *multiCalServer) { fs.principalElsewhere = true }, ""}, + {"calendar home on another origin", func(fs *multiCalServer) { fs.homeElsewhere = true }, ""}, + {"home listing redirects to another origin", func(fs *multiCalServer) { fs.homeRedirectsElsewhere = true }, + "refusing to follow a redirect"}, + } { + t.Run(tc.name, func(t *testing.T) { + c, fs := connectMultiCal(t) + fs.set(tc.set) + ctx := context.Background() + stolen := fs.other.URL + "/calendars/user/stolen/" + + cals, err := c.ListCalendars(ctx, "u1", multiCalAccount) + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("ListCalendars err = %v, want one containing %q", err, tc.wantErr) + } + } else { + if err != nil { + t.Fatalf("ListCalendars: %v", err) + } + var ids []string + for _, cal := range cals { + ids = append(ids, cal.ID) + } + want := []string{fs.url("/calendars/user/personal/"), fs.url("/calendars/user/team/"), fs.url("/calendars/user/holidays/")} + if !slices.Equal(ids, want) { + t.Errorf("ListCalendars ids = %v, want the account's own calendars %v", ids, want) + } + } + + if err := c.ValidateSelection(ctx, "u1", multiCalAccount, []string{stolen}); err == nil { + t.Errorf("ValidateSelection accepted %s", stolen) + } + svc := calendar.NewService(c.db) + svc.Register(c) + if err := svc.SetAccountCalendars(ctx, "u1", "caldav", multiCalAccount, + []calendar.CalendarSelection{sel(stolen, true, true)}); err == nil { + t.Errorf("SetAccountCalendars saved %s", stolen) + } + var n int + if err := c.db.QueryRow(`SELECT COUNT(*) FROM connection_calendars WHERE user_id = 'u1'`).Scan(&n); err != nil { + t.Fatal(err) + } + if n != 0 { + t.Errorf("%d selection row(s) saved", n) + } + if n := fs.hitsElsewhere(); n != 0 { + t.Errorf("%d request(s) reached the other origin with the account's credentials", n) + } + }) + } +} + // A self-hosted server behind a TLS-terminating reverse proxy can report its internal scheme or // host in every href. Those calendars are skipped for origin, and connect must say that is why it // found nothing, rather than "no calendar found", which points the operator at the wrong thing. diff --git a/internal/caldav/discovery.go b/internal/caldav/discovery.go index a1c38840..a65385d9 100644 --- a/internal/caldav/discovery.go +++ b/internal/caldav/discovery.go @@ -59,7 +59,7 @@ func (c *Client) discoverCalendar(ctx context.Context, serverURL, username, pass } // 2. calendar-home-set on the principal. - home, err := c.calendarHome(ctx, principal, username, password) + home, err := c.calendarHome(ctx, "", principal, username, password) if err != nil { return "", err } @@ -69,7 +69,7 @@ func (c *Client) discoverCalendar(ctx context.Context, serverURL, username, pass } // 3. list calendar collections (Depth 1) and pick the default among them. - l, err := c.listCollections(ctx, home, username, password) + l, err := c.listCollections(ctx, "", home, username, password) if err != nil { return "", err } @@ -94,9 +94,9 @@ func (c *Client) discoverCalendar(ctx context.Context, serverURL, username, pass } // calendarHome returns the calendar-home-set a principal reports (RFC 4791 §6.2.1), or "" when -// it reports none. -func (c *Client) calendarHome(ctx context.Context, principal, username, password string) (string, error) { - ms, reqURL, err := c.propfind(ctx, principal, username, password, "0", propCalendarHomeSet) +// it reports none. pinOrigin is passed to propfind. +func (c *Client) calendarHome(ctx context.Context, pinOrigin, principal, username, password string) (string, error) { + ms, reqURL, err := c.propfind(ctx, pinOrigin, principal, username, password, "0", propCalendarHomeSet) if err != nil { return "", err } @@ -138,9 +138,9 @@ type listing struct { // stored id is then sent the account's Basic credentials on every free/busy read and booking // write. The server already sees those credentials; an href must not be able to make them go to // some other host for as long as the selection is saved. Skipped URLs are reported in foreign so -// connect can say why it found nothing. -func (c *Client) listCollections(ctx context.Context, home, username, password string) (listing, error) { - ms, homeReqURL, err := c.propfind(ctx, home, username, password, "1", propCalendarCollections) +// connect can say why it found nothing. pinOrigin is passed to propfind. +func (c *Client) listCollections(ctx context.Context, pinOrigin, home, username, password string) (listing, error) { + ms, homeReqURL, err := c.propfind(ctx, pinOrigin, home, username, password, "1", propCalendarCollections) if err != nil { return listing{}, err } @@ -178,7 +178,7 @@ func (c *Client) findPrincipal(ctx context.Context, serverURL, username, passwor } var lastErr error for _, cand := range candidates { - ms, reqURL, err := c.propfind(ctx, cand, username, password, "0", propCurrentUserPrincipal) + ms, reqURL, err := c.propfind(ctx, "", cand, username, password, "0", propCurrentUserPrincipal) if err != nil { lastErr = err continue diff --git a/internal/caldav/webdav.go b/internal/caldav/webdav.go index 7c19c518..02447a9f 100644 --- a/internal/caldav/webdav.go +++ b/internal/caldav/webdav.go @@ -48,7 +48,14 @@ func (c *Client) do(ctx context.Context, method, rawURL, username, password, dep // propfind issues a PROPFIND, following up to 5 redirects with the method preserved, and // returns the parsed multistatus. A non-2xx terminal status is an error. -func (c *Client) propfind(ctx context.Context, rawURL, username, password, depth, body string) (*msMultistatus, string, error) { +// +// pinOrigin, when set, is a URL whose origin every redirect must stay on: a redirect elsewhere +// is refused before anything is sent to it, so the account's credentials cannot follow it. +// Listing an existing account's calendars pins to the connected calendar. Connect-time +// discovery passes "" and is unchanged: it starts from the URL the user typed, and discovery +// from there can legitimately cross hosts (iCloud's calendar home is on a per-account partition +// host, and a /.well-known/caldav redirect may point anywhere). +func (c *Client) propfind(ctx context.Context, pinOrigin, rawURL, username, password, depth, body string) (*msMultistatus, string, error) { cur := rawURL for hop := 0; hop < 6; hop++ { status, b, loc, err := c.do(ctx, "PROPFIND", cur, username, password, depth, body) @@ -60,7 +67,11 @@ func (c *Client) propfind(ctx context.Context, rawURL, username, password, depth if loc == "" { return nil, cur, fmt.Errorf("caldav: redirect without Location from %s", cur) } - cur = resolveRef(cur, loc) + next := resolveRef(cur, loc) + if pinOrigin != "" && !sameOrigin(next, pinOrigin) { + return nil, cur, fmt.Errorf("caldav: refusing to follow a redirect from %s to another origin (%s)", cur, displayOrigin(next)) + } + cur = next continue } if status == http.StatusUnauthorized || status == http.StatusForbidden {