scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-14 23:22:51 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

Drop fabro's explicit-env credential filter

Browse source 
SandboxExec carried an ExplicitEnvPolicy that, for local runs, dropped
credential-shaped names out of the caller's explicit environment before
the spec reached the driver. The filter duplicated the sandbox driver's
Host provider, which applies the same safelist and suffix list to the
inherited process environment and, by its own contract, leaves explicit
spec env alone as the deliberate channel for secrets. Since fabro
composes the explicit environment itself, the second filter added no
protection. It only stripped variables a caller had set on purpose, such
as a GITHUB_TOKEN for a local command stage, and it forced every
constructor to pick a policy by provider kind.

This removes ExplicitEnvPolicy, the safelist, is_sensitive_env_var, and
the env_policy field on SandboxExec and RunSandbox. SandboxExec::new
takes only the exec facet, and the explicit environment goes to the
provider as composed on every provider. The tests that exercised the
filter are replaced by one that shows a credential-shaped explicit
variable reaching the command on the Host provider; the BASH_ENV test
stays, since that blank is the driver's and still holds.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-09-11 14:53:10 -06:00
parent ab3ef9371a
commit 99d226250e
No known key found for this signature in database
5 changed files with 53 additions and 147 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
lib/components/fabro-sandbox/src/clone.rs
View file

@ -202,7 +202,6 @@ mod tests {
use sandbox_driver_testing::ScriptedSandbox;
use super::*;
use crate::exec::ExplicitEnvPolicy;
const ORIGIN: &str = "https://github.com/acme/widgets";
@ -238,7 +237,7 @@ mod tests {
}
async fn clone_with(handle: &ScriptedSandbox, credentials: &RepoCredentials) -> CloneOutcome {
let exec = SandboxExec::new(handle.exec(), ExplicitEnvPolicy::TrustCaller);
let exec = SandboxExec::new(handle.exec());
clone_github_repo(
&SandboxProviderKind::DOCKER,
handle,

40
lib/components/fabro-sandbox/src/driver_sandbox.rs
View file

@ -6,11 +6,10 @@
//! through the handle itself. Nothing here knows which provider is behind
//! the handle or whether it runs in-process or over the plugin wire.
//!
//! What stays fabro's: the exec ladder, the credential filter on explicit
//! environment variables, and the run-facing conventions (`platform` names,
//! grep line format, walk results relative to a caller-declared base). The
//! driver reports lifecycle events itself, through the [`EventContext`] a
//! sandbox is created or attached with.
//! What stays fabro's: the exec ladder and the run-facing conventions
//! (`platform` names, grep line format, walk results relative to a
//! caller-declared base). The driver reports lifecycle events itself,
//! through the [`EventContext`] a sandbox is created or attached with.
use std::collections::HashMap;
use std::path::{Path, PathBuf};
@ -70,7 +69,7 @@ pub async fn local_sandbox_with_events(
sandbox.learn_platform().await?;
Ok(sandbox)
}
use crate::exec::{ExplicitEnvPolicy, SandboxExec};
use crate::exec::SandboxExec;
use crate::sandbox::{self, PushError, PushReport, SandboxFile, SandboxWorkspaceLayout};
/// Where a clone-based provider puts its files: the run works under
@ -281,28 +280,25 @@ struct PendingCreate {
/// A fabro sandbox backed by a sandbox-driver handle.
pub struct RunSandbox {
kind: SandboxProviderKind,
kind: SandboxProviderKind,
/// Set at construction for an existing sandbox, at `initialize` for a
/// pending one.
handle: OnceCell<Arc<dyn DriverHandle>>,
pending: Option<PendingCreate>,
workspace: Option<RepoWorkspace>,
env_policy: ExplicitEnvPolicy,
handle: OnceCell<Arc<dyn DriverHandle>>,
pending: Option<PendingCreate>,
workspace: Option<RepoWorkspace>,
/// Where the driver reports the lifecycle of a sandbox this creates.
/// Set before `initialize` on a pending sandbox; an existing handle
/// already carries the context it was created or attached with.
events: Option<EventContext>,
events: Option<EventContext>,
/// `(platform, os_version)` learned from the sandbox at initialize or
/// start; unknown until then.
platform: OnceLock<(String, String)>,
platform: OnceLock<(String, String)>,
/// The provider snapshot the sandbox was created from, when known.
snapshot: OnceLock<String>,
snapshot: OnceLock<String>,
}
impl RunSandbox {
/// Wraps a driver handle. `local` runs on the worker host, so explicit
/// environment variables pass the credential filter; every other kind
/// is isolated and takes the caller's environment as composed.
/// Wraps an existing driver handle as a sandbox of `kind`.
#[must_use]
pub fn new(kind: SandboxProviderKind, handle: Arc<dyn DriverHandle>) -> Self {
let sandbox = Self::empty(kind);
@ -357,17 +353,11 @@ impl RunSandbox {
}
fn empty(kind: SandboxProviderKind) -> Self {
let env_policy = if kind.is_local() {
ExplicitEnvPolicy::FilterSensitive
} else {
ExplicitEnvPolicy::TrustCaller
};
Self {
kind,
handle: OnceCell::new(),
pending: None,
workspace: None,
env_policy,
events: None,
platform: OnceLock::new(),
snapshot: OnceLock::new(),
@ -402,7 +392,7 @@ impl RunSandbox {
/// Fabro's exec policy over the driver's exec facet, working in the
/// run's directory. Absent until a pending sandbox is initialized.
pub fn exec(&self) -> crate::Result<SandboxExec<'_>> {
let mut exec = SandboxExec::new(self.handle()?.exec(), self.env_policy);
let mut exec = SandboxExec::new(self.handle()?.exec());
if let Some(workspace) = &self.workspace {
if let Some(dir) = workspace.execution_directory.get() {
exec = exec.with_working_dir(dir.clone());
@ -540,7 +530,7 @@ impl RunSandbox {
let handle = self.handle()?;
// The clone names every directory it touches, so it runs
// without fabro's working-directory override.
let exec = SandboxExec::new(handle.exec(), self.env_policy);
let exec = SandboxExec::new(handle.exec());
let outcome = clone::clone_github_repo(
&self.kind,
handle.as_ref(),

149
lib/components/fabro-sandbox/src/exec.rs
View file

@ -5,8 +5,8 @@
//! module adds fabro's policy on the way in and fabro's reading of a result
//! on the way out.
//!
//! A command runs as Bash source under `bash -c` with `BASH_ENV` blanked,
//! and ends in one of three ways:
//! A command runs as Bash source under `bash -c` with `BASH_ENV` blanked by
//! the driver whatever the caller passed, and ends in one of three ways:
//!
//! - **timeout**: the spec's timeout fires and the provider runs the stop
//! ladder fabro asks for — `TERM`, then `KILL` after
@ -22,15 +22,15 @@
//! [`OutputSanitization::StripAll`]: terminal escape sequences and stray
//! control characters never reach a result, a sink chunk, or a tail. Secret
//! redaction stays fabro's job and happens only when a tail is rendered for
//! events or logs ([`ExecResultExt`]). Explicit environment variables pass
//! through a fail-closed secret filter under
//! [`ExplicitEnvPolicy::FilterSensitive`], matching what the Host provider
//! already does for inherited variables.
//! events or logs ([`ExecResultExt`]). The explicit environment reaches the
//! provider as the caller composed it: the driver filters credential-shaped
//! names out of the *inherited* host environment itself and treats the
//! spec's own variables as the deliberate channel for secrets, so fabro adds
//! no filter of its own.
use std::collections::{BTreeMap, HashMap};
use std::collections::HashMap;
use std::time::Duration;
use fabro_static::EnvVars;
use fabro_types::{CommandTermination, ExecOutputTail};
use sandbox_driver::{
Exec, ExecControls, ExecFailure, ExecResult, ExecSpec, ExecStreamingResult, OutputSanitization,
@ -47,50 +47,9 @@ pub const DEFAULT_STOP_GRACE: Duration = Duration::from_secs(2);
/// renders, bounded so a runaway command cannot exhaust memory.
pub const DEFAULT_RETAINED_OUTPUT_BYTES: usize = sandbox_driver::DEFAULT_BUFFER_BYTES;
/// How explicit per-command environment variables are treated.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ExplicitEnvPolicy {
/// Drop variables whose names look like credentials unless safelisted.
/// Used where the command runs on the worker host and the caller's env
/// may carry worker secrets.
FilterSensitive,
/// Pass every variable through. Used for isolated providers, where the
/// caller composed the environment deliberately.
TrustCaller,
}
/// Variables that look like credentials but are needed by ordinary tools.
const ENV_SAFELIST: &[&str] = &[
EnvVars::PATH,
EnvVars::HOME,
EnvVars::USER,
EnvVars::SHELL,
EnvVars::LANG,
EnvVars::TERM,
EnvVars::TMPDIR,
EnvVars::GOPATH,
EnvVars::CARGO_HOME,
EnvVars::NVM_DIR,
];
/// Whether an environment variable name looks like a credential.
#[must_use]
pub fn is_sensitive_env_var(key: &str) -> bool {
if ENV_SAFELIST.contains(&key) {
return false;
}
let lower = key.to_lowercase();
lower.ends_with("_api_key")
|| lower.ends_with("_secret")
|| lower.ends_with("_token")
|| lower.ends_with("_password")
|| lower.ends_with("_credential")
}
/// Fabro's exec policy bound to one driver [`Exec`] facet.
pub struct SandboxExec<'a> {
exec: &'a dyn Exec,
env_policy: ExplicitEnvPolicy,
stop_grace: Duration,
/// Where a command runs when the caller names no directory. `None`
/// leaves the choice to the provider's own working directory.
@ -99,10 +58,9 @@ pub struct SandboxExec<'a> {
impl<'a> SandboxExec<'a> {
#[must_use]
pub fn new(exec: &'a dyn Exec, env_policy: ExplicitEnvPolicy) -> Self {
pub fn new(exec: &'a dyn Exec) -> Self {
Self {
exec,
env_policy,
stop_grace: DEFAULT_STOP_GRACE,
working_dir: None,
}
@ -165,11 +123,11 @@ impl<'a> SandboxExec<'a> {
/// `controls.sink` as it arrives.
///
/// The policy fills what the spec leaves open: the stop grace, the
/// working directory, the text output policy, and the explicit
/// environment filter. The caller's `controls.term` is the `term` stop;
/// the provider runs the grace and the `kill` itself. Output beyond
/// `controls.retained_output_limit` (fabro's default when unset) is
/// drained and counted, not kept.
/// working directory, and the text output policy. The spec's environment
/// goes to the provider as the caller composed it. The caller's
/// `controls.term` is the `term` stop; the provider runs the grace and
/// the `kill` itself. Output beyond `controls.retained_output_limit`
/// (fabro's default when unset) is drained and counted, not kept.
pub async fn run_streaming(
&self,
spec: ExecSpec,
@ -200,7 +158,6 @@ impl<'a> SandboxExec<'a> {
for (key, value) in env_vars.into_iter().flatten() {
spec = spec.env_var(key, value);
}
self.apply_env_policy(&mut spec.env);
Ok(self.exec.spawn_stdio(&spec).await?)
}
@ -220,19 +177,8 @@ impl<'a> SandboxExec<'a> {
if spec.output_sanitization == OutputSanitization::default() {
spec.output_sanitization = OutputSanitization::StripAll;
}
self.apply_env_policy(&mut spec.env);
spec
}
/// The explicit environment after policy: credential-shaped names pass
/// only under `TrustCaller`. The driver's Bash helper blanks `BASH_ENV`
/// at launch whatever the caller passed, so a worker's startup file
/// never runs inside a sandboxed `bash -c`.
fn apply_env_policy(&self, env: &mut BTreeMap<String, String>) {
if self.env_policy == ExplicitEnvPolicy::FilterSensitive {
env.retain(|key, _| !is_sensitive_env_var(key));
}
}
}
/// The driver says how the command ended; fabro's event vocabulary has two
@ -371,15 +317,15 @@ mod tests {
}
}
fn exec(&self, policy: ExplicitEnvPolicy) -> SandboxExec<'_> {
fn exec(&self) -> SandboxExec<'_> {
let _ = &self.provider;
SandboxExec::new(self.sandbox.exec(), policy)
SandboxExec::new(self.sandbox.exec())
}
}
async fn run(fixture: &HostFixture, command: &str) -> ExecResult {
fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
.exec()
.run(command, Some(Duration::from_secs(10)), None, None, None)
.await
.unwrap()
@ -432,7 +378,7 @@ mod tests {
.unwrap();
let env = HashMap::from([(BASH_ENV_VAR.to_string(), startup.display().to_string())]);
let result = fixture
.exec(ExplicitEnvPolicy::TrustCaller)
.exec()
.run(
"echo body",
Some(Duration::from_secs(10)),
@ -446,45 +392,20 @@ mod tests {
}
#[tokio::test]
async fn filter_sensitive_drops_credential_shaped_explicit_variables() {
async fn explicit_variables_reach_the_command_as_composed() {
let fixture = HostFixture::new().await;
let env = HashMap::from([
("FABRO_WORKER_TOKEN".to_string(), "leaked".to_string()),
("FABRO_WORKER_TOKEN".to_string(), "deliberate".to_string()),
("MY_VAR".to_string(), "ok".to_string()),
]);
let filtered = fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
let stdout = fixture
.exec()
.run("env", Some(Duration::from_secs(10)), None, Some(&env), None)
.await
.unwrap()
.stdout_lossy();
assert!(!filtered.contains("FABRO_WORKER_TOKEN=leaked"));
assert!(filtered.contains("MY_VAR=ok"));
let trusted = fixture
.exec(ExplicitEnvPolicy::TrustCaller)
.run("env", Some(Duration::from_secs(10)), None, Some(&env), None)
.await
.unwrap()
.stdout_lossy();
assert!(trusted.contains("FABRO_WORKER_TOKEN=leaked"));
}
#[test]
fn sensitive_name_classification_matches_the_worker_policy() {
for key in [
"OPENAI_API_KEY",
"DB_PASSWORD",
"AWS_SECRET",
"AUTH_TOKEN",
"MY_CREDENTIAL",
"FABRO_WORKER_TOKEN",
] {
assert!(is_sensitive_env_var(key), "{key}");
}
for key in ["PATH", "HOME", "MY_VAR", "GITHUB_ACTOR"] {
assert!(!is_sensitive_env_var(key), "{key}");
}
assert!(stdout.contains("FABRO_WORKER_TOKEN=deliberate"), "{stdout}");
assert!(stdout.contains("MY_VAR=ok"), "{stdout}");
}
#[tokio::test]
@ -492,7 +413,7 @@ mod tests {
let fixture = HostFixture::new().await;
let started = Instant::now();
let result = fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
.exec()
.run(
"sleep 10",
Some(Duration::from_millis(200)),
@ -515,7 +436,7 @@ mod tests {
let fixture = HostFixture::new().await;
let started = Instant::now();
let result = fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
.exec()
.with_stop_grace(Duration::from_millis(300))
.run(
"trap '' TERM; sleep 10",
@ -542,7 +463,7 @@ mod tests {
cancel.cancel();
});
let result = fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
.exec()
.run(
"sleep 10",
Some(Duration::from_secs(30)),
@ -570,7 +491,7 @@ mod tests {
})
});
let streaming = fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
.exec()
.run_streaming(
ExecSpec::bash("for i in $(seq 1 200); do echo line-$i; done")
.timeout(Duration::from_secs(10)),
@ -598,7 +519,7 @@ mod tests {
let fixture = HostFixture::new().await;
let stdin = b"first line\n$(touch must-not-run)\nlast line".to_vec();
let streaming = fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
.exec()
.run_streaming(
ExecSpec::bash("cat; test -e must-not-run && echo RAN")
.timeout(Duration::from_secs(10))
@ -621,7 +542,7 @@ mod tests {
})
});
let error = fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
.exec()
.run_streaming(
ExecSpec::bash("echo hello; sleep 5").timeout(Duration::from_secs(10)),
ExecControls {
@ -642,11 +563,7 @@ mod tests {
#[tokio::test]
async fn stdio_process_round_trips_lines_and_reports_exit() {
let fixture = HostFixture::new().await;
let process = fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
.spawn_stdio("cat", None, None)
.await
.unwrap();
let process = fixture.exec().spawn_stdio("cat", None, None).await.unwrap();
let mut stdin = process.stdin;
let mut stdout = BufReader::new(process.stdout);
stdin.write_all(b"ping\n").await.unwrap();
@ -663,7 +580,7 @@ mod tests {
async fn stdio_process_terminates_on_request_and_keeps_a_stderr_tail() {
let fixture = HostFixture::new().await;
let process = fixture
.exec(ExplicitEnvPolicy::FilterSensitive)
.exec()
.spawn_stdio("sh -c 'echo diag >&2; sleep 30'", None, None)
.await
.unwrap();
@ -771,7 +688,7 @@ mod tests {
#[tokio::test]
async fn policy_strips_output_unless_the_caller_chose_another_policy() {
let fixture = HostFixture::new().await;
let exec = fixture.exec(ExplicitEnvPolicy::FilterSensitive);
let exec = fixture.exec();
assert_eq!(
exec.apply_policy(ExecSpec::bash("true"))
.output_sanitization,

4
lib/components/fabro-sandbox/src/lib.rs
View file

@ -37,8 +37,8 @@ pub use driver_sandbox::{RunSandbox, local_sandbox};
pub use environment::{CloneRequest, sandbox_spec_for_environment};
pub use error::{Error, Result, default_redacted_output_tail, display_for_log};
pub use exec::{
DEFAULT_RETAINED_OUTPUT_BYTES, DEFAULT_STOP_GRACE, ExecResultExt, ExplicitEnvPolicy,
SandboxExec, command_termination, is_sensitive_env_var, program_exit_code,
DEFAULT_RETAINED_OUTPUT_BYTES, DEFAULT_STOP_GRACE, ExecResultExt, SandboxExec,
command_termination, program_exit_code,
};
pub use fabro_github::token_source::{
InstallationTokenSource, ResolvedToken, TokenProvenance, TokenSnapshot,

4
lib/components/fabro-sandbox/src/test_support.rs
View file

@ -176,8 +176,8 @@ impl MockSandbox {
fn built(&self) -> &Built {
self.built.get_or_init(|| {
let driver = Arc::new(self.build_driver());
// An isolated provider: explicit environment passes as the
// caller composed it, as it does for Docker and Daytona runs.
// The kind is nominal for exec: the explicit environment reaches
// the scripted driver as the caller composed it on every provider.
let run = RunSandbox::new_with_platform(
SandboxProviderKind::DOCKER,
Arc::clone(&driver) as Arc<dyn sandbox_driver::Sandbox>,