From f483efab401f32c3e58f4cbe425f0bb0d0c5a60b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 23 May 2026 22:10:02 +0000 Subject: [PATCH 1/6] Return 403 access denied for auth failures on public projects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, any project auth failure (anonymous, missing role, or a project key scoped to a different project) responded with a generic 404 warning that the project "may be private and require authentication or it may not exist." That message exists so private projects do not leak their existence, but it is actively misleading for public projects whose existence is already obvious from the UI. Add `project_auth_error` next to `resource_not_found_error` and branch its message on visibility: public projects get a 403 with a clear "access denied" explanation; private/missing projects keep the existing info-hiding 404. Hoist `from_resource_id` into the public auth entry points so the loaded `QueryProject` (and thus `is_public()`) is in scope when building the error — no extra DB query. Covers the three project auth paths: `is_allowed` (JWT + permission), `is_allowed_actor_pub` (read endpoints), and the project-key branch of `is_allowed_actor_auth` (write endpoints). The anonymous-on-private path inside `is_allowed_public` is unchanged in outward behavior. Includes unit tests for the new helper and integration tests covering both the new 403 path and the preserved 404 info-hiding path for private projects, plus updates to the two existing tests whose 404 / 401 expectations now correctly observe the 403 access-denied response. --- lib/api_projects/tests/project_key_auth.rs | 85 ++++++++++++++++++- lib/api_projects/tests/projects.rs | 92 +++++++++++++++++++++ lib/bencher_schema/src/error.rs | 49 +++++++++++ lib/bencher_schema/src/model/project/mod.rs | 65 ++++++++------- 4 files changed, 255 insertions(+), 36 deletions(-) diff --git a/lib/api_projects/tests/project_key_auth.rs b/lib/api_projects/tests/project_key_auth.rs index 2614be65ac..cf98859901 100644 --- a/lib/api_projects/tests/project_key_auth.rs +++ b/lib/api_projects/tests/project_key_auth.rs @@ -282,9 +282,11 @@ async fn project_key_list_plots() { assert_eq!(resp.status(), StatusCode::OK); } -// Negative: project key cannot access a different project +// Negative: project key for one project gets a clear 403 when targeting a +// different *public* project — there's no need to hide the project's existence +// because the public UI already exposes it. #[tokio::test] -async fn project_key_wrong_project() { +async fn project_key_for_other_project_on_public_target_returns_403() { let server = TestServer::new().await; let user = server.signup("Key User", "keywrong@example.com").await; let org = server.create_org(&user, "Key Wrong Org").await; @@ -310,6 +312,67 @@ async fn project_key_wrong_project() { let key_created: JsonProjectKeyCreated = resp.json().await.expect("Failed to parse key"); // Try to access project B with project A's key + let resp = server + .client + .get(server.api_url(&format!("/v0/projects/{}/branches", slug_b))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(key_created.key.as_ref()), + ) + .send() + .await + .expect("Request failed"); + + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + let body = resp.text().await.expect("Failed to read response body"); + assert!( + body.contains("access denied"), + "Expected 'access denied' in body, got: {}", + body + ); +} + +// Counterpart to the public-target test above: when project B is *private*, +// we must keep info-hiding (return 404 with the standard wording) so the +// project's existence is not leaked to a holder of a key for project A. +#[cfg(feature = "plus")] +#[tokio::test] +async fn project_key_for_other_project_on_private_target_returns_404() { + use bencher_json::project::Visibility; + use bencher_schema::schema; + use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; + + let server = TestServer::new().await; + let user = server.signup("Key User", "keywrongpriv@example.com").await; + let org = server.create_org(&user, "Key Wrong Priv Org").await; + let project_a = server.create_project(&user, &org, "Priv Project A").await; + let project_b = server.create_project(&user, &org, "Priv Project B").await; + + // Make project B private — info hiding must apply on auth failure. + { + let mut conn = server.db_conn(); + diesel::update(schema::project::table.filter(schema::project::uuid.eq(project_b.uuid))) + .set(schema::project::visibility.eq(Visibility::Private)) + .execute(&mut conn) + .expect("Failed to update project visibility"); + } + + let slug_a: &str = project_a.slug.as_ref(); + let slug_b: &str = project_b.slug.as_ref(); + + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{}/keys", slug_a))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&user.token), + ) + .json(&serde_json::json!({"name": "a-key"})) + .send() + .await + .expect("Failed to create key"); + let key_created: JsonProjectKeyCreated = resp.json().await.expect("Failed to parse key"); + let resp = server .client .get(server.api_url(&format!("/v0/projects/{}/branches", slug_b))) @@ -322,6 +385,12 @@ async fn project_key_wrong_project() { .expect("Request failed"); assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let body = resp.text().await.expect("Failed to read response body"); + assert!( + body.contains("may be private"), + "Expected info-hiding wording in body, got: {}", + body + ); } // Negative: project key cannot list keys (requires Manage permission) @@ -530,7 +599,9 @@ async fn project_key_can_create_report() { let _report: JsonReport = resp.json().await.expect("Failed to parse response"); } -// Negative: project key cannot create in wrong project +// Negative: project key cannot create in a different *public* project. +// Returns 403 with a clear "access denied" message since project B's +// existence is already public information. #[tokio::test] async fn project_key_cannot_create_in_wrong_project() { let server = TestServer::new().await; @@ -571,7 +642,13 @@ async fn project_key_cannot_create_in_wrong_project() { .await .expect("Request failed"); - assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + let body = resp.text().await.expect("Failed to read response body"); + assert!( + body.contains("access denied"), + "Expected 'access denied' in body, got: {}", + body + ); } // Negative: revoked project key is rejected diff --git a/lib/api_projects/tests/projects.rs b/lib/api_projects/tests/projects.rs index 4ed9c5dce2..2094aa9bed 100644 --- a/lib/api_projects/tests/projects.rs +++ b/lib/api_projects/tests/projects.rs @@ -545,6 +545,98 @@ async fn projects_hard_delete_nonexistent() { assert_eq!(resp.status(), StatusCode::NOT_FOUND); } +// Authenticated user without an Edit role gets a clear 403 instead of the +// misleading "may be private" 404 when modifying a *public* project. +#[tokio::test] +async fn non_member_patch_public_project_returns_403() { + let server = TestServer::new().await; + let owner = server + .signup("Owner", "projpatchpubowner@example.com") + .await; + let outsider = server + .signup("Outsider", "projpatchpubother@example.com") + .await; + let org = server.create_org(&owner, "Patch Pub Org").await; + let project = server + .create_project(&owner, &org, "Patch Pub Project") + .await; + + let project_slug: &str = project.slug.as_ref(); + let body = serde_json::json!({ "name": "Hijacked" }); + let resp = server + .client + .patch(server.api_url(&format!("/v0/projects/{project_slug}"))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&outsider.token), + ) + .json(&body) + .send() + .await + .expect("Request failed"); + + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + let resp_body = resp.text().await.expect("Failed to read response body"); + assert!( + resp_body.contains("access denied"), + "Expected 'access denied' in body, got: {}", + resp_body + ); +} + +// Counterpart: when the project is *private*, the outsider must still see the +// info-hiding 404 — the project's existence must not be leaked. +#[cfg(feature = "plus")] +#[tokio::test] +async fn non_member_patch_private_project_returns_404() { + use bencher_json::project::Visibility; + use bencher_schema::schema; + use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; + + let server = TestServer::new().await; + let owner = server + .signup("Owner", "projpatchprivowner@example.com") + .await; + let outsider = server + .signup("Outsider", "projpatchprivother@example.com") + .await; + let org = server.create_org(&owner, "Patch Priv Org").await; + let project = server + .create_project(&owner, &org, "Patch Priv Project") + .await; + + // Make the project private to exercise info-hiding. + { + let mut conn = server.db_conn(); + diesel::update(schema::project::table.filter(schema::project::uuid.eq(project.uuid))) + .set(schema::project::visibility.eq(Visibility::Private)) + .execute(&mut conn) + .expect("Failed to update project visibility"); + } + + let project_slug: &str = project.slug.as_ref(); + let body = serde_json::json!({ "name": "Hijacked" }); + let resp = server + .client + .patch(server.api_url(&format!("/v0/projects/{project_slug}"))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&outsider.token), + ) + .json(&body) + .send() + .await + .expect("Request failed"); + + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let resp_body = resp.text().await.expect("Failed to read response body"); + assert!( + resp_body.contains("may be private"), + "Expected info-hiding wording in body, got: {}", + resp_body + ); +} + // Soft-delete project, verify child resource endpoints return 404 #[tokio::test] async fn projects_soft_delete_endpoints_inaccessible() { diff --git a/lib/bencher_schema/src/error.rs b/lib/bencher_schema/src/error.rs index e821b57757..b223c2775a 100644 --- a/lib/bencher_schema/src/error.rs +++ b/lib/bencher_schema/src/error.rs @@ -213,6 +213,28 @@ where )) } +/// Build an auth-failure error for a project whose visibility is known. +/// +/// When `is_public` is true the project obviously exists and revealing that +/// the request was denied costs nothing, so return a clear 403 "access denied" +/// message. When `is_public` is false the standard info-hiding 404 message is +/// returned — callers should not distinguish "private project" from "no such +/// project" to anonymous or unrelated principals. +pub fn project_auth_error(is_public: bool, value: V, error: E) -> HttpError +where + V: fmt::Debug, + E: fmt::Display, +{ + if is_public { + forbidden_error(format!( + "{resource} ({value:?}) access denied: {error}. This {resource} is public but you do not have the required permission.", + resource = BencherResource::Project, + )) + } else { + resource_not_found_error(BencherResource::Project, value, error) + } +} + pub fn with_token_hint(mut err: HttpError) -> HttpError { if err.status_code == ClientErrorStatusCode::NOT_FOUND { err.external_message = format!("{}\n{BEARER_TOKEN_FORMAT}", err.external_message); @@ -453,4 +475,31 @@ mod tests { resource_conflict_error(BencherResource::Project, "test-project", "UNIQUE failed"); assert!(is_conflict(&error)); } + + #[test] + fn project_auth_error_public_returns_forbidden() { + let error = project_auth_error(true, "my-project", "view"); + assert_eq!(error.status_code, ClientErrorStatusCode::FORBIDDEN); + assert!( + error.external_message.contains("access denied"), + "expected access denied message, got: {}", + error.external_message + ); + assert!( + error.external_message.contains("public"), + "expected public hint, got: {}", + error.external_message + ); + } + + #[test] + fn project_auth_error_private_returns_not_found_with_info_hiding() { + let error = project_auth_error(false, "my-project", "view"); + assert_eq!(error.status_code, ClientErrorStatusCode::NOT_FOUND); + assert!( + error.external_message.contains("may be private"), + "expected info-hiding wording, got: {}", + error.external_message + ); + } } diff --git a/lib/bencher_schema/src/model/project/mod.rs b/lib/bencher_schema/src/model/project/mod.rs index fd86585bf3..05bcf1f6bf 100644 --- a/lib/bencher_schema/src/model/project/mod.rs +++ b/lib/bencher_schema/src/model/project/mod.rs @@ -19,8 +19,8 @@ use crate::{ ApiContext, auth_conn, context::{DbConnection, Rbac}, error::{ - BencherResource, assert_parentage, forbidden_error, issue_error, resource_conflict_err, - resource_not_found_err, resource_not_found_error, unauthorized_error, + BencherResource, assert_parentage, forbidden_error, issue_error, project_auth_error, + resource_conflict_err, resource_not_found_err, unauthorized_error, }, macros::{ fn_get::{fn_from_uuid, fn_get, fn_get_uuid}, @@ -405,45 +405,39 @@ impl QueryProject { auth_user: &AuthUser, permission: Permission, ) -> Result { - let query_project = Self::is_allowed_inner(conn, rbac, project, auth_user, permission) - .map_err(|_e| { - resource_not_found_error(BencherResource::Project, project, permission) - })?; + let query_project = Self::from_resource_id(conn, project)?; + Self::is_allowed_inner(&query_project, rbac, auth_user, permission) + .map_err(|_e| project_auth_error(query_project.is_public(), project, permission))?; #[cfg(feature = "plus")] rate_limiting.project_request(query_project.uuid)?; Ok(query_project) } fn is_allowed_inner( - conn: &mut DbConnection, + query_project: &Self, rbac: &Rbac, - project: &ProjectResourceId, auth_user: &AuthUser, permission: Permission, - ) -> Result { - let query_project = Self::from_resource_id(conn, project)?; - query_project.try_allowed(rbac, auth_user, permission)?; - Ok(query_project) + ) -> Result<(), HttpError> { + query_project.try_allowed(rbac, auth_user, permission) } fn is_allowed_public( - conn: &mut DbConnection, + query_project: &Self, rbac: &Rbac, - project: &ProjectResourceId, public_user: &PublicUser, - ) -> Result { - let query_project = Self::from_resource_id(conn, project)?; + ) -> Result<(), HttpError> { // Check to see if the project is public // If so, anyone can access it if query_project.is_public() { - Ok(query_project) + Ok(()) } else if let PublicUser::Auth(auth_user) = public_user { // If there is an `AuthUser` then validate access // Verify that the user is allowed - query_project.try_allowed(rbac, auth_user, Permission::View)?; - Ok(query_project) + query_project.try_allowed(rbac, auth_user, Permission::View) } else { - Err(unauthorized_error(project)) + // Private project + anonymous: outer caller wraps to info-hiding 404 + Err(unauthorized_error(BencherResource::Project)) } } @@ -454,10 +448,10 @@ impl QueryProject { project: &ProjectResourceId, api_actor: &ApiActor, ) -> Result { - let query_project = - Self::is_allowed_actor_inner(conn, rbac, project, api_actor).map_err(|_e| { - resource_not_found_error(BencherResource::Project, project, Permission::View) - })?; + let query_project = Self::from_resource_id(conn, project)?; + Self::is_allowed_actor_inner(&query_project, rbac, api_actor).map_err(|_e| { + project_auth_error(query_project.is_public(), project, Permission::View) + })?; #[cfg(feature = "plus")] if api_actor.is_auth() { rate_limiting.project_request(query_project.uuid)?; @@ -466,19 +460,16 @@ impl QueryProject { } fn is_allowed_actor_inner( - conn: &mut DbConnection, + query_project: &Self, rbac: &Rbac, - project: &ProjectResourceId, api_actor: &ApiActor, - ) -> Result { + ) -> Result<(), HttpError> { match api_actor { ApiActor::Public(public_user) => { - Self::is_allowed_public(conn, rbac, project, public_user) + Self::is_allowed_public(query_project, rbac, public_user) }, ApiActor::ProjectKey(project_key_actor) => { - let query_project = Self::from_resource_id(conn, project)?; - project_key_actor.verify_project(query_project.id)?; - Ok(query_project) + project_key_actor.verify_project(query_project.id) }, } } @@ -506,7 +497,17 @@ impl QueryProject { ), ApiActor::ProjectKey(project_key_actor) => { let query_project = Self::from_resource_id(conn, project)?; - project_key_actor.verify_project(query_project.id)?; + project_key_actor + .verify_project(query_project.id) + .map_err(|e| { + if query_project.is_public() { + project_auth_error(true, project, permission) + } else { + // Private project: preserve the existing 401 so the + // project key error path is unchanged. + e + } + })?; #[cfg(feature = "plus")] rate_limiting.project_request(query_project.uuid)?; Ok(query_project) From 598687f6dd90df1f6a4fae750c956dcd52f5fe3b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 23 May 2026 22:29:00 +0000 Subject: [PATCH 2/6] Address review: tighten doc/test comments, restore uuid in inner error - Drop verbose narrative comments from test cases and the helper doc; CLAUDE.md prefers no comments when the identifier conveys intent. - Pass `query_project.uuid` into the inner `unauthorized_error` for the private-anonymous branch so the discarded inner error keeps the project identifier for logging (regression from prior commit that passed `BencherResource::Project`). --- lib/api_projects/tests/project_key_auth.rs | 10 ---------- lib/api_projects/tests/projects.rs | 5 ----- lib/bencher_schema/src/error.rs | 9 ++------- lib/bencher_schema/src/model/project/mod.rs | 4 +--- 4 files changed, 3 insertions(+), 25 deletions(-) diff --git a/lib/api_projects/tests/project_key_auth.rs b/lib/api_projects/tests/project_key_auth.rs index cf98859901..8c6615ab5d 100644 --- a/lib/api_projects/tests/project_key_auth.rs +++ b/lib/api_projects/tests/project_key_auth.rs @@ -282,9 +282,6 @@ async fn project_key_list_plots() { assert_eq!(resp.status(), StatusCode::OK); } -// Negative: project key for one project gets a clear 403 when targeting a -// different *public* project — there's no need to hide the project's existence -// because the public UI already exposes it. #[tokio::test] async fn project_key_for_other_project_on_public_target_returns_403() { let server = TestServer::new().await; @@ -332,9 +329,6 @@ async fn project_key_for_other_project_on_public_target_returns_403() { ); } -// Counterpart to the public-target test above: when project B is *private*, -// we must keep info-hiding (return 404 with the standard wording) so the -// project's existence is not leaked to a holder of a key for project A. #[cfg(feature = "plus")] #[tokio::test] async fn project_key_for_other_project_on_private_target_returns_404() { @@ -348,7 +342,6 @@ async fn project_key_for_other_project_on_private_target_returns_404() { let project_a = server.create_project(&user, &org, "Priv Project A").await; let project_b = server.create_project(&user, &org, "Priv Project B").await; - // Make project B private — info hiding must apply on auth failure. { let mut conn = server.db_conn(); diesel::update(schema::project::table.filter(schema::project::uuid.eq(project_b.uuid))) @@ -599,9 +592,6 @@ async fn project_key_can_create_report() { let _report: JsonReport = resp.json().await.expect("Failed to parse response"); } -// Negative: project key cannot create in a different *public* project. -// Returns 403 with a clear "access denied" message since project B's -// existence is already public information. #[tokio::test] async fn project_key_cannot_create_in_wrong_project() { let server = TestServer::new().await; diff --git a/lib/api_projects/tests/projects.rs b/lib/api_projects/tests/projects.rs index 2094aa9bed..43bf116d57 100644 --- a/lib/api_projects/tests/projects.rs +++ b/lib/api_projects/tests/projects.rs @@ -545,8 +545,6 @@ async fn projects_hard_delete_nonexistent() { assert_eq!(resp.status(), StatusCode::NOT_FOUND); } -// Authenticated user without an Edit role gets a clear 403 instead of the -// misleading "may be private" 404 when modifying a *public* project. #[tokio::test] async fn non_member_patch_public_project_returns_403() { let server = TestServer::new().await; @@ -584,8 +582,6 @@ async fn non_member_patch_public_project_returns_403() { ); } -// Counterpart: when the project is *private*, the outsider must still see the -// info-hiding 404 — the project's existence must not be leaked. #[cfg(feature = "plus")] #[tokio::test] async fn non_member_patch_private_project_returns_404() { @@ -605,7 +601,6 @@ async fn non_member_patch_private_project_returns_404() { .create_project(&owner, &org, "Patch Priv Project") .await; - // Make the project private to exercise info-hiding. { let mut conn = server.db_conn(); diesel::update(schema::project::table.filter(schema::project::uuid.eq(project.uuid))) diff --git a/lib/bencher_schema/src/error.rs b/lib/bencher_schema/src/error.rs index b223c2775a..fcf04261d0 100644 --- a/lib/bencher_schema/src/error.rs +++ b/lib/bencher_schema/src/error.rs @@ -213,13 +213,8 @@ where )) } -/// Build an auth-failure error for a project whose visibility is known. -/// -/// When `is_public` is true the project obviously exists and revealing that -/// the request was denied costs nothing, so return a clear 403 "access denied" -/// message. When `is_public` is false the standard info-hiding 404 message is -/// returned — callers should not distinguish "private project" from "no such -/// project" to anonymous or unrelated principals. +/// Private projects must return the info-hiding 404 — callers should not be +/// able to distinguish "private" from "nonexistent" without authorization. pub fn project_auth_error(is_public: bool, value: V, error: E) -> HttpError where V: fmt::Debug, diff --git a/lib/bencher_schema/src/model/project/mod.rs b/lib/bencher_schema/src/model/project/mod.rs index 05bcf1f6bf..163ed8545c 100644 --- a/lib/bencher_schema/src/model/project/mod.rs +++ b/lib/bencher_schema/src/model/project/mod.rs @@ -437,7 +437,7 @@ impl QueryProject { query_project.try_allowed(rbac, auth_user, Permission::View) } else { // Private project + anonymous: outer caller wraps to info-hiding 404 - Err(unauthorized_error(BencherResource::Project)) + Err(unauthorized_error(query_project.uuid)) } } @@ -503,8 +503,6 @@ impl QueryProject { if query_project.is_public() { project_auth_error(true, project, permission) } else { - // Private project: preserve the existing 401 so the - // project key error path is unchanged. e } })?; From 3d6ccef5bc68fec62b9859fa68e2c67a58846c89 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 23 May 2026 23:02:16 +0000 Subject: [PATCH 3/6] Address rearview: unify private-key error + use &self methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Close info-hiding gap in is_allowed_actor_auth's ProjectKey arm: for a private project + wrong key, return the standard project_auth_error (404 "may be private") instead of the raw 401 from verify_project. Without this fix a key holder could distinguish "private exists" from "doesn't exist" via 401 vs 404 and enumerate private slugs. - Inline is_allowed_inner — after the from_resource_id hoist it was a single delegation to try_allowed. - Reshape is_allowed_public and is_allowed_actor_inner as &self methods instead of taking a `query_project: &Self` parameter. - Lock in the new 404 wording for the private-write + wrong-key path with project_key_cannot_create_in_wrong_private_project_returns_404. --- lib/api_projects/tests/project_key_auth.rs | 60 +++++++++++++++++++++ lib/bencher_schema/src/model/project/mod.rs | 54 ++++++------------- 2 files changed, 76 insertions(+), 38 deletions(-) diff --git a/lib/api_projects/tests/project_key_auth.rs b/lib/api_projects/tests/project_key_auth.rs index 8c6615ab5d..dd07539ec5 100644 --- a/lib/api_projects/tests/project_key_auth.rs +++ b/lib/api_projects/tests/project_key_auth.rs @@ -641,6 +641,66 @@ async fn project_key_cannot_create_in_wrong_project() { ); } +#[cfg(feature = "plus")] +#[tokio::test] +async fn project_key_cannot_create_in_wrong_private_project_returns_404() { + use bencher_json::project::Visibility; + use bencher_schema::schema; + use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; + + let server = TestServer::new().await; + let user = server + .signup("Key User", "keycrosscreatepriv@example.com") + .await; + let org = server.create_org(&user, "Cross Create Priv Org").await; + let project_a = server.create_project(&user, &org, "Priv Create A").await; + let project_b = server.create_project(&user, &org, "Priv Create B").await; + + { + let mut conn = server.db_conn(); + diesel::update(schema::project::table.filter(schema::project::uuid.eq(project_b.uuid))) + .set(schema::project::visibility.eq(Visibility::Private)) + .execute(&mut conn) + .expect("Failed to update project visibility"); + } + + let slug_a: &str = project_a.slug.as_ref(); + let slug_b: &str = project_b.slug.as_ref(); + + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{}/keys", slug_a))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&user.token), + ) + .json(&serde_json::json!({"name": "cross-priv-key"})) + .send() + .await + .expect("Failed to create key"); + let key_created: JsonProjectKeyCreated = resp.json().await.expect("Failed to parse key"); + + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{}/branches", slug_b))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(key_created.key.as_ref()), + ) + .json(&serde_json::json!({"name": "cross-priv-branch"})) + .send() + .await + .expect("Request failed"); + + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let body = resp.text().await.expect("Failed to read response body"); + assert!( + body.contains("may be private"), + "Expected info-hiding wording in body, got: {}", + body + ); +} + // Negative: revoked project key is rejected #[tokio::test] async fn project_key_revoked() { diff --git a/lib/bencher_schema/src/model/project/mod.rs b/lib/bencher_schema/src/model/project/mod.rs index 163ed8545c..a496249511 100644 --- a/lib/bencher_schema/src/model/project/mod.rs +++ b/lib/bencher_schema/src/model/project/mod.rs @@ -406,38 +406,26 @@ impl QueryProject { permission: Permission, ) -> Result { let query_project = Self::from_resource_id(conn, project)?; - Self::is_allowed_inner(&query_project, rbac, auth_user, permission) + query_project + .try_allowed(rbac, auth_user, permission) .map_err(|_e| project_auth_error(query_project.is_public(), project, permission))?; #[cfg(feature = "plus")] rate_limiting.project_request(query_project.uuid)?; Ok(query_project) } - fn is_allowed_inner( - query_project: &Self, - rbac: &Rbac, - auth_user: &AuthUser, - permission: Permission, - ) -> Result<(), HttpError> { - query_project.try_allowed(rbac, auth_user, permission) - } - - fn is_allowed_public( - query_project: &Self, - rbac: &Rbac, - public_user: &PublicUser, - ) -> Result<(), HttpError> { + fn is_allowed_public(&self, rbac: &Rbac, public_user: &PublicUser) -> Result<(), HttpError> { // Check to see if the project is public // If so, anyone can access it - if query_project.is_public() { + if self.is_public() { Ok(()) } else if let PublicUser::Auth(auth_user) = public_user { // If there is an `AuthUser` then validate access // Verify that the user is allowed - query_project.try_allowed(rbac, auth_user, Permission::View) + self.try_allowed(rbac, auth_user, Permission::View) } else { // Private project + anonymous: outer caller wraps to info-hiding 404 - Err(unauthorized_error(query_project.uuid)) + Err(unauthorized_error(self.uuid)) } } @@ -449,9 +437,11 @@ impl QueryProject { api_actor: &ApiActor, ) -> Result { let query_project = Self::from_resource_id(conn, project)?; - Self::is_allowed_actor_inner(&query_project, rbac, api_actor).map_err(|_e| { - project_auth_error(query_project.is_public(), project, Permission::View) - })?; + query_project + .is_allowed_actor_inner(rbac, api_actor) + .map_err(|_e| { + project_auth_error(query_project.is_public(), project, Permission::View) + })?; #[cfg(feature = "plus")] if api_actor.is_auth() { rate_limiting.project_request(query_project.uuid)?; @@ -459,18 +449,10 @@ impl QueryProject { Ok(query_project) } - fn is_allowed_actor_inner( - query_project: &Self, - rbac: &Rbac, - api_actor: &ApiActor, - ) -> Result<(), HttpError> { + fn is_allowed_actor_inner(&self, rbac: &Rbac, api_actor: &ApiActor) -> Result<(), HttpError> { match api_actor { - ApiActor::Public(public_user) => { - Self::is_allowed_public(query_project, rbac, public_user) - }, - ApiActor::ProjectKey(project_key_actor) => { - project_key_actor.verify_project(query_project.id) - }, + ApiActor::Public(public_user) => self.is_allowed_public(rbac, public_user), + ApiActor::ProjectKey(project_key_actor) => project_key_actor.verify_project(self.id), } } @@ -499,12 +481,8 @@ impl QueryProject { let query_project = Self::from_resource_id(conn, project)?; project_key_actor .verify_project(query_project.id) - .map_err(|e| { - if query_project.is_public() { - project_auth_error(true, project, permission) - } else { - e - } + .map_err(|_e| { + project_auth_error(query_project.is_public(), project, permission) })?; #[cfg(feature = "plus")] rate_limiting.project_request(query_project.uuid)?; From 2f501e5b427f7b0b5dc5ea5dec9466f73435127b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 24 May 2026 02:45:17 +0000 Subject: [PATCH 4/6] Address rearview: hide private project existence from anonymous writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_allowed_actor_auth` was returning 401 "Authentication required" for anonymous requests before checking the project's visibility. Combined with the 404 returned by `from_resource_id` for nonexistent projects, this let an unauthenticated caller distinguish "private project exists" (401) from "no such project" (404) just by hitting a write endpoint. Restructure the method so `from_resource_id` runs first for every actor variant, then branch by visibility on the anonymous arm: - public project → honest 401 (auth required) - private project → 404 with the same "may be private" message used for nonexistent projects, hiding existence This also unifies the three actor arms around a single loaded `query_project`, matching the pattern already used in `is_allowed` and `is_allowed_actor_pub`. Add integration coverage in `testbeds.rs` for both anonymous-write paths (public → 401, private → 404). --- lib/api_projects/tests/testbeds.rs | 74 +++++++++++++++++++++ lib/bencher_schema/src/model/project/mod.rs | 34 ++++++---- 2 files changed, 94 insertions(+), 14 deletions(-) diff --git a/lib/api_projects/tests/testbeds.rs b/lib/api_projects/tests/testbeds.rs index 15c7c8a9c7..5d2d14e686 100644 --- a/lib/api_projects/tests/testbeds.rs +++ b/lib/api_projects/tests/testbeds.rs @@ -570,3 +570,77 @@ async fn testbeds_patch_spec_null() { let testbed: JsonTestbed = resp.json().await.expect("Failed to parse testbed"); assert!(testbed.spec.is_none()); } + +// POST testbeds with no auth on a public project: honest 401 (auth required). +// Exercises `is_allowed_actor_auth` anonymous + public-project path. +#[tokio::test] +async fn anonymous_create_testbed_on_public_project_returns_401() { + let server = TestServer::new().await; + let owner = server.signup("Owner", "tbanonpubowner@example.com").await; + let org = server.create_org(&owner, "Testbed Anon Pub Org").await; + let project = server + .create_project(&owner, &org, "Testbed Anon Pub Project") + .await; + + let project_slug: &str = project.slug.as_ref(); + let body = serde_json::json!({ "name": "linux-server", "slug": "linux-server" }); + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{project_slug}/testbeds"))) + .json(&body) + .send() + .await + .expect("Request failed"); + + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + let resp_body = resp.text().await.expect("Failed to read response body"); + assert!( + resp_body.contains("Authentication required"), + "Expected 'Authentication required' in body, got: {}", + resp_body + ); +} + +// POST testbeds with no auth on a private project: info-hiding 404 +// (must not distinguish "private exists" from "nonexistent"). +// Exercises `is_allowed_actor_auth` anonymous + private-project path. +#[cfg(feature = "plus")] +#[tokio::test] +async fn anonymous_create_testbed_on_private_project_returns_404() { + use bencher_json::project::Visibility; + use bencher_schema::schema; + use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; + + let server = TestServer::new().await; + let owner = server.signup("Owner", "tbanonprivowner@example.com").await; + let org = server.create_org(&owner, "Testbed Anon Priv Org").await; + let project = server + .create_project(&owner, &org, "Testbed Anon Priv Project") + .await; + + { + let mut conn = server.db_conn(); + diesel::update(schema::project::table.filter(schema::project::uuid.eq(project.uuid))) + .set(schema::project::visibility.eq(Visibility::Private)) + .execute(&mut conn) + .expect("Failed to update project visibility"); + } + + let project_slug: &str = project.slug.as_ref(); + let body = serde_json::json!({ "name": "linux-server", "slug": "linux-server" }); + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{project_slug}/testbeds"))) + .json(&body) + .send() + .await + .expect("Request failed"); + + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let resp_body = resp.text().await.expect("Failed to read response body"); + assert!( + resp_body.contains("may be private"), + "Expected info-hiding wording in body, got: {}", + resp_body + ); +} diff --git a/lib/bencher_schema/src/model/project/mod.rs b/lib/bencher_schema/src/model/project/mod.rs index a496249511..9efdd00418 100644 --- a/lib/bencher_schema/src/model/project/mod.rs +++ b/lib/bencher_schema/src/model/project/mod.rs @@ -464,31 +464,37 @@ impl QueryProject { api_actor: &ApiActor, permission: Permission, ) -> Result { + let query_project = Self::from_resource_id(conn, project)?; match api_actor { ApiActor::Public(PublicUser::Public(_)) => { - Err(unauthorized_error("Authentication required")) + // Public project: tell the client authentication is required. + // Private project: hide existence behind the same 404 used for + // nonexistent projects, so an unauthenticated caller cannot + // distinguish "private project exists" from "no such project". + return Err(if query_project.is_public() { + unauthorized_error("Authentication required") + } else { + project_auth_error(false, project, permission) + }); + }, + ApiActor::Public(PublicUser::Auth(auth_user)) => { + query_project + .try_allowed(rbac, auth_user, permission) + .map_err(|_e| { + project_auth_error(query_project.is_public(), project, permission) + })?; }, - ApiActor::Public(PublicUser::Auth(auth_user)) => Self::is_allowed( - conn, - rbac, - #[cfg(feature = "plus")] - rate_limiting, - project, - auth_user, - permission, - ), ApiActor::ProjectKey(project_key_actor) => { - let query_project = Self::from_resource_id(conn, project)?; project_key_actor .verify_project(query_project.id) .map_err(|_e| { project_auth_error(query_project.is_public(), project, permission) })?; - #[cfg(feature = "plus")] - rate_limiting.project_request(query_project.uuid)?; - Ok(query_project) }, } + #[cfg(feature = "plus")] + rate_limiting.project_request(query_project.uuid)?; + Ok(query_project) } pub fn try_allowed( From e1be36a2edf8af9143b5f7feab350b4abd7c9795 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 24 May 2026 02:57:46 +0000 Subject: [PATCH 5/6] Add non-member write-path coverage on testbed create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round out the actor matrix for `is_allowed_actor_auth`'s `Public(Auth)` arm with two cases that were missing at the child-resource level (existing PATCH non-member tests on /v0/projects/{slug} go through BearerToken, not ApiActor): - Non-member POST /testbeds on a public project → 403 "access denied" - Non-member POST /testbeds on a private project → 404 info-hide --- lib/api_projects/tests/testbeds.rs | 89 ++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/lib/api_projects/tests/testbeds.rs b/lib/api_projects/tests/testbeds.rs index 5d2d14e686..3bb7b71391 100644 --- a/lib/api_projects/tests/testbeds.rs +++ b/lib/api_projects/tests/testbeds.rs @@ -644,3 +644,92 @@ async fn anonymous_create_testbed_on_private_project_returns_404() { resp_body ); } + +// POST testbeds as an authenticated non-member on a public project: 403 "access denied". +// Exercises `is_allowed_actor_auth` Public(Auth) arm + RBAC failure on a public project. +#[tokio::test] +async fn non_member_create_testbed_on_public_project_returns_403() { + let server = TestServer::new().await; + let owner = server.signup("Owner", "tbnonmempubowner@example.com").await; + let outsider = server + .signup("Outsider", "tbnonmempubother@example.com") + .await; + let org = server.create_org(&owner, "Testbed NonMem Pub Org").await; + let project = server + .create_project(&owner, &org, "Testbed NonMem Pub Project") + .await; + + let project_slug: &str = project.slug.as_ref(); + let body = serde_json::json!({ "name": "linux-server", "slug": "linux-server" }); + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{project_slug}/testbeds"))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&outsider.token), + ) + .json(&body) + .send() + .await + .expect("Request failed"); + + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + let resp_body = resp.text().await.expect("Failed to read response body"); + assert!( + resp_body.contains("access denied"), + "Expected 'access denied' in body, got: {}", + resp_body + ); +} + +// POST testbeds as an authenticated non-member on a private project: info-hiding 404. +// Exercises `is_allowed_actor_auth` Public(Auth) arm + RBAC failure on a private project. +#[cfg(feature = "plus")] +#[tokio::test] +async fn non_member_create_testbed_on_private_project_returns_404() { + use bencher_json::project::Visibility; + use bencher_schema::schema; + use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; + + let server = TestServer::new().await; + let owner = server + .signup("Owner", "tbnonmemprivowner@example.com") + .await; + let outsider = server + .signup("Outsider", "tbnonmemprivother@example.com") + .await; + let org = server.create_org(&owner, "Testbed NonMem Priv Org").await; + let project = server + .create_project(&owner, &org, "Testbed NonMem Priv Project") + .await; + + { + let mut conn = server.db_conn(); + diesel::update(schema::project::table.filter(schema::project::uuid.eq(project.uuid))) + .set(schema::project::visibility.eq(Visibility::Private)) + .execute(&mut conn) + .expect("Failed to update project visibility"); + } + + let project_slug: &str = project.slug.as_ref(); + let body = serde_json::json!({ "name": "linux-server", "slug": "linux-server" }); + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{project_slug}/testbeds"))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&outsider.token), + ) + .json(&body) + .send() + .await + .expect("Request failed"); + + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let resp_body = resp.text().await.expect("Failed to read response body"); + assert!( + resp_body.contains("may be private"), + "Expected info-hiding wording in body, got: {}", + resp_body + ); +} From bedccffab815e3d26777a91b1fc8dbd3555558a6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 24 May 2026 14:39:05 +0000 Subject: [PATCH 6/6] Replace project_auth_error with ProjectAuthState enum The free helper only knew the 403 vs 404 split; the new ApiActor::Public(PublicUser::Public(_)) write path also needs 401 "Authentication required" on public projects (anonymous + public) without leaking private-project existence (anonymous + private). Introduces a `ProjectAuthState` enum in `error.rs` covering the four visibility x authentication combinations (PublicAnonymous, PublicAuthenticated, PrivateAnonymous, PrivateAuthenticated). The `auth_error` method on the enum centralizes the 401/403/404 policy in one place, so callers cannot accidentally pair a visibility with the wrong status. Adds two helpers on `QueryProject` so call sites read the project's own visibility rather than spelling `query_project.is_public()` at each map_err: - `auth_state(&self, &ApiActor)` for the actor-aware paths - `auth_state_authenticated(&self)` for paths that have already required authentication (no ApiActor in scope) `is_allowed_actor_auth` now closes over a single `auth_error` closure shared by all three actor arms, replacing the previous inline if/else conditional in the anonymous arm. --- lib/bencher_schema/src/error.rs | 89 ++++++++++++++++----- lib/bencher_schema/src/model/project/mod.rs | 50 +++++++----- 2 files changed, 101 insertions(+), 38 deletions(-) diff --git a/lib/bencher_schema/src/error.rs b/lib/bencher_schema/src/error.rs index fcf04261d0..16cefaa76d 100644 --- a/lib/bencher_schema/src/error.rs +++ b/lib/bencher_schema/src/error.rs @@ -213,20 +213,51 @@ where )) } -/// Private projects must return the info-hiding 404 — callers should not be -/// able to distinguish "private" from "nonexistent" without authorization. -pub fn project_auth_error(is_public: bool, value: V, error: E) -> HttpError -where - V: fmt::Debug, - E: fmt::Display, -{ - if is_public { - forbidden_error(format!( - "{resource} ({value:?}) access denied: {error}. This {resource} is public but you do not have the required permission.", - resource = BencherResource::Project, - )) - } else { - resource_not_found_error(BencherResource::Project, value, error) +/// Visibility × authentication state for project access denials. +/// Encodes the four combinations so callers cannot accidentally produce the +/// wrong HTTP status for a visibility/actor pair. +pub enum ProjectAuthState { + /// Anonymous request on a public project requiring authentication → 401. + PublicAnonymous, + /// Authenticated principal on a public project lacking permission → 403. + PublicAuthenticated, + /// Anonymous request on a private project → 404 (info-hide, same as nonexistent). + PrivateAnonymous, + /// Authenticated non-member on a private project → 404 (info-hide). + PrivateAuthenticated, +} + +impl ProjectAuthState { + pub fn new(is_public: bool, is_authenticated: bool) -> Self { + match (is_public, is_authenticated) { + (true, false) => Self::PublicAnonymous, + (true, true) => Self::PublicAuthenticated, + (false, false) => Self::PrivateAnonymous, + (false, true) => Self::PrivateAuthenticated, + } + } + + /// Build the HTTP error for this access denial. + /// Private projects must return the info-hiding 404 — callers should not + /// be able to distinguish "private" from "nonexistent" without authorization. + pub fn auth_error(self, value: V, error: E) -> HttpError + where + V: fmt::Debug, + E: fmt::Display, + { + match self { + Self::PublicAnonymous => unauthorized_error(format!( + "Authentication required to {error} {resource} ({value:?})", + resource = BencherResource::Project, + )), + Self::PublicAuthenticated => forbidden_error(format!( + "{resource} ({value:?}) access denied: {error}. This {resource} is public but you do not have the required permission.", + resource = BencherResource::Project, + )), + Self::PrivateAnonymous | Self::PrivateAuthenticated => { + resource_not_found_error(BencherResource::Project, value, error) + }, + } } } @@ -472,8 +503,8 @@ mod tests { } #[test] - fn project_auth_error_public_returns_forbidden() { - let error = project_auth_error(true, "my-project", "view"); + fn project_auth_state_public_authenticated_returns_forbidden() { + let error = ProjectAuthState::new(true, true).auth_error("my-project", "view"); assert_eq!(error.status_code, ClientErrorStatusCode::FORBIDDEN); assert!( error.external_message.contains("access denied"), @@ -488,8 +519,30 @@ mod tests { } #[test] - fn project_auth_error_private_returns_not_found_with_info_hiding() { - let error = project_auth_error(false, "my-project", "view"); + fn project_auth_state_public_anonymous_returns_unauthorized() { + let error = ProjectAuthState::new(true, false).auth_error("my-project", "view"); + assert_eq!(error.status_code, ClientErrorStatusCode::UNAUTHORIZED); + assert!( + error.external_message.contains("Authentication required"), + "expected Authentication required wording, got: {}", + error.external_message + ); + } + + #[test] + fn project_auth_state_private_authenticated_returns_not_found_with_info_hiding() { + let error = ProjectAuthState::new(false, true).auth_error("my-project", "view"); + assert_eq!(error.status_code, ClientErrorStatusCode::NOT_FOUND); + assert!( + error.external_message.contains("may be private"), + "expected info-hiding wording, got: {}", + error.external_message + ); + } + + #[test] + fn project_auth_state_private_anonymous_returns_not_found_with_info_hiding() { + let error = ProjectAuthState::new(false, false).auth_error("my-project", "view"); assert_eq!(error.status_code, ClientErrorStatusCode::NOT_FOUND); assert!( error.external_message.contains("may be private"), diff --git a/lib/bencher_schema/src/model/project/mod.rs b/lib/bencher_schema/src/model/project/mod.rs index 9efdd00418..20f10acb4c 100644 --- a/lib/bencher_schema/src/model/project/mod.rs +++ b/lib/bencher_schema/src/model/project/mod.rs @@ -19,7 +19,7 @@ use crate::{ ApiContext, auth_conn, context::{DbConnection, Rbac}, error::{ - BencherResource, assert_parentage, forbidden_error, issue_error, project_auth_error, + BencherResource, ProjectAuthState, assert_parentage, forbidden_error, issue_error, resource_conflict_err, resource_not_found_err, unauthorized_error, }, macros::{ @@ -387,6 +387,17 @@ impl QueryProject { self.visibility.is_public() } + /// Access-denial state for a given actor against this project. + pub fn auth_state(&self, api_actor: &ApiActor) -> ProjectAuthState { + ProjectAuthState::new(self.is_public(), api_actor.is_auth()) + } + + /// Access-denial state for paths that already require authentication + /// (no `ApiActor` in scope, the principal is always an authenticated user). + pub fn auth_state_authenticated(&self) -> ProjectAuthState { + ProjectAuthState::new(self.is_public(), true) + } + #[cfg(not(feature = "plus"))] pub fn is_visibility_public(visibility: Visibility) -> Result<(), HttpError> { visibility @@ -408,7 +419,11 @@ impl QueryProject { let query_project = Self::from_resource_id(conn, project)?; query_project .try_allowed(rbac, auth_user, permission) - .map_err(|_e| project_auth_error(query_project.is_public(), project, permission))?; + .map_err(|_e| { + query_project + .auth_state_authenticated() + .auth_error(project, permission) + })?; #[cfg(feature = "plus")] rate_limiting.project_request(query_project.uuid)?; Ok(query_project) @@ -440,7 +455,9 @@ impl QueryProject { query_project .is_allowed_actor_inner(rbac, api_actor) .map_err(|_e| { - project_auth_error(query_project.is_public(), project, Permission::View) + query_project + .auth_state(api_actor) + .auth_error(project, Permission::View) })?; #[cfg(feature = "plus")] if api_actor.is_auth() { @@ -465,31 +482,24 @@ impl QueryProject { permission: Permission, ) -> Result { let query_project = Self::from_resource_id(conn, project)?; + let auth_error = || { + query_project + .auth_state(api_actor) + .auth_error(project, permission) + }; match api_actor { - ApiActor::Public(PublicUser::Public(_)) => { - // Public project: tell the client authentication is required. - // Private project: hide existence behind the same 404 used for - // nonexistent projects, so an unauthenticated caller cannot - // distinguish "private project exists" from "no such project". - return Err(if query_project.is_public() { - unauthorized_error("Authentication required") - } else { - project_auth_error(false, project, permission) - }); - }, + // Anonymous on public → 401; anonymous on private → 404 info-hide + // (same as nonexistent, so callers cannot distinguish). + ApiActor::Public(PublicUser::Public(_)) => return Err(auth_error()), ApiActor::Public(PublicUser::Auth(auth_user)) => { query_project .try_allowed(rbac, auth_user, permission) - .map_err(|_e| { - project_auth_error(query_project.is_public(), project, permission) - })?; + .map_err(|_e| auth_error())?; }, ApiActor::ProjectKey(project_key_actor) => { project_key_actor .verify_project(query_project.id) - .map_err(|_e| { - project_auth_error(query_project.is_public(), project, permission) - })?; + .map_err(|_e| auth_error())?; }, } #[cfg(feature = "plus")]