Repository navigation
caldav: list every calendar on the account, and read each one ticked - #54
distronode-com wants to merge 3 commits into
Conversation
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) <noreply@anthropic.com>
shockalotti
left a comment
There was a problem hiding this comment.
Strong PR — this is the correct fix for #42 and the design is careful in the right places: provider-generic SelectionValidator with a stated reason Google/Microsoft are exempt (opaque ids inside an already-authorized API), fail-closed on made-up ids (a CalDAV id is a URL credentials get sent to, so validation is SSRF-adjacent and treated as such), same-origin listing, and a 400/502/409 error taxonomy that tells the user which of 'you picked wrong / provider down / reconnect' applies. Two non-blocking notes pending green CI:
- Confirm the listing and validation dials go through the same guarded transport (narrow tier) as the rest of the CalDAV client — i.e. no new unguarded dial path was introduced for discovery-from-collection. My read is everything funnels through c.propfind → c.hc.Do, but an explicit yes would close it.
Writablecounts as true when the server reports no privilege-set (fail-open). Reasonable for servers like Synology that may not report it — failure then surfaces at write time — just noting the direction is deliberate.
There was a problem hiding this comment.
Important
Listing follows current-user-principal / calendar-home-set from the bound collection without the origin pin used for collection hrefs, so opening the picker can send the app password to another host and then offer that host's calendars to save.
Reviewed changes
CalDAV accounts can list every same-origin VEVENT collection, tick several for free/busy, and refuse saved ids the server did not list.
- Multi-calendar listing —
ListCalendarsrediscovers the home from the stored collection URL and lists VEVENT collections, withWritablefromDAV:current-user-privilege-set. - Free/busy per tick —
conflictConnsissues one REPORT per selected calendar; accounts with no saved selection still read only the bound collection. - SelectionValidator — CalDAV rejects unlisted ids before
SetAccountCalendarswrites; Google/Microsoft saves stay local. Handler maps unknown / reauth / list-failure to 400 / 409 / 502. - Connect origin skip — collection hrefs on another origin from the home are skipped; all-foreign event calendars fail connect with an address-mismatch error. PROPFIND 401/403 unwraps to
ErrReauthRequired. - Picker copy — Check vs Book (and read-only) is spelled out on the calendar settings page.
Grok | 𝕏
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 Calnode#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) <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the last review (d4a055b): listing an existing CalDAV account now stays on the bound collection's origin, which was the remaining credential-leak.
- Principal and home stay put —
homeForCalendarno longer follows acurrent-user-principalorcalendar-home-seton another origin fromcn.calURL; it lists the bound collection's parent instead. - Redirects are refused before the hop — listing
propfinds takepinOriginand reject an off-originLocationbefore credentials go there. Connect still passes"", which is the iCloud / well-known case. - Listed ids match the bound origin —
ListCalendarsdrops any collection that is not same-origin with the connected calendar, soValidateSelectioncannot accept it. - The second origin is a real CalDAV account in tests — principal elsewhere, home elsewhere, and a home-listing 301 each assert zero requests to it and that the stolen calendar is not saved.
Grok | 𝕏
CHANGELOG conflict only: kept both [Unreleased] entries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both confirmed.
|
|
Closing this now that c22fcc2 is merged in #57. Reading it against this PR, four things it does not cover, in case they are useful:
Happy to send any of these as a small PR against |

What was broken
Connecting a CalDAV account binds one collection, and
caldav.ListCalendarsreported only that one. The calendar picker is already provider-generic (connection_calendars, Check and Book), but for CalDAV it had exactly one row to show, so an account with several calendars (Synology Calendar in #42) could only be checked and booked through its default calendar.What it does now
ListCalendarsrepeats discovery from the stored collection URL with the stored credentials:current-user-principal(RFC 5397 defines it on every resource), thencalendar-home-set, falling back to the collection's parent when a server answers neither. Every VEVENT-capable collection under the home is listed. A collection with no display name is named by its last path segment, and the bound one isPrimary.Writablecomes fromDAV:current-user-privilege-set(all,write,write-contentorbind), and a server that does not report it is treated as writable, as before. The server URL typed at connect is not stored, so starting from the collection avoids a migration. Existing connections work without reconnecting.DestinationCalendarID). This PR adds tests pinning that a booking lands there and that update and cancel use the stored event URL.calendar.ErrReauthRequired, andIsReauthErrrecognises the sentinel. Listing is now a network call, so a revoked app password shows as "needs reconnecting" rather than "could not reach the calendar provider". The connect form's message is unchanged.Two refusals, and why
Both apply to CalDAV only, because a CalDAV calendar id is the URL that free/busy and bookings send the account's Basic credentials to for as long as it stays selected.
A collection listed on a different origin (scheme, host, port) from its calendar home is skipped and logged, both at connect and when listing, so an href in a listing cannot point those credentials at another host.
This changes connecting for one setup. A self-hosted server behind a reverse proxy that reports its internal scheme or host in every href (for example
http://behind a TLS-terminating proxy) used to connect and now does not. For that case connect fails with an error naming the cause, not "no writable calendar found". For example: "the server lists its calendars at a different address (http://nas.lan:5000) than the one Calnode reached it at (https://cal.example.com); check the server's reverse proxy or base URL settings". A server with some calendars on its own origin still connects to those.Saving a selection that names a calendar the server did not list is refused.
PUT .../calendarsanswers 400 with "calendar "…" is not one of this account's calendars. Reload the list of calendars and save again.", and nothing is saved. Before this, the endpoint stored whatever ids the client sent, which for CalDAV meant any URL, skipping both the connect-time URL check and discovery.This goes through a new optional interface,
calendar.SelectionValidator, whichSetAccountCalendarscalls (before its transaction opens) only when the provider implements it. CalDAV implements it against its own listing. Google and Microsoft do not, so their saves behave exactly as onmain, with no provider call and aliases such as Google's"primary"still accepted. Their ids name calendars inside an API the account's token already scopes, so a check there would add a network call and a new way to fail while protecting nothing; the behaviour change is kept to where the risk is. For CalDAV, a failure to check answers 502 and a rejected login 409, matching the GET.Every new request goes through the existing metadata-only dial guard; no new SSRF tier is added.
Tests
internal/caldav/calendars_test.go: anhttptestCalDAV account with two VEVENT calendars, a VTODO-only list, a collection whose href is on a second test server (another origin), a read-only calendar (read+write-properties) and privileges reported only under a 404 propstat. It asserts:Primary,Writable, no VTODO list, nothing from the other origin, and zero requests to it;sameOrigintreats default ports as equal and a different scheme, host or port as different.internal/calendar/calselect_test.go:SelectionValidatorsaves an unlisted id and"primary"as given, and is never asked to list;ErrCalendarListand still recognisable as a reauth error.internal/handler/calendar_caldav_test.go: connect, list andPUTthrough the real handler and client. An unlisted id gets a 400 with the message, and a listed one a 204.gofmt -l .clean,go vet ./...,go build ./...,go test ./...,pnpm checkandpnpm buildall pass.Deliberately not done
Fixes #42
🤖 Generated with Claude Code