Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds a CalDAV client/service, Celery tasks, mailbox-scoped DRF endpoints (rsvp/add/conflicts/calendars), settings/openapi/schema updates, extensive backend tests, Celery task registration, and frontend calendar-invite UI/components, hooks, styles and locales to support CalDAV-backed RSVP and add-to-calendar flows. ChangesCalDAV integration
Frontend calendar UI and supporting changes
Sequence Diagram(s) sequenceDiagram
participant Client
participant API
participant Celery
participant CalDAV
Client->>API: POST /mailboxes/{mailbox_id}/calendar/rsvp or /add or /conflicts
API->>Celery: enqueue calendar task (rsvp/add) with mailbox/channel ids
Celery->>CalDAV: CalDAVService.add_event/respond_to_event/check_conflicts
CalDAV-->>Celery: result or CalDAVError
Celery-->>API: task registered (task_id) or FAILURE
API-->>Client: 200 (task_id) / 502 / 503 / 404 as mapped
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scss (1)
267-273: Consider layout for three-item conflict rows.The conflict item uses
justify-content: space-betweenwith three children (conflict-summary,conflict-calendar,conflict-time). This distributes items with the summary on the left, calendar in the middle, and time on the right. On narrow viewports, this may cause text truncation or awkward spacing.Consider using
flex-wrap: wrapor adjusting the layout so content can flow more gracefully when space is constrained.💡 Optional: Allow wrapping for conflict items
.calendar-invite__conflict-item { display: flex; justify-content: space-between; align-items: center; gap: var(--c--globals--spacings--sm); padding: var(--c--globals--spacings--3xs) 0; + flex-wrap: wrap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scss` around lines 267 - 273, The .calendar-invite__conflict-item currently uses justify-content: space-between which spreads .conflict-summary, .conflict-calendar, and .conflict-time awkwardly on narrow viewports; update .calendar-invite__conflict-item to allow wrapping (e.g., add flex-wrap: wrap) and adjust child flex rules so .conflict-summary can shrink/grow (e.g., give it flex: 1 1 auto and min-width: 0) while keeping .conflict-calendar and .conflict-time fixed or non-growing (e.g., flex: 0 0 auto), and tweak gaps or padding as needed so the three-item row flows gracefully on small screens.src/backend/core/services/calendar/service.py (2)
133-133: Moveimport reto module level.The
remodule is imported inside methods at lines 133 and 157. Moving it to the top of the file alongside other imports improves readability and follows Python conventions.Proposed fix at module level
"""CalDAV calendar service for RSVP and event management.""" import logging +import re import caldav from caldav.elements import davThen remove the local imports at lines 133 and 157.
Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/calendar/service.py` at line 133, Move the "import re" statement to the top of the module alongside the other imports (add a single module-level "import re") and remove the local "import re" statements that currently appear inside methods in this file; ensure those methods (which use regex functionality) continue to reference the module-level "re" symbol rather than importing it locally.
20-28: Add a timeout to DAVClient to prevent indefinite request blocking.The
caldav.DAVClientis created without a timeout parameter. If the CalDAV server is slow or unresponsive, this could cause request threads (inCalendarConflictsViewandCalendarListView) or Celery workers to block indefinitely.Proposed fix
`@property` def client(self): if self._client is None: self._client = caldav.DAVClient( url=self.url, username=self.username, password=self.password, + timeout=30, # seconds ) return self._client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/calendar/service.py` around lines 20 - 28, The DAVClient instantiation in the client property lacks a timeout, risking indefinite blocking; update the client property to pass a sensible timeout value (e.g., timeout=10 or use a configured constant) into caldav.DAVClient when creating self._client so requests from CalendarConflictsView/CalendarListView and Celery workers don't hang indefinitely; ensure the timeout value is configurable via settings or an environment variable and referenced where caldav.DAVClient is constructed in the client property.
🤖 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/calendar.py`:
- Around line 259-267: CalendarListView.get is making blocking I/O via
CalDAVService.from_channel and service.list_calendars (synchronous calls) which
can block the event loop; refactor this handler to offload the blocking calls to
an async-friendly pattern—either convert CalDAVService methods to async or run
the synchronous calls in a thread/process executor (e.g., using
asyncio.to_thread or a background task) before returning the Response;
specifically wrap CalDAVService.from_channel(...) and service.list_calendars()
so they execute off the main thread and handle/propagate exceptions the same way
as existing logger.exception and Response logic.
- Around line 219-229: CalendarConflictsView is performing a blocking call to
the external CalDAV server via CalDAVService.from_channel(...) and
service.check_conflicts(...); change this to avoid blocking the request thread
by either converting the view to an async Django view and awaiting an async
CalDAVService.check_conflicts (ensure CalDAVService implements an async API and
enforces a network timeout), or move the conflict check into a background Celery
task (enqueue a task from CalendarConflictsView that calls
CalDAVService.check_conflicts and return a 202 with a task id), and in either
case ensure CalDAVService has a strict configurable timeout for network calls to
prevent long waits.
- Around line 25-46: The mailbox lookup in CalDAVChannelMixin is missing an
authorization check allowing any authenticated user to access arbitrary
mailboxes; update the cached_property mailbox to resolve the Mailbox through the
same authorized/queryset pattern used in blob.py and contacts.py (i.e., restrict
the queryset to mailboxes the current request.user may access or call the shared
helper that returns an authorized mailbox) so that get_object_or_404 only
returns a mailbox the requester is permitted to use; leave get_caldav_channel
and require_caldav_channel unchanged except that they should rely on the
now-authorized mailbox property.
In
`@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsx`:
- Around line 358-361: The CalDAV query errors are being swallowed and replaced
with empty arrays, making transient failures indistinguishable from genuine "no
data" and hiding UI like the ConflictWarning; update the query hooks used around
the referenced areas (the calendar fetch and event fetch calls used to compute
`conflicts`, `calendars`, and related data) so they propagate an error state
instead of returning `[]` on failure, and adjust the rendering in this component
to check for that error state (e.g., `calendarsError`/`eventsError` or similar)
and render a local "Calendar unavailable — retry" UI with a retry action; ensure
`ConflictWarning` is still shown when `conflicts.length > 0` but that when the
queries report an error you show the unavailable/retry UI instead of collapsing
to an empty state.
- Around line 469-470: The RSVP/Add flow is committing rsvpResponse and relying
on isPending only after the POST returns a task_id, allowing multiple clicks and
lost/incorrect state; add a local "pendingAction" boolean (or enum) state (e.g.,
isSubmitting or pendingRsvp) that is set immediately in the RSVP/Add click
handlers (before the POST) to disable controls, and preserve the intended
response plus the returned task_id in a small pending map (taskId -> intended
response). Keep selectedCalendarId handling but do not call setRsvpResponse
until the background poll for that task_id reaches SUCCESS; on FAILURE/timeout
clear the pendingAction and do not commit rsvpResponse (and re-enable controls).
Update the RSVP/Add handler functions and the polling completion logic to look
up the pending map by task_id and commit or clear state accordingly so clicks
cannot enqueue multiple jobs or prematurely highlight a choice.
---
Nitpick comments:
In `@src/backend/core/services/calendar/service.py`:
- Line 133: Move the "import re" statement to the top of the module alongside
the other imports (add a single module-level "import re") and remove the local
"import re" statements that currently appear inside methods in this file; ensure
those methods (which use regex functionality) continue to reference the
module-level "re" symbol rather than importing it locally.
- Around line 20-28: The DAVClient instantiation in the client property lacks a
timeout, risking indefinite blocking; update the client property to pass a
sensible timeout value (e.g., timeout=10 or use a configured constant) into
caldav.DAVClient when creating self._client so requests from
CalendarConflictsView/CalendarListView and Celery workers don't hang
indefinitely; ensure the timeout value is configurable via settings or an
environment variable and referenced where caldav.DAVClient is constructed in the
client property.
In
`@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scss`:
- Around line 267-273: The .calendar-invite__conflict-item currently uses
justify-content: space-between which spreads .conflict-summary,
.conflict-calendar, and .conflict-time awkwardly on narrow viewports; update
.calendar-invite__conflict-item to allow wrapping (e.g., add flex-wrap: wrap)
and adjust child flex rules so .conflict-summary can shrink/grow (e.g., give it
flex: 1 1 auto and min-width: 0) while keeping .conflict-calendar and
.conflict-time fixed or non-growing (e.g., flex: 0 0 auto), and tweak gaps or
padding as needed so the three-item row flows gracefully on small screens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7bfb200-20b4-433f-ad17-21d8b2b39b4e
⛔ Files ignored due to path filters (1)
src/backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
src/backend/core/api/viewsets/calendar.pysrc/backend/core/services/calendar/__init__.pysrc/backend/core/services/calendar/service.pysrc/backend/core/services/calendar/tasks.pysrc/backend/core/tasks.pysrc/backend/core/urls.pysrc/backend/pyproject.tomlsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/src/features/controlled-modals/message-importer/step-loader.tsxsrc/frontend/src/features/layouts/components/main/header/authenticated.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-footer.tsxsrc/frontend/src/hooks/use-task-status.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/calendar.py`:
- Around line 97-129: The view currently forwards calendar_id from post into
calendar_rsvp_task/calendar_add_event_task without verifying it belongs to the
mailbox; instead of adding validation here, implement strict ownership
validation inside the calendar service by updating _pick_calendar_url in
src/backend/core/services/calendar/service.py to resolve/verify that a provided
calendar_id (or URL) is actually part of the requester’s calendar-home-set
(raise an error or return None on mismatch), and ensure calendar_rsvp_task and
calendar_add_event_task call that service helper (or surface the validation
error) so the view (post) can safely forward calendar_id knowing the service
enforces ownership.
In `@src/backend/core/services/calendar/service.py`:
- Around line 218-268: The _pick_calendar_url implementation currently returns
caller-supplied calendar_id verbatim, allowing authenticated PUTs to arbitrary
URLs; change _pick_calendar_url to validate any provided calendar_id against the
set returned by list_calendars() (or the parsed IDs from each calendar dict) and
only use it if it exactly equals one of those known calendar URLs (and
optionally ensure its origin matches self.home_set/origin); if it does not
match, fall back to selecting the first calendar from list_calendars() or raise
CalDAVError. Update callers (respond_to_event / add_event flow that call
_pick_calendar_url before _put_event) to rely on this validated URL so
_put_event never issues authenticated requests to attacker-provided hosts.
- Around line 246-260: The matching logic in _update_partstat incorrectly uses
substring containment (email_lower in str(att).lower()) which can match sibling
addresses; change it to parse and compare the attendee's canonical email address
for exact equality (e.g., extract the address from the vCalAddress/ATTENDEE
value by stripping any "mailto:"/scheme and lowercasing, or use the library's
property that returns the email) and only update when equal, keeping the rest of
the loop (comp walk, attendees normalization, setting att.params["PARTSTAT"],
and popping RSVP) the same; add a regression test TestUpdatePartstat in
test_calendar.py that creates two attendees where one address is a substring of
the other and asserts only the exact match is updated.
🪄 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: 3b03b3fe-934a-4af2-a929-82681362e162
⛔ Files ignored due to path filters (1)
src/backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
src/backend/core/api/viewsets/calendar.pysrc/backend/core/services/calendar/__init__.pysrc/backend/core/services/calendar/service.pysrc/backend/core/services/calendar/tasks.pysrc/backend/core/tasks.pysrc/backend/core/tests/api/test_calendar.pysrc/backend/core/urls.pysrc/backend/messages/settings.pysrc/backend/pyproject.tomlsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/src/features/controlled-modals/message-importer/step-loader.tsxsrc/frontend/src/features/layouts/components/main/header/authenticated.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-footer.tsx
✅ Files skipped from review due to trivial changes (3)
- src/frontend/public/locales/common/en-US.json
- src/backend/messages/settings.py
- src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scss
🚧 Files skipped from review as they are similar to previous changes (6)
- src/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-footer.tsx
- src/backend/pyproject.toml
- src/backend/core/tasks.py
- src/frontend/src/features/controlled-modals/message-importer/step-loader.tsx
- src/backend/core/urls.py
- src/backend/core/services/calendar/tasks.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/backend/core/services/calendar/service.py (2)
193-198: Minor:findtextcalled twice per response.The list comprehension evaluates
findtext(...)twice for every matching response (once in the filter, once in the expression). For large calendar result sets this doubles XML tree traversal. A simple loop (or walrus) keeps it single-pass:Proposed fix
- root = ET.fromstring(resp.text) - return [ - r.findtext(f".//{_q(CALDAV_NS, 'calendar-data')}") or "" - for r in root.findall(_q(DAV_NS, "response")) - if r.findtext(f".//{_q(CALDAV_NS, 'calendar-data')}") - ] + root = ET.fromstring(resp.text) + results = [] + tag = f".//{_q(CALDAV_NS, 'calendar-data')}" + for r in root.findall(_q(DAV_NS, "response")): + data = r.findtext(tag) + if data: + results.append(data) + return results🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/calendar/service.py` around lines 193 - 198, The list comprehension calls r.findtext(...) twice per response; change it to a single-pass by iterating over root.findall(_q(DAV_NS, "response")) and storing the calendar-data result in a local variable (or use the walrus operator) once per response (use the same _q(CALDAV_NS, 'calendar-data') key), then append only non-empty values to the result list and return that list from the function where ET.fromstring(resp.text) and root.findall are used.
32-36: Naive datetime silently treated as UTC.If a caller passes a tz-naive
datetime, theif dt.tzinfo is not Nonebranch is skipped andstrftime("…Z")stamps it as UTC regardless of what timezone it actually represented. With Django'sUSE_TZ=Truethis is usually fine, but a bug upstream (e.g. adatetime.now()slipping in from a view/serializer) will silently produce wrong CalDAV time-range queries rather than fail loudly.Consider either asserting aware input or assuming a documented timezone explicitly:
Proposed fix
def _format_utc(dt): - if dt.tzinfo is not None: - dt = dt.astimezone(timezone.utc) - return dt.strftime("%Y%m%dT%H%M%SZ") + if dt.tzinfo is None: + raise ValueError("_format_utc requires a timezone-aware datetime") + return dt.astimezone(timezone.utc).strftime("%Y%m%dT%H%M%SZ")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/calendar/service.py` around lines 32 - 36, The function _format_utc currently treats tz-naive datetimes as UTC; change it to require timezone-aware input by checking with django.utils.timezone.is_naive (or dt.tzinfo is None) and raise a clear exception (ValueError) if the datetime is naive so callers must pass an aware datetime; retain the existing behavior of converting an aware datetime via dt.astimezone(timezone.utc) and then formatting with "%Y%m%dT%H%M%SZ" to preserve output.
🤖 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/openapi.json`:
- Around line 2563-2715: Update the OpenAPI responses by adding the missing 400
and 502 response schemas to the `@extend_schema`(responses=...) annotations in
src/backend/core/api/viewsets/calendar.py for the affected view methods (the
RSVP handler, add event handler, conflicts handler, and list-calendars handler
referenced around lines 65-292); specifically include 400 validation responses
for RSVP, add event, and conflicts, and include 502 for conflicts and list
calendars, then re-run the OpenAPI regeneration (drf-spectacular / your
project’s generate-openapi step) so
/api/v1.0/mailboxes/{mailbox_id}/calendar/... endpoints
(mailboxes_calendar_rsvp_create, mailboxes_calendar_add_create,
mailboxes_calendar_conflicts_create, mailboxes_calendar_calendars_retrieve)
reflect those responses.
In `@src/backend/core/services/calendar/service.py`:
- Around line 291-310: The _put_event function builds event_url by concatenating
an attacker-controlled UID into the path; fix by sanitizing or encoding the UID
before use: after extracting uid in _put_event validate it against a strict
allowlist (e.g. only alphanumerics, dot, hyphen, underscore and a reasonable max
length) and reject or replace any UID with path delimiters, CR/LF, whitespace,
percent sequences, or ".."; alternatively percent-encode the UID as a single
path segment (use urllib.parse.quote with no "/" in the safe set) and then build
event_url using calendar_url.rstrip("/") + "/" + quoted_uid + ".ics"; ensure the
code raises a clear error for rejected UIDs and does not allow raw uid to flow
into event_url (refer to variables uid, calendar_url, event_url and function
_put_event).
---
Nitpick comments:
In `@src/backend/core/services/calendar/service.py`:
- Around line 193-198: The list comprehension calls r.findtext(...) twice per
response; change it to a single-pass by iterating over root.findall(_q(DAV_NS,
"response")) and storing the calendar-data result in a local variable (or use
the walrus operator) once per response (use the same _q(CALDAV_NS,
'calendar-data') key), then append only non-empty values to the result list and
return that list from the function where ET.fromstring(resp.text) and
root.findall are used.
- Around line 32-36: The function _format_utc currently treats tz-naive
datetimes as UTC; change it to require timezone-aware input by checking with
django.utils.timezone.is_naive (or dt.tzinfo is None) and raise a clear
exception (ValueError) if the datetime is naive so callers must pass an aware
datetime; retain the existing behavior of converting an aware datetime via
dt.astimezone(timezone.utc) and then formatting with "%Y%m%dT%H%M%SZ" to
preserve output.
🪄 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: a9df9385-b1a7-427e-9d83-ed8bb879d1db
⛔ Files ignored due to path filters (13)
src/frontend/src/features/api/gen/calendar/calendar.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/calendar_add_event_request_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/calendar_add_event_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/calendar_conflicts_request_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/calendar_conflicts_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/calendar_conflicts_response_conflicts_item.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/calendar_list_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/calendar_list_response_calendars_item.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/calendar_rsvp_request_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/calendar_rsvp_response.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/response_enum.tsis excluded by!**/gen/**
📒 Files selected for processing (5)
src/backend/core/api/openapi.jsonsrc/backend/core/services/calendar/service.pysrc/backend/core/tests/api/test_calendar.pysrc/backend/core/tests/api/test_messages_import.pysrc/backend/core/tests/importer/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/hooks/use-task-status.ts (1)
16-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset retry/poll state when
taskIdchanges.The hook can carry exhausted state across different tasks in the same component instance. A new
taskIdmay immediately appear failed or never poll.🔧 Suggested fix
export function useTaskStatus( taskId: string | null, @@ ) { const { t } = useTranslation(); const [queryEnabled, setQueryEnabled] = useState(enabled); const [hasExhaustedRetries, setHasExhaustedRetries] = useState(false); const errorCountRef = useRef(0); + + useEffect(() => { + errorCountRef.current = 0; + setHasExhaustedRetries(false); + setQueryEnabled(enabled); + }, [taskId, enabled]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/hooks/use-task-status.ts` around lines 16 - 35, The hook useTaskStatus is not resetting its retry/poll state when taskId changes, causing hasExhaustedRetries and errorCountRef to leak between tasks; add a useEffect that runs when taskId changes to reset errorCountRef.current = 0, call setHasExhaustedRetries(false) and restore setQueryEnabled(enabled) (and any other per-task state) so each new taskId starts with a clean polling/retry state; reference useTaskStatus, taskId, errorCountRef, setHasExhaustedRetries and setQueryEnabled when making the change.
♻️ Duplicate comments (1)
src/backend/core/services/calendar/service.py (1)
360-380:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftUID must be validated or percent-encoded before URL construction.
The UID is extracted from caller-supplied ICS (line 365) and concatenated directly into
event_url(line 372) without validation or encoding. A malicious UID containing path delimiters (/,..), query/fragment separators (?,#), percent-encoded sequences (%2e%2e), or CRLF (\r\n) can escape the validatedcalendar_url, redirect to unintended endpoints, or inject headers into the HTTP request.🔒 Recommended hardening
def _put_event(self, calendar_url, ics_data): uid = "" try: cal = ICalendar.from_ical(ics_data) for comp in cal.walk("VEVENT"): uid = str(comp.get("UID") or "") break except Exception: # pylint: disable=broad-exception-caught logger.debug("Could not extract UID from ICS, using random", exc_info=True) if not uid: uid = str(uuid.uuid4()) + + # Reject or sanitize UIDs with path delimiters, CRLF, or other unsafe chars. + # CalDAV UIDs are typically stable identifiers (e.g., UUID@domain); anything + # unusual should be replaced to prevent path traversal or header injection. + import re + if not re.fullmatch(r"[A-Za-z0-9._@+\-]{1,255}", uid): + logger.warning("UID contains unsafe characters, replacing: %s", uid) + uid = str(uuid.uuid4()) - event_url = calendar_url.rstrip("/") + f"/{uid}.ics" + from urllib.parse import quote + # Percent-encode the UID as a single path segment (no "/" in safe set). + event_url = calendar_url.rstrip("/") + "/" + quote(uid, safe="") + ".ics"Based on learnings, logger.exception() will report to Sentry automatically via DjangoIntegration's default LoggingIntegration at event_level=logging.ERROR. The added logger.warning() will not trigger Sentry events unless log level is ERROR or above, so explicit capture_exception() is not needed here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/services/calendar/service.py` around lines 360 - 380, The UID extracted in _put_event is used directly in event_url and must be validated/encoded to prevent path traversal, query/fragment injection, CRLF injection, or malicious percent-encodings; update _put_event to sanitize uid after extraction (e.g., reject or normalize any control characters or sequences like CR/LF, remove or fail on path separators and "..", and ensure it contains only an allowed charset) and then percent-encode the sanitized uid (use urllib.parse.quote or equivalent) before concatenating with calendar_url to form event_url, or else raise/return an error when uid is invalid so you never build a URL from an unsafe UID.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/api/viewsets/calendar.py`:
- Line 298: The expression assigns web_url using getattr(settings,
"CALDAV_WEB_URL", None) or None which is redundant; remove the trailing "or
None" and simply set web_url = getattr(settings, "CALDAV_WEB_URL", None) (refer
to the web_url assignment and getattr(settings, "CALDAV_WEB_URL", None) in
calendar.py) to simplify the code.
In `@src/frontend/public/locales/common/en-US.json`:
- Line 637: The singular pluralization variant key "{{count}} messages were
imported before the error._one" has incorrect grammar ("message were"); update
its value to use correct singular verb tense, e.g. change the string to
"{{count}} message was imported before the error." so the _one variant reads
properly.
In
`@src/frontend/src/features/controlled-modals/message-importer/step-loader.tsx`:
- Around line 29-31: The code forces a non-null assertion on useTaskStatus which
can return null; update the importStatus handling in step-loader.tsx (the
useTaskStatus call and any subsequent reads of importStatus) to guard against a
null return instead of using the `!` operator: call useTaskStatus(taskId, ...)
without `!`, check if importStatus is null and handle it (e.g., render a
loading/placeholder or an error UI) before accessing properties like
importStatus.status or importStatus.error, ensuring all uses of importStatus are
null-safe.
In
`@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scss`:
- Line 89: Replace the deprecated CSS declaration "word-break: break-word" in
_index.scss with the modern equivalent by removing that property and adding
"overflow-wrap: break-word" (optionally keep "word-wrap: break-word" for legacy
support) within the same selector where "word-break: break-word" appears to
ensure proper wrapping and cross-browser compatibility.
In
`@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/calendar-select.tsx`:
- Around line 44-59: The label is currently cast to any which weakens type
safety; update the options creation in the useMemo (the calendars -> optionJsx
block) to give label a proper React type instead of any—e.g., type it as
React.ReactNode or JSX.Element (or define the options array generically to match
Cunningham's Select label type) and remove the eslint-disable; keep render
returning the JSX element (CalendarOptionItem) and ensure the options variable
signature reflects the chosen label type so TypeScript enforces correct usage.
---
Outside diff comments:
In `@src/frontend/src/hooks/use-task-status.ts`:
- Around line 16-35: The hook useTaskStatus is not resetting its retry/poll
state when taskId changes, causing hasExhaustedRetries and errorCountRef to leak
between tasks; add a useEffect that runs when taskId changes to reset
errorCountRef.current = 0, call setHasExhaustedRetries(false) and restore
setQueryEnabled(enabled) (and any other per-task state) so each new taskId
starts with a clean polling/retry state; reference useTaskStatus, taskId,
errorCountRef, setHasExhaustedRetries and setQueryEnabled when making the
change.
---
Duplicate comments:
In `@src/backend/core/services/calendar/service.py`:
- Around line 360-380: The UID extracted in _put_event is used directly in
event_url and must be validated/encoded to prevent path traversal,
query/fragment injection, CRLF injection, or malicious percent-encodings; update
_put_event to sanitize uid after extraction (e.g., reject or normalize any
control characters or sequences like CR/LF, remove or fail on path separators
and "..", and ensure it contains only an allowed charset) and then
percent-encode the sanitized uid (use urllib.parse.quote or equivalent) before
concatenating with calendar_url to form event_url, or else raise/return an error
when uid is invalid so you never build a URL from an unsafe UID.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85d8a5d1-d6bf-4457-9d5e-67f2c8f6f09c
⛔ Files ignored due to path filters (1)
src/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
src/backend/core/api/viewsets/calendar.pysrc/backend/core/services/calendar/service.pysrc/backend/core/tests/api/test_calendar.pysrc/backend/messages/settings.pysrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/controlled-modals/message-importer/step-loader.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/calendar-helper.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/calendar-select.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/event-display.test.tssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/event-display.tssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-footer.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/types.tssrc/frontend/src/features/ui/components/contact-chip/index.tsxsrc/frontend/src/hooks/use-task-status.ts
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/tests/api/test_calendar.py (1)
836-1001: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a regression test for
_put_event()path escaping.This helper coverage stops at calendar selection. Please add one test that feeds a
UIDcontaining path separators / traversal characters and asserts the final PUT target is percent-encoded (or rejected). That will lock the upload-path fix in place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/tests/api/test_calendar.py` around lines 836 - 1001, Add a regression test that exercises CalDAVService._put_event() with a UID containing path separators/traversal characters (e.g. "/" or "..") and assert that the final HTTP PUT target used by _put_event is percent-encoded (or that the call is rejected with CalDAVError); specifically, create a test that constructs a CalDAVService (or uses the existing test helpers), stub/spy the service._request or service._put method to capture the request URL when calling service._put_event(uid=bad_uid, ...) and then assert the captured URL does not contain raw separators (it contains percent-encoding) or that _put_event raises for malformed UIDs, ensuring the upload-path escaping fix is covered.
♻️ Duplicate comments (2)
src/frontend/src/features/controlled-modals/message-importer/step-loader.tsx (1)
29-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the forced non-null assertion on task status.
useTaskStatusis treated as nullable elsewhere (importStatus?.state), but!makes render-time reads unsafe (renderProgressText,ProgressBar,importStatus.state) if it returnsnull.Suggested fix
- const importStatus = useTaskStatus(taskId, { + const importStatus = useTaskStatus(taskId, { exhaustedError: t('An error occurred while importing messages.'), - })!; + }); + + if (!importStatus) return null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/features/controlled-modals/message-importer/step-loader.tsx` around lines 29 - 31, Remove the forced non-null assertion on the result of useTaskStatus (the importStatus variable) and make all accesses to its properties safe: delete the trailing `!` on the call to useTaskStatus and update any direct reads like `importStatus.state` to use optional chaining or guard early (e.g., return a loading/null state if importStatus is null) so usages in renderProgressText, ProgressBar, and anywhere else referencing importStatus are null-safe; ensure taskId is still passed into useTaskStatus and that components receive importStatus?.state or are not rendered until importStatus is non-null.src/backend/core/services/calendar/service.py (1)
360-379:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEncode or reject the extracted
UIDbefore building the PUT URL.This is still concatenating attacker-controlled ICS content into the request path verbatim. A
UIDcontaining/,..,?,#, or control characters can escape the selected calendar collection or target a different resource.🔒 Proposed hardening
+from urllib.parse import quote @@ - event_url = calendar_url.rstrip("/") + f"/{uid}.ics" + event_url = calendar_url.rstrip("/") + "/" + quote(uid, safe="") + ".ics" data = ics_data.encode("utf-8") if isinstance(ics_data, str) else ics_dataYou may also want to reject obviously dangerous UIDs up front and fall back to a generated UUID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/core/services/calendar/service.py` around lines 360 - 379, In _put_event, the extracted UID is used directly to build event_url which allows path-traversal or URI injection; before concatenating, validate and sanitize uid by either rejecting UIDs containing unsafe characters (e.g. '/', '\\', '..', control chars, '?', '#') and falling back to a generated uuid, or percent-encoding the UID using a safe encoder (e.g. urllib.parse.quote) so reserved/path characters are escaped; ensure the sanitized/encoded uid is used when creating event_url and keep the fallback behavior to uuid.uuid4() for any rejected or empty UIDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/core/api/viewsets/calendar.py`:
- Around line 241-250: After parsing start and end in the calendar view (the
block that converts ISO strings to datetimes), add a validation that rejects
inverted ranges by checking if end <= start and returning a HTTP 400 Response;
update the same view/function handling conflicts (the variables start and end in
this try/except block inside the calendar viewset) to return Response({"detail":
"end must be after start."}, status=status.HTTP_400_BAD_REQUEST) when the range
is invalid so the request fails fast before any conflict querying.
- Around line 120-129: The Celery task enqueue calls (calendar_rsvp_task.delay)
are unguarded and may raise uncaught exceptions; wrap each .delay(...) call
(both the one before register_task_owner and the other occurrence at lines
176–181) in a try-except that catches Celery/broker/serialization errors, log
the exception, and return or raise a controlled API error response instead of
letting a 500 propagate; only call register_task_owner(task.id, request.user.id)
after the task is successfully created inside the try block so you don't
reference task when enqueue fails.
In `@src/backend/core/services/calendar/service.py`:
- Around line 65-70: The _request method currently calls self.session.request
directly so RequestException (e.g. timeouts, connection errors) can escape and
not be wrapped as CalDAVError; update _request to catch
requests.exceptions.RequestException around self.session.request and re-raise a
CalDAVError with context (include method, url and original exception) so callers
like check_conflicts() receive a CalDAVError; reference the _request method,
CalDAVError class, and session.request/RequestException when making the change.
In
`@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsx`:
- Around line 590-595: The RSVP/Add controls are interactive even when the raw
ICS query (useQuery returning icsContent) is loading or errored; gate their
enabled/visible state on the query status by checking icsContent (and query
error/loading) before rendering or enabling the buttons. Update the useQuery
call usage around downloadUrl/mailboxId and in renderFooter() treat isIcsError
or missing icsContent as a local-unavailable state so the RSVP/Add controls are
disabled/hidden (same fix for the other render blocks at the other ranges
mentioned) to prevent no-op clicks when the payload is absent.
- Around line 661-665: The logic currently treats isConflictsError as making the
entire CalDAV service unavailable via showUnavailable; change showUnavailable to
only consider isCalendarsError and isCalDAVEmpty (remove isConflictsError from
that expression) so calendar-loaded flows still render footer actions
(RSVP/Add), and handle conflictsResponse errors separately by surfacing a
non-blocking warning near the existing warning area (use the
conflicts/conflictsResponse and isConflictsError flags to render that warning).
Also update the same pattern around the other block referenced (the code at the
845-853 area) to ensure conflict-query failures do not hide the footer controls.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsx`:
- Around line 116-127: The calendar detection and deduplication is brittle:
CALENDAR_MIME_TYPES exact matching misses params like "text/calendar;
method=REQUEST" and deduping only by att.sha256 can incorrectly drop items when
sha256 is empty; update the logic that builds calendar/regular from
message.attachments to treat a MIME as calendar when its type
startsWith('text/calendar') or when parsing the media type ignores parameters,
and change the dedupe for uniqueCalendar to use a composite key (e.g.,
att.sha256 if present else a fallback combining att.name and att.size or
att.filename) so missing/empty sha256 doesn't collapse distinct files; adjust
references to CALENDAR_MIME_TYPES, message.attachments, and uniqueCalendar
accordingly.
---
Outside diff comments:
In `@src/backend/core/tests/api/test_calendar.py`:
- Around line 836-1001: Add a regression test that exercises
CalDAVService._put_event() with a UID containing path separators/traversal
characters (e.g. "/" or "..") and assert that the final HTTP PUT target used by
_put_event is percent-encoded (or that the call is rejected with CalDAVError);
specifically, create a test that constructs a CalDAVService (or uses the
existing test helpers), stub/spy the service._request or service._put method to
capture the request URL when calling service._put_event(uid=bad_uid, ...) and
then assert the captured URL does not contain raw separators (it contains
percent-encoding) or that _put_event raises for malformed UIDs, ensuring the
upload-path escaping fix is covered.
---
Duplicate comments:
In `@src/backend/core/services/calendar/service.py`:
- Around line 360-379: In _put_event, the extracted UID is used directly to
build event_url which allows path-traversal or URI injection; before
concatenating, validate and sanitize uid by either rejecting UIDs containing
unsafe characters (e.g. '/', '\\', '..', control chars, '?', '#') and falling
back to a generated uuid, or percent-encoding the UID using a safe encoder (e.g.
urllib.parse.quote) so reserved/path characters are escaped; ensure the
sanitized/encoded uid is used when creating event_url and keep the fallback
behavior to uuid.uuid4() for any rejected or empty UIDs.
In
`@src/frontend/src/features/controlled-modals/message-importer/step-loader.tsx`:
- Around line 29-31: Remove the forced non-null assertion on the result of
useTaskStatus (the importStatus variable) and make all accesses to its
properties safe: delete the trailing `!` on the call to useTaskStatus and update
any direct reads like `importStatus.state` to use optional chaining or guard
early (e.g., return a loading/null state if importStatus is null) so usages in
renderProgressText, ProgressBar, and anywhere else referencing importStatus are
null-safe; ensure taskId is still passed into useTaskStatus and that components
receive importStatus?.state or are not rendered until importStatus is non-null.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8d4514a-4b11-4d48-a702-6c74cfff378d
⛔ Files ignored due to path filters (1)
src/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
src/backend/core/api/viewsets/calendar.pysrc/backend/core/services/calendar/service.pysrc/backend/core/tests/api/test_calendar.pysrc/backend/messages/settings.pysrc/frontend/package.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/controlled-modals/message-importer/step-loader.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/calendar-helper.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/calendar-select.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/event-display.test.tssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/event-display.tssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-footer.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/types.tssrc/frontend/src/features/ui/components/contact-chip/index.tsxsrc/frontend/src/hooks/use-task-status.ts
sylvinus
force-pushed
the
caldav
branch
2 times, most recently
from
74b64b2 to
a0984ff
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
1 participant
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.
First step in the interop with https://github.com/suitenumerique/calendars
Summary by CodeRabbit
New Features
UX / Style
Behavior
Localization
Tests