✨(imports) external feed subscription v2#48
Open
sylvinus wants to merge 13 commits into
Open
Conversation
Add URL validation with SSRF protection for external calendar subscription URLs. Includes hostname resolution checks, private IP blocking, safe redirect handling, conditional request support (ETag/If-Modified-Since), and response size limits.
Add diff-based sync service that compares UIDs from an external ICS feed with events in a SabreDAV calendar, then creates, updates, or deletes events as needed. Includes Redis-based locking, auto-stop after consecutive errors, infinite RRULE capping, and volatile property filtering.
Add Dramatiq tasks for syncing one or all active subscriptions, with staggered dispatch based on channel ID. Add a management command that runs the scheduler loop, dispatching sync_all_subscriptions at a configurable interval.
Add serializers and viewset actions for creating, updating, deleting, and reactivating subscription channels. Includes per-user limit enforcement, immediate sync on creation, event purge on URL change, and SabreDAV calendar cleanup on deletion. Add ICalSubscriptionChannelFactory for tests.
Add read-only enforcement in the CalDAV proxy: write methods (PUT, DELETE, MOVE, PROPPATCH) are rejected with 403 for calendars owned by an ical-subscription channel, even if the subscription is stopped.
Add a subscription-scheduler service in compose.yaml that runs the management command for periodic subscription syncs. Update Makefile start targets to include the new service. Add reset-db-full target for full database recreation.
Add API functions and React Query hooks for subscription channel CRUD: create, update, delete, reactivate, and list. Add config.ts with SYNC_POLL_INTERVAL constant for periodic refresh.
Add components for managing external calendar subscriptions: add/edit modals, status badge, calendar section in the sidebar. Extract ColorPicker from CalendarModal for reuse in subscription modals.
Mark subscription calendar events as non-editable in FullCalendar. Open a read-only modal on click instead of the edit form. Block drag-and-drop and resize on subscription events. Add periodic calendar refresh for sync updates. Filter subscription calendars from the event creation calendar picker.
Add translation keys for subscription management UI: add/edit modals, status messages, section headers, and read-only event modal. Add external subscriptions test plan document.
📝 WalkthroughWalkthroughThis PR introduces a complete external calendar subscription feature, enabling users to import and sync read-only ICS calendar feeds. Changes span backend settings, Django REST API viewsets, CalDAV server plugins with distributed sync coordination, SSRF-hardened URL validation, and comprehensive React UI components for subscription management. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant Frontend as React Frontend
participant DjangoAPI as Django API
participant CalDAVSrv as CalDAV Server
participant ExtCal as External Calendar<br/>(ICS URL)
participant DB as Database
User->>Frontend: Clicks "Add Subscription"
Frontend->>Frontend: Opens AddSubscriptionModal
User->>Frontend: Enters URL, Name, Color
User->>Frontend: Submits form
Frontend->>DjangoAPI: POST /subscriptions/ {sourceUrl, name, color}
DjangoAPI->>DjangoAPI: Validate source_url presence
DjangoAPI->>DjangoAPI: Call fetch_ics()
DjangoAPI->>ExtCal: GET ICS URL (with DNS pinning)
ExtCal-->>DjangoAPI: 200 ICS content + ETag
DjangoAPI->>DjangoAPI: Verify ICS validity, check user limit
DjangoAPI->>CalDAVSrv: POST internal-api/subscriptions/subscribe/
CalDAVSrv->>DB: Create/find subscription principal
CalDAVSrv-->>DjangoAPI: 200 {subscription_id, caldav_path}
DjangoAPI->>DjangoAPI: Instantiate SubscriptionSyncService
DjangoAPI->>DjangoAPI: Call apply_initial_sync()
DjangoAPI->>CalDAVSrv: POST internal-api/subscriptions/{id}/events-batch/
CalDAVSrv->>DB: Upsert events from initial ICS
CalDAVSrv-->>DjangoAPI: 200 {created, updated, ...}
DjangoAPI-->>Frontend: 201 {subscription with caldav_path}
Frontend->>Frontend: Clear form, close modal
Frontend->>DjangoAPI: GET /subscriptions/ (refetch)
DjangoAPI->>CalDAVSrv: GET internal-api/subscriptions/for-user/{email}
CalDAVSrv-->>DjangoAPI: 200 [subscriptions]
DjangoAPI-->>Frontend: 200 [subscriptions]
Frontend->>Frontend: Update CalendarContext.subscriptionCalendarUrls
Frontend->>Frontend: Re-render CalendarList with new subscription
User-->>User: Sees subscription in calendar list with "ok" status badge
sequenceDiagram
participant Worker as Background Worker
participant DjangoTask as Django Task
participant SyncSvc as SubscriptionSyncService
participant CalDAVSrv as CalDAV Server
participant ExtCal as External Calendar
participant DB as Database
Worker->>DjangoTask: Dequeue sync_all_subscriptions()
DjangoTask->>CalDAVSrv: GET internal-api/subscriptions/due/?limit=500
CalDAVSrv->>DB: Query subscriptions needing sync
CalDAVSrv-->>DjangoTask: 200 [{subscription_id, last_sync, ...}]
DjangoTask->>DjangoTask: For each due subscription, dispatch sync_one_subscription with staggered delay
Worker->>DjangoTask: Dequeue sync_one_subscription(subscription_id)
DjangoTask->>SyncSvc: Instantiate SubscriptionSyncService
DjangoTask->>SyncSvc: Call sync_subscription(subscription_id)
SyncSvc->>SyncSvc: Acquire distributed lock (token-based cache lease)
alt Lock unavailable
SyncSvc-->>DjangoTask: return False (skip)
else Lock acquired
SyncSvc->>CalDAVSrv: GET internal-api/subscriptions/{id}/
CalDAVSrv-->>SyncSvc: 200 {caldav_path, last_etag, last_modified}
SyncSvc->>ExtCal: GET ICS URL with If-None-Match/If-Modified-Since
alt HTTP 304 Not Modified
SyncSvc->>CalDAVSrv: POST internal-api/subscriptions/{id}/sync-result/ {status: "ok"}
CalDAVSrv-->>SyncSvc: 200
else HTTP 200 with new content
SyncSvc->>SyncSvc: Parse ICS, fetch existing events
SyncSvc->>SyncSvc: Compute create/update/delete sets
SyncSvc->>CalDAVSrv: POST internal-api/subscriptions/{id}/events-batch/ {creates, updates, deletes}
CalDAVSrv->>DB: Upsert/delete events in subscription calendar
CalDAVSrv-->>SyncSvc: 200 {created, updated, deleted, unchanged}
SyncSvc->>CalDAVSrv: POST internal-api/subscriptions/{id}/sync-result/ {status: "ok", ...metrics}
CalDAVSrv-->>SyncSvc: 200
else Error (network/HTTP 404/etc)
SyncSvc->>SyncSvc: Increment error_count
alt error_count >= 3
SyncSvc->>CalDAVSrv: POST internal-api/subscriptions/{id}/sync-result/ {status: "stopped", error: "..."}
else error_count < 3
SyncSvc->>CalDAVSrv: POST internal-api/subscriptions/{id}/sync-result/ {status: "error", error: "..."}
end
CalDAVSrv-->>SyncSvc: 200
end
SyncSvc->>SyncSvc: Release distributed lock
SyncSvc-->>DjangoTask: return True
end
DjangoTask-->>Worker: Task complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
src/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsx (1)
33-33: Consider more descriptive aria-labels for accessibility.Using hex color values as
aria-label(e.g., "#3b82f6") isn't very meaningful for screen reader users. Consider adding descriptive color names if i18n support for color names becomes available in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsx` at line 33, The aria-label currently uses raw hex values (aria-label={c}) in the ColorPicker component; replace this with a more descriptive label by mapping the color hex (c) to a human-readable name (e.g., via a lookup or helper like getColorName(hex)) and use that value for aria-label (or fallback to a phrase such as `color ${c}`) so screen readers receive meaningful text; update the ColorPicker rendering to call the new helper (or mapping) and ensure the helper handles unknown hexes and supports future i18n integration.src/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsx (1)
327-337: Consider updating the comment to reflect the configurable interval.The polling implementation is correct. The comment mentions "5 minutes" but
SYNC_POLL_INTERVALis configurable via environment variable. Consider updating the comment to say "poll calendars at SYNC_POLL_INTERVAL" for accuracy.📝 Suggested comment update
- // Periodic refresh: poll calendars every 5 minutes to pick up + // Periodic refresh: poll calendars at SYNC_POLL_INTERVAL to pick up // subscription sync changes. Scheduler.tsx already refetches events // when davCalendars changes, so we don't call refetchEvents() here // — doing both would double CalDAV traffic on every tick.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsx` around lines 327 - 337, The existing comment incorrectly hardcodes "5 minutes"; update the comment above the useEffect that calls refreshCalendars to state that it polls at the configurable SYNC_POLL_INTERVAL (which can be set via env) and keep the note about Scheduler.tsx refetching on davCalendars to avoid double CalDAV traffic; reference the useEffect, refreshCalendars, SYNC_POLL_INTERVAL, Scheduler.tsx, refetchEvents, davCalendars and isConnected to locate and edit the comment.src/frontend/apps/calendars/src/features/calendar/components/scheduler/ReadOnlyEventModal.tsx (1)
50-50: Consider extracting the magic number to a named constant.The value
86400000(milliseconds in a day) is used for end-date adjustment. A named constant improves readability.♻️ Suggested improvement
+const MS_PER_DAY = 86400000; + export const ReadOnlyEventModal = ({ ... }) => { // ... - const endMinusOne = new Date(end.getTime() - 86400000); + const endMinusOne = new Date(end.getTime() - MS_PER_DAY);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/scheduler/ReadOnlyEventModal.tsx` at line 50, Extract the magic number 86400000 used to compute endMinusOne in ReadOnlyEventModal.tsx into a clearly named constant (e.g., MILLISECONDS_PER_DAY or MS_PER_DAY) at module or function scope, replace the numeric literal in the calculation const endMinusOne = new Date(end.getTime() - 86400000) with end.getTime() - MILLISECONDS_PER_DAY, and ensure the constant name communicates its meaning (milliseconds in a day) so the intent is clearer when reading the ReadOnlyEventModal component.src/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsx (1)
406-434: Consider extracting the IIFE into a separate component oruseMemo.The inline IIFE for computing
writableCalendarsandsanitizedCalendarUrlworks but reduces readability. Extracting this logic would improve clarity.♻️ Example refactor using useMemo
+ const { writableCalendars, sanitizedCalendarUrl } = useMemo(() => { + const writable = davCalendars.filter( + (cal) => !subscriptionCalendarUrls.has(cal.url), + ); + const sanitized = subscriptionCalendarUrls.has(modalState.calendarUrl) + ? (writable[0]?.url ?? "") + : modalState.calendarUrl; + return { writableCalendars: writable, sanitizedCalendarUrl: sanitized }; + }, [davCalendars, subscriptionCalendarUrls, modalState.calendarUrl]); {modalState.mode === "view" ? ( <ReadOnlyEventModal ... /> ) : ( - (() => { - const writableCalendars = davCalendars.filter(...); - const sanitizedCalendarUrl = ...; - return ( - <EventModal - ... - calendarUrl={sanitizedCalendarUrl} - calendars={writableCalendars} - /> - ); - })() + <EventModal + ... + calendarUrl={sanitizedCalendarUrl} + calendars={writableCalendars} + /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsx` around lines 406 - 434, Extract the IIFE rendering EventModal into a memoized value or a small child component to improve readability: move the computation of writableCalendars (filtering davCalendars by subscriptionCalendarUrls) and sanitizedCalendarUrl (fallback logic based on modalState.calendarUrl) out of the JSX, compute them with useMemo or inside a new component (e.g., EventModalWrapper) and then render <EventModal ... /> with the same props (isOpen/mode/event/calendarUrl/calendars/adapter and handlers handleModalSave, handleModalDelete, handleRespondToInvitation, handleModalClose) so behavior remains identical but the JSX is cleaner and easier to test.src/frontend/apps/calendars/src/features/calendar/config.ts (1)
14-18: Consider aligning the default polling interval with backend sync frequency.The frontend default of 60 seconds may result in redundant API calls since the backend
SUBSCRIPTION_SYNC_INTERVALdefaults to 300 seconds (persrc/backend/calendars/settings.py:103-111). Polling more frequently than the backend syncs won't yield fresh data and adds unnecessary load.Consider either:
- Increasing the default to match or exceed the backend interval (e.g.,
300_000ms)- Documenting that operators should align
NEXT_PUBLIC_SYNC_POLL_INTERVALwithSUBSCRIPTION_SYNC_INTERVAL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/config.ts` around lines 14 - 18, The frontend uses SYNC_POLL_INTERVAL (set via parsePositiveInt on NEXT_PUBLIC_SYNC_POLL_INTERVAL) with a 60_000 ms default which is more frequent than the backend SUBSCRIPTION_SYNC_INTERVAL; update the default to 300_000 ms (or another value >= backend interval) so the frontend polling aligns with backend syncs, i.e., change the fallback passed to parsePositiveInt to 300_000 and add a brief comment noting operators can override NEXT_PUBLIC_SYNC_POLL_INTERVAL to match backend SUBSCRIPTION_SYNC_INTERVAL if needed.src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx (1)
92-104: Consider showing the specific error message from the API.The error display uses a generic translation key. If the API returns a specific error message (e.g., invalid URL format, SSRF blocked), showing it would help users understand what went wrong.
💡 Suggested improvement
text={ createMutation.isError - ? t("calendar.subscription.add.error") + ? (createMutation.error as Error)?.message || + t("calendar.subscription.add.error") : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx` around lines 92 - 104, Replace the generic error text with the API's specific message when available: read the error payload from createMutation.error (e.g., createMutation.error?.message or createMutation.error?.response?.data?.message depending on your mutation shape) and pass that string to the Input text prop; fall back to t("calendar.subscription.add.error") if no specific message exists. Update the Input usage in AddSubscriptionModal (where url, setUrl, Input and createMutation are used) so the state prop remains unchanged but text becomes the specificError || translatedFallback.src/backend/core/tests/test_subscription_tasks.py (1)
53-58: Consider adding a test for exception handling.The actual task implementation catches exceptions when querying due subscriptions and logs them before returning 0. Adding a test that raises an exception from
internal_requestwould improve coverage of that code path.🧪 Suggested test case
+ `@patch`("core.tasks.sync_one_subscription") + `@patch`("core.services.caldav_service.CalDAVHTTPClient.internal_request") + def test_returns_zero_on_exception(self, mock_request, mock_sync_one): + mock_request.side_effect = Exception("Connection failed") + assert sync_all_subscriptions() == 0 + mock_sync_one.send_with_options.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/test_subscription_tasks.py` around lines 53 - 58, Add a new unit test that simulates internal_request raising an exception to exercise the exception-handling path in sync_all_subscriptions: patch core.services.caldav_service.CalDAVHTTPClient.internal_request to side_effect an Exception (e.g., Exception("boom")), call sync_all_subscriptions(), assert it returns 0, and verify the patched sync_one_subscription (mock_sync_one) is not invoked; reference the existing test pattern in test_returns_zero_on_api_error and reuse the same patches/mocks for consistency.Makefile (1)
275-288: Consider replacing the fixed sleep with a readiness check.The
sleep 2on line 279 is a timing-based wait that may be insufficient on slower systems or excessive on faster ones. A readiness check would be more reliable.♻️ Suggested improvement
@$(COMPOSE) up -d postgresql - `@sleep` 2 + `@until` $(COMPOSE) exec postgresql pg_isready -U pgroot -q; do sleep 0.5; done `@echo` "$(BOLD)Dropping and recreating database...$(RESET)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 275 - 288, In the reset-db-full Makefile target replace the fixed sleep(2) pause with a looped readiness check against the postgresql container: after bringing up postgresql with $(COMPOSE) up -d postgresql, poll the container using $(COMPOSE) exec postgresql (e.g., psql -U pgroot -d postgres -c 'select 1') or docker-compose/docker inspect health status until it returns success or a timeout, then proceed to drop/create the DB and run $(MAKE) migrate and $(MAKE) migrate-caldav; update reset-db-full to fail with a clear error if the readiness check times out.src/backend/core/tasks.py (1)
91-91: Usesettings.SUBSCRIPTION_ORPHAN_MAX_AGE_SECONDSdirectly.The
getattrfallback is redundant sinceSUBSCRIPTION_ORPHAN_MAX_AGE_SECONDSis defined with a default in settings and validated inpost_setup(). Using the setting directly is clearer and ensures consistency.♻️ Suggested simplification
- min_age = getattr(settings, "SUBSCRIPTION_ORPHAN_MAX_AGE_SECONDS", 300) + min_age = settings.SUBSCRIPTION_ORPHAN_MAX_AGE_SECONDS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tasks.py` at line 91, Replace the redundant getattr call by using the validated setting directly: assign min_age = settings.SUBSCRIPTION_ORPHAN_MAX_AGE_SECONDS in src/backend/core/tasks.py (the min_age assignment) instead of using getattr; this relies on the setting's default and post_setup() validation and keeps the code clearer and consistent.src/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsx (1)
155-183: Consider caching path lookups in the sorting comparator.The sorting logic calls
extractCaldavPathup to 4 times per comparison (fora,b, and their subscription lookups). For small lists this is fine, but could be optimized by pre-computing a map of paths to subscriptions.♻️ Optional optimization
const { regularCalendars, subscriptionCalendars } = useMemo(() => { const regular: CalDavCalendar[] = []; const subscription: CalDavCalendar[] = []; + // Pre-compute path → subscription map for efficient sorting + const pathToSub = new Map( + subscriptions.map((s) => [s.caldav_path, s]), + ); for (const cal of ownedCalendars) { const calPath = extractCaldavPath(cal.url); if (calPath && deletingPaths.has(calPath)) continue; if (cal.ownerType === "SUBSCRIPTION") { subscription.push(cal); } else { regular.push(cal); } } // Sort by subscription created_at (oldest first). subscription.sort((a, b) => { const pathA = extractCaldavPath(a.url); const pathB = extractCaldavPath(b.url); - const sA = subscriptions.find( - (s) => extractCaldavPath(s.caldav_path) === pathA, - ); - const sB = subscriptions.find( - (s) => extractCaldavPath(s.caldav_path) === pathB, - ); + const sA = pathA ? pathToSub.get(pathA) : undefined; + const sB = pathB ? pathToSub.get(pathB) : undefined; if (!sA?.created_at || !sB?.created_at) return 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsx` around lines 155 - 183, Precompute caldav paths and a lookup map before calling subscription.sort to avoid repeated extractCaldavPath calls: build a Map from path -> subscription (using subscriptions and extractCaldavPath once per subscription), and when iterating ownedCalendars compute and attach each calendar's extracted path (or keep a small temporary array of {cal, path}) so the comparator for subscription.sort uses those precomputed paths and the Map to fetch sA/sB and created_at without calling extractCaldavPath repeatedly; update references in the component (the block creating regularCalendars and subscriptionCalendars and the subscription.sort comparator) to use the precomputed values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/viewsets_subscriptions.py`:
- Around line 104-127: The current quota check only enforces
MAX_SUBSCRIPTIONS_PER_USER when list_resp.status_code == 200, allowing creations
on transient errors; change the logic around the http.internal_request call (the
list_resp variable returned by http.internal_request in
viewsets_subscriptions.py) to fail closed: if the request raises an exception or
returns any non-200 status, return an error response (e.g.,
HTTP_500_INTERNAL_SERVER_ERROR with a descriptive message like "Unable to verify
subscription quota") instead of proceeding; keep the existing ValueError except
branch and add a branch that checks list_resp.status_code != 200 and returns the
failure, ensuring the MAX_SUBSCRIPTIONS_PER_USER check always runs only when
quota verification succeeds.
In `@src/backend/core/services/subscription_sync_service.py`:
- Around line 505-528: _events_differ currently only compares VEVENTs and
ignores VTIMEZONE changes; update SubscriptionSyncService._events_differ to also
detect differences in VTIMEZONE definitions by parsing both calendars' VTIMEZONE
components (e.g., via cal.walk("VTIMEZONE")), normalizing/serializing them to a
stable form (or comparing by TZID + relevant properties) and including that
comparison in the final equality check (return True if timezone sets differ).
Keep the existing VEVENT keyed comparison and exception handling, but add the
VTIMEZONE comparison step so timezone-only updates are treated as meaningful
changes.
- Around line 124-145: apply_initial_sync currently enters the context manager
from _subscription_sync_lock(subscription_id) but ignores the boolean it yields;
change the with usage to capture the yielded value (e.g., "with
_subscription_sync_lock(subscription_id) as acquired:") and if not acquired,
return early (and optionally call self._post_sync_result(subscription_id,
status="error" or "locked", error_message="sync already running") instead of
proceeding). Ensure you only call self._sync_events(subscription_id, ics_data)
and the success/error _post_sync_result calls when acquired is True so
concurrent initial syncs cannot mutate the same subscription.
In `@src/backend/core/services/url_validation.py`:
- Around line 197-221: In _pinned_get, the requests.get call can honor
environment proxies and bypass DNS pinning; update the call in _pinned_get to
disable environment proxies for this SSRF-sensitive path by either passing
proxies={} to requests.get or by using a requests.Session(trust_env=False) and
calling session.get; ensure the change is applied where requests.get(...) is
invoked inside the with _pin_hostname(...) block so that DNS pinning performed
by _resolve_and_check and _pin_hostname cannot be bypassed.
In `@src/caldav/src/InternalApiPlugin.php`:
- Around line 1777-1839: The current flow applies the "due" filter in PHP after
fetching the first $limit rows, which can starve overdue subscriptions; update
the SQL prepared in the block that builds $stmt (the SELECT using
PrincipalBackend::TYPE_SUBSCRIPTION and LIMIT $limit) to perform the due
filtering in the query itself by joining the properties where SUBS_PROP_SOURCE
is present and computing whether (COALESCE(strtotime(SUBS_PROP_LAST_SYNC_AT), 0)
+ COALESCE(SUBS_PROP_SYNC_INTERVAL, SUBS_DEFAULT_INTERVAL) <= :now) OR
SUBS_PROP_LAST_SYNC_AT IS NULL (or equivalent depending on how properties are
stored), then bind :now and remove the PHP-side time check so the DB returns
only due rows before applying LIMIT; reference the existing symbols $stmt,
$limit, PrincipalBackend::TYPE_SUBSCRIPTION, self::SUBS_PROP_SOURCE,
self::SUBS_PROP_LAST_SYNC_AT, self::SUBS_PROP_SYNC_INTERVAL, and
self::SUBS_DEFAULT_INTERVAL.
- Around line 1590-1613: The SELECT ... FOR UPDATE approach still races when the
principal row is missing; modify the subscribe flow around
$this->pdo->beginTransaction() so the INSERT into principals is wrapped in a
try/catch: attempt the INSERT (the current $stmt->execute([...]) call), and if
it throws a unique-constraint PDOException (SQLSTATE 23000), catch it, then
re-run the SELECT id FROM principals WHERE uri = ? FOR UPDATE to load the
now-existing row and proceed as if $existed were true; ensure the transaction is
preserved and only rethrow unexpected exceptions.
In `@src/caldav/src/SubscriptionPlugin.php`:
- Around line 70-78: blockWrite currently only checks the request path (the MOVE
source) allowing clients to MOVE objects into subscription calendars via the
Destination header; update blockWrite (and use pathIsUnderSubscription) to also
detect MOVE requests and inspect the Destination header (resolve and normalize
its path) and throw the same \Sabre\DAV\Exception\Forbidden when the destination
path is under a subscription calendar, ensuring both source and destination are
protected.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsx`:
- Around line 45-52: getSubscriptionForCalendar incorrectly calls
extractCaldavPath on Subscription.caldav_path which is already normalized,
causing no matches; change the lookup in getSubscriptionForCalendar to extract
the calPath from calendar.url (using extractCaldavPath(calendar.url)) and
compare it directly to s.caldav_path (i.e. (s) => s.caldav_path === calPath),
optionally guarding for undefined s.caldav_path, so subscriptions.find can
correctly return the matching Subscription.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx`:
- Around line 27-32: The icon-only span in SubscriptionStatusBadge (the <span>
with className "subscription-status__icon material-icons" and title set to
t("calendar.subscription.status.stopped")) lacks an accessible name; add an
aria-label prop using the same translation key
(aria-label={t("calendar.subscription.status.stopped")}) to provide screen
readers with the icon's meaning while keeping the existing title.
- Around line 44-52: The toggle button in SubscriptionStatusBadge toggling
showError (onClick={() => setShowError(!showError)}) only uses title but should
also include an aria-label for screen readers; update that button to add
aria-label={t("calendar.subscription.status.viewError")} (or the same i18n key
used on the error-state button) to match the other button's accessibility
coverage.
---
Nitpick comments:
In `@Makefile`:
- Around line 275-288: In the reset-db-full Makefile target replace the fixed
sleep(2) pause with a looped readiness check against the postgresql container:
after bringing up postgresql with $(COMPOSE) up -d postgresql, poll the
container using $(COMPOSE) exec postgresql (e.g., psql -U pgroot -d postgres -c
'select 1') or docker-compose/docker inspect health status until it returns
success or a timeout, then proceed to drop/create the DB and run $(MAKE) migrate
and $(MAKE) migrate-caldav; update reset-db-full to fail with a clear error if
the readiness check times out.
In `@src/backend/core/tasks.py`:
- Line 91: Replace the redundant getattr call by using the validated setting
directly: assign min_age = settings.SUBSCRIPTION_ORPHAN_MAX_AGE_SECONDS in
src/backend/core/tasks.py (the min_age assignment) instead of using getattr;
this relies on the setting's default and post_setup() validation and keeps the
code clearer and consistent.
In `@src/backend/core/tests/test_subscription_tasks.py`:
- Around line 53-58: Add a new unit test that simulates internal_request raising
an exception to exercise the exception-handling path in sync_all_subscriptions:
patch core.services.caldav_service.CalDAVHTTPClient.internal_request to
side_effect an Exception (e.g., Exception("boom")), call
sync_all_subscriptions(), assert it returns 0, and verify the patched
sync_one_subscription (mock_sync_one) is not invoked; reference the existing
test pattern in test_returns_zero_on_api_error and reuse the same patches/mocks
for consistency.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx`:
- Around line 92-104: Replace the generic error text with the API's specific
message when available: read the error payload from createMutation.error (e.g.,
createMutation.error?.message or createMutation.error?.response?.data?.message
depending on your mutation shape) and pass that string to the Input text prop;
fall back to t("calendar.subscription.add.error") if no specific message exists.
Update the Input usage in AddSubscriptionModal (where url, setUrl, Input and
createMutation are used) so the state prop remains unchanged but text becomes
the specificError || translatedFallback.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsx`:
- Around line 155-183: Precompute caldav paths and a lookup map before calling
subscription.sort to avoid repeated extractCaldavPath calls: build a Map from
path -> subscription (using subscriptions and extractCaldavPath once per
subscription), and when iterating ownedCalendars compute and attach each
calendar's extracted path (or keep a small temporary array of {cal, path}) so
the comparator for subscription.sort uses those precomputed paths and the Map to
fetch sA/sB and created_at without calling extractCaldavPath repeatedly; update
references in the component (the block creating regularCalendars and
subscriptionCalendars and the subscription.sort comparator) to use the
precomputed values.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsx`:
- Line 33: The aria-label currently uses raw hex values (aria-label={c}) in the
ColorPicker component; replace this with a more descriptive label by mapping the
color hex (c) to a human-readable name (e.g., via a lookup or helper like
getColorName(hex)) and use that value for aria-label (or fallback to a phrase
such as `color ${c}`) so screen readers receive meaningful text; update the
ColorPicker rendering to call the new helper (or mapping) and ensure the helper
handles unknown hexes and supports future i18n integration.
In
`@src/frontend/apps/calendars/src/features/calendar/components/scheduler/ReadOnlyEventModal.tsx`:
- Line 50: Extract the magic number 86400000 used to compute endMinusOne in
ReadOnlyEventModal.tsx into a clearly named constant (e.g., MILLISECONDS_PER_DAY
or MS_PER_DAY) at module or function scope, replace the numeric literal in the
calculation const endMinusOne = new Date(end.getTime() - 86400000) with
end.getTime() - MILLISECONDS_PER_DAY, and ensure the constant name communicates
its meaning (milliseconds in a day) so the intent is clearer when reading the
ReadOnlyEventModal component.
In
`@src/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsx`:
- Around line 406-434: Extract the IIFE rendering EventModal into a memoized
value or a small child component to improve readability: move the computation of
writableCalendars (filtering davCalendars by subscriptionCalendarUrls) and
sanitizedCalendarUrl (fallback logic based on modalState.calendarUrl) out of the
JSX, compute them with useMemo or inside a new component (e.g.,
EventModalWrapper) and then render <EventModal ... /> with the same props
(isOpen/mode/event/calendarUrl/calendars/adapter and handlers handleModalSave,
handleModalDelete, handleRespondToInvitation, handleModalClose) so behavior
remains identical but the JSX is cleaner and easier to test.
In `@src/frontend/apps/calendars/src/features/calendar/config.ts`:
- Around line 14-18: The frontend uses SYNC_POLL_INTERVAL (set via
parsePositiveInt on NEXT_PUBLIC_SYNC_POLL_INTERVAL) with a 60_000 ms default
which is more frequent than the backend SUBSCRIPTION_SYNC_INTERVAL; update the
default to 300_000 ms (or another value >= backend interval) so the frontend
polling aligns with backend syncs, i.e., change the fallback passed to
parsePositiveInt to 300_000 and add a brief comment noting operators can
override NEXT_PUBLIC_SYNC_POLL_INTERVAL to match backend
SUBSCRIPTION_SYNC_INTERVAL if needed.
In
`@src/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsx`:
- Around line 327-337: The existing comment incorrectly hardcodes "5 minutes";
update the comment above the useEffect that calls refreshCalendars to state that
it polls at the configurable SYNC_POLL_INTERVAL (which can be set via env) and
keep the note about Scheduler.tsx refetching on davCalendars to avoid double
CalDAV traffic; reference the useEffect, refreshCalendars, SYNC_POLL_INTERVAL,
Scheduler.tsx, refetchEvents, davCalendars and isConnected to locate and edit
the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe0dfe03-b6e6-4ba0-9d34-7054110cf854
📒 Files selected for processing (42)
Makefiledocs/external-subscriptions-test-plan.mdsrc/backend/calendars/settings.pysrc/backend/core/api/viewsets_caldav.pysrc/backend/core/api/viewsets_channels.pysrc/backend/core/api/viewsets_subscriptions.pysrc/backend/core/management/__init__.pysrc/backend/core/management/commands/sync_mailbox_acls.pysrc/backend/core/services/subscription_sync_service.pysrc/backend/core/services/url_validation.pysrc/backend/core/tasks.pysrc/backend/core/tests/test_calendar_subscription_api.pysrc/backend/core/tests/test_subscription_sync.pysrc/backend/core/tests/test_subscription_tasks.pysrc/backend/core/tests/test_url_validation.pysrc/backend/core/urls.pysrc/caldav/server.phpsrc/caldav/sql/pgsql.principals.sqlsrc/caldav/src/CustomCalendarHome.phpsrc/caldav/src/InternalApiPlugin.phpsrc/caldav/src/PrincipalBackend.phpsrc/caldav/src/SubscriptionPlugin.phpsrc/frontend/apps/calendars/src/features/calendar/api.tssrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/EventModal.scsssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/ReadOnlyEventModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.tssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerInit.tssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/types.tssrc/frontend/apps/calendars/src/features/calendar/config.tssrc/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsxsrc/frontend/apps/calendars/src/features/calendar/hooks/useCalendars.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/EventCalendarAdapter.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/types/caldav-service.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/types/event-calendar.tssrc/frontend/apps/calendars/src/features/i18n/translations.json
💤 Files with no reviewable changes (1)
- src/backend/core/tests/test_calendar_subscription_api.py
Comment on lines
+104
to
+127
| try: | ||
| list_resp = http.internal_request( | ||
| "GET", | ||
| request.user, | ||
| f"internal-api/subscriptions/for-user/{request.user.email}", | ||
| ) | ||
| except ValueError as exc: | ||
| return Response( | ||
| {"detail": str(exc)}, | ||
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| ) | ||
| if list_resp.status_code == 200: | ||
| current = list_resp.json().get("subscriptions", []) | ||
| if len(current) >= settings.MAX_SUBSCRIPTIONS_PER_USER: | ||
| return Response( | ||
| { | ||
| "detail": ( | ||
| "Maximum number of subscriptions reached " | ||
| f"({settings.MAX_SUBSCRIPTIONS_PER_USER})." | ||
| ) | ||
| }, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fail closed when the quota lookup fails.
Right now the cap is enforced only if for-user returns 200. Any transient 5xx/502 from that endpoint skips the check and still creates the subscription, so MAX_SUBSCRIPTIONS_PER_USER becomes best-effort instead of guaranteed.
Suggested fix
- if list_resp.status_code == 200:
- current = list_resp.json().get("subscriptions", [])
- if len(current) >= settings.MAX_SUBSCRIPTIONS_PER_USER:
- return Response(
- {
- "detail": (
- "Maximum number of subscriptions reached "
- f"({settings.MAX_SUBSCRIPTIONS_PER_USER})."
- )
- },
- status=status.HTTP_400_BAD_REQUEST,
- )
+ if list_resp.status_code != 200:
+ logger.error(
+ "for-user quota check returned %s: %s",
+ list_resp.status_code,
+ list_resp.text,
+ )
+ return Response(
+ {"detail": "Failed to verify current subscription count."},
+ status=status.HTTP_502_BAD_GATEWAY,
+ )
+
+ current = list_resp.json().get("subscriptions", [])
+ if len(current) >= settings.MAX_SUBSCRIPTIONS_PER_USER:
+ return Response(
+ {
+ "detail": (
+ "Maximum number of subscriptions reached "
+ f"({settings.MAX_SUBSCRIPTIONS_PER_USER})."
+ )
+ },
+ status=status.HTTP_400_BAD_REQUEST,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets_subscriptions.py` around lines 104 - 127, The
current quota check only enforces MAX_SUBSCRIPTIONS_PER_USER when
list_resp.status_code == 200, allowing creations on transient errors; change the
logic around the http.internal_request call (the list_resp variable returned by
http.internal_request in viewsets_subscriptions.py) to fail closed: if the
request raises an exception or returns any non-200 status, return an error
response (e.g., HTTP_500_INTERNAL_SERVER_ERROR with a descriptive message like
"Unable to verify subscription quota") instead of proceeding; keep the existing
ValueError except branch and add a branch that checks list_resp.status_code !=
200 and returns the failure, ensuring the MAX_SUBSCRIPTIONS_PER_USER check
always runs only when quota verification succeeds.
Comment on lines
+124
to
+145
| with _subscription_sync_lock(subscription_id): | ||
| try: | ||
| result = self._sync_events(subscription_id, ics_data) | ||
| except Exception as exc: # noqa: BLE001 | ||
| self._post_sync_result( | ||
| subscription_id, | ||
| status="error", | ||
| error_message=str(exc)[:500], | ||
| error_count=1, | ||
| ) | ||
| return | ||
|
|
||
| self._post_sync_result( | ||
| subscription_id, | ||
| status="ok", | ||
| etag=etag, | ||
| last_modified=last_modified, | ||
| error_count=0, | ||
| error_message=( | ||
| f"{len(result.errors)} event error(s)" if result.errors else "" | ||
| ), | ||
| ) |
There was a problem hiding this comment.
apply_initial_sync() ignores whether the lock was actually acquired.
_subscription_sync_lock() yields a boolean, but this with block does not inspect it. If a scheduler/reactivation sync is already running, the initial sync still proceeds and mutates the same subscription concurrently.
Suggested fix
- with _subscription_sync_lock(subscription_id):
+ with _subscription_sync_lock(subscription_id) as acquired:
+ if not acquired:
+ logger.info(
+ "Initial sync already running for %s, skipping",
+ subscription_id,
+ )
+ return
try:
result = self._sync_events(subscription_id, ics_data)
except Exception as exc: # noqa: BLE001📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| with _subscription_sync_lock(subscription_id): | |
| try: | |
| result = self._sync_events(subscription_id, ics_data) | |
| except Exception as exc: # noqa: BLE001 | |
| self._post_sync_result( | |
| subscription_id, | |
| status="error", | |
| error_message=str(exc)[:500], | |
| error_count=1, | |
| ) | |
| return | |
| self._post_sync_result( | |
| subscription_id, | |
| status="ok", | |
| etag=etag, | |
| last_modified=last_modified, | |
| error_count=0, | |
| error_message=( | |
| f"{len(result.errors)} event error(s)" if result.errors else "" | |
| ), | |
| ) | |
| with _subscription_sync_lock(subscription_id) as acquired: | |
| if not acquired: | |
| logger.info( | |
| "Initial sync already running for %s, skipping", | |
| subscription_id, | |
| ) | |
| return | |
| try: | |
| result = self._sync_events(subscription_id, ics_data) | |
| except Exception as exc: # noqa: BLE001 | |
| self._post_sync_result( | |
| subscription_id, | |
| status="error", | |
| error_message=str(exc)[:500], | |
| error_count=1, | |
| ) | |
| return | |
| self._post_sync_result( | |
| subscription_id, | |
| status="ok", | |
| etag=etag, | |
| last_modified=last_modified, | |
| error_count=0, | |
| error_message=( | |
| f"{len(result.errors)} event error(s)" if result.errors else "" | |
| ), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/services/subscription_sync_service.py` around lines 124 -
145, apply_initial_sync currently enters the context manager from
_subscription_sync_lock(subscription_id) but ignores the boolean it yields;
change the with usage to capture the yielded value (e.g., "with
_subscription_sync_lock(subscription_id) as acquired:") and if not acquired,
return early (and optionally call self._post_sync_result(subscription_id,
status="error" or "locked", error_message="sync already running") instead of
proceeding). Ensure you only call self._sync_events(subscription_id, ics_data)
and the success/error _post_sync_result calls when acquired is True so
concurrent initial syncs cannot mutate the same subscription.
Comment on lines
+505
to
+528
| @staticmethod | ||
| def _events_differ(existing_data: str, new_data: str) -> bool: | ||
| """Whether two VCALENDAR strings differ in anything that matters.""" | ||
| try: | ||
| existing_cal = icalendar.Calendar.from_ical(existing_data) | ||
| new_cal = icalendar.Calendar.from_ical(new_data) | ||
|
|
||
| existing_events = list(existing_cal.walk("VEVENT")) | ||
| new_events = list(new_cal.walk("VEVENT")) | ||
|
|
||
| if len(existing_events) != len(new_events): | ||
| return True | ||
|
|
||
| def _keyed(events): | ||
| return { | ||
| str( | ||
| ev.get("RECURRENCE-ID", "") | ||
| ): SubscriptionSyncService._event_props(ev) | ||
| for ev in events | ||
| } | ||
|
|
||
| return _keyed(existing_events) != _keyed(new_events) | ||
| except Exception: # noqa: BLE001 | ||
| return True |
There was a problem hiding this comment.
Timezone-only changes are currently invisible to the diff.
This comparison only looks at VEVENT content. If the upstream feed updates a VTIMEZONE definition without touching event fields, the sync treats the object as unchanged and leaves the stored VCALENDAR on stale timezone rules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/services/subscription_sync_service.py` around lines 505 -
528, _events_differ currently only compares VEVENTs and ignores VTIMEZONE
changes; update SubscriptionSyncService._events_differ to also detect
differences in VTIMEZONE definitions by parsing both calendars' VTIMEZONE
components (e.g., via cal.walk("VTIMEZONE")), normalizing/serializing them to a
stable form (or comparing by TZID + relevant properties) and including that
comparison in the final equality check (return True if timezone sets differ).
Keep the existing VEVENT keyed comparison and exception handling, but add the
VTIMEZONE comparison step so timezone-only updates are treated as meaningful
changes.
Comment on lines
+197
to
+221
| def _pinned_get(url: str, headers: dict, *, timeout_msg: str): | ||
| """Issue a streaming GET with the hostname pinned to a validated IP. | ||
|
|
||
| Validates the URL's hostname, pins DNS for the lifetime of the | ||
| ``requests.get`` call, and returns the streamed response. The TCP | ||
| socket is already open by the time the pin is released, so later | ||
| body reads on the returned response are safe. | ||
| """ | ||
| parsed = urlparse(url) | ||
| if not parsed.hostname: | ||
| raise URLValidationError("URL without hostname") | ||
| safe_ip = _resolve_and_check(parsed.hostname) | ||
| try: | ||
| with _pin_hostname(parsed.hostname, safe_ip): | ||
| return requests.get( | ||
| url, | ||
| headers=headers, | ||
| timeout=(CONNECT_TIMEOUT, READ_TIMEOUT), | ||
| stream=True, | ||
| allow_redirects=False, | ||
| ) | ||
| except requests.exceptions.Timeout as exc: | ||
| raise URLValidationError(timeout_msg) from exc | ||
| except requests.exceptions.RequestException as exc: | ||
| raise URLValidationError(f"Failed to fetch URL: {_redact_url(url)}") from exc |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/backend/core/services/url_validation.py | head -250Repository: suitenumerique/calendars
Length of output: 10591
🏁 Script executed:
rg "Session|trust_env|proxy" src/backend/core/services/url_validation.py -iRepository: suitenumerique/calendars
Length of output: 50
🌐 Web query:
requests library requests.get() HTTP_PROXY HTTPS_PROXY environment variable default behavior
💡 Result:
By default, the Python requests library's requests.get (and other request methods) automatically uses proxy servers specified via standard environment variables: http_proxy (or HTTP_PROXY) for HTTP requests, https_proxy (or HTTPS_PROXY) for HTTPS requests, no_proxy (or NO_PROXY) to bypass proxies for specific hosts/domains, and all_proxy (or ALL_PROXY) as a fallback. Both lowercase and uppercase variants are supported. This behavior occurs when no proxies argument is provided to the request method. Requests obtains these proxies via requests.utils.get_environ_proxies, which internally uses urllib.request.getproxies (aliased as getproxies in requests.compat), respecting the environment variables defined by Python's urllib. Explicit proxies passed via the proxies= dict argument to requests.get override environment variables. However, session.proxies settings are overridden by environment proxies unless proxies= is specified per-request. Proxy URLs should include the scheme (e.g., http://proxy:port), and credentials in the URL are discouraged for security reasons. Example: export http_proxy=http://10.10.1.10:3128 export https_proxy=http://10.10.1.10:1080 python -c "import requests; print(requests.get('http://example.com').status_code)" This is the default behavior in recent versions (e.g., 2.33.1 as of 2026), with fixes over time for edge cases like redirects and Session.send.
Citations:
- 1: https://requests.readthedocs.io/en/master/user/advanced/
- 2: http://docs.python-requests.org/en/master/user/advanced/?highlight=http_proxy
- 3: https://requests.readthedocs.io/en/latest/user/advanced/?highlight=proxy
- 4: https://docs.python-requests.org/en/master/user/advanced.html
- 5: https://github.com/psf/requests/blob/main/src/requests/utils.py
- 6: Session does not consistently respects proxy environment variables. psf/requests#5677
- 7: https://github.com/psf/requests/blob/master/HISTORY.md
🏁 Script executed:
rg "requests\." src/backend/core/services/url_validation.py -A 2Repository: suitenumerique/calendars
Length of output: 966
🏁 Script executed:
grep -n "Session\|trust_env\|proxies" src/backend/core/services/url_validation.pyRepository: suitenumerique/calendars
Length of output: 50
🏁 Script executed:
tail -100 src/backend/core/services/url_validation.pyRepository: suitenumerique/calendars
Length of output: 3703
Disable environment proxies in requests.get() to prevent SSRF bypass.
The requests.get() call at line 211 honors HTTP_PROXY, HTTPS_PROXY, and ALL_PROXY environment variables by default. If the process runs with an outbound proxy configured, traffic routes through the proxy instead of directly to the pinned IP, causing the proxy to resolve the hostname. This completely bypasses both _resolve_and_check() and the DNS pinning logic that protects against rebinding attacks.
Pass proxies={} to requests.get() to disable proxy routing, or create a requests.Session(trust_env=False) for this SSRF-sensitive code path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/services/url_validation.py` around lines 197 - 221, In
_pinned_get, the requests.get call can honor environment proxies and bypass DNS
pinning; update the call in _pinned_get to disable environment proxies for this
SSRF-sensitive path by either passing proxies={} to requests.get or by using a
requests.Session(trust_env=False) and calling session.get; ensure the change is
applied where requests.get(...) is invoked inside the with _pin_hostname(...)
block so that DNS pinning performed by _resolve_and_check and _pin_hostname
cannot be bypassed.
Comment on lines
+1590
to
+1613
| $this->pdo->beginTransaction(); | ||
| try { | ||
| // Lock the principal row (if any) so concurrent subscribes | ||
| // for the same URL can't race past the find-or-create. | ||
| $stmt = $this->pdo->prepare( | ||
| 'SELECT id FROM principals WHERE uri = ? FOR UPDATE' | ||
| ); | ||
| $stmt->execute([$principalUri]); | ||
| $existed = (bool) $stmt->fetchColumn(); | ||
|
|
||
| if (!$existed) { | ||
| // Create the subscription principal. NULL email, NULL | ||
| // org_id (system-wide). | ||
| $stmt = $this->pdo->prepare( | ||
| 'INSERT INTO principals' | ||
| . ' (uri, email, displayname, calendar_user_type, org_id)' | ||
| . ' VALUES (?, NULL, ?, ?, NULL)' | ||
| ); | ||
| $stmt->execute([ | ||
| $principalUri, | ||
| $displayName, | ||
| PrincipalBackend::TYPE_SUBSCRIPTION, | ||
| ]); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the actual file and check the lines in question
cat -n src/caldav/src/InternalApiPlugin.php | sed -n '1580,1620p'Repository: suitenumerique/calendars
Length of output: 2090
🏁 Script executed:
# Check if there's database configuration or initialization to understand the database system
fd -t f '(config|database|db)' --ignore-case | head -20Repository: suitenumerique/calendars
Length of output: 896
🏁 Script executed:
# Search for PDO driver/database initialization
rg -i 'pdo|database.*driver|mysql|postgresql|sqlite' --type php -A 3 | head -50Repository: suitenumerique/calendars
Length of output: 3256
🏁 Script executed:
# Check for any transaction isolation level settings
rg -i 'isolation|transaction.*level' --type phpRepository: suitenumerique/calendars
Length of output: 576
🏁 Script executed:
# Check for PostgreSQL initialization and configuration
cat docker/postgresql/init-databases.sh | head -30Repository: suitenumerique/calendars
Length of output: 1260
🏁 Script executed:
# Check caldav database initialization
cat src/caldav/init-database.sh | head -50Repository: suitenumerique/calendars
Length of output: 1378
🏁 Script executed:
# Look for schema or migrations defining the principals table
fd -t f '(schema|migration)' --ignore-case src/caldav | xargs grep -l principals 2>/dev/nullRepository: suitenumerique/calendars
Length of output: 52
🏁 Script executed:
# Check the principals table schema in the caldav database
rg 'CREATE TABLE.*principals' -A 10 --type sql --type phpRepository: suitenumerique/calendars
Length of output: 764
🏁 Script executed:
# Get the full principals schema definition
cat src/caldav/sql/pgsql.principals.sqlRepository: suitenumerique/calendars
Length of output: 1412
🏁 Script executed:
# Check if there are any constraints on uri column
rg 'principals.*uri' src/caldav/sql/ -A 5 -B 5Repository: suitenumerique/calendars
Length of output: 713
🏁 Script executed:
# Look for any unique constraints or indexes on principals
fd pgsql.principals src/caldav/sql | xargs catRepository: suitenumerique/calendars
Length of output: 1412
The first subscribe still races on a missing principal row.
SELECT ... FOR UPDATE only serializes callers once the row exists. Two workers subscribing the same new source_url can both see "no principal", then one insert wins and the other fails with a UNIQUE constraint violation on principals.uri. This endpoint is supposed to be idempotent.
Suggested fix
- $stmt = $this->pdo->prepare(
- 'SELECT id FROM principals WHERE uri = ? FOR UPDATE'
- );
- $stmt->execute([$principalUri]);
- $existed = (bool) $stmt->fetchColumn();
+ $stmt = $this->pdo->prepare(
+ 'INSERT INTO principals'
+ . ' (uri, email, displayname, calendar_user_type, org_id)'
+ . ' VALUES (?, NULL, ?, ?, NULL)'
+ . ' ON CONFLICT (uri) DO NOTHING'
+ );
+ $stmt->execute([
+ $principalUri,
+ $displayName,
+ PrincipalBackend::TYPE_SUBSCRIPTION,
+ ]);
+ $existed = ($stmt->rowCount() === 0);
+
+ $lockStmt = $this->pdo->prepare(
+ 'SELECT id FROM principals WHERE uri = ? FOR UPDATE'
+ );
+ $lockStmt->execute([$principalUri]);
if (!$existed) {
- // Create the subscription principal. NULL email, NULL
- // org_id (system-wide).
- $stmt = $this->pdo->prepare(
- 'INSERT INTO principals'
- . ' (uri, email, displayname, calendar_user_type, org_id)'
- . ' VALUES (?, NULL, ?, ?, NULL)'
- );
- $stmt->execute([
- $principalUri,
- $displayName,
- PrincipalBackend::TYPE_SUBSCRIPTION,
- ]);
-
// Create the owner calendar under the subscription
// principal. URI is always 'default' (one calendar per
// subscription principal).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/caldav/src/InternalApiPlugin.php` around lines 1590 - 1613, The SELECT
... FOR UPDATE approach still races when the principal row is missing; modify
the subscribe flow around $this->pdo->beginTransaction() so the INSERT into
principals is wrapped in a try/catch: attempt the INSERT (the current
$stmt->execute([...]) call), and if it throws a unique-constraint PDOException
(SQLSTATE 23000), catch it, then re-run the SELECT id FROM principals WHERE uri
= ? FOR UPDATE to load the now-existing row and proceed as if $existed were
true; ensure the transaction is preserved and only rethrow unexpected
exceptions.
Comment on lines
+1777
to
+1839
| try { | ||
| $stmt = $this->pdo->prepare( | ||
| 'SELECT p.uri AS principal_uri, ci.uri AS calendar_uri,' | ||
| . ' SUBSTR(p.uri, LENGTH(\'principals/subscriptions/\') + 1) AS subscription_id' | ||
| . ' FROM principals p' | ||
| . ' JOIN calendarinstances ci' | ||
| . ' ON ci.principaluri = p.uri AND ci.access = 1' | ||
| . ' WHERE p.calendar_user_type = ?' | ||
| . ' ORDER BY p.id ASC' | ||
| . ' LIMIT ' . $limit | ||
| ); | ||
| $stmt->execute([PrincipalBackend::TYPE_SUBSCRIPTION]); | ||
| $candidates = $stmt->fetchAll(\PDO::FETCH_ASSOC); | ||
| } catch (\Exception $e) { | ||
| error_log('[InternalApiPlugin] due query failed: ' . $e->getMessage()); | ||
| $this->jsonError($response, 500, 'Failed to query due subscriptions'); | ||
| return; | ||
| } | ||
|
|
||
| $now = time(); | ||
| $due = []; | ||
| foreach ($candidates as $row) { | ||
| $path = 'calendars/subscriptions/' . $row['subscription_id'] | ||
| . '/' . $row['calendar_uri']; | ||
| $props = $this->readProperties($path, [ | ||
| self::SUBS_PROP_SOURCE, | ||
| self::SUBS_PROP_SYNC_INTERVAL, | ||
| self::SUBS_PROP_LAST_SYNC_AT, | ||
| self::SUBS_PROP_ETAG, | ||
| self::SUBS_PROP_LAST_MODIFIED, | ||
| ]); | ||
|
|
||
| // Drop rows with no source — something went wrong on subscribe. | ||
| if (empty($props[self::SUBS_PROP_SOURCE])) { | ||
| continue; | ||
| } | ||
|
|
||
| $interval = (int) ($props[self::SUBS_PROP_SYNC_INTERVAL] | ||
| ?? self::SUBS_DEFAULT_INTERVAL); | ||
| if ($interval < 1) { | ||
| $interval = self::SUBS_DEFAULT_INTERVAL; | ||
| } | ||
|
|
||
| $lastSyncAt = $props[self::SUBS_PROP_LAST_SYNC_AT] ?? null; | ||
| if ($lastSyncAt) { | ||
| $lastTs = strtotime($lastSyncAt); | ||
| if ($lastTs !== false && $lastTs + $interval > $now) { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| $due[] = [ | ||
| 'subscription_id' => $row['subscription_id'], | ||
| 'principal_uri' => $row['principal_uri'], | ||
| 'calendar_uri' => $row['calendar_uri'], | ||
| 'caldav_path' => 'calendars/subscriptions/' . $row['subscription_id'] | ||
| . '/' . $row['calendar_uri'] . '/', | ||
| 'source_url' => $props[self::SUBS_PROP_SOURCE], | ||
| 'sync_interval' => $interval, | ||
| 'etag' => $props[self::SUBS_PROP_ETAG] ?? '', | ||
| 'last_modified' => $props[self::SUBS_PROP_LAST_MODIFIED] ?? '', | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Apply the “due” filter before LIMIT.
This endpoint limits the first N subscription principals and only then filters them in PHP. After the system grows past limit, overdue subscriptions outside that initial window can starve forever if earlier IDs are not yet due.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/caldav/src/InternalApiPlugin.php` around lines 1777 - 1839, The current
flow applies the "due" filter in PHP after fetching the first $limit rows, which
can starve overdue subscriptions; update the SQL prepared in the block that
builds $stmt (the SELECT using PrincipalBackend::TYPE_SUBSCRIPTION and LIMIT
$limit) to perform the due filtering in the query itself by joining the
properties where SUBS_PROP_SOURCE is present and computing whether
(COALESCE(strtotime(SUBS_PROP_LAST_SYNC_AT), 0) +
COALESCE(SUBS_PROP_SYNC_INTERVAL, SUBS_DEFAULT_INTERVAL) <= :now) OR
SUBS_PROP_LAST_SYNC_AT IS NULL (or equivalent depending on how properties are
stored), then bind :now and remove the PHP-side time check so the DB returns
only due rows before applying LIMIT; reference the existing symbols $stmt,
$limit, PrincipalBackend::TYPE_SUBSCRIPTION, self::SUBS_PROP_SOURCE,
self::SUBS_PROP_LAST_SYNC_AT, self::SUBS_PROP_SYNC_INTERVAL, and
self::SUBS_DEFAULT_INTERVAL.
Comment on lines
+70
to
+78
| public function blockWrite(RequestInterface $request, ResponseInterface $response) | ||
| { | ||
| $path = $request->getPath(); | ||
| if (!$this->pathIsUnderSubscription($path)) { | ||
| return; | ||
| } | ||
| throw new \Sabre\DAV\Exception\Forbidden( | ||
| 'Cannot modify events in a subscription calendar.' | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the SubscriptionPlugin.php file
find . -name "SubscriptionPlugin.php" -type fRepository: suitenumerique/calendars
Length of output: 107
🏁 Script executed:
# Get the full file to understand the complete context
cd src/caldav/src && wc -l SubscriptionPlugin.php && cd ../../..Repository: suitenumerique/calendars
Length of output: 94
🏁 Script executed:
# Read the SubscriptionPlugin.php file
cat -n src/caldav/src/SubscriptionPlugin.phpRepository: suitenumerique/calendars
Length of output: 6758
Block MOVE into subscription calendars.
The current implementation only checks the request path, which on MOVE operations is the source object. A client can move an event from a writable calendar into a subscription calendar via the Destination header, bypassing the read-only protection.
Suggested fix
public function blockWrite(RequestInterface $request, ResponseInterface $response)
{
- $path = $request->getPath();
- if (!$this->pathIsUnderSubscription($path)) {
- return;
+ $paths = [$request->getPath()];
+
+ if ($request->getMethod() === 'MOVE') {
+ $destination = $request->getHeader('Destination');
+ if ($destination) {
+ $destinationPath = parse_url($destination, PHP_URL_PATH) ?: '';
+ $paths[] = ltrim($destinationPath, '/');
+ }
}
- throw new \Sabre\DAV\Exception\Forbidden(
- 'Cannot modify events in a subscription calendar.'
- );
+
+ foreach ($paths as $path) {
+ if ($this->pathIsUnderSubscription($path)) {
+ throw new \Sabre\DAV\Exception\Forbidden(
+ 'Cannot modify events in a subscription calendar.'
+ );
+ }
+ }
}🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 70-70: Avoid unused parameters such as '$response'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/caldav/src/SubscriptionPlugin.php` around lines 70 - 78, blockWrite
currently only checks the request path (the MOVE source) allowing clients to
MOVE objects into subscription calendars via the Destination header; update
blockWrite (and use pathIsUnderSubscription) to also detect MOVE requests and
inspect the Destination header (resolve and normalize its path) and throw the
same \Sabre\DAV\Exception\Forbidden when the destination path is under a
subscription calendar, ensuring both source and destination are protected.
Comment on lines
+45
to
+52
| const getSubscriptionForCalendar = ( | ||
| calendar: CalDavCalendar, | ||
| ): Subscription | undefined => { | ||
| const calPath = extractCaldavPath(calendar.url); | ||
| if (!calPath) return undefined; | ||
| return subscriptions.find( | ||
| (s) => extractCaldavPath(s.caldav_path) === calPath, | ||
| ); |
There was a problem hiding this comment.
Subscription lookup never matches the API payload.
extractCaldavPath() parses full calendar URLs, but Subscription.caldav_path is already the normalized path from the API. Feeding that path back into the helper returns null, so subscription stays undefined and the status badge / delete / reactivate wiring never attaches.
Suggested fix
+ const normalizeCalendarPath = (path: string) =>
+ path.startsWith("/") ? path : `/${path}`;
+
const getSubscriptionForCalendar = (
calendar: CalDavCalendar,
): Subscription | undefined => {
const calPath = extractCaldavPath(calendar.url);
if (!calPath) return undefined;
return subscriptions.find(
- (s) => extractCaldavPath(s.caldav_path) === calPath,
+ (s) => normalizeCalendarPath(s.caldav_path) === calPath,
);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const getSubscriptionForCalendar = ( | |
| calendar: CalDavCalendar, | |
| ): Subscription | undefined => { | |
| const calPath = extractCaldavPath(calendar.url); | |
| if (!calPath) return undefined; | |
| return subscriptions.find( | |
| (s) => extractCaldavPath(s.caldav_path) === calPath, | |
| ); | |
| const normalizeCalendarPath = (path: string) => | |
| path.startsWith("/") ? path : `/${path}`; | |
| const getSubscriptionForCalendar = ( | |
| calendar: CalDavCalendar, | |
| ): Subscription | undefined => { | |
| const calPath = extractCaldavPath(calendar.url); | |
| if (!calPath) return undefined; | |
| return subscriptions.find( | |
| (s) => normalizeCalendarPath(s.caldav_path) === calPath, | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsx`
around lines 45 - 52, getSubscriptionForCalendar incorrectly calls
extractCaldavPath on Subscription.caldav_path which is already normalized,
causing no matches; change the lookup in getSubscriptionForCalendar to extract
the calPath from calendar.url (using extractCaldavPath(calendar.url)) and
compare it directly to s.caldav_path (i.e. (s) => s.caldav_path === calPath),
optionally guarding for undefined s.caldav_path, so subscriptions.find can
correctly return the matching Subscription.
Comment on lines
+27
to
+32
| <span | ||
| className="subscription-status__icon material-icons" | ||
| title={t("calendar.subscription.status.stopped")} | ||
| > | ||
| block | ||
| </span> |
There was a problem hiding this comment.
Add aria-label to the icon-only span for screen reader users.
The <span> with title displays an icon but lacks text content. Screen readers may not convey its meaning without an accessible label.
♿ Proposed fix
<span
className="subscription-status__icon material-icons"
title={t("calendar.subscription.status.stopped")}
+ role="img"
+ aria-label={t("calendar.subscription.status.stopped")}
>
block
</span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <span | |
| className="subscription-status__icon material-icons" | |
| title={t("calendar.subscription.status.stopped")} | |
| > | |
| block | |
| </span> | |
| <span | |
| className="subscription-status__icon material-icons" | |
| title={t("calendar.subscription.status.stopped")} | |
| role="img" | |
| aria-label={t("calendar.subscription.status.stopped")} | |
| > | |
| block | |
| </span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx`
around lines 27 - 32, The icon-only span in SubscriptionStatusBadge (the <span>
with className "subscription-status__icon material-icons" and title set to
t("calendar.subscription.status.stopped")) lacks an accessible name; add an
aria-label prop using the same translation key
(aria-label={t("calendar.subscription.status.stopped")}) to provide screen
readers with the icon's meaning while keeping the existing title.
Comment on lines
+44
to
+52
| <button | ||
| className="subscription-status__toggle" | ||
| onClick={() => setShowError(!showError)} | ||
| title={t("calendar.subscription.status.viewError")} | ||
| > | ||
| <span className="material-icons" style={{ fontSize: "14px" }}> | ||
| {showError ? "expand_less" : "expand_more"} | ||
| </span> | ||
| </button> |
There was a problem hiding this comment.
Add aria-label for accessibility consistency.
The toggle button in the "stopped" state uses only title for accessibility, while the "error" state button (lines 64-80) has both title and aria-label. Screen readers may not announce title attributes consistently.
♿ Proposed fix
<button
className="subscription-status__toggle"
onClick={() => setShowError(!showError)}
title={t("calendar.subscription.status.viewError")}
+ aria-label={t("calendar.subscription.status.viewError")}
+ aria-expanded={showError}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| className="subscription-status__toggle" | |
| onClick={() => setShowError(!showError)} | |
| title={t("calendar.subscription.status.viewError")} | |
| > | |
| <span className="material-icons" style={{ fontSize: "14px" }}> | |
| {showError ? "expand_less" : "expand_more"} | |
| </span> | |
| </button> | |
| <button | |
| className="subscription-status__toggle" | |
| onClick={() => setShowError(!showError)} | |
| title={t("calendar.subscription.status.viewError")} | |
| aria-label={t("calendar.subscription.status.viewError")} | |
| aria-expanded={showError} | |
| > | |
| <span className="material-icons" style={{ fontSize: "14px" }}> | |
| {showError ? "expand_less" : "expand_more"} | |
| </span> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx`
around lines 44 - 52, The toggle button in SubscriptionStatusBadge toggling
showError (onClick={() => setShowError(!showError)}) only uses title but should
also include an aria-label for screen readers; update that button to add
aria-label={t("calendar.subscription.status.viewError")} (or the same i18n key
used on the error-state button) to match the other button's accessibility
coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor of #45 based on caldav storage only for models + dedup
Summary by CodeRabbit
New Features
Documentation