fix(security): enforce ownership on medication/appointment write paths (#12) #13

Merged

alvaro merged 1 commit from fix/12-write-path-ownership into main

2026-07-19 15:20:15 +00:00

alvaro commented

2026-07-19 15:00:57 +00:00

Owner

Summary

Closes #12. The update/delete handlers for medications and appointments took the auth Claims but never used them — any authenticated user could update or delete any other user's records by id (an IDOR / insecure direct object reference). Same gap on log_dose (could skew another user's adherence stats) and get_adherence (leaked dose history).

Surfaced while wiring the Phase B share-gate into the read paths (PR #11); filed separately as a security item.

Fix

Each affected handler now looks up the item first and confirms user_id == claims.sub before mutating or returning. Mismatches return 404 (not 403) to avoid leaking the existence of other users' records. Recipients of shared profiles remain read-only per the ADR — writes stay owner-only.

Covered handlers:

update_medication, delete_medication, log_dose, get_adherence
update_appointment, delete_appointment

health_stats was already checking user_id == claims.sub on update and delete — unaffected.

Audit method

Grepped every handler taking Extension<Claims> / Extension(_claims)>; the _claims (underscore-prefixed = unused) ones were the red flags. Confirmed each write/delete path's actual behavior, not just the signature.

Tests

New ownership_tests.rs: cross-user update/delete/log-dose/adherence all return 404; the legitimate owner can still do all of the above. Existing tests unaffected (they operate as the same user that created the data).

Verification

cargo build + clippy --all-targets --all-features -D warnings + fmt --check all clean. Tests compile. ⚠️ Same Mongo caveat as before — integration tests run in CI, not locally; I'll fix any failures.

Out of scope

Share-recipient write access (a later phase if desired; currently read-only by design).
The medications/:id/delete and medications/:id (update) routes use POST (not DELETE/PUT) — that's a pre-existing route-shape choice, unchanged here.

## Summary Closes #12. The update/delete handlers for medications and appointments took the auth `Claims` but never used them — **any authenticated user could update or delete any other user's records by id** (an IDOR / insecure direct object reference). Same gap on `log_dose` (could skew another user's adherence stats) and `get_adherence` (leaked dose history). Surfaced while wiring the Phase B share-gate into the read paths (PR #11); filed separately as a security item. ## Fix Each affected handler now looks up the item first and confirms `user_id == claims.sub` before mutating or returning. Mismatches return **404** (not 403) to avoid leaking the existence of other users' records. Recipients of shared profiles remain **read-only** per the ADR — writes stay owner-only. Covered handlers: - `update_medication`, `delete_medication`, `log_dose`, `get_adherence` - `update_appointment`, `delete_appointment` **`health_stats` was already checking** `user_id == claims.sub` on update and delete — unaffected. ## Audit method Grepped every handler taking `Extension<Claims>` / `Extension(_claims)>`; the `_claims` (underscore-prefixed = unused) ones were the red flags. Confirmed each write/delete path's actual behavior, not just the signature. ## Tests New `ownership_tests.rs`: cross-user update/delete/log-dose/adherence all return 404; the legitimate owner can still do all of the above. Existing tests unaffected (they operate as the same user that created the data). ## Verification `cargo build` + `clippy --all-targets --all-features -D warnings` + `fmt --check` all clean. Tests compile. ⚠️ Same Mongo caveat as before — integration tests run in CI, not locally; I'll fix any failures. ## Out of scope - Share-recipient **write** access (a later phase if desired; currently read-only by design). - The `medications/:id/delete` and `medications/:id` (update) routes use POST (not DELETE/PUT) — that's a pre-existing route-shape choice, unchanged here.

alvaro added 1 commit 2026-07-19 15:00:57 +00:00

fix(security): enforce ownership on medication/appointment write paths (#12 )

Lint and Build / format (pull_request) Successful in 38s

Details

Lint and Build / clippy (pull_request) Successful in 1m40s

Details

Lint and Build / build (pull_request) Successful in 3m46s

Details

Lint and Build / test (pull_request) Successful in 4m0s

Details

ff185a60e4

The update/delete handlers for medications and appointments took the
auth Claims but never used them — any authenticated user could update
or delete any other user's records by id (an IDOR). Same gap on
log_dose (could skew another's adherence stats) and get_adherence
(leaked dose history).

Fix: each affected handler now looks up the item first and confirms
user_id == claims.sub before mutating/returning. Mismatches return 404
(not 403) to avoid leaking the existence of other users' records.
Recipients of shared profiles remain read-only per the ADR — writes
stay owner-only.

Covered handlers:
- update_medication, delete_medication, log_dose, get_adherence
- update_appointment, delete_appointment

health_stats update/delete were already checking user_id == claims.sub
(unaffected).

New ownership_tests.rs: cross-user update/delete/log-dose/adherence all
404; the legitimate owner can still do all of the above.

Verification: cargo build/clippy (-D warnings)/fmt clean; existing
tests unaffected (they operate as the same user that created the data).

Closes #12.