security: medication/appointment write + delete paths don't check ownership #12

New issue

Closed

opened 2026-07-19 10:30:50 +00:00 by alvaro · 0 comments

alvaro commented

2026-07-19 10:30:50 +00:00

Owner

Summary

Any authenticated user can update or delete any other user's medication or appointment by id. The update_* and delete_* handlers take the auth Claims but don't use them — there is no ownership check before mutating the document.

Surfaced while wiring the Phase B share-gate into the read paths (PR #11). Filing separately because it's a pre-existing security gap, independent of the sharing work.

Evidence

In backend/src/handlers/medications.rs and backend/src/handlers/appointments.rs, the write handlers bind _claims (underscore-prefixed = unused):

pub async fn update_medication(
    State(state): State<AppState>,
    Extension(_claims): Extension<Claims>,   // <-- unused
    Path(id): Path<String>,
    Json(req): Json<UpdateMedicationRequest>,
) -> ... {
    let repo = MedicationRepository::new(database.collection("medications"));
    match repo.update_by_medication_id(&id, req).await {   // <-- no owner check
        ...
    }
}

Same shape for delete_medication, update_appointment, delete_appointment. A logged-in user who guesses/learns another user's medication_id (a UUID, but exposed in API responses) can rewrite or remove it.

For contrast, the read paths now check ownership-or-share via authorize_profile_read (landed in PR #11, Phase B). The write paths were left unchecked because writes-by-recipients are explicitly out of scope for Phase B (recipients are read-only), but that decision left the owner-vs-other-owner hole unaddressed too.

Also affected

log_dose (medications) — same pattern; needs an ownership check on the parent medication.
Health-stat update_health_stat / delete_health_stat — update checks user_id via a direct comparison in the handler (not the repo), so it may be OK; delete should be audited. Worth confirming all four data types uniformly.

Fix

Mirror the read-path gate but owner-only (not share-aware, since recipients are read-only):

// Before mutating, confirm the caller owns the item's profile.
let item = repo.find_by_medication_id(&id).await?.ok_or(404)?;
authorize_profile_read(&state, &claims, &item.profile_id).await?;
// then proceed with the mutation

authorize_profile_read admits owners (and share recipients); to enforce owner-only on writes, either add an authorize_profile_write variant that rejects shares, or check item.user_id == claims.sub directly. The latter is simpler and unambiguous for write paths.

Severity

Confidentiality: data is end-to-end encrypted, so an attacker can't read plaintext via this hole — but they can destroy or corrupt another user's encrypted records (integrity/availability), and since dose logs drive adherence stats, deletion could silently skew a patient's medication-adherence history.
Scope: any authenticated user → any other user's meds/appointments.
This is the kind of bug a security review or pentest would flag as a high-severity IDOR (insecure direct object reference).

Suggested priority

High. Small, localized fix (a few lines per handler). Should land before any production deployment.

Labels: bug.

## Summary Any authenticated user can **update or delete** any other user's medication or appointment by id. The `update_*` and `delete_*` handlers take the auth `Claims` but don't use them — there is no ownership check before mutating the document. Surfaced while wiring the Phase B share-gate into the read paths (PR #11). Filing separately because it's a pre-existing security gap, independent of the sharing work. ## Evidence In `backend/src/handlers/medications.rs` and `backend/src/handlers/appointments.rs`, the write handlers bind `_claims` (underscore-prefixed = unused): ```rust pub async fn update_medication( State(state): State<AppState>, Extension(_claims): Extension<Claims>, // <-- unused Path(id): Path<String>, Json(req): Json<UpdateMedicationRequest>, ) -> ... { let repo = MedicationRepository::new(database.collection("medications")); match repo.update_by_medication_id(&id, req).await { // <-- no owner check ... } } ``` Same shape for `delete_medication`, `update_appointment`, `delete_appointment`. A logged-in user who guesses/learns another user's `medication_id` (a UUID, but exposed in API responses) can rewrite or remove it. For contrast, the **read** paths now check ownership-or-share via `authorize_profile_read` (landed in PR #11, Phase B). The write paths were left unchecked because writes-by-recipients are explicitly out of scope for Phase B (recipients are read-only), but that decision left the *owner-vs-other-owner* hole unaddressed too. ## Also affected - `log_dose` (medications) — same pattern; needs an ownership check on the parent medication. - Health-stat `update_health_stat` / `delete_health_stat` — `update` checks `user_id` via a direct comparison in the handler (not the repo), so it may be OK; `delete` should be audited. Worth confirming all four data types uniformly. ## Fix Mirror the read-path gate but **owner-only** (not share-aware, since recipients are read-only): ```rust // Before mutating, confirm the caller owns the item's profile. let item = repo.find_by_medication_id(&id).await?.ok_or(404)?; authorize_profile_read(&state, &claims, &item.profile_id).await?; // then proceed with the mutation ``` `authorize_profile_read` admits owners (and share recipients); to enforce owner-only on writes, either add an `authorize_profile_write` variant that rejects shares, or check `item.user_id == claims.sub` directly. The latter is simpler and unambiguous for write paths. ## Severity - **Confidentiality**: data is end-to-end encrypted, so an attacker can't *read* plaintext via this hole — but they can destroy or corrupt another user's encrypted records (integrity/availability), and since dose logs drive adherence stats, deletion could silently skew a patient's medication-adherence history. - **Scope**: any authenticated user → any other user's meds/appointments. - This is the kind of bug a security review or pentest would flag as a high-severity IDOR (insecure direct object reference). ## Suggested priority High. Small, localized fix (a few lines per handler). Should land before any production deployment. Labels: `bug`.