From 93452001a1ca66fa34aa77b9de2ed99547bd1cdc Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Sun, 17 May 2026 14:15:20 -0700 Subject: [PATCH] feat(api): require typed PermissionLevel on session create (#300) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - `POST /api/v1/sessions` now requires `permissions` as a typed enum (`read-only` | `read-write` | `full`) instead of accepting an optional plain string. - Removes the silent fallback at `sessions.rs:906-911` where unknown values (e.g. `"readonly"`) were coerced to `read-write` — a real security footgun: a client trying to lock the agent down would get write access instead. - Invalid or missing values are now rejected by axum's `Json` extractor with `422 Unprocessable Entity`. ## Approach - New `PermissionLevel` OpenAPI schema (`type: string, enum: [...]`). - Moves `PermissionLevel` from `fabro_agent::cli` to `fabro_types::session` so `fabro-api` can `with_replacement` it without a circular dep. `fabro_agent::cli::PermissionLevel` remains as a `pub use` re-export so existing call sites keep working. - `SessionRecord.permissions` becomes required and non-nullable for coherence — every created session has a concrete level. - `build_tool_approval` in the server takes `PermissionLevel` directly; the string-match fallback is deleted. - CLI's `session_permissions` returns a concrete `PermissionLevel` (defaults to `read-write` when neither flag nor settings provide one) and is sent explicitly on every request. ## Scope notes Confirmed out of scope and not addressed here: - Mid-session model/permission switching - Interactive tool approval / HITL ## Breaking change The `permissions` field is now required on `CreateSessionRequest` and non-nullable on `SessionRecord`. Existing on-disk session records persisted with `"permissions": null` will fail to deserialize. Acceptable per project policy (no migration); local dev users may need to clear `~/.fabro/storage/sessions/` once. ## Test plan - [x] `cargo build --workspace` - [x] `cargo nextest run -p fabro-api` — 125/125 (includes new `permission_level_round_trip` parity tests) - [x] `cargo nextest run -p fabro-server` — 554/554 (includes new 422 tests for missing + invalid permissions) - [x] `cargo nextest run -p fabro-cli` — 892/892 - [x] `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` - [x] `cargo +nightly-2026-04-14 fmt --check --all` - [x] `bun run generate` on `fabro-api-client` — emits typed `PermissionLevel` union and required field on `CreateSessionRequest` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) --- docs/public/api-reference/fabro-api.yaml | 15 +++++- lib/crates/fabro-agent/Cargo.toml | 2 +- lib/crates/fabro-agent/src/cli.rs | 11 +--- lib/crates/fabro-api/build.rs | 1 + lib/crates/fabro-api/src/lib.rs | 6 +-- .../tests/permission_level_round_trip.rs | 53 +++++++++++++++++++ lib/crates/fabro-cli/src/commands/session.rs | 11 ++-- .../src/server/handler/sessions.rs | 16 ++---- lib/crates/fabro-server/src/server/tests.rs | 40 ++++++++++++++ lib/crates/fabro-types/src/lib.rs | 4 +- lib/crates/fabro-types/src/session.rs | 27 +++++++++- .../src/.openapi-generator/FILES | 1 + .../src/models/create-session-request.ts | 7 ++- .../fabro-api-client/src/models/index.ts | 1 + .../src/models/permission-level.ts | 30 +++++++++++ .../src/models/session-record.ts | 5 +- 16 files changed, 189 insertions(+), 41 deletions(-) create mode 100644 lib/crates/fabro-api/tests/permission_level_round_trip.rs create mode 100644 lib/packages/fabro-api-client/src/models/permission-level.ts diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index c45f52933..ff0e2fa2e 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -4714,6 +4714,7 @@ components: required: - id - status + - permissions - created_at - updated_at - runtime_context @@ -4731,7 +4732,7 @@ components: model: type: ["string", "null"] permissions: - type: ["string", "null"] + $ref: "#/components/schemas/PermissionLevel" created_at: type: string format: date-time @@ -4746,6 +4747,14 @@ components: items: $ref: "#/components/schemas/SessionMessage" + PermissionLevel: + description: Agent tool permission level applied to a session. + type: string + enum: + - read-only + - read-write + - full + SessionSummary: description: List projection of a durable session. type: object @@ -4836,6 +4845,8 @@ components: CreateSessionRequest: type: object + required: + - permissions properties: title: type: string @@ -4846,7 +4857,7 @@ components: model: type: string permissions: - type: string + $ref: "#/components/schemas/PermissionLevel" UpdateSessionRequest: type: object diff --git a/lib/crates/fabro-agent/Cargo.toml b/lib/crates/fabro-agent/Cargo.toml index 058c76d25..68dbddc89 100644 --- a/lib/crates/fabro-agent/Cargo.toml +++ b/lib/crates/fabro-agent/Cargo.toml @@ -26,7 +26,7 @@ clap.workspace = true anyhow.workspace = true fabro-auth = { path = "../fabro-auth" } fabro-config = { path = "../fabro-config", features = ["clap"] } -fabro-types = { path = "../fabro-types" } +fabro-types = { path = "../fabro-types", features = ["clap"] } fabro-llm = { path = "../fabro-llm" } fabro-model = { path = "../fabro-model" } fabro-mcp = { path = "../fabro-mcp" } diff --git a/lib/crates/fabro-agent/src/cli.rs b/lib/crates/fabro-agent/src/cli.rs index af12f575b..0518d592e 100644 --- a/lib/crates/fabro-agent/src/cli.rs +++ b/lib/crates/fabro-agent/src/cli.rs @@ -92,16 +92,7 @@ pub enum OutputFormat { Json, } -/// Agent tool permission level. -#[derive( - Clone, Copy, Debug, PartialEq, Eq, serde::Deserialize, serde::Serialize, clap::ValueEnum, -)] -#[serde(rename_all = "kebab-case")] -pub enum PermissionLevel { - ReadOnly, - ReadWrite, - Full, -} +pub use fabro_types::PermissionLevel; impl AgentArgs { /// Fill `None` fields from settings.toml values, then hardcoded defaults. diff --git a/lib/crates/fabro-api/build.rs b/lib/crates/fabro-api/build.rs index 92d4d43b6..d18345aca 100644 --- a/lib/crates/fabro-api/build.rs +++ b/lib/crates/fabro-api/build.rs @@ -447,6 +447,7 @@ fn main() { ("TurnId", "fabro_types::TurnId", &[]), ("SessionStatus", "fabro_types::SessionStatus", &[]), ("TurnStatus", "fabro_types::TurnStatus", &[]), + ("PermissionLevel", "fabro_types::PermissionLevel", &[]), ("SessionMessage", "fabro_types::SessionMessage", &[]), ("SessionRecord", "fabro_types::SessionRecord", &[]), ("SessionSummary", "fabro_types::SessionSummary", &[]), diff --git a/lib/crates/fabro-api/src/lib.rs b/lib/crates/fabro-api/src/lib.rs index 4d8786b9f..4874218f2 100644 --- a/lib/crates/fabro-api/src/lib.rs +++ b/lib/crates/fabro-api/src/lib.rs @@ -35,9 +35,9 @@ pub mod types { AuthMethod, BilledTokenCounts, CommandTermination, Conclusion, DiffStats, DiffSummary, DirtyStatus, EventEnvelope, ExecOutputTail, FailureCategory, FailureDetail, FailureSignature, GitContext, IdpIdentity, InterviewOption, InterviewQuestionRecord, - PendingInterviewRecord, PreRunPushOutcome, Principal, PullRequest, PullRequestDetails, - PullRequestDetailsStatus, PullRequestDetailsUnavailableReason, PullRequestLink, - PullRequestMeta, PullRequestResponse, QuestionType, RepositoryRef, Run, + PendingInterviewRecord, PermissionLevel, PreRunPushOutcome, Principal, PullRequest, + PullRequestDetails, PullRequestDetailsStatus, PullRequestDetailsUnavailableReason, + PullRequestLink, PullRequestMeta, PullRequestResponse, QuestionType, RepositoryRef, Run, RunClientProvenance, RunEvent, RunFailure, RunProjection, RunProvenance, RunSandbox, RunSandboxRuntime, RunServerProvenance, SandboxDetails, SandboxNetwork, SandboxNetworkPolicy, SandboxNetworkPolicyMode, SandboxProvider, SandboxResources, diff --git a/lib/crates/fabro-api/tests/permission_level_round_trip.rs b/lib/crates/fabro-api/tests/permission_level_round_trip.rs new file mode 100644 index 000000000..f6473b450 --- /dev/null +++ b/lib/crates/fabro-api/tests/permission_level_round_trip.rs @@ -0,0 +1,53 @@ +use std::any::{TypeId, type_name}; + +use fabro_api::types::PermissionLevel as ApiPermissionLevel; +use fabro_types::PermissionLevel; +use serde_json::json; + +#[test] +fn permission_level_reuses_canonical_type() { + assert_same_type::(); +} + +#[test] +fn permission_level_serializes_as_kebab_case_strings() { + assert_eq!( + serde_json::to_value(PermissionLevel::ReadOnly).unwrap(), + json!("read-only") + ); + assert_eq!( + serde_json::to_value(PermissionLevel::ReadWrite).unwrap(), + json!("read-write") + ); + assert_eq!( + serde_json::to_value(PermissionLevel::Full).unwrap(), + json!("full") + ); +} + +#[test] +fn permission_level_deserializes_each_variant() { + let read_only: PermissionLevel = serde_json::from_value(json!("read-only")).unwrap(); + assert_eq!(read_only, PermissionLevel::ReadOnly); + let read_write: PermissionLevel = serde_json::from_value(json!("read-write")).unwrap(); + assert_eq!(read_write, PermissionLevel::ReadWrite); + let full: PermissionLevel = serde_json::from_value(json!("full")).unwrap(); + assert_eq!(full, PermissionLevel::Full); +} + +#[test] +fn permission_level_rejects_unknown_values() { + assert!(serde_json::from_value::(json!("readonly")).is_err()); + assert!(serde_json::from_value::(json!("read_only")).is_err()); + assert!(serde_json::from_value::(json!("")).is_err()); +} + +fn assert_same_type() { + assert_eq!( + TypeId::of::(), + TypeId::of::(), + "{} should be the same type as {}", + type_name::(), + type_name::() + ); +} diff --git a/lib/crates/fabro-cli/src/commands/session.rs b/lib/crates/fabro-cli/src/commands/session.rs index 6b42c1930..7ffb33324 100644 --- a/lib/crates/fabro-cli/src/commands/session.rs +++ b/lib/crates/fabro-cli/src/commands/session.rs @@ -1,9 +1,8 @@ use anyhow::{Result, bail}; -use fabro_agent::cli::PermissionLevel; use fabro_api::types::CreateSessionRequest; -use fabro_types::SessionEventEnvelope; use fabro_types::settings::InterpString; use fabro_types::settings::run::AgentPermissions; +use fabro_types::{PermissionLevel, SessionEventEnvelope}; use crate::args::SessionArgs; use crate::command_context::CommandContext; @@ -88,7 +87,7 @@ fn session_model(args: &SessionArgs, ctx: &CommandContext) -> Option { }) } -fn session_permissions(args: &SessionArgs, ctx: &CommandContext) -> Option { +fn session_permissions(args: &SessionArgs, ctx: &CommandContext) -> PermissionLevel { args.permissions .or_else(|| { ctx.user_settings() @@ -102,11 +101,7 @@ fn session_permissions(args: &SessionArgs, ctx: &CommandContext) -> Option PermissionLevel::Full, }) }) - .map(|permissions| match permissions { - PermissionLevel::ReadOnly => "read-only".to_string(), - PermissionLevel::ReadWrite => "read-write".to_string(), - PermissionLevel::Full => "full".to_string(), - }) + .unwrap_or(PermissionLevel::ReadWrite) } #[allow( diff --git a/lib/crates/fabro-server/src/server/handler/sessions.rs b/lib/crates/fabro-server/src/server/handler/sessions.rs index a83ed7ac0..f60ee9172 100644 --- a/lib/crates/fabro-server/src/server/handler/sessions.rs +++ b/lib/crates/fabro-server/src/server/handler/sessions.rs @@ -9,7 +9,6 @@ use axum::response::{IntoResponse, Response}; use axum::routing::{get, post}; use axum::{Json, Router}; use chrono::Utc; -use fabro_agent::cli::PermissionLevel; use fabro_agent::config::ToolApprovalFn; use fabro_agent::{ AgentEvent, AgentProfile, AnthropicProfile, Error as AgentError, GeminiProfile, LocalSandbox, @@ -19,7 +18,8 @@ use fabro_agent::{ use fabro_llm::client::Client as LlmClient; use fabro_model::{AgentProfileKind, Catalog, ModelHandle, ProviderId}; use fabro_types::{ - SessionEventEnvelope, SessionId, SessionRecord, SessionStatus, TurnId, TurnRecord, TurnStatus, + PermissionLevel, SessionEventEnvelope, SessionId, SessionRecord, SessionStatus, TurnId, + TurnRecord, TurnStatus, }; use serde_json::json; use tokio::fs; @@ -65,8 +65,7 @@ struct CreateSessionRequest { provider: Option, #[serde(default)] model: Option, - #[serde(default)] - permissions: Option, + permissions: PermissionLevel, } #[derive(Debug, serde::Deserialize)] @@ -818,7 +817,7 @@ async fn build_agent_session(state: &AppState, record: &SessionRecord) -> anyhow let config = SessionOptions { git_root: Some(working_dir.to_string_lossy().into_owned()), tool_hooks: Some(Arc::new(ToolApprovalAdapter(build_tool_approval( - record.permissions.as_deref(), + record.permissions, )))), ..SessionOptions::default() }; @@ -903,12 +902,7 @@ fn summarizer_model_id( } } -fn build_tool_approval(raw: Option<&str>) -> ToolApprovalFn { - let level = match raw.unwrap_or("read-write") { - "read-only" => PermissionLevel::ReadOnly, - "full" => PermissionLevel::Full, - _ => PermissionLevel::ReadWrite, - }; +fn build_tool_approval(level: PermissionLevel) -> ToolApprovalFn { Arc::new(move |tool_name: &str, _args: &serde_json::Value| { if is_auto_approved(level, tool_category(tool_name)) { Ok(()) diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index cb2225375..15f2e8c49 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -2179,6 +2179,46 @@ async fn create_run(app: &Router, dot_source: &str) -> String { body["id"].as_str().unwrap().to_string() } +#[tokio::test] +async fn create_session_rejects_missing_permissions() { + let state = test_app_state_with_isolated_storage(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let response = app + .oneshot( + Request::builder() + .method("POST") + .uri(api("/sessions")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::json!({"working_dir": "/tmp"}).to_string(), + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY); +} + +#[tokio::test] +async fn create_session_rejects_unknown_permission_value() { + let state = test_app_state_with_isolated_storage(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let response = app + .oneshot( + Request::builder() + .method("POST") + .uri(api("/sessions")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::json!({"permissions": "readonly"}).to_string(), + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY); +} + #[tokio::test] async fn session_apis_create_list_replay_events_and_delete() { let state = test_app_state_with_isolated_storage(); diff --git a/lib/crates/fabro-types/src/lib.rs b/lib/crates/fabro-types/src/lib.rs index 709be5963..b4d634881 100644 --- a/lib/crates/fabro-types/src/lib.rs +++ b/lib/crates/fabro-types/src/lib.rs @@ -102,8 +102,8 @@ pub use sandbox_services::{ }; pub use secret::{SecretMetadata, SecretType}; pub use session::{ - SessionEventEnvelope, SessionId, SessionMessage, SessionRecord, SessionStatus, SessionSummary, - TurnId, TurnRecord, TurnStatus, + PermissionLevel, SessionEventEnvelope, SessionId, SessionMessage, SessionRecord, SessionStatus, + SessionSummary, TurnId, TurnRecord, TurnStatus, }; pub use stage_completion::StageCompletion; pub use stage_handler::StageHandler; diff --git a/lib/crates/fabro-types/src/session.rs b/lib/crates/fabro-types/src/session.rs index a0b38d66c..99f92143c 100644 --- a/lib/crates/fabro-types/src/session.rs +++ b/lib/crates/fabro-types/src/session.rs @@ -90,6 +90,29 @@ macro_rules! ulid_id { ulid_id!(SessionId); ulid_id!(TurnId); +/// Agent tool permission level applied to a session. +#[derive( + Clone, + Copy, + Debug, + PartialEq, + Eq, + Hash, + Serialize, + Deserialize, + Display, + EnumString, + IntoStaticStr, +)] +#[cfg_attr(feature = "clap", derive(clap::ValueEnum))] +#[serde(rename_all = "kebab-case")] +#[strum(serialize_all = "kebab-case")] +pub enum PermissionLevel { + ReadOnly, + ReadWrite, + Full, +} + #[derive( Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Display, EnumString, IntoStaticStr, )] @@ -135,7 +158,7 @@ pub struct SessionRecord { pub working_dir: Option, pub provider: Option, pub model: Option, - pub permissions: Option, + pub permissions: PermissionLevel, pub created_at: DateTime, pub updated_at: DateTime, pub deleted_at: Option>, @@ -152,7 +175,7 @@ impl SessionRecord { working_dir: None, provider: None, model: None, - permissions: None, + permissions: PermissionLevel::ReadWrite, created_at: now, updated_at: now, deleted_at: None, diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index f33120152..bff855da8 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -196,6 +196,7 @@ models/paginated-turn-list.ts models/paginated-workflow-list-response.ts models/pagination-meta.ts models/pending-interview-record.ts +models/permission-level.ts models/pre-run-push-outcome-failed.ts models/pre-run-push-outcome-not-attempted.ts models/pre-run-push-outcome-skipped-no-remote.ts diff --git a/lib/packages/fabro-api-client/src/models/create-session-request.ts b/lib/packages/fabro-api-client/src/models/create-session-request.ts index 1fb3d2350..ab72408b5 100644 --- a/lib/packages/fabro-api-client/src/models/create-session-request.ts +++ b/lib/packages/fabro-api-client/src/models/create-session-request.ts @@ -13,12 +13,17 @@ */ +// May contain unused imports in some cases +// @ts-ignore +import type { PermissionLevel } from './permission-level'; export interface CreateSessionRequest { 'title'?: string; 'working_dir'?: string; 'provider'?: string; 'model'?: string; - 'permissions'?: string; + 'permissions': PermissionLevel; } + + diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index 376bfa06b..a0f8b2dd9 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -172,6 +172,7 @@ export * from './paginated-turn-list'; export * from './paginated-workflow-list-response'; export * from './pagination-meta'; export * from './pending-interview-record'; +export * from './permission-level'; export * from './pre-run-push-outcome'; export * from './pre-run-push-outcome-failed'; export * from './pre-run-push-outcome-not-attempted'; diff --git a/lib/packages/fabro-api-client/src/models/permission-level.ts b/lib/packages/fabro-api-client/src/models/permission-level.ts new file mode 100644 index 000000000..0b4042c99 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/permission-level.ts @@ -0,0 +1,30 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + + +/** + * Agent tool permission level applied to a session. + */ + +export const PermissionLevel = { + READ_ONLY: 'read-only', + READ_WRITE: 'read-write', + FULL: 'full' +} as const; + +export type PermissionLevel = typeof PermissionLevel[keyof typeof PermissionLevel]; + + + diff --git a/lib/packages/fabro-api-client/src/models/session-record.ts b/lib/packages/fabro-api-client/src/models/session-record.ts index b5cda39a9..25e1094c9 100644 --- a/lib/packages/fabro-api-client/src/models/session-record.ts +++ b/lib/packages/fabro-api-client/src/models/session-record.ts @@ -13,6 +13,9 @@ */ +// May contain unused imports in some cases +// @ts-ignore +import type { PermissionLevel } from './permission-level'; // May contain unused imports in some cases // @ts-ignore import type { SessionMessage } from './session-message'; @@ -33,7 +36,7 @@ export interface SessionRecord { 'working_dir'?: string | null; 'provider'?: string | null; 'model'?: string | null; - 'permissions'?: string | null; + 'permissions': PermissionLevel; 'created_at': string; 'updated_at': string; 'deleted_at'?: string | null;