diff --git a/src/interfaces/api/handlers/caldav_handler.rs b/src/interfaces/api/handlers/caldav_handler.rs index 485eaf71..e668acd3 100644 --- a/src/interfaces/api/handlers/caldav_handler.rs +++ b/src/interfaces/api/handlers/caldav_handler.rs @@ -248,7 +248,7 @@ async fn handle_propfind( calendar_service .list_my_calendars(user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to list calendars: {}", e)))? + .map_err(AppError::from)? }; let base_href = "/caldav/"; @@ -364,9 +364,7 @@ async fn handle_propfind( let calendars = calendar_service .list_my_calendars(user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to list calendars: {}", e)) - })?; + .map_err(AppError::from)?; let base_href = &format!("/caldav/{}/", first_segment); let mut response_body = Vec::new(); @@ -448,7 +446,7 @@ async fn handle_propfind( let event = calendar_service .get_event_by_ical_uid(calendar_id, ical_uid, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to look up event: {}", e)))? + .map_err(AppError::from)? .ok_or_else(|| AppError::not_found(format!("Event not found: {}", ical_uid)))?; let base_href = &format!("/caldav/{}/", calendar_id); @@ -505,16 +503,12 @@ async fn handle_report( calendar_service .get_events_in_range(calendar_id, *start, *end, user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to query events: {}", e)) - })? + .map_err(AppError::from)? } else { calendar_service .list_events(calendar_id, None, None, user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to list events: {}", e)) - })? + .map_err(AppError::from)? } } CalDavReportType::CalendarMultiget { hrefs, .. } => { @@ -529,12 +523,12 @@ async fn handle_report( calendar_service .get_events_by_ical_uids(calendar_id, &uids, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to fetch events: {}", e)))? + .map_err(AppError::from)? } CalDavReportType::SyncCollection { .. } => calendar_service .list_events(calendar_id, None, None, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to list events: {}", e)))?, + .map_err(AppError::from)?, }; let base_href = &format!("/caldav/{}/", calendar_id); @@ -693,12 +687,12 @@ async fn handle_get( let events = calendar_service .list_events(calendar_id, None, None, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to list events: {}", e)))?; + .map_err(AppError::from)?; let calendar = calendar_service .get_calendar(calendar_id, user.id) .await - .map_err(|e| AppError::not_found(format!("Calendar not found: {}", e)))?; + .map_err(AppError::from)?; let ical = generate_full_calendar_ical(&calendar.name, &events); @@ -716,7 +710,7 @@ async fn handle_get( let event = calendar_service .get_event_by_ical_uid(calendar_id, ical_uid, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to look up event: {}", e)))? + .map_err(AppError::from)? .ok_or_else(|| AppError::not_found(format!("Event not found: {}", ical_uid)))?; let ical = generate_event_ical(&event); @@ -809,7 +803,7 @@ async fn handle_delete( calendar_service .delete_calendar(calendar_id, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to delete calendar: {}", e)))?; + .map_err(AppError::from)?; } else { let event_file = parts[1]; let ical_uid = event_file.trim_end_matches(".ics"); @@ -818,13 +812,13 @@ async fn handle_delete( let event = calendar_service .get_event_by_ical_uid(calendar_id, ical_uid, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to look up event: {}", e)))? + .map_err(AppError::from)? .ok_or_else(|| AppError::not_found(format!("Event not found: {}", ical_uid)))?; calendar_service .delete_event(&event.id, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to delete event: {}", e)))?; + .map_err(AppError::from)?; } Ok(Response::builder() @@ -881,7 +875,7 @@ async fn handle_proppatch( calendar_service .update_calendar(calendar_id, update, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to update calendar: {}", e)))?; + .map_err(AppError::from)?; } let mut results = Vec::new(); diff --git a/src/interfaces/api/handlers/carddav_handler.rs b/src/interfaces/api/handlers/carddav_handler.rs index 50421e6c..b79be10b 100644 --- a/src/interfaces/api/handlers/carddav_handler.rs +++ b/src/interfaces/api/handlers/carddav_handler.rs @@ -254,9 +254,7 @@ async fn handle_propfind( addressbook_service .list_user_address_books(user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to list address books: {}", e)) - })? + .map_err(AppError::from)? }; let mut response_body = Vec::new(); @@ -307,9 +305,7 @@ async fn handle_propfind( let address_books = addressbook_service .list_user_address_books(user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to list address books: {}", e)) - })?; + .map_err(AppError::from)?; let user_part = path.split('/').next().unwrap_or(path); let base_href = format!("/carddav/{}/", user_part); @@ -373,7 +369,7 @@ async fn handle_propfind( let contact = contact_svc .get_contact_by_uid(address_book_id, contact_uid, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to look up contact: {}", e)))? + .map_err(AppError::from)? .ok_or_else(|| { AppError::not_found(format!("Contact not found: {}", contact_uid)) })?; @@ -432,7 +428,7 @@ async fn handle_report( CardDavReportType::AddressbookQuery { .. } => contact_svc .list_contacts(address_book_id, None, None, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?, + .map_err(AppError::from)?, CardDavReportType::AddressbookMultiget { hrefs, .. } => { // Indexed batch lookup (`uid = ANY(...)`) — a multiget for a // handful of contacts must not pay for listing the whole @@ -445,12 +441,12 @@ async fn handle_report( contact_svc .get_contacts_by_uids(address_book_id, &uids, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to fetch contacts: {}", e)))? + .map_err(AppError::from)? } CardDavReportType::SyncCollection { .. } => contact_svc .list_contacts(address_book_id, None, None, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?, + .map_err(AppError::from)?, }; // Generate vCards @@ -646,7 +642,7 @@ async fn handle_get( let contacts = contact_svc .list_contacts(address_book_id, None, None, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to list contacts: {}", e)))?; + .map_err(AppError::from)?; let mut vcf_data = String::new(); for contact in &contacts { @@ -666,7 +662,7 @@ async fn handle_get( let contact = contact_svc .get_contact_by_uid(address_book_id, contact_uid, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to look up contact: {}", e)))? + .map_err(AppError::from)? .ok_or_else(|| AppError::not_found(format!("Contact not found: {}", contact_uid)))?; let vcard = contact_to_vcard(&contact); @@ -704,9 +700,7 @@ async fn handle_delete( addressbook_service .delete_address_book(address_book_id, user.id) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to delete address book: {}", e)) - })?; + .map_err(AppError::from)?; } else { // Delete contact — indexed lookup by vCard UID. let contact_file = parts[1]; @@ -715,13 +709,13 @@ async fn handle_delete( let contact = contact_svc .get_contact_by_uid(address_book_id, contact_uid, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to look up contact: {}", e)))? + .map_err(AppError::from)? .ok_or_else(|| AppError::not_found(format!("Contact not found: {}", contact_uid)))?; contact_svc .delete_contact(&contact.id, user.id) .await - .map_err(|e| AppError::internal_error(format!("Failed to delete contact: {}", e)))?; + .map_err(AppError::from)?; } Ok(Response::builder() @@ -779,9 +773,7 @@ async fn handle_proppatch( addressbook_service .update_address_book(address_book_id, update) .await - .map_err(|e| { - AppError::internal_error(format!("Failed to update address book: {}", e)) - })?; + .map_err(AppError::from)?; } let mut results = Vec::new(); diff --git a/tests/api/dav_error_mapping.hurl b/tests/api/dav_error_mapping.hurl index d89eab4f..1815397f 100644 --- a/tests/api/dav_error_mapping.hurl +++ b/tests/api/dav_error_mapping.hurl @@ -205,3 +205,189 @@ HTTP * [Asserts] status >= 200 status < 300 + + +# ───────────────────────────────────────────────────────────── +# Cross-user AuthZ mapping (fix/caldav-carddav-error-mapping) +# ───────────────────────────────────────────────────────────── +# Regression pin for the second half of the CalDAV/CardDAV +# error-mapping sweep: EVERY handler used to +# `map_err(|e| AppError::internal_error(format!("Failed to ...: {}", e)))`, +# turning a domain-layer `NotFound` (which is what AuthZ returns +# for anti-enum on denied resources) into a 500 InternalError. +# +# Symptom: PROPPATCH / DELETE on a calendar the caller has no +# permission on returned 500 with the calendar UUID leaked in +# the body; on-call metrics tripped for benign perm denials. +# +# Fix: `.map_err(AppError::from)` — the kind-aware mapping via +# `From for AppError` routes NotFound → 404. +# +# Provision a second user (Alice), have her hit admin's default +# calendar + address book across the four verbs. Every response +# MUST be a 4xx client error, NOT a 5xx server error. We don't +# assert an exact 404 in every case because some paths naturally +# return 403 or 401 depending on the auth stack; the invariant +# the fix defends is "never 5xx for a perm denial". +# ───────────────────────────────────────────────────────────── + + +# ───────────────────────────────────────────────────────────── +# Step 8 — Provision + log in Alice (a distinct throwaway user). +# HTTP * on the create because a re-run inside the same DB will +# hit 409 Conflict; login is the actual precondition. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ + "username": "dav-err-alice", + "password": "DavErrAlicePassword1!", + "email": "dav-err-alice@example.com", + "role": "user" +} + +HTTP * +[Captures] +alice_id: jsonpath "$.id" + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ + "username": "dav-err-alice", + "password": "DavErrAlicePassword1!" +} + +HTTP 200 +[Captures] +alice_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 9 — Alice PROPPATCH on admin's default calendar. +# Pre-fix: 500 InternalError with "Failed to update calendar: +# Not Found: Calendar not found: " in the body. +# Post-fix: 4xx (typically 404 anti-enum from `authz.require`). +# ───────────────────────────────────────────────────────────── +PROPPATCH {{base_url}}/caldav/{{default_calendar_id}}/ +Authorization: Bearer {{alice_token}} +Content-Type: application/xml +``` + + + + + hijacked + + + +``` + +HTTP * +[Asserts] +status >= 400 +status < 500 + + +# ───────────────────────────────────────────────────────────── +# Step 10 — Alice DELETE on admin's default calendar. Same +# invariant — 4xx, never 5xx. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/caldav/{{default_calendar_id}}/ +Authorization: Bearer {{alice_token}} + +HTTP * +[Asserts] +status >= 400 +status < 500 + + +# ───────────────────────────────────────────────────────────── +# Step 11 — Alice DELETE on the well-formed event Step 4 created +# in admin's calendar. Pre-fix: 500 on the lookup or delete step. +# Post-fix: 4xx via NotFound anti-enum. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/caldav/{{default_calendar_id}}/dav-error-test-ok.ics +Authorization: Bearer {{alice_token}} + +HTTP * +[Asserts] +status >= 400 +status < 500 + + +# ───────────────────────────────────────────────────────────── +# Step 12 — Alice PROPPATCH on admin's default address book. +# Mirror of Step 9 on the CardDAV side. Pre-fix: 500. Post-fix: 4xx. +# ───────────────────────────────────────────────────────────── +PROPPATCH {{base_url}}/carddav/{{default_book_id}}/ +Authorization: Bearer {{alice_token}} +Content-Type: application/xml +``` + + + + + hijacked + + + +``` + +HTTP * +[Asserts] +status >= 400 +status < 500 + + +# ───────────────────────────────────────────────────────────── +# Step 13 — Alice DELETE on admin's default address book. +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/carddav/{{default_book_id}}/ +Authorization: Bearer {{alice_token}} + +HTTP * +[Asserts] +status >= 400 +status < 500 + + +# ───────────────────────────────────────────────────────────── +# Step 14 — Sanity: admin's own PROPPATCH still succeeds. Guards +# against a fix that over-corrects and starts denying legitimate +# writes. `HTTP *` because PROPPATCH multi-status can be 207 or +# 200 depending on the property set; we assert the negative +# invariant (no 4xx/5xx). +# ───────────────────────────────────────────────────────────── +PROPPATCH {{base_url}}/caldav/{{default_calendar_id}}/ +Authorization: Bearer {{admin_token}} +Content-Type: application/xml +``` + + + + + Personal (renamed by sanity step) + + + +``` + +HTTP * +[Asserts] +status >= 200 +status < 400 + + +# ───────────────────────────────────────────────────────────── +# Step 15 — Cleanup: delete Alice so downstream test files don't +# inherit an extra user (per feedback_hurl_teardown_shared_db — +# state carries across the run.sh invocation). +# ───────────────────────────────────────────────────────────── +DELETE {{base_url}}/api/admin/users/{{alice_id}} +Authorization: Bearer {{admin_token}} + +HTTP * +[Asserts] +status < 500