From a61aaab1ef9fec583f5e80f5b8d99dba7e94f0c0 Mon Sep 17 00:00:00 2001 From: Rita Agafonova <36133358+rita-aga@users.noreply.github.com> Date: Tue, 7 Jul 2026 00:09:42 -0700 Subject: [PATCH] fix(observe): authorize OTS trajectory endpoints (ARN-187) handle_get_ots_trajectories and handle_post_ots_trajectory read the raw X-Tenant-Id header and selected that tenant's store with no authorization, so any caller could read another tenant's OTS agent traces or forge writes by setting a header. Add require_observe_auth + observe_tenant_scope at the top of both handlers, matching the sibling gated handlers (handle_trajectories). Same systemic class as ARN-170 (the auth edge); this per-handler gate is correct regardless and hardens further once ARN-170 lands. Co-Authored-By: Claude Fable 5 --- .../src/observe/evolution/trajectories.rs | 36 ++- .../observe/evolution/trajectories_test.rs | 266 ++++++++++++++++++ 2 files changed, 294 insertions(+), 8 deletions(-) create mode 100644 crates/temper-server/src/observe/evolution/trajectories_test.rs diff --git a/crates/temper-server/src/observe/evolution/trajectories.rs b/crates/temper-server/src/observe/evolution/trajectories.rs index cf4e6396..766bed6d 100644 --- a/crates/temper-server/src/observe/evolution/trajectories.rs +++ b/crates/temper-server/src/observe/evolution/trajectories.rs @@ -221,6 +221,21 @@ pub(crate) async fn handle_post_ots_trajectory( headers: HeaderMap, body: String, ) -> Result { + require_observe_auth(&state, &headers, "write_trajectories", "Trajectory") + .map_err(|sc| (sc, "unauthorized".to_string()))?; + // Resolve the target tenant through the shared scope helper (never the raw + // header): non-admins without a valid X-Tenant-Id in multi-tenant mode are + // rejected, and cross-tenant writes are already gated by the Cedar check + // above (which authorizes against the requested tenant's policies). `None` + // (admin/system cross-tenant or single-tenant compat) maps to the default + // tenant, matching prior behavior. + let tenant_scope = + observe_tenant_scope(&state, &headers).map_err(|sc| (sc, "unauthorized".to_string()))?; + let tenant = tenant_scope + .as_ref() + .map(|t| t.as_str()) + .unwrap_or("default"); + // Parse the OTS trajectory JSON to extract indexed fields. let trajectory: serde_json::Value = serde_json::from_str(&body) .map_err(|e| (StatusCode::BAD_REQUEST, format!("invalid JSON: {e}")))?; @@ -260,11 +275,6 @@ pub(crate) async fn handle_post_ots_trajectory( .map(|a| a.len() as i64) .unwrap_or(0); - let tenant = headers - .get("X-Tenant-Id") - .and_then(|v| v.to_str().ok()) - .unwrap_or("default"); - let Some(store) = state.metadata_store_for_tenant(tenant).await else { tracing::warn!( tenant = %tenant, @@ -328,9 +338,15 @@ pub(crate) async fn handle_get_ots_trajectories( headers: HeaderMap, Query(params): Query, ) -> Result, StatusCode> { - let tenant = headers - .get("X-Tenant-Id") - .and_then(|v| v.to_str().ok()) + require_observe_auth(&state, &headers, "read_trajectories", "Trajectory")?; + // Resolve the tenant through the shared scope helper (never the raw header), + // matching `handle_trajectories`. `None` (admin/system cross-tenant or + // single-tenant compat) maps to the default tenant, since OTS listing is + // per-tenant by construction (`list_ots_trajectories` filters on tenant). + let tenant_scope = observe_tenant_scope(&state, &headers)?; + let tenant = tenant_scope + .as_ref() + .map(|t| t.as_str()) .unwrap_or("default"); let limit = params.limit.unwrap_or(50).min(500); @@ -366,3 +382,7 @@ pub(crate) async fn handle_get_ots_trajectories( } } } + +#[cfg(test)] +#[path = "trajectories_test.rs"] +mod tests; diff --git a/crates/temper-server/src/observe/evolution/trajectories_test.rs b/crates/temper-server/src/observe/evolution/trajectories_test.rs new file mode 100644 index 00000000..8bea045f --- /dev/null +++ b/crates/temper-server/src/observe/evolution/trajectories_test.rs @@ -0,0 +1,266 @@ +//! Security regression tests for the OTS trajectory endpoints (ARN-187). +//! +//! The OTS list/ingest handlers (`GET`/`POST /api/ots/trajectories`) previously +//! read the raw `X-Tenant-Id` header and selected that tenant's store **without +//! any authorization check**, so any caller could read another tenant's full +//! agent-execution traces or forge writes into any tenant simply by setting a +//! header. These tests lock in the gate: both handlers now call +//! `require_observe_auth` and resolve the tenant via `observe_tenant_scope`, +//! matching the sibling `handle_trajectories` / `handle_unmet_intent` handlers. +//! +//! NOTE: these tests live behind the `observe` feature (the whole `observe`/ +//! `api` module tree is `#[cfg(feature = "observe")]`), so they only compile +//! and run with `--features observe` (or `cargo test --workspace`, where +//! `temper-cli`/`temper-mcp` turn the feature on). A bare +//! `cargo test -p temper-server` silently filters them out. + +use axum::Router; +use axum::body::Body; +use axum::http::{Request, StatusCode}; +use tower::ServiceExt; + +use temper_runtime::ActorSystem; + +use crate::ServerState; +use crate::registry::SpecRegistry; + +/// Enforcing baseline policy: only `Admin` principals are permitted anything. +/// Non-admin (Customer/Agent) principals are denied unless a per-tenant policy +/// grants them access. Production never runs the permissive engine, so we swap +/// it out here to exercise the real deny path. +const ADMIN_ONLY_POLICY: &str = r#"permit(principal is Admin, action, resource);"#; + +/// A minimal, well-formed OTS trajectory payload. +const SAMPLE_TRAJECTORY: &str = + r#"{"metadata":{"trajectory_id":"traj-1","agent_id":"a1","outcome":"success"},"turns":[]}"#; + +/// Build a `ServerState` whose Cedar engine actually enforces (admin-only +/// baseline) instead of the permissive test default. +fn enforcing_state() -> ServerState { + let system = ActorSystem::new("test-ots-auth"); + let state = ServerState::from_registry(system, SpecRegistry::new()); + state + .authz + .reload_policies(ADMIN_ONLY_POLICY) + .expect("baseline policy should parse"); + state +} + +/// Mount the management API (which owns `/ots/trajectories`) under `/api`. +fn app(state: ServerState) -> Router { + Router::new() + .nest("/api", crate::api::build_api_router()) + .with_state(state) +} + +/// Exploit (a): an anonymous caller spoofs a victim tenant via `X-Tenant-Id` +/// and reads its OTS traces. Before the fix this returned `200` with the +/// victim tenant's data; it must now be denied. +#[tokio::test] +async fn unauthenticated_get_ots_trajectories_is_denied() { + let app = app(enforcing_state()); + let resp = app + .oneshot( + Request::get("/api/ots/trajectories") + .header("X-Tenant-Id", "victim-tenant") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::FORBIDDEN, + "anonymous cross-tenant OTS read must be denied (was 200 before ARN-187 fix)" + ); +} + +/// Exploit (b): an anonymous caller forges an OTS trajectory into a victim +/// tenant. Before the fix this returned `202/201`; it must now be denied. +#[tokio::test] +async fn unauthenticated_post_ots_trajectory_is_denied() { + let app = app(enforcing_state()); + let resp = app + .oneshot( + Request::post("/api/ots/trajectories") + .header("content-type", "application/json") + .header("X-Tenant-Id", "victim-tenant") + .body(Body::from(SAMPLE_TRAJECTORY)) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::FORBIDDEN, + "anonymous forged OTS write must be denied (was accepted before ARN-187 fix)" + ); +} + +/// A properly-authorized (admin) same-tenant GET still succeeds — the gate must +/// not over-block legitimate callers. +#[tokio::test] +async fn authorized_admin_get_ots_trajectories_succeeds() { + let app = app(enforcing_state()); + let resp = app + .oneshot( + Request::get("/api/ots/trajectories") + .header("X-Temper-Principal-Kind", "admin") + .header("X-Tenant-Id", "tenant-a") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::OK, + "authorized admin read should still succeed" + ); +} + +/// A properly-authorized (admin) same-tenant POST still succeeds. +#[tokio::test] +async fn authorized_admin_post_ots_trajectory_succeeds() { + let app = app(enforcing_state()); + let resp = app + .oneshot( + Request::post("/api/ots/trajectories") + .header("content-type", "application/json") + .header("X-Temper-Principal-Kind", "admin") + .header("X-Tenant-Id", "tenant-a") + .body(Body::from(SAMPLE_TRAJECTORY)) + .unwrap(), + ) + .await + .unwrap(); + // With no durable metadata store configured the handler short-circuits to + // `201 Created`; the point is that an authorized write is *not* forbidden. + assert_eq!( + resp.status(), + StatusCode::CREATED, + "authorized admin write should still succeed" + ); +} + +/// Cross-tenant isolation: a credential scoped to `tenant-a` can read its own +/// trajectories but MUST NOT read `tenant-b`'s by spoofing the header. +#[tokio::test] +async fn tenant_scoped_credential_cannot_read_other_tenant() { + let system = ActorSystem::new("test-ots-cross-tenant"); + let state = ServerState::from_registry(system, SpecRegistry::new()); + state + .authz + .reload_policies(ADMIN_ONLY_POLICY) + .expect("baseline policy should parse"); + // `tenant-a` grants its agents read access to trajectories. `tenant-b` + // grants this principal nothing (falls back to the admin-only baseline). + state + .authz + .reload_tenant_policies( + "tenant-a", + r#"permit(principal is Agent, action == Action::"read_trajectories", resource is Trajectory);"#, + ) + .expect("tenant-a policy should parse"); + + let app = app(state); + + // Same-tenant read (tenant-a agent → tenant-a) succeeds. + let same_tenant = app + .clone() + .oneshot( + Request::get("/api/ots/trajectories") + .header("X-Temper-Principal-Kind", "agent") + .header("X-Temper-Principal-Id", "agent-a") + .header("X-Tenant-Id", "tenant-a") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + same_tenant.status(), + StatusCode::OK, + "same-tenant authorized read should succeed" + ); + + // Cross-tenant read (tenant-a agent → tenant-b) is denied. + let cross_tenant = app + .oneshot( + Request::get("/api/ots/trajectories") + .header("X-Temper-Principal-Kind", "agent") + .header("X-Temper-Principal-Id", "agent-a") + .header("X-Tenant-Id", "tenant-b") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + cross_tenant.status(), + StatusCode::FORBIDDEN, + "tenant-a credential must not read tenant-b trajectories" + ); +} + +/// Cross-tenant isolation on the write path: a credential scoped to `tenant-a` +/// can ingest its own trajectories but MUST NOT forge writes into `tenant-b`. +#[tokio::test] +async fn tenant_scoped_credential_cannot_write_other_tenant() { + let system = ActorSystem::new("test-ots-cross-tenant-write"); + let state = ServerState::from_registry(system, SpecRegistry::new()); + state + .authz + .reload_policies(ADMIN_ONLY_POLICY) + .expect("baseline policy should parse"); + // `tenant-a` grants its agents write access; `tenant-b` grants this + // principal nothing (falls back to the admin-only baseline). + state + .authz + .reload_tenant_policies( + "tenant-a", + r#"permit(principal is Agent, action == Action::"write_trajectories", resource is Trajectory);"#, + ) + .expect("tenant-a policy should parse"); + + let app = app(state); + + // Same-tenant write (tenant-a agent → tenant-a) succeeds. + let same_tenant = app + .clone() + .oneshot( + Request::post("/api/ots/trajectories") + .header("content-type", "application/json") + .header("X-Temper-Principal-Kind", "agent") + .header("X-Temper-Principal-Id", "agent-a") + .header("X-Tenant-Id", "tenant-a") + .body(Body::from(SAMPLE_TRAJECTORY)) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + same_tenant.status(), + StatusCode::CREATED, + "same-tenant authorized write should succeed" + ); + + // Cross-tenant write (tenant-a agent → tenant-b) is denied. + let cross_tenant = app + .oneshot( + Request::post("/api/ots/trajectories") + .header("content-type", "application/json") + .header("X-Temper-Principal-Kind", "agent") + .header("X-Temper-Principal-Id", "agent-a") + .header("X-Tenant-Id", "tenant-b") + .body(Body::from(SAMPLE_TRAJECTORY)) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + cross_tenant.status(), + StatusCode::FORBIDDEN, + "tenant-a credential must not write tenant-b trajectories" + ); +}