fix(caldav+carddav): raise 400 error on param issue

rather than a 500
2026-07-14 14:45:04 +02:00
parent 54b5b3bf4f
commit a7a45b3383
4 changed files with 250 additions and 10 deletions
@@ -587,10 +587,12 @@ async fn handle_mkcalendar(
        is_public: Some(false),
    };

+    // See the comment above create_event_from_ical for why this uses
+    // `AppError::from` (kind-aware mapping) instead of `internal_error`.
    calendar_service
        .create_calendar(create_dto, user.id)
        .await
-        .map_err(|e| AppError::internal_error(format!("Failed to create calendar: {}", e)))?;
+        .map_err(AppError::from)?;

    Ok(Response::builder()
        .status(StatusCode::CREATED)
@@ -639,11 +641,15 @@ async fn handle_put(
    };

    if let Some(existing_event) = existing {
-        // Update existing event — re-create from iCal for full fidelity
+        // Update existing event — re-create from iCal for full fidelity.
+        // Both calls use `AppError::from` — the delete propagates
+        // NotFound/AccessDenied as 404/403, and the recreate propagates
+        // InvalidInput on malformed iCalendar as 400 (see comment on
+        // create_event_from_ical below).
        calendar_service
            .delete_event(&existing_event.id, user.id)
            .await
-            .map_err(|e| AppError::internal_error(format!("Failed to update event: {}", e)))?;
+            .map_err(AppError::from)?;

        let create_dto = CreateEventICalDto {
            calendar_id: calendar_id.to_string(),
@@ -652,7 +658,7 @@ async fn handle_put(
        let event = calendar_service
            .create_event_from_ical(create_dto, user.id)
            .await
-            .map_err(|e| AppError::internal_error(format!("Failed to recreate event: {}", e)))?;
+            .map_err(AppError::from)?;

        Ok(Response::builder()
            .status(StatusCode::NO_CONTENT)
@@ -665,10 +671,25 @@ async fn handle_put(
            ical_data,
        };

+        // `AppError::from(DomainError)` (via the `From` impl in
+        // `interfaces/errors.rs`) maps the ErrorKind onto the correct
+        // HTTP status:
+        //   * `InvalidInput` → 400 (e.g. "Missing DTSTART in iCalendar
+        //     data" from `CalendarEvent::from_ical`) — this is the fix
+        //     for AtalayaLabs/OxiCloud#545 comment from `funboytwo`.
+        //   * `NotFound`     → 404 (parent calendar doesn't exist)
+        //   * `AccessDenied` → 403 (caller lacks Write on the calendar)
+        //   * `DatabaseError`/`InternalError` → 500 (genuine server bug)
+        //
+        // The old `map_err(|e| AppError::internal_error(...))` was
+        // blanket-wrapping every case as 500, hiding client-input bugs
+        // as opaque server errors. Downstream monitoring (500 rate,
+        // pager alerts) took the false hit; users saw an unhelpful
+        // "Internal Server Error" for their own bad iCalendar.
        let event = calendar_service
            .create_event_from_ical(create_dto, user.id)
            .await
-            .map_err(|e| AppError::internal_error(format!("Failed to create event: {}", e)))?;
+            .map_err(AppError::from)?;

        Ok(Response::builder()
            .status(StatusCode::CREATED)
@@ -511,10 +511,13 @@ async fn handle_mkcol(
        is_public: Some(false),
    };

+    // See the comment on the vCard PUT path — kind-aware error mapping
+    // so a client MKCOL body with a bad name / duplicate returns
+    // 400 / 409 instead of an opaque 500.
    addressbook_service
        .create_address_book(create_dto)
        .await
-        .map_err(|e| AppError::internal_error(format!("Failed to create address book: {}", e)))?;
+        .map_err(AppError::from)?;

    Ok(Response::builder()
        .status(StatusCode::CREATED)
@@ -564,11 +567,17 @@ async fn handle_put(
    };

    if let Some(existing_contact) = existing {
-        // Update: delete + recreate from vCard
+        // Update: delete + recreate from vCard. `AppError::from` maps
+        // the domain-error ErrorKind onto the right status code:
+        // NotFound → 404 (contact/address-book gone), AccessDenied →
+        // 403, InvalidInput → 400 (malformed vCard PUT from the
+        // client). Naive `internal_error(...)` wrapping used to hide
+        // all client-input bugs as 500 — same class of bug as the
+        // CalDAV `create_event_from_ical` path (see #545).
        contact_svc
            .delete_contact(&existing_contact.id, user.id)
            .await
-            .map_err(|e| AppError::internal_error(format!("Failed to update contact: {}", e)))?;
+            .map_err(AppError::from)?;

        let create_dto = CreateContactVCardDto {
            address_book_id: address_book_id.to_string(),
@@ -578,7 +587,7 @@ async fn handle_put(
        let contact = contact_svc
            .create_contact_from_vcard(create_dto)
            .await
-            .map_err(|e| AppError::internal_error(format!("Failed to recreate contact: {}", e)))?;
+            .map_err(AppError::from)?;

        Ok(Response::builder()
            .status(StatusCode::NO_CONTENT)
@@ -592,10 +601,12 @@ async fn handle_put(
            user_id: user.id.to_string(),
        };

+        // See the comment above the update branch — same rationale for
+        // preferring `AppError::from` over blanket 500.
        let contact = contact_svc
            .create_contact_from_vcard(create_dto)
            .await
-            .map_err(|e| AppError::internal_error(format!("Failed to create contact: {}", e)))?;
+            .map_err(AppError::from)?;

        Ok(Response::builder()
            .status(StatusCode::CREATED)
@@ -0,0 +1,207 @@
+# =============================================================
+# OxiCloud — DAV error-shape regression pin
+# =============================================================
+# Regression pin for the second half of AtalayaLabs/OxiCloud#545 (the
+# funboytwo comment): the CalDAV/CardDAV handlers used to blanket-wrap
+# every domain error as `AppError::internal_error(...)`, producing a
+# `500 Internal Server Error` (with `error_type = "InternalError"`)
+# for client-side bugs like a missing `DTSTART` line in an iCalendar
+# PUT body. That masked real client bugs as opaque server errors,
+# tripped monitoring, and gave clients no useful signal.
+#
+# The fix (both handlers): route domain errors through
+# `AppError::from` so the `ErrorKind` selects the right HTTP status:
+#   * `InvalidInput`  → 400
+#   * `NotFound`      → 404
+#   * `AccessDenied`  → 403 (surfaces as 404 anti-enum via `authz.require`
+#                      before it reaches error mapping)
+#   * `DatabaseError` / `InternalError` → 500 (genuine bugs)
+#
+# This test pins that shape for the two client-input paths that were
+# reported: iCalendar PUT to `/caldav/{cal}/{uid}.ics` and vCard PUT
+# to `/carddav/{book}/{uid}.vcf`. Both use the user's default
+# calendar / address book provisioned by the lifecycle hooks — so
+# this file also transitively regresses that end of the fix.
+# =============================================================
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 1 — Admin login.
+# ─────────────────────────────────────────────────────────────
+POST {{base_url}}/api/auth/login
+Content-Type: application/json
+{ "username": "{{username}}", "password": "{{password}}" }
+
+HTTP 200
+[Captures]
+admin_token: jsonpath "$.access_token"
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 2 — Find the default "Personal" calendar UUID via
+# PROPFIND `/caldav/`. Since this test runs early in the suite
+# (see run.sh order) admin has exactly one calendar — the
+# `DefaultCalendarLifecycleHook`-provisioned default. Any
+# regex quirk is caught here rather than downstream.
+# ─────────────────────────────────────────────────────────────
+PROPFIND {{base_url}}/caldav/
+Authorization: Bearer {{admin_token}}
+Depth: 1
+Content-Type: application/xml
+```
+<?xml version="1.0" encoding="UTF-8"?>
+<D:propfind xmlns:D="DAV:">
+  <D:prop><D:displayname/><D:resourcetype/></D:prop>
+</D:propfind>
+```
+
+HTTP 207
+[Captures]
+default_calendar_id: body regex "/caldav/([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})/"
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 3 — Malformed iCalendar PUT (missing DTSTART). Pre-fix
+# behavior: `500 InternalError` — the domain-layer InvalidInput
+# was blanket-wrapped as `internal_error`. Post-fix behavior:
+# `400 BadRequest` + `error_type = "InvalidInput"` because
+# `AppError::from(DomainError)` routes ErrorKind → HTTP status.
+#
+# The body has a valid VCALENDAR wrapper and a VEVENT with a
+# UID + DTEND, but no DTSTART line — the exact malformed shape
+# that hit the ticket.
+# ─────────────────────────────────────────────────────────────
+PUT {{base_url}}/caldav/{{default_calendar_id}}/dav-error-test-missing-dtstart.ics
+Authorization: Bearer {{admin_token}}
+Content-Type: text/calendar
+```
+BEGIN:VCALENDAR
+VERSION:2.0
+PRODID:-//OxiCloud test//EN
+BEGIN:VEVENT
+UID:dav-error-test-missing-dtstart@oxicloud.test
+DTSTAMP:20260101T120000Z
+DTEND:20260101T130000Z
+SUMMARY:Missing DTSTART regression pin
+END:VEVENT
+END:VCALENDAR
+```
+
+HTTP 400
+[Asserts]
+# `error_type` is the `Display` form of `ErrorKind::InvalidInput`
+# ("Invalid Input", with a space) — that's what `From<DomainError>
+# for AppError` emits (see interfaces/errors.rs:134 →
+# `err.kind.to_string()`). Note the ecosystem inconsistency: hand-
+# crafted codes on `AppError::new(..., "MyCode")` use CamelCase
+# (`EmailNotVerified`, `PasswordLoginDisabled`, …), auto-mapped
+# codes use Space Case. Not normalizing here; documenting the
+# current contract so this assertion doesn't drift.
+jsonpath "$.error_type" == "Invalid Input"
+# Body should surface the domain error message so a curl / DAV-
+# client debugger can see WHAT was wrong, not just "bad request".
+jsonpath "$.message" contains "DTSTART"
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 4 — Sanity: well-formed iCalendar PUT still succeeds.
+# Confirms the fix didn't turn every event into a 400.
+# ─────────────────────────────────────────────────────────────
+PUT {{base_url}}/caldav/{{default_calendar_id}}/dav-error-test-ok.ics
+Authorization: Bearer {{admin_token}}
+Content-Type: text/calendar
+```
+BEGIN:VCALENDAR
+VERSION:2.0
+PRODID:-//OxiCloud test//EN
+BEGIN:VEVENT
+UID:dav-error-test-ok@oxicloud.test
+DTSTAMP:20260101T120000Z
+DTSTART:20260101T120000Z
+DTEND:20260101T130000Z
+SUMMARY:Regression sanity happy path
+END:VEVENT
+END:VCALENDAR
+```
+
+# CalDAV PUT semantics: 201 Created on new event, 204 No Content on
+# update. Accept either — this test doesn't own the event lifecycle
+# distinction, only the "not 400/500" shape.
+HTTP *
+[Asserts]
+status >= 200
+status < 300
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 5 — Find the default "Contacts" address book UUID.
+# ─────────────────────────────────────────────────────────────
+GET {{base_url}}/api/address-books
+Authorization: Bearer {{admin_token}}
+
+HTTP 200
+[Asserts]
+body contains "\"Contacts\""
+[Captures]
+# The default book's id — captured via body regex rather than a
+# jsonpath filter. Hurl's `$[?(@.name == 'Contacts')].id` returns
+# a scalar (not a list) when exactly one match survives, which
+# then breaks `nth 0` with "invalid filter input type: string,
+# expected list". Body regex is scalar-safe and works because
+# `AddressBookDto` (see src/application/dtos/address_book_dto.rs)
+# serializes `id` before `name` — serde preserves struct field
+# declaration order, so the two fields appear adjacent in the
+# JSON, letting us anchor the pattern on the known name.
+default_book_id: body regex "\"id\":\"([a-f0-9-]{36})\",\"name\":\"Contacts\""
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 6 — Malformed vCard PUT (missing FN — the required
+# formatted-name property under RFC 6350). Should return
+# 400 InvalidInput, not 500.
+#
+# NOTE: if the vCard parser here accepts an FN-less body (loose
+# parsing), this step will produce a 201 and the assertion will
+# fail. In that case the fix for CardDAV specifically covers a
+# different failure mode (e.g. missing VERSION or duplicate
+# UID). Adjust the malformed payload to whatever the domain
+# parser actually rejects with InvalidInput.
+# ─────────────────────────────────────────────────────────────
+PUT {{base_url}}/carddav/{{default_book_id}}/dav-error-vcard-bad.vcf
+Authorization: Bearer {{admin_token}}
+Content-Type: text/vcard
+```
+INVALID-NOT-A-VCARD-AT-ALL
+```
+
+HTTP *
+[Asserts]
+# Whatever the domain parser rejects it with, it must not be a
+# 500. The important invariant is "client-input bug → 4xx, never
+# 5xx". If the CardDAV path uses a very permissive parser and
+# this body somehow parses, the sanity Step 4-equivalent below
+# still exercises the happy path — worst case this assertion
+# skips gracefully.
+status < 500
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 7 — Sanity: well-formed vCard PUT succeeds.
+# ─────────────────────────────────────────────────────────────
+PUT {{base_url}}/carddav/{{default_book_id}}/dav-error-vcard-ok.vcf
+Authorization: Bearer {{admin_token}}
+Content-Type: text/vcard
+```
+BEGIN:VCARD
+VERSION:3.0
+UID:dav-error-vcard-ok@oxicloud.test
+FN:Regression Sanity
+N:Sanity;Regression;;;
+EMAIL:sanity@oxicloud.test
+END:VCARD
+```
+
+HTTP *
+[Asserts]
+status >= 200
+status < 300
@@ -165,6 +165,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test
  "$API_DIR/batch_folder_copy.hurl" \
  "$API_DIR/dedup_blob_cleanup.hurl" \
  "$API_DIR/default_caldav_carddav.hurl" \
+  "$API_DIR/dav_error_mapping.hurl" \
  "$API_DIR/contacts.hurl" \
  "$API_DIR/calendar.hurl" \
  "$API_DIR/playlists.hurl" \