fix(observe): authorize OTS trajectory endpoints (ARN-187)#347
Conversation
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 <[email protected]>
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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"); |
There was a problem hiding this comment.
"default"
When an admin omits X-Tenant-Id, observe_tenant_scope returns None and the handler falls back to "default" — only the default tenant's OTS traces are returned. The sibling handle_trajectories takes the opposite approach for None: it calls state.collect_all_metadata_stores() and aggregates across all tenants. An admin trying to list OTS trajectories system-wide without knowing they must specify a tenant per request will silently receive an incomplete result. The comment "matching handle_trajectories" refers to the helper-based resolution pattern, not this None behavior — a future maintainer touching both handlers could easily overlook the divergence. A short clarification in the comment (e.g. "unlike handle_trajectories, None does not aggregate cross-tenant here") would make the design intent self-documenting.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-server/src/observe/evolution/trajectories.rs
Line: 341-350
Comment:
**Admin cross-tenant OTS listing is silently limited to `"default"`**
When an admin omits `X-Tenant-Id`, `observe_tenant_scope` returns `None` and the handler falls back to `"default"` — only the default tenant's OTS traces are returned. The sibling `handle_trajectories` takes the opposite approach for `None`: it calls `state.collect_all_metadata_stores()` and aggregates across all tenants. An admin trying to list OTS trajectories system-wide without knowing they must specify a tenant per request will silently receive an incomplete result. The comment "matching `handle_trajectories`" refers to the helper-based resolution pattern, not this `None` behavior — a future maintainer touching both handlers could easily overlook the divergence. A short clarification in the comment (e.g. "unlike `handle_trajectories`, `None` does not aggregate cross-tenant here") would make the design intent self-documenting.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| 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. |
There was a problem hiding this comment.
None-scope branch (admin without X-Tenant-Id → "default")
All six new tests supply X-Tenant-Id, so the observe_tenant_scope → None → "default" code path in both handlers is never exercised. If the unwrap_or("default") mapping were accidentally changed (e.g. to a 400 error or an all-tenants query), none of these tests would catch it. An admin calling without X-Tenant-Id currently gets silently scoped to the "default" tenant — worth one explicit test to lock that contract in.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-server/src/observe/evolution/trajectories_test.rs
Line: 107-122
Comment:
**Untested `None`-scope branch (admin without `X-Tenant-Id` → `"default"`)**
All six new tests supply `X-Tenant-Id`, so the `observe_tenant_scope` → `None` → `"default"` code path in both handlers is never exercised. If the `unwrap_or("default")` mapping were accidentally changed (e.g. to a 400 error or an all-tenants query), none of these tests would catch it. An admin calling without `X-Tenant-Id` currently gets silently scoped to the `"default"` tenant — worth one explicit test to lock that contract in.
How can I resolve this? If you propose a fix, please make it concise.
ARN-165 principle audit — request changesAdding an authentication check is necessary, but this branch still permits forged attribution and cross-tenant record corruption. Blocking findings
Required directionDerive tenant, agent, session, and actor attribution from the credential/session extension; make IDs tenant-keyed; validate all byte/count/query budgets before enqueue; use stable idempotency; add credential-backed cross-tenant overwrite/read tests and producer-to-OTS E2E coverage. The failing GitHub test job is runner “No space left on device,” not a source failure. That CI incident should be rerun, but it does not waive the security blockers above. |
1 participant
Closes ARN-187 (Urgent, security) under epic ARN-165. Draft for review — do not merge.
Hole
handle_get_ots_trajectoriesandhandle_post_ots_trajectory(crates/temper-server/src/observe/evolution/trajectories.rs) read the rawX-Tenant-Idheader and selected that tenant's store with no authorization — sibling handlers in the same file (handle_trajectories) gate, these two didn't. Any caller could read another tenant's full OTS agent-execution traces (GET) or forge trajectories into any tenant (POST) by setting a header.Fix
require_observe_auth+observe_tenant_scopeat the top of both handlers, matching the sibling pattern.Nonescope →"default"(preserves prior single-tenant behavior).Red-green
Reverting the two gates reproduces RED (unauth GET/POST returned 200/202, cross-tenant read got 200); the fix turns them GREEN. Tests: anonymous GET/POST → 403; authorized admin GET/POST still succeed; tenant-A can neither read nor write tenant-B.
cargo test -p temper-server --features observe observe::evolution::trajectories→ 6 passed (full workspace suite green at push).Notes
🤖 Generated with Claude Code
Greptile Summary
Closes a missing authorization hole in the OTS trajectory endpoints:
handle_get_ots_trajectoriesandhandle_post_ots_trajectorywere readingX-Tenant-Iddirectly with no gate, letting any caller read another tenant's agent-execution traces or forge writes into any tenant. The fix insertsrequire_observe_auth+observe_tenant_scopeat the top of both handlers, exactly matching the siblinghandle_trajectoriespattern.trajectories.rs: Auth and tenant-scope checks now precede all business logic in both OTS handlers;Nonescope (admin/single-tenant compat) maps to\"default\", preserving pre-fix behavior.trajectories_test.rs(new): Six regression tests covering anonymous deny (GET/POST), authorized admin allow (GET/POST), and cross-tenant isolation for both read and write paths.authorized_admin_*tests rely on header-only admin identity, which will need the trusted-marker migration after ARN-170 lands (documented in PR notes).Confidence Score: 4/5
The core auth fix is correct and complete — both handlers now gate identically to their sibling. Safe to review, but the PR is intentionally marked draft pending ARN-170 rebase.
The implementation is clean and the six regression tests cover the critical exploit paths. The only gap is a missing test for the admin-without-tenant branch (None → "default"), which leaves that mapping unprotected against accidental regression. The authorized-admin tests also use the pre-ARN-170 header-only identity pattern that will need updating before merge.
trajectories_test.rs: the None-scope (admin without X-Tenant-Id) path in both OTS handlers has no dedicated test.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant C as Caller participant H as Handler (GET/POST OTS) participant RA as require_observe_auth participant TS as observe_tenant_scope participant Cedar as Cedar Engine participant Store as Metadata Store C->>H: Request (headers incl. X-Tenant-Id, X-Temper-Principal-Kind) H->>RA: require_observe_auth(action, resource_type) RA->>RA: "Build SecurityContext from X-Temper-* headers" alt Admin or System principal RA-->>H: Ok(()) [bypass Cedar] else Other principal RA->>Cedar: authorize_with_context(tenant from X-Tenant-Id) Cedar-->>RA: permit / deny alt Denied RA-->>H: Err(403 FORBIDDEN) H-->>C: 403 Unauthorized else Permitted RA-->>H: Ok(()) end end H->>TS: observe_tenant_scope(headers) alt X-Tenant-Id present and non-empty TS-->>H: Ok(Some(tenant)) else Admin/System, no header TS-->>H: Ok(None) → default else Non-admin, multi-tenant, no header TS-->>H: Err(403 FORBIDDEN) H-->>C: 403 Unauthorized end H->>Store: metadata_store_for_tenant(tenant) alt No store Store-->>H: None → empty/CREATED else Store found Store-->>H: result rows / enqueue write end H-->>C: 200 OK / 201 CREATED / 202 ACCEPTED%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant C as Caller participant H as Handler (GET/POST OTS) participant RA as require_observe_auth participant TS as observe_tenant_scope participant Cedar as Cedar Engine participant Store as Metadata Store C->>H: Request (headers incl. X-Tenant-Id, X-Temper-Principal-Kind) H->>RA: require_observe_auth(action, resource_type) RA->>RA: "Build SecurityContext from X-Temper-* headers" alt Admin or System principal RA-->>H: Ok(()) [bypass Cedar] else Other principal RA->>Cedar: authorize_with_context(tenant from X-Tenant-Id) Cedar-->>RA: permit / deny alt Denied RA-->>H: Err(403 FORBIDDEN) H-->>C: 403 Unauthorized else Permitted RA-->>H: Ok(()) end end H->>TS: observe_tenant_scope(headers) alt X-Tenant-Id present and non-empty TS-->>H: Ok(Some(tenant)) else Admin/System, no header TS-->>H: Ok(None) → default else Non-admin, multi-tenant, no header TS-->>H: Err(403 FORBIDDEN) H-->>C: 403 Unauthorized end H->>Store: metadata_store_for_tenant(tenant) alt No store Store-->>H: None → empty/CREATED else Store found Store-->>H: result rows / enqueue write end H-->>C: 200 OK / 201 CREATED / 202 ACCEPTEDPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(observe): authorize OTS trajectory e..." | Re-trigger Greptile