fix(security): enforce ownership on medication/appointment write paths (#12)
All checks were successful
Lint and Build / format (pull_request) Successful in 38s
Lint and Build / clippy (pull_request) Successful in 1m40s
Lint and Build / build (pull_request) Successful in 3m46s
Lint and Build / test (pull_request) Successful in 4m0s

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.
This commit is contained in:
goose 2026-07-19 12:00:13 -03:00
parent d59487fa3b
commit ff185a60e4
3 changed files with 484 additions and 30 deletions

View file

@ -142,31 +142,98 @@ pub async fn get_appointment(
pub async fn update_appointment(
State(state): State<AppState>,
Extension(_claims): Extension<Claims>,
Extension(claims): Extension<Claims>,
Path(id): Path<String>,
Json(req): Json<UpdateAppointmentRequest>,
) -> Result<Json<AppointmentResponse>, StatusCode> {
) -> Result<Json<AppointmentResponse>, (StatusCode, Json<serde_json::Value>)> {
let database = state.db.get_database();
let repo = AppointmentRepository::new(database.collection("appointments"));
// Authorization: owner-only (recipients are read-only). 404 on mismatch to
// avoid leaking existence.
let existing = match repo.find_by_appointment_id(&id).await {
Ok(Some(a)) => a,
Ok(None) => {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
Err(e) => {
tracing::error!("update_appointment lookup failed: {}", e);
return Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
));
}
};
if existing.user_id != claims.sub {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
match repo.update_by_appointment_id(&id, req).await {
Ok(Some(appt)) => Ok(Json(appt.into())),
Ok(None) => Err(StatusCode::NOT_FOUND),
Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
Ok(None) => Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
)),
Err(e) => {
tracing::error!("update_appointment failed: {}", e);
Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
))
}
}
}
pub async fn delete_appointment(
State(state): State<AppState>,
Extension(_claims): Extension<Claims>,
Extension(claims): Extension<Claims>,
Path(id): Path<String>,
) -> Result<StatusCode, StatusCode> {
) -> Result<StatusCode, (StatusCode, Json<serde_json::Value>)> {
let database = state.db.get_database();
let repo = AppointmentRepository::new(database.collection("appointments"));
// Authorization: owner-only.
let existing = match repo.find_by_appointment_id(&id).await {
Ok(Some(a)) => a,
Ok(None) => {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
Err(e) => {
tracing::error!("delete_appointment lookup failed: {}", e);
return Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
));
}
};
if existing.user_id != claims.sub {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
match repo.delete_by_appointment_id(&id).await {
Ok(true) => Ok(StatusCode::NO_CONTENT),
Ok(false) => Err(StatusCode::NOT_FOUND),
Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
Ok(false) => Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
)),
Err(e) => {
tracing::error!("delete_appointment failed: {}", e);
Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
))
}
}
}

View file

