security(search): ensure that search suggenstion returns answer the user has access to
This commit is contained in:
@@ -27,11 +27,15 @@ pub trait SearchUseCase: Send + Sync + 'static {
|
||||
) -> Result<Arc<SearchResultsDto>, 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<SearchSuggestionsDto, DomainError>;
|
||||
|
||||
/// Clears the search results cache.
|
||||
|
||||
@@ -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<Vec<File>, DomainError> {
|
||||
let all = self.list_files(folder_id).await?;
|
||||
let q = query.to_lowercase();
|
||||
|
||||
@@ -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<SearchSuggestionsDto> {
|
||||
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<SearchSuggestionsDto> {
|
||||
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<SearchSuggestionsDto> {
|
||||
Ok(SearchSuggestionsDto {
|
||||
suggestions: Vec::new(),
|
||||
|
||||
@@ -725,6 +725,7 @@ impl SearchUseCase for StubSearchUseCase {
|
||||
_query: &str,
|
||||
_folder_id: Option<&str>,
|
||||
_limit: usize,
|
||||
_caller_id: Uuid,
|
||||
) -> Result<SearchSuggestionsDto, DomainError> {
|
||||
Ok(SearchSuggestionsDto {
|
||||
suggestions: Vec::new(),
|
||||
|
||||
@@ -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<Vec<Folder>, DomainError> {
|
||||
let all = self.list_folders(parent_id).await?;
|
||||
let q = query.to_lowercase();
|
||||
|
||||
@@ -1404,12 +1404,20 @@ impl FileReadPort for FileBlobReadRepository {
|
||||
folder_id: Option<&str>,
|
||||
query: &str,
|
||||
limit: usize,
|
||||
caller_id: Uuid,
|
||||
) -> Result<Vec<File>, 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<FileRow> = 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)
|
||||
|
||||
@@ -1178,31 +1178,39 @@ impl FolderRepository for FolderDbRepository {
|
||||
parent_id: Option<&str>,
|
||||
query: &str,
|
||||
limit: usize,
|
||||
caller_id: uuid::Uuid,
|
||||
) -> Result<Vec<Folder>, 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<FolderRow> = 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)
|
||||
|
||||
@@ -140,6 +140,7 @@ impl SearchHandler {
|
||||
/// Autocomplete suggestions for search.
|
||||
pub(super) async fn suggest_files_impl(
|
||||
State(state): State<Arc<AppState>>,
|
||||
auth_user: AuthUser,
|
||||
Query(params): Query<SuggestParams>,
|
||||
) -> 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<Arc<AppState>>,
|
||||
auth_user: AuthUser,
|
||||
query: Query<SuggestParams>,
|
||||
) -> impl IntoResponse {
|
||||
SearchHandler::suggest_files_impl(state, query).await
|
||||
SearchHandler::suggest_files_impl(state, auth_user, query).await
|
||||
}
|
||||
|
||||
#[utoipa::path(
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
# ─────────────────────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user