From b95e740b2f31d12f6aeff1d868f9227031b34d58 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 4 Jul 2026 23:31:10 +0200 Subject: [PATCH] security(music): ensure read permission via authz --- src/application/services/music_service.rs | 34 +++++++- src/common/di.rs | 2 +- .../api/handlers/favorites_handler.rs | 50 +++++------- src/interfaces/api/handlers/recent_handler.rs | 42 +++------- tests/api/favorites.hurl | 74 +++++++++++++++++ tests/api/public_shares.hurl | 79 +++++++++++++++++++ tests/api/recent.hurl | 64 +++++++++++++++ 7 files changed, 280 insertions(+), 65 deletions(-) diff --git a/src/application/services/music_service.rs b/src/application/services/music_service.rs index 78ce6757..d4d4456a 100644 --- a/src/application/services/music_service.rs +++ b/src/application/services/music_service.rs @@ -5,17 +5,31 @@ use crate::application::dtos::playlist_dto::{ AddTracksDto, AudioMetadataDto, CreatePlaylistDto, PlaylistDto, PlaylistItemDto, PlaylistQueryDto, PlaylistShareInfoDto, ReorderTracksDto, SharePlaylistDto, UpdatePlaylistDto, }; +use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::music_ports::{MusicStoragePort, MusicUseCase}; use crate::common::errors::{DomainError, ErrorKind}; +use crate::domain::services::authorization::{Permission, Resource, Subject}; use crate::infrastructure::adapters::music_storage_adapter::MusicStorageAdapter; +use crate::infrastructure::services::pg_acl_engine::PgAclEngine; pub struct MusicService { storage: Arc, + /// ReBAC engine — Round 1 fix from `docs/plan/authz_audit/`. + /// Currently used ONLY by `get_audio_metadata` to close the + /// cross-tenant IDOR (`_user_id: Uuid` was deliberately unused). + /// The full engine rewrite (Round 3 — `Resource::Playlist` + + /// authz.require on every playlist verb) is a separate PR; + /// don't extend the bespoke `user_has_access` / `user_can_write` + /// pattern to new methods, use `require` here instead. + authorization: Arc, } impl MusicService { - pub fn new(storage: Arc) -> Self { - Self { storage } + pub fn new(storage: Arc, authorization: Arc) -> Self { + Self { + storage, + authorization, + } } } @@ -375,10 +389,24 @@ impl MusicUseCase for MusicService { async fn get_audio_metadata( &self, file_id: &str, - _user_id: Uuid, + caller_id: Uuid, ) -> Result, DomainError> { let file_uuid = Uuid::parse_str(file_id) .map_err(|_| DomainError::new(ErrorKind::InvalidInput, "Music", "Invalid file ID"))?; + // AuthZ pre-read: caller must have `Read` on the underlying + // audio file. Before this check the endpoint returned + // metadata for any known file id (cross-tenant IDOR — the + // `_user_id` parameter was deliberately unused). `require` + // returns 404 on denial to match the anti-enum shape used + // everywhere else. Post-Drive AuthZ audit fix (Round 1 + // BLOCKER — `docs/plan/authz_audit/rest_storage.md`). + self.authorization + .require( + Subject::User(caller_id), + Permission::Read, + Resource::File(file_uuid), + ) + .await?; self.storage.get_audio_metadata(&file_uuid).await } } diff --git a/src/common/di.rs b/src/common/di.rs index 8ac3a379..959ad7c2 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -1865,7 +1865,7 @@ impl AppServiceFactory { audio_metadata_repo, ), ); - let music_svc = Arc::new(MusicService::new(music_storage)); + let music_svc = Arc::new(MusicService::new(music_storage, authorization.clone())); app_state.music_service = Some(music_svc); tracing::info!("Music service initialized"); } diff --git a/src/interfaces/api/handlers/favorites_handler.rs b/src/interfaces/api/handlers/favorites_handler.rs index 72b97a8b..cb887a58 100644 --- a/src/interfaces/api/handlers/favorites_handler.rs +++ b/src/interfaces/api/handlers/favorites_handler.rs @@ -6,7 +6,7 @@ use axum::{ }; use serde::Deserialize; use std::sync::Arc; -use tracing::{error, info}; +use tracing::info; use utoipa::ToSchema; use crate::application::dtos::display_helpers::{ @@ -66,7 +66,8 @@ pub async fn add_favorite( Json(serde_json::json!({ "error": "Item type must be 'file' or 'folder'" })), - ); + ) + .into_response(); } match favorites_service @@ -81,16 +82,14 @@ pub async fn add_favorite( "message": "Item added to favorites" })), ) + .into_response() } - Err(err) => { - error!("Error adding to favorites: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to add to favorites" - })), - ) - } + // Route through AppError so the `DomainError::kind` maps to the + // right status code (NotFound → 404 anti-enum for the pre-write + // authz gate, InvalidInput → 400 for a malformed UUID, etc.). + // A hardcoded 500 here would mask the 404 the Round 1 AuthZ + // fix relies on. + Err(err) => AppError::from(err).into_response(), } } @@ -129,6 +128,7 @@ pub async fn remove_favorite( "message": "Item removed from favorites" })), ) + .into_response() } else { info!("Item {} '{}' was not in favorites", item_type, item_id); ( @@ -137,17 +137,12 @@ pub async fn remove_favorite( "message": "Item was not in favorites" })), ) + .into_response() } } - Err(err) => { - error!("Error removing from favorites: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to remove from favorites" - })), - ) - } + // Same rationale as `add_favorite` — preserve DomainError→HTTP + // status mapping instead of collapsing every error to 500. + Err(err) => AppError::from(err).into_response(), } } @@ -347,15 +342,10 @@ pub async fn batch_add_favorites( ); (StatusCode::OK, Json(serde_json::json!(result))).into_response() } - Err(err) => { - error!("Error in batch add favorites: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to batch add favorites" - })), - ) - .into_response() - } + // Preserve DomainError→HTTP status mapping — the Round 1 + // AuthZ fix relies on a per-item NotFound propagating out + // of the batch. A hardcoded 500 would mask the 404 that + // signals a cross-tenant probe. + Err(err) => AppError::from(err).into_response(), } } diff --git a/src/interfaces/api/handlers/recent_handler.rs b/src/interfaces/api/handlers/recent_handler.rs index 3878e783..690548d7 100644 --- a/src/interfaces/api/handlers/recent_handler.rs +++ b/src/interfaces/api/handlers/recent_handler.rs @@ -5,7 +5,7 @@ use axum::{ response::IntoResponse, }; use std::sync::Arc; -use tracing::{error, info}; +use tracing::info; use crate::application::dtos::display_helpers::{ category_for, format_file_size, icon_class_for, icon_special_class_for, @@ -70,16 +70,10 @@ pub async fn record_item_access( ) .into_response() } - Err(err) => { - error!("Error recording access in recents: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to record access" - })), - ) - .into_response() - } + // Preserve DomainError→HTTP status mapping — the Round 1 + // AuthZ fix relies on the NotFound from `authz.require` + // propagating as 404 (anti-enum), not being masked as 500. + Err(err) => AppError::from(err).into_response(), } } @@ -130,16 +124,9 @@ pub async fn remove_from_recent( .into_response() } } - Err(err) => { - error!("Error removing from recents: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to remove from recents" - })), - ) - .into_response() - } + // Same rationale as `record_item_access` — preserve the + // DomainError→HTTP mapping instead of collapsing to 500. + Err(err) => AppError::from(err).into_response(), } } @@ -170,16 +157,9 @@ pub async fn clear_recent_items( ) .into_response() } - Err(err) => { - error!("Error clearing recent items: {}", err); - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(serde_json::json!({ - "error": "Failed to clear recent items" - })), - ) - .into_response() - } + // Same rationale as `record_item_access` — preserve the + // DomainError→HTTP mapping instead of collapsing to 500. + Err(err) => AppError::from(err).into_response(), } } diff --git a/tests/api/favorites.hurl b/tests/api/favorites.hurl index 6c8d8d5f..d8609ab8 100644 --- a/tests/api/favorites.hurl +++ b/tests/api/favorites.hurl @@ -159,3 +159,77 @@ Authorization: Bearer {{token}} HTTP 200 [Asserts] jsonpath "$.items" count == 0 + + +# ───────────────────────────────────────────────────────────── +# Step 12 — Cross-tenant regression (post-Drive AuthZ audit, +# Round 1 HIGH). Before this fix, `POST /api/favorites/…` +# accepted any UUID and enrolled it; the listing endpoint +# then JOINed back to storage.files/folders and returned +# name/mime/size/drive_id for anything the caller had +# managed to add — an information oracle over the whole +# tenant. Now the write path calls `authz.require(Read, …)` +# per item; a caller with no grant gets 404 (anti-enum) +# + `authz.denied` audit line. See +# `docs/plan/authz_audit/rest_storage.md`. +# ───────────────────────────────────────────────────────────── + +# Create a second, unprivileged user. Idempotent: `HTTP *` accepts +# either 201 (first run) or 409 (subsequent runs). The login below +# is the actual precondition — if it succeeds we know the user +# exists with the expected password. +POST {{base_url}}/api/admin/users +Authorization: Bearer {{token}} +Content-Type: application/json +{ "username": "fav_mallory", "password": "FavMalloryPassword1!", "email": "fav_mallory@example.com", "role": "user" } + +HTTP * + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "fav_mallory", "password": "FavMalloryPassword1!" } + +HTTP 200 +[Captures] +mallory_token: jsonpath "$.access_token" + + +# Step 12a — Single-add on admin's file: 404 (anti-enum shape). +POST {{base_url}}/api/favorites/file/{{file_id}} +Authorization: Bearer {{mallory_token}} + +HTTP 404 + + +# Step 12b — Single-add on admin's folder: 404. +POST {{base_url}}/api/favorites/folder/{{test1_id}} +Authorization: Bearer {{mallory_token}} + +HTTP 404 + + +# Step 12c — Batch: must fail wholesale on the first denial. A partial +# success would still leak "which items are valid" — the same +# oracle we're closing. +POST {{base_url}}/api/favorites/batch +Authorization: Bearer {{mallory_token}} +Content-Type: application/json +{ + "items": [ + { "item_id": "{{file_id}}", "item_type": "file" }, + { "item_id": "{{test1_id}}", "item_type": "folder" } + ] +} + +HTTP 404 + + +# Step 12d — Mallory's favorites list is EMPTY — no partial success +# slipped through. +GET {{base_url}}/api/favorites/resources +Authorization: Bearer {{mallory_token}} + +HTTP 200 +[Asserts] +jsonpath "$.items" count == 0 diff --git a/tests/api/public_shares.hurl b/tests/api/public_shares.hurl index 81b02edc..641f2073 100644 --- a/tests/api/public_shares.hurl +++ b/tests/api/public_shares.hurl @@ -274,6 +274,85 @@ status >= 400 status < 500 +# ───────────────────────────────────────────────────────────── +# 14b — Viewer-laundering regression (post-Drive AuthZ audit, +# Round 1 HIGH). Before the fix, `POST /api/shares` checked +# only "does the item exist" — any authenticated user who +# could name the UUID could mint a public Viewer link, +# laundering read access into a permanent anonymous URL +# that survived their own grant revocation. Now the +# service calls `authz.require(Share, resource)` before +# minting the token; a caller without `Share` +# (Viewer/Commenter/Contributor/no-grant-at-all) gets 404 +# (anti-enum) + `authz.denied` audit line. See +# `docs/plan/authz_audit/admin_membership.md`. +# +# We test the strongest form: an unrelated user with no +# grant at all. The intermediate case (Viewer with Read +# but not Share) is covered by the same code path — Share +# is bundled only with owner/editor role_grants. +# ───────────────────────────────────────────────────────────── + +# Create/lookup the attacker. Idempotent: `HTTP *` accepts either +# 201 (first run) or 409 (subsequent runs). Login below is the real +# precondition. +POST {{base_url}}/api/admin/users +Authorization: Bearer {{admin_token}} +Content-Type: application/json +{ "username": "sh_mallory", "password": "ShMalloryPassword1!", "email": "sh_mallory@example.com", "role": "user" } + +HTTP * + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "sh_mallory", "password": "ShMalloryPassword1!" } + +HTTP 200 +[Captures] +mallory_token: jsonpath "$.access_token" + + +# Step 14b.i — Mallory tries to mint a public share on admin's +# folder: 404 (anti-enum). No token appears in the +# response body. +POST {{base_url}}/api/shares +Authorization: Bearer {{mallory_token}} +Content-Type: application/json +{ + "item_id": "{{share_folder_id}}", + "item_name": "public-share-test", + "item_type": "folder" +} + +HTTP 404 + + +# Step 14b.ii — Same attempt on admin's file: 404. +POST {{base_url}}/api/shares +Authorization: Bearer {{mallory_token}} +Content-Type: application/json +{ + "item_id": "{{shared_file_id}}", + "item_name": "hello.txt", + "item_type": "file" +} + +HTTP 404 + + +# Step 14b.iii — Mallory has no shares — no partial success slipped +# through. (`GET /api/shares` returns only shares the +# caller created; response is paginated.) +GET {{base_url}}/api/shares +Authorization: Bearer {{mallory_token}} + +HTTP 200 +[Asserts] +jsonpath "$.items" isCollection +jsonpath "$.items" count == 0 + + # ───────────────────────────────────────────────────────────── # 15 — Teardown: revoke the password share + the direct # file-share, then delete the folder. diff --git a/tests/api/recent.hurl b/tests/api/recent.hurl index 4f423908..f290aba4 100644 --- a/tests/api/recent.hurl +++ b/tests/api/recent.hurl @@ -187,3 +187,67 @@ DELETE {{base_url}}/api/recent/clear Authorization: Bearer {{token}} HTTP 200 + + +# ───────────────────────────────────────────────────────────── +# Step 10 — Cross-tenant regression (post-Drive AuthZ audit, +# Round 1 HIGH). Before this fix, `POST /api/recent/…` +# accepted any UUID and the listing endpoint JOINed back +# to storage.files/folders (name/mime/size/drive_id) — a +# metadata oracle over the whole tenant. Now the write +# path calls `authz.require(Read, …)`; unauthorised +# callers get 404 (anti-enum) + `authz.denied` audit line. +# See `docs/plan/authz_audit/rest_storage.md`. +# ───────────────────────────────────────────────────────────── + +# Re-discover a folder id so the attacker has TWO targets to probe +# (file + folder). Same test1 folder as favorites.hurl. +GET {{base_url}}/api/folders/{{home_folder_id}}/resources?resource_types=folder +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +test1_id: jsonpath "$.items[0].resource.id" + + +# Create/lookup the attacker. Idempotent: `HTTP *` accepts either +# 201 (first run) or 409 (subsequent runs). Login below is the real +# precondition. +POST {{base_url}}/api/admin/users +Authorization: Bearer {{token}} +Content-Type: application/json +{ "username": "rec_mallory", "password": "RecMalloryPassword1!", "email": "rec_mallory@example.com", "role": "user" } + +HTTP * + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "rec_mallory", "password": "RecMalloryPassword1!" } + +HTTP 200 +[Captures] +mallory_token: jsonpath "$.access_token" + + +# Step 10a — Record admin's file into mallory's recent: 404. +POST {{base_url}}/api/recent/file/{{file_id}} +Authorization: Bearer {{mallory_token}} + +HTTP 404 + + +# Step 10b — Same for admin's folder: 404. +POST {{base_url}}/api/recent/folder/{{test1_id}} +Authorization: Bearer {{mallory_token}} + +HTTP 404 + + +# Step 10c — Mallory's recent list stays empty. +GET {{base_url}}/api/recent/resources +Authorization: Bearer {{mallory_token}} + +HTTP 200 +[Asserts] +jsonpath "$.items" count == 0