mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-07 08:27:12 +00:00
feat(api): require typed PermissionLevel on session create (#300)
## 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) <noreply@anthropic.com>
This commit is contained in:
parent
581ab41d28
commit
93452001a1
16 changed files with 189 additions and 41 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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" }
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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", &[]),
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
53
lib/crates/fabro-api/tests/permission_level_round_trip.rs
Normal file
53
lib/crates/fabro-api/tests/permission_level_round_trip.rs
Normal file
|
|
@ -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::<ApiPermissionLevel, PermissionLevel>();
|
||||
}
|
||||
|
||||
#[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::<PermissionLevel>(json!("readonly")).is_err());
|
||||
assert!(serde_json::from_value::<PermissionLevel>(json!("read_only")).is_err());
|
||||
assert!(serde_json::from_value::<PermissionLevel>(json!("")).is_err());
|
||||
}
|
||||
|
||||
fn assert_same_type<T: 'static, U: 'static>() {
|
||||
assert_eq!(
|
||||
TypeId::of::<T>(),
|
||||
TypeId::of::<U>(),
|
||||
"{} should be the same type as {}",
|
||||
type_name::<T>(),
|
||||
type_name::<U>()
|
||||
);
|
||||
}
|
||||
|
|
@ -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<String> {
|
|||
})
|
||||
}
|
||||
|
||||
fn session_permissions(args: &SessionArgs, ctx: &CommandContext) -> Option<String> {
|
||||
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<Strin
|
|||
AgentPermissions::Full => 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(
|
||||
|
|
|
|||
|
|
@ -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<String>,
|
||||
#[serde(default)]
|
||||
model: Option<String>,
|
||||
#[serde(default)]
|
||||
permissions: Option<String>,
|
||||
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(())
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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<String>,
|
||||
pub provider: Option<String>,
|
||||
pub model: Option<String>,
|
||||
pub permissions: Option<String>,
|
||||
pub permissions: PermissionLevel,
|
||||
pub created_at: DateTime<Utc>,
|
||||
pub updated_at: DateTime<Utc>,
|
||||
pub deleted_at: Option<DateTime<Utc>>,
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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';
|
||||
|
|
|
|||
30
lib/packages/fabro-api-client/src/models/permission-level.ts
generated
Normal file
30
lib/packages/fabro-api-client/src/models/permission-level.ts
generated
Normal file
|
|
@ -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];
|
||||
|
||||
|
||||
|
||||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue