From c1924c825b4c7c5eb9995bcd4baca673629309e0 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Thu, 16 Jul 2026 21:07:18 +0200 Subject: [PATCH] security(search): ensure that search suggenstion returns answer the user has access to --- src/application/ports/inbound.rs | 4 + src/application/ports/storage_ports.rs | 12 ++- src/application/services/search_service.rs | 25 ++++-- src/common/stubs.rs | 1 + src/domain/repositories/folder_repository.rs | 10 ++- .../pg/file_blob_read_repository.rs | 80 +++++++++++-------- .../repositories/pg/folder_db_repository.rs | 74 +++++++++-------- src/interfaces/api/handlers/search_handler.rs | 11 ++- tests/api/search_basic.hurl | 37 +++++++-- 9 files changed, 171 insertions(+), 83 deletions(-) diff --git a/src/application/ports/inbound.rs b/src/application/ports/inbound.rs index 96d71970..456ce9e8 100644 --- a/src/application/ports/inbound.rs +++ b/src/application/ports/inbound.rs @@ -27,11 +27,15 @@ pub trait SearchUseCase: Send + Sync + 'static { ) -> Result, DomainError>; /// Returns quick suggestions for autocomplete (lightweight, fast). + /// `caller_id` scopes results to drives the caller can Read — without + /// it the endpoint leaks names + paths across every tenant on the + /// instance (AuthZ audit finding #1, 2026-07-12). async fn suggest( &self, query: &str, folder_id: Option<&str>, limit: usize, + caller_id: Uuid, ) -> Result; /// Clears the search results cache. diff --git a/src/application/ports/storage_ports.rs b/src/application/ports/storage_ports.rs index 896fc01c..9f75e790 100644 --- a/src/application/ports/storage_ports.rs +++ b/src/application/ports/storage_ports.rs @@ -205,13 +205,21 @@ pub trait FileReadPort: Send + Sync + 'static { /// Results are ordered by relevance (exact > starts-with > contains) so the /// caller can use them directly for autocomplete suggestions. /// - /// The default implementation falls back to `list_files` + in-memory filter - /// so that stubs and mocks compile without changes. + /// `caller_id` scopes results to files whose owning drive the caller can + /// Read (direct or group-mediated `role_grants`). Without it the endpoint + /// leaks names + paths across every tenant on the instance — closed as + /// AuthZ audit finding #1 (2026-07-12). + /// + /// The default implementation falls back to `list_files` + in-memory + /// filter so that stubs and mocks compile without changes. Stub-mode + /// callers already operate against a single tenant's data, so ignoring + /// `caller_id` here is safe; the PG impl enforces the real scope. async fn suggest_files_by_name( &self, folder_id: Option<&str>, query: &str, limit: usize, + _caller_id: Uuid, ) -> Result, DomainError> { let all = self.list_files(folder_id).await?; let q = query.to_lowercase(); diff --git a/src/application/services/search_service.rs b/src/application/services/search_service.rs index ff1ccd11..f6918728 100644 --- a/src/application/services/search_service.rs +++ b/src/application/services/search_service.rs @@ -425,20 +425,28 @@ impl SearchService { /// Quick suggestions search — returns up to `limit` name suggestions /// matching the query. Pushes filtering, relevance sort and LIMIT to SQL /// so only a handful of rows cross the DB→app boundary. - pub async fn suggest( + /// + /// `caller_id` scopes the underlying repo queries to drives the caller + /// can Read. Without it (the pre-fix shape) any authenticated user — + /// including external magic-link recipients — could autocomplete both + /// names and full paths across every tenant on the instance (AuthZ + /// audit finding #1, 2026-07-12). Named `_with_perms` per the + /// AGENTS.md AuthZ convention. + pub async fn suggest_with_perms( &self, query: &str, folder_id: Option<&str>, limit: usize, + caller_id: Uuid, ) -> Result { let start = Instant::now(); // Ask SQL for at most `limit` best-matching files and folders let (files, folders) = tokio::join!( self.file_repository - .suggest_files_by_name(folder_id, query, limit), + .suggest_files_by_name(folder_id, query, limit, caller_id), self.folder_repository - .suggest_folders_by_name(folder_id, query, limit), + .suggest_folders_by_name(folder_id, query, limit, caller_id), ); let files = files?; let folders = folders?; @@ -724,14 +732,20 @@ impl SearchUseCase for SearchService { }) } - /// Returns quick suggestions for autocomplete. + /// Returns quick suggestions for autocomplete. Delegates to the + /// inherent `suggest_with_perms` — the trait method is preserved as + /// the polymorphic entry point (e.g. for `StubSearchUseCase` in + /// tests); production callers can equivalently call the inherent + /// method directly. async fn suggest( &self, query: &str, folder_id: Option<&str>, limit: usize, + caller_id: Uuid, ) -> Result { - self.suggest(query, folder_id, limit).await + self.suggest_with_perms(query, folder_id, limit, caller_id) + .await } /// Clears the search results cache. @@ -763,6 +777,7 @@ impl SearchService { _query: &str, _folder_id: Option<&str>, _limit: usize, + _caller_id: Uuid, ) -> Result { Ok(SearchSuggestionsDto { suggestions: Vec::new(), diff --git a/src/common/stubs.rs b/src/common/stubs.rs index bdd15bf5..4d72d9d4 100644 --- a/src/common/stubs.rs +++ b/src/common/stubs.rs @@ -725,6 +725,7 @@ impl SearchUseCase for StubSearchUseCase { _query: &str, _folder_id: Option<&str>, _limit: usize, + _caller_id: Uuid, ) -> Result { Ok(SearchSuggestionsDto { suggestions: Vec::new(), diff --git a/src/domain/repositories/folder_repository.rs b/src/domain/repositories/folder_repository.rs index 011695ab..c819439e 100644 --- a/src/domain/repositories/folder_repository.rs +++ b/src/domain/repositories/folder_repository.rs @@ -243,13 +243,21 @@ pub trait FolderRepository: Send + Sync + 'static { /// Results are ordered by relevance (exact > starts-with > contains) for /// autocomplete suggestions. /// + /// `caller_id` scopes results to folders whose owning drive the caller + /// can Read (direct or group-mediated `role_grants`). Without it the + /// endpoint leaked names + paths across every tenant on the instance — + /// closed as AuthZ audit finding #1 (2026-07-12). + /// /// The default implementation falls back to `list_folders` + in-memory - /// filter so that stubs and mocks compile without changes. + /// filter so that stubs and mocks compile without changes. Stub-mode + /// callers already operate against a single tenant's data, so ignoring + /// `caller_id` here is safe; the PG impl enforces the real scope. async fn suggest_folders_by_name( &self, parent_id: Option<&str>, query: &str, limit: usize, + _caller_id: uuid::Uuid, ) -> Result, DomainError> { let all = self.list_folders(parent_id).await?; let q = query.to_lowercase(); diff --git a/src/infrastructure/repositories/pg/file_blob_read_repository.rs b/src/infrastructure/repositories/pg/file_blob_read_repository.rs index e631aa7c..42aad261 100644 --- a/src/infrastructure/repositories/pg/file_blob_read_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_read_repository.rs @@ -1404,12 +1404,20 @@ impl FileReadPort for FileBlobReadRepository { folder_id: Option<&str>, query: &str, limit: usize, + caller_id: Uuid, ) -> Result, DomainError> { + // Scope by drive membership: `CALLER_CAN_READ_DRIVE` (`$1` = + // caller_id) restricts the result set to files whose owning drive + // the caller has any active `role_grants` on — direct or via a + // transitive group cascade. Pre-fix, the query only filtered on + // `NOT is_trashed AND name ILIKE $pattern`, exposing names + paths + // across every tenant on the instance (AuthZ audit finding #1, + // 2026-07-12). let pattern = super::like_escape(query); let limit_i64 = limit as i64; let rows: Vec = if let Some(fid) = folder_id { - sqlx::query_as( + sqlx::query_as(&format!( r#" SELECT fi.id::text, fi.name, fi.folder_id::text, fo.path, fi.size, fi.mime_type, @@ -1420,7 +1428,40 @@ impl FileReadPort for FileBlobReadRepository { fi.created_by, fi.updated_by FROM storage.files fi LEFT JOIN storage.folders fo ON fo.id = fi.folder_id - WHERE fi.folder_id = $1::uuid + WHERE {CALLER_CAN_READ_DRIVE} + AND fi.folder_id = $2::uuid + AND NOT fi.is_trashed + AND fi.name ILIKE $3 + ORDER BY CASE + WHEN fi.name ILIKE $4 THEN 0 + WHEN fi.name ILIKE $4 || '%' THEN 1 + ELSE 2 + END, + fi.name + LIMIT $5 + "# + )) + .bind(caller_id) + .bind(fid) + .bind(&pattern) + .bind(query) + .bind(limit_i64) + .fetch_all(self.pool.as_ref()) + .await + } else { + sqlx::query_as(&format!( + r#" + SELECT fi.id::text, fi.name, fi.folder_id::text, fo.path, + fi.size, fi.mime_type, + EXTRACT(EPOCH FROM fi.created_at)::bigint, + EXTRACT(EPOCH FROM fi.updated_at)::bigint, + fi.blob_hash, + + fi.created_by, fi.updated_by + FROM storage.files fi + LEFT JOIN storage.folders fo ON fo.id = fi.folder_id + WHERE {CALLER_CAN_READ_DRIVE} + AND fi.folder_id IS NULL AND NOT fi.is_trashed AND fi.name ILIKE $2 ORDER BY CASE @@ -1430,38 +1471,9 @@ impl FileReadPort for FileBlobReadRepository { END, fi.name LIMIT $4 - "#, - ) - .bind(fid) - .bind(&pattern) - .bind(query) - .bind(limit_i64) - .fetch_all(self.pool.as_ref()) - .await - } else { - sqlx::query_as( - r#" - SELECT fi.id::text, fi.name, fi.folder_id::text, fo.path, - fi.size, fi.mime_type, - EXTRACT(EPOCH FROM fi.created_at)::bigint, - EXTRACT(EPOCH FROM fi.updated_at)::bigint, - fi.blob_hash, - - fi.created_by, fi.updated_by - FROM storage.files fi - LEFT JOIN storage.folders fo ON fo.id = fi.folder_id - WHERE fi.folder_id IS NULL - AND NOT fi.is_trashed - AND fi.name ILIKE $1 - ORDER BY CASE - WHEN fi.name ILIKE $2 THEN 0 - WHEN fi.name ILIKE $2 || '%' THEN 1 - ELSE 2 - END, - fi.name - LIMIT $3 - "#, - ) + "# + )) + .bind(caller_id) .bind(&pattern) .bind(query) .bind(limit_i64) diff --git a/src/infrastructure/repositories/pg/folder_db_repository.rs b/src/infrastructure/repositories/pg/folder_db_repository.rs index 4dc65f9d..df23e590 100644 --- a/src/infrastructure/repositories/pg/folder_db_repository.rs +++ b/src/infrastructure/repositories/pg/folder_db_repository.rs @@ -1178,31 +1178,39 @@ impl FolderRepository for FolderDbRepository { parent_id: Option<&str>, query: &str, limit: usize, + caller_id: uuid::Uuid, ) -> Result, DomainError> { + // Same drive-scope filter as `suggest_files_by_name` — closed as + // AuthZ audit finding #1 (2026-07-12). `CALLER_CAN_READ_DRIVE` + // aliases `storage.folders` as `fo`; the pre-fix query aliased it + // as an unqualified `storage.folders`, so this rewrite adds the + // `fo` alias in every branch. let pattern = super::like_escape(query); let limit_i64 = limit as i64; let rows: Vec = if let Some(pid) = parent_id { - sqlx::query_as( + sqlx::query_as(&format!( r#" - SELECT id::text, name, path, parent_id::text, drive_id, - EXTRACT(EPOCH FROM created_at)::bigint, - EXTRACT(EPOCH FROM updated_at)::bigint, - EXTRACT(EPOCH FROM tree_modified_at)::bigint, - created_by, updated_by - FROM storage.folders - WHERE parent_id = $1::uuid - AND NOT is_trashed - AND name ILIKE $2 + SELECT fo.id::text, fo.name, fo.path, fo.parent_id::text, fo.drive_id, + EXTRACT(EPOCH FROM fo.created_at)::bigint, + EXTRACT(EPOCH FROM fo.updated_at)::bigint, + EXTRACT(EPOCH FROM fo.tree_modified_at)::bigint, + fo.created_by, fo.updated_by + FROM storage.folders fo + WHERE {CALLER_CAN_READ_DRIVE} + AND fo.parent_id = $2::uuid + AND NOT fo.is_trashed + AND fo.name ILIKE $3 ORDER BY CASE - WHEN name ILIKE $3 THEN 0 - WHEN name ILIKE $3 || '%' THEN 1 + WHEN fo.name ILIKE $4 THEN 0 + WHEN fo.name ILIKE $4 || '%' THEN 1 ELSE 2 END, - name - LIMIT $4 - "#, - ) + fo.name + LIMIT $5 + "# + )) + .bind(caller_id) .bind(pid) .bind(&pattern) .bind(query) @@ -1210,26 +1218,28 @@ impl FolderRepository for FolderDbRepository { .fetch_all(self.pool()) .await } else { - sqlx::query_as( + sqlx::query_as(&format!( r#" - SELECT id::text, name, path, parent_id::text, drive_id, - EXTRACT(EPOCH FROM created_at)::bigint, - EXTRACT(EPOCH FROM updated_at)::bigint, - EXTRACT(EPOCH FROM tree_modified_at)::bigint, - created_by, updated_by - FROM storage.folders - WHERE parent_id IS NULL - AND NOT is_trashed - AND name ILIKE $1 + SELECT fo.id::text, fo.name, fo.path, fo.parent_id::text, fo.drive_id, + EXTRACT(EPOCH FROM fo.created_at)::bigint, + EXTRACT(EPOCH FROM fo.updated_at)::bigint, + EXTRACT(EPOCH FROM fo.tree_modified_at)::bigint, + fo.created_by, fo.updated_by + FROM storage.folders fo + WHERE {CALLER_CAN_READ_DRIVE} + AND fo.parent_id IS NULL + AND NOT fo.is_trashed + AND fo.name ILIKE $2 ORDER BY CASE - WHEN name ILIKE $2 THEN 0 - WHEN name ILIKE $2 || '%' THEN 1 + WHEN fo.name ILIKE $3 THEN 0 + WHEN fo.name ILIKE $3 || '%' THEN 1 ELSE 2 END, - name - LIMIT $3 - "#, - ) + fo.name + LIMIT $4 + "# + )) + .bind(caller_id) .bind(&pattern) .bind(query) .bind(limit_i64) diff --git a/src/interfaces/api/handlers/search_handler.rs b/src/interfaces/api/handlers/search_handler.rs index 5fba8009..2893103f 100644 --- a/src/interfaces/api/handlers/search_handler.rs +++ b/src/interfaces/api/handlers/search_handler.rs @@ -140,6 +140,7 @@ impl SearchHandler { /// Autocomplete suggestions for search. pub(super) async fn suggest_files_impl( State(state): State>, + auth_user: AuthUser, Query(params): Query, ) -> impl IntoResponse { info!("API: Search suggestions for {:?}", params.query); @@ -159,7 +160,12 @@ impl SearchHandler { let limit = params.limit.unwrap_or(10).min(20); match search_service - .suggest(¶ms.query, params.folder_id.as_deref(), limit) + .suggest_with_perms( + ¶ms.query, + params.folder_id.as_deref(), + limit, + auth_user.id, + ) .await { Ok(suggestions) => { @@ -354,9 +360,10 @@ pub async fn search_files_post( )] pub async fn suggest_files( state: State>, + auth_user: AuthUser, query: Query, ) -> impl IntoResponse { - SearchHandler::suggest_files_impl(state, query).await + SearchHandler::suggest_files_impl(state, auth_user, query).await } #[utoipa::path( diff --git a/tests/api/search_basic.hurl b/tests/api/search_basic.hurl index 919ca997..02a7b92b 100644 --- a/tests/api/search_basic.hurl +++ b/tests/api/search_basic.hurl @@ -110,7 +110,7 @@ Authorization: Bearer {{admin_token}} HTTP 200 [Asserts] -jsonpath "$.files" count >= 1 +jsonpath "$.files" count >= 1 body contains "{{needle_file_id}}" @@ -124,7 +124,7 @@ Authorization: Bearer {{admin_token}} HTTP 200 [Asserts] -jsonpath "$.files" count == 0 +jsonpath "$.files" count == 0 jsonpath "$.folders" count == 0 @@ -152,6 +152,29 @@ body not contains "unique-search-needle" body not contains "{{needle_file_id}}" +# ───────────────────────────────────────────────────────────── +# 5b — REGRESSION: `/api/search/suggest` MUST also refuse to +# surface admin's file to bob. Pre-fix (AuthZ audit #1, +# 2026-07-12) the suggest endpoint had NO `AuthUser` +# extractor and its underlying `suggest_files_by_name` / +# `suggest_folders_by_name` filtered only on +# `NOT is_trashed AND name ILIKE $1` — any authenticated +# user (including externals) could autocomplete names and +# full `path` values across every tenant on the instance. +# Fix: added `caller_id` to both repo queries via the +# shared `CALLER_CAN_READ_DRIVE` predicate (`role_grants` +# + `caller_group_ids`). This assertion is the anti- +# regression pin. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/search/suggest?query=unique-search-needle +Authorization: Bearer {{bob_token}} + +HTTP 200 +[Asserts] +body not contains "unique-search-needle" +body not contains "{{needle_file_id}}" + + # ───────────────────────────────────────────────────────────── # 6 — CONTENT-search cross-drive isolation (docs/plan/drive.md §11). # The cross-user check above (step 5) verifies the NAME-search @@ -209,7 +232,7 @@ HTTP 200 # Bob has no access to admin's drive → Tantivy's Must-clause # filters every doc that doesn't carry one of Bob's drive_ids, # so the file vanishes entirely. -jsonpath "$.files" count == 0 +jsonpath "$.files" count == 0 jsonpath "$.folders" count == 0 body not contains "{{canary_file_id}}" body not contains "ContentIndexCanaryXyzzy2026Drive" @@ -221,11 +244,11 @@ body not contains "ContentIndexCanaryXyzzy2026Drive" # other field names below MUST stay absent: a future field # called `hidden_count`/`filtered`/etc. that reveals matches # Bob can't see would be the regression. -jsonpath "$.total_count" == 0 -jsonpath "$.has_more" == false +jsonpath "$.total_count" == 0 +jsonpath "$.has_more" == false jsonpath "$.hidden_count" not exists -jsonpath "$.filtered" not exists -jsonpath "$.total" not exists +jsonpath "$.filtered" not exists +jsonpath "$.total" not exists # ─────────────────────────────────────────────────────────────