@ -155,34 +155,102 @@ pub async fn get_medication(
pub async fn update_medication(
State(state): State<AppState>,
Extension(_claims): Extension<Claims>,
Extension(claims): Extension<Claims>,
Path(id): Path<String>,
Json(req): Json<UpdateMedicationRequest>,
) -> Result<Json<MedicationResponse>, StatusCode> {
) -> Result<Json<MedicationResponse>, (StatusCode, Json<serde_json::Value>)> {
let database = state.db.get_database();
let repo = MedicationRepository::new(database.collection("medications"));
// Look up by medication_id (UUID) — the path param is not a Mongo ObjectId.
// Authorization: look up first, confirm the caller owns the medication
// (writes are owner-only — recipients are read-only per the ADR). Use 404
// (not 403) on a mismatch to avoid leaking the existence of other users'
// records.
let existing = match repo.find_by_medication_id(&id).await {
Ok(Some(m)) => m,
Ok(None) => {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
Err(e) => {
tracing::error!("update_medication lookup failed: {}", e);
return Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
));
}
};
if existing.user_id != claims.sub {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
match repo.update_by_medication_id(&id, req).await {
Ok(Some(medication)) => Ok(Json(medication.into())),
Ok(None) => Err(StatusCode::NOT_FOUND),
Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
Ok(None) => Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
)),
Err(e) => {
tracing::error!("update_medication failed: {}", e);
Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
))
}
}
}
pub async fn delete_medication(
State(state): State<AppState>,
Extension(_claims): Extension<Claims>,
Extension(claims): Extension<Claims>,
Path(id): Path<String>,
) -> Result<StatusCode, StatusCode> {
) -> Result<StatusCode, (StatusCode, Json<serde_json::Value>)> {
let database = state.db.get_database();
let repo = MedicationRepository::new(database.collection("medications"));
// Look up by medication_id (UUID), not Mongo _id.
// Authorization: owner-only (see update_medication). Look up first to
// avoid leaking existence via the deleted_count.
let existing = match repo.find_by_medication_id(&id).await {
Ok(Some(m)) => m,
Ok(None) => {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
Err(e) => {
tracing::error!("delete_medication lookup failed: {}", e);
return Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
));
}
};
if existing.user_id != claims.sub {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
match repo.delete_by_medication_id(&id).await {
Ok(true) => Ok(StatusCode::NO_CONTENT),
Ok(false) => Err(StatusCode::NOT_FOUND),
Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
Ok(false) => Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
)),
Err(e) => {
tracing::error!("delete_medication failed: {}", e);
Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
))
}
}
}
@ -191,8 +259,34 @@ pub async fn log_dose(
Extension(claims): Extension<Claims>,
Path(id): Path<String>,
Json(req): Json<LogDoseRequest>,
) -> Result<(StatusCode, Json<MedicationDose>), StatusCode> {
) -> Result<(StatusCode, Json<MedicationDose>), (StatusCode, Json<serde_json::Value>)> {
let database = state.db.get_database();
let repo = MedicationRepository::new(database.collection("medications"));
// Authorization: confirm the caller owns the medication before logging a
// dose against it (otherwise a user could skew another's adherence stats).
let med = match repo.find_by_medication_id(&id).await {
Ok(Some(m)) => m,
Ok(None) => {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
Err(e) => {
tracing::error!("log_dose lookup failed: {}", e);
return Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
));
}
};
if med.user_id != claims.sub {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
let now = SystemTime::now();
@ -210,7 +304,13 @@ pub async fn log_dose(
.collection::<crate::models::medication::MedicationDose>("medication_doses")
.insert_one(&dose, None)
.await
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
.map_err(|e| {
tracing::error!("log_dose insert failed: {}", e);
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
)
})?;
// Populate the generated _id so the caller gets the persisted dose back.
dose.id = result.inserted_id.as_object_id();
@ -220,9 +320,10 @@ pub async fn log_dose(
pub async fn get_adherence(
State(state): State<AppState>,
Extension(_claims): Extension<Claims>,
Extension(claims): Extension<Claims>,
Path(id): Path<String>,
) -> Result<Json<crate::models::medication::AdherenceStats>, StatusCode> {
) -> Result<Json<crate::models::medication::AdherenceStats>, (StatusCode, Json<serde_json::Value>)>
{
use mongodb::bson::{doc, DateTime};
const PERIOD_DAYS: i64 = 30;
@ -231,6 +332,28 @@ pub async fn get_adherence(
let doses: mongodb::Collection<MedicationDose> = database.collection("medication_doses");
let med_repo = MedicationRepository::new(database.collection("medications"));
// Authorization: owner-only. Look up the medication and confirm ownership
// before computing/returning adherence (which leaks dose history).
let medication = match med_repo.find_by_medication_id(&id).await {
Ok(Some(m)) => {
if m.user_id != claims.sub {
return Err((
StatusCode::NOT_FOUND,
Json(serde_json::json!({ "error": "not found" })),
));
}
Some(m)
}
Ok(None) => None,
Err(e) => {
tracing::error!("get_adherence lookup failed: {}", e);
return Err((
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
));
}
};
// Look at doses logged in the last PERIOD_DAYS days for this medication.
let since = DateTime::from_system_time(
SystemTime::now() - std::time::Duration::from_secs(PERIOD_DAYS as u64 * 86400),
@ -243,19 +366,25 @@ pub async fn get_adherence(
let total_logged = doses
.count_documents(filter.clone(), None)
.await
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
.map_err(|e| {
tracing::error!("get_adherence count failed: {}", e);
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
)
})?;
let taken_filter = doc! { "$and": [filter, doc! { "taken": true }] };
let taken = doses
.count_documents(taken_filter, None)
.await
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
// Fetch the medication to check for a dose schedule.
let medication = med_repo
.find_by_medication_id(&id)
.await
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
.map_err(|e| {
tracing::error!("get_adherence taken count failed: {}", e);
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(serde_json::json!({ "error": "database error" })),
)
})?;
let (scheduled_doses, total_doses, missed_doses, rate) =
match medication.and_then(|m| m.dose_schedule) {