From 2668d16799f3fae89e6e0cbc73d0e97ea47e7aa9 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 1 May 2026 14:30:05 -0400 Subject: [PATCH] fix(cli): remove dev-token env auth override --- docs/public/reference/cli.mdx | 2 +- .../fabro-cli/src/commands/auth/status.rs | 41 +----------------- lib/crates/fabro-cli/src/server_client.rs | 43 +++---------------- lib/crates/fabro-cli/tests/it/cmd/auth.rs | 27 ++++++------ lib/crates/fabro-cli/tests/it/cmd/dump.rs | 12 ++++-- .../fabro-cli/tests/it/cmd/json_global.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/ps.rs | 9 ++-- lib/crates/fabro-cli/tests/it/cmd/runner.rs | 6 ++- .../tests/it/support/auth_harness.rs | 14 ++++++ lib/crates/fabro-cli/tests/it/support/mod.rs | 6 ++- 10 files changed, 59 insertions(+), 103 deletions(-) diff --git a/docs/public/reference/cli.mdx b/docs/public/reference/cli.mdx index 8ce0d8580..ace79f859 100644 --- a/docs/public/reference/cli.mdx +++ b/docs/public/reference/cli.mdx @@ -48,7 +48,7 @@ url = "https://fabro.example.com/api/v1" `fabro model` uses `[cli.target]` by default when no explicit `--storage-dir` is passed. `fabro exec` remains a local session unless you pass `--server`, even if `[cli.target]` is configured. -Explicit `http(s)://...` targets are always treated as remote servers. They do not inherit identity from a local daemon, a local storage dir, or `~/.fabro/dev-token`. For remote URLs, use `fabro auth login --server ...`, a stored CLI auth session, or an explicit `FABRO_DEV_TOKEN`. +Explicit `http(s)://...` targets are always treated as remote servers. They do not inherit identity from a local daemon, a local storage dir, or `~/.fabro/dev-token`. For remote URLs, use `fabro auth login --server ...` or a stored CLI auth session. CLI flags always override `settings.toml` values, which override hardcoded defaults. diff --git a/lib/crates/fabro-cli/src/commands/auth/status.rs b/lib/crates/fabro-cli/src/commands/auth/status.rs index f5bf3272f..db30eb115 100644 --- a/lib/crates/fabro-cli/src/commands/auth/status.rs +++ b/lib/crates/fabro-cli/src/commands/auth/status.rs @@ -1,9 +1,6 @@ use anyhow::Result; use chrono::{DateTime, Utc}; use fabro_client::{AuthEntry, AuthStore, OAuthEntry}; -use fabro_static::EnvVars; -use fabro_util::dev_token::validate_dev_token_format; -use fabro_util::printer::Printer; use serde::Serialize; use crate::args::AuthStatusArgs; @@ -45,8 +42,7 @@ enum StatusRow { #[derive(Serialize)] struct StatusOutput { - servers: Vec, - env_dev_token: &'static str, + servers: Vec, } pub(super) fn status_command(args: &AuthStatusArgs, ctx: &CommandContext) -> Result<()> { @@ -59,23 +55,13 @@ pub(super) fn status_command(args: &AuthStatusArgs, ctx: &CommandContext) -> Res } else { all_rows(&store, now)? }; - let env_dev_token = if load_env_dev_token_if_available() { - "active" - } else { - "not_set" - }; - if ctx.explicit_json_requested() { - print_json_pretty(&StatusOutput { - servers: rows, - env_dev_token, - })?; + print_json_pretty(&StatusOutput { servers: rows })?; return Ok(()); } if rows.is_empty() { fabro_util::printerr!(printer, "Not logged in to any servers."); - print_env_dev_token_status(env_dev_token, printer); return Ok(()); } @@ -141,7 +127,6 @@ pub(super) fn status_command(args: &AuthStatusArgs, ctx: &CommandContext) -> Res } } fabro_util::printerr!(printer, ""); - print_env_dev_token_status(env_dev_token, printer); Ok(()) } @@ -196,17 +181,6 @@ fn oauth_state(entry: &OAuthEntry, now: DateTime) -> OAuthState { } } -fn print_env_dev_token_status(env_dev_token: &str, printer: Printer) { - if env_dev_token == "active" { - fabro_util::printerr!( - printer, - "FABRO_DEV_TOKEN: active (overrides persisted credentials)" - ); - } else { - fabro_util::printerr!(printer, "FABRO_DEV_TOKEN: not_set"); - } -} - fn human_state(state: OAuthState) -> &'static str { match state { OAuthState::Active => "active", @@ -215,17 +189,6 @@ fn human_state(state: OAuthState) -> &'static str { } } -#[expect( - clippy::disallowed_methods, - reason = "Auth status reports whether the documented dev-token env source is configured." -)] -fn load_env_dev_token_if_available() -> bool { - let env_token = std::env::var(EnvVars::FABRO_DEV_TOKEN) - .ok() - .filter(|token| validate_dev_token_format(token)); - env_token.is_some() -} - #[cfg(test)] mod tests { use chrono::Duration; diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index eb3dfe725..86646aec3 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -10,11 +10,9 @@ use fabro_client::{ pub(crate) use fabro_client::{Client, RunEventStream}; use fabro_config::Storage; use fabro_config::bind::Bind; -use fabro_static::EnvVars; pub(crate) use fabro_types::RunProjection; use fabro_types::UserSettings; use fabro_util::dev_token; -use fabro_util::dev_token::validate_dev_token_format; use tokio::time::sleep; use crate::args::ServerTargetArgs; @@ -207,14 +205,6 @@ fn connect_cli_target_transport( Ok((http_client, "http://fabro".to_string())) } -#[expect( - clippy::disallowed_methods, - reason = "Server client authentication supports the documented local dev-token env source." -)] -fn process_env_var(name: &str) -> Option { - std::env::var(name).ok() -} - async fn wait_for_runtime_dev_token(path: &Path) -> Result { let deadline = std::time::Instant::now() + Duration::from_secs(5); @@ -273,14 +263,9 @@ async fn connect_unix_socket_http_client( fn resolve_target_credential_with_store( target: &ServerTarget, - env_token: Option<&str>, store: &AuthStore, now: chrono::DateTime, ) -> Result> { - if let Some(token) = env_token.filter(|token| validate_dev_token_format(token)) { - return Ok(Some(Credential::DevToken(token.to_owned()))); - } - let Some(entry) = store.get(target)? else { return Ok(None); }; @@ -296,9 +281,8 @@ fn resolve_target_credential_with_store( } fn resolve_target_credential(target: &ServerTarget) -> Result> { - let env_token = process_env_var(EnvVars::FABRO_DEV_TOKEN); let store = AuthStore::default(); - resolve_target_credential_with_store(target, env_token.as_deref(), &store, chrono::Utc::now()) + resolve_target_credential_with_store(target, &store, chrono::Utc::now()) } #[expect( @@ -352,22 +336,6 @@ mod tests { use super::*; - #[test] - fn resolve_local_tcp_credential_prefers_valid_env_token() { - let target = ServerTarget::http_url("http://127.0.0.1:32276").unwrap(); - let store = AuthStore::new(tempfile::tempdir().unwrap().path().join("auth.json")); - - let credential = resolve_target_credential_with_store( - &target, - Some("fabro_dev_cdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcd"), - &store, - Utc::now(), - ) - .unwrap(); - - assert!(matches!(credential, Some(Credential::DevToken(_)))); - } - #[test] fn resolve_target_credential_uses_persisted_dev_token_entry() { let dir = tempfile::tempdir().unwrap(); @@ -384,8 +352,7 @@ mod tests { ) .unwrap(); - let credential = - resolve_target_credential_with_store(&target, None, &store, Utc::now()).unwrap(); + let credential = resolve_target_credential_with_store(&target, &store, Utc::now()).unwrap(); assert!(matches!(credential, Some(Credential::DevToken(found)) if found == token)); } @@ -406,7 +373,7 @@ mod tests { ) .unwrap(); - let credential = resolve_target_credential_with_store(&target, None, &store, now).unwrap(); + let credential = resolve_target_credential_with_store(&target, &store, now).unwrap(); assert!(matches!(credential, Some(Credential::OAuth(_)))); } @@ -427,7 +394,7 @@ mod tests { ) .unwrap(); - let credential = resolve_target_credential_with_store(&target, None, &store, now).unwrap(); + let credential = resolve_target_credential_with_store(&target, &store, now).unwrap(); assert!(matches!(credential, Some(Credential::OAuth(_)))); } @@ -448,7 +415,7 @@ mod tests { ) .unwrap(); - let credential = resolve_target_credential_with_store(&target, None, &store, now).unwrap(); + let credential = resolve_target_credential_with_store(&target, &store, now).unwrap(); assert!(credential.is_none()); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/auth.rs b/lib/crates/fabro-cli/tests/it/cmd/auth.rs index bcaa953c2..15ddc8e30 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/auth.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/auth.rs @@ -1,4 +1,5 @@ use fabro_test::{fabro_snapshot, test_context}; +use serde_json::Value; const DEV_TOKEN: &str = "fabro_dev_abababababababababababababababababababababababababababababababab"; @@ -149,19 +150,17 @@ fn status_help() { } #[test] -fn status_json_reports_env_dev_token_separately() { +fn status_json_ignores_env_dev_token() { let context = test_context!(); - let mut cmd = context.command(); - cmd.args(["auth", "status", "--json"]) - .env("FABRO_DEV_TOKEN", DEV_TOKEN); - fabro_snapshot!(context.filters(), cmd, @" - success: true - exit_code: 0 - ----- stdout ----- - { - \"servers\": [], - \"env_dev_token\": \"active\" - } - ----- stderr ----- - "); + let output = context + .command() + .env("FABRO_DEV_TOKEN", DEV_TOKEN) + .args(["auth", "status", "--json"]) + .output() + .expect("auth status should run"); + + assert!(output.status.success()); + let value: Value = serde_json::from_slice(&output.stdout).expect("status JSON should parse"); + assert!(value["servers"].as_array().unwrap().is_empty()); + assert!(value.get("env_dev_token").is_none()); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/dump.rs b/lib/crates/fabro-cli/tests/it/cmd/dump.rs index c78246ea5..11b92c8ed 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/dump.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/dump.rs @@ -6,6 +6,7 @@ use std::fs; use std::time::Duration; +use fabro_client::ServerTarget; use fabro_test::{fabro_snapshot, test_context}; use insta::assert_snapshot; @@ -13,7 +14,7 @@ use super::support::{ local_dev_token, server_target, setup_completed_dry_run, setup_seeded_completed_dry_run, setup_seeded_created_dry_run, }; -use crate::support::{LightweightCli, unique_run_id}; +use crate::support::{LightweightCli, seed_dev_token_auth, unique_run_id}; #[test] fn help() { @@ -51,6 +52,12 @@ fn dump_accepts_server_target_from_separate_home() { let cli = LightweightCli::new(); let output_dir = context.temp_dir.join("remote-export"); let server = server_target(&context.storage_dir); + if let Some(dev_token) = local_dev_token(&context.storage_dir) { + let target = server + .parse::() + .expect("server target should parse"); + seed_dev_token_auth(cli.home(), &target, &dev_token); + } let mut cmd = cli.command(); cmd.args([ @@ -61,9 +68,6 @@ fn dump_accepts_server_target_from_separate_home() { output_dir.to_str().unwrap(), &run.run_id, ]); - if let Some(dev_token) = local_dev_token(&context.storage_dir) { - cmd.env("FABRO_DEV_TOKEN", dev_token); - } let output = cmd.output().expect("dump should execute"); assert!( diff --git a/lib/crates/fabro-cli/tests/it/cmd/json_global.rs b/lib/crates/fabro-cli/tests/it/cmd/json_global.rs index 5c94712e3..1303a451b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/json_global.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/json_global.rs @@ -69,7 +69,7 @@ fn auth_status_ignores_json_output_format_from_home_config() { ); let stderr = output_stderr(&output); assert!(stderr.contains("Not logged in to any servers.")); - assert!(stderr.contains("FABRO_DEV_TOKEN:")); + assert!(!stderr.contains("FABRO_DEV_TOKEN:")); } #[test] diff --git a/lib/crates/fabro-cli/tests/it/cmd/ps.rs b/lib/crates/fabro-cli/tests/it/cmd/ps.rs index ebe6f0f33..59d5b10d2 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/ps.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/ps.rs @@ -1,3 +1,4 @@ +use fabro_client::ServerTarget; use fabro_config::{Storage, envfile}; use fabro_test::{fabro_snapshot, test_context}; use fabro_util::dev_token; @@ -7,7 +8,7 @@ use serde_json::Value; use super::support::{ local_dev_token, setup_seeded_completed_dry_run, setup_seeded_created_dry_run, }; -use crate::support::{fatal_error_line, unique_run_id}; +use crate::support::{fatal_error_line, seed_dev_token_auth, unique_run_id}; const TEST_DEV_TOKEN: &str = "fabro_dev_abababababababababababababababababababababababababababababababab"; @@ -89,6 +90,7 @@ fn ps_explicit_local_tcp_server_target_requires_explicit_auth() { let output = context .command() .env("FABRO_STORAGE_DIR", &storage_dir) + .env("FABRO_DEV_TOKEN", TEST_DEV_TOKEN) .args(["ps", "-a", "--json", "--server", &format!("http://{bind}")]) .output() .expect("ps should run"); @@ -102,7 +104,7 @@ fn ps_explicit_local_tcp_server_target_requires_explicit_auth() { assert!( !output.status.success(), - "ps against an explicit local TCP target should require explicit auth:\nstdout:\n{}\nstderr:\n{}", + "ps against an explicit local TCP target should ignore FABRO_DEV_TOKEN and require persisted auth:\nstdout:\n{}\nstderr:\n{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); @@ -139,10 +141,11 @@ fn ps_explicit_local_tcp_server_target_accepts_explicit_dev_token() { .as_str() .expect("bind should be present"); let token = local_dev_token(&storage_dir).expect("local dev token should exist"); + let target = ServerTarget::http_url(format!("http://{bind}")).expect("bind should parse"); + seed_dev_token_auth(&context.home_dir, &target, &token); let output = context .command() - .env("FABRO_DEV_TOKEN", &token) .args(["ps", "-a", "--json", "--server", &format!("http://{bind}")]) .output() .expect("ps should run"); diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index a3db2ec3f..d93c7c1cf 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -12,6 +12,7 @@ use std::path::Path; use std::process::{Child, ExitStatus, Output, Stdio}; use std::time::{Duration, Instant}; +use fabro_client::ServerTarget; use fabro_config::{Storage, envfile}; use fabro_store::EventEnvelope; use fabro_test::{ @@ -27,7 +28,7 @@ use super::support::{ command_log_text, find_run_dir, local_dev_token, output_stderr, run_events, run_state, server_endpoint, server_target, wait_for_event_names, wait_for_status, write_gated_workflow, }; -use crate::support::unique_run_id; +use crate::support::{seed_dev_token_auth, unique_run_id}; const SHARED_DAEMON_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); const LEAKED_WORKER_PARENT_TOKEN: &str = "leak-worker-parent-token"; @@ -519,9 +520,10 @@ methods = ["dev-token"] let run_id = unique_run_id(); let dev_token = local_dev_token(&storage_dir).expect("managed server should have a dev token"); + let target = ServerTarget::unix_socket_path(&socket_path).expect("socket path should parse"); + seed_dev_token_auth(&context.home_dir, &target, &dev_token); let run_output = context .run_cmd() - .env("FABRO_DEV_TOKEN", dev_token) .args([ "--server", socket_path.to_str().expect("socket path should be UTF-8"), diff --git a/lib/crates/fabro-cli/tests/it/support/auth_harness.rs b/lib/crates/fabro-cli/tests/it/support/auth_harness.rs index e5f1d7547..881df4a87 100644 --- a/lib/crates/fabro-cli/tests/it/support/auth_harness.rs +++ b/lib/crates/fabro-cli/tests/it/support/auth_harness.rs @@ -8,6 +8,7 @@ )] use std::io::Read; +use std::path::Path; use std::process::{Command, Output, Stdio}; use std::sync::{Arc, Mutex, mpsc}; use std::time::{Duration, Instant}; @@ -17,6 +18,7 @@ use axum::extract::{Request, State as AxumState}; use axum::middleware::{self, Next}; use axum::response::Response as AxumResponse; use chrono::{Duration as ChronoDuration, Utc}; +use fabro_client::{AuthEntry, AuthStore, DevTokenEntry, ServerTarget}; use fabro_config::{RunLayer, ServerSettingsBuilder}; use fabro_server::auth::GithubEndpoints; use fabro_server::ip_allowlist::IpAllowlistConfig; @@ -248,6 +250,18 @@ pub(crate) fn expire_saved_access_token(context: &TestContext, issuer: &str) { .unwrap_or_else(|err| panic!("failed to write {}: {err}", path.display())); } +pub(crate) fn seed_dev_token_auth(home_dir: &Path, target: &ServerTarget, token: &str) { + AuthStore::new(home_dir.join(".fabro/auth.json")) + .put( + target, + AuthEntry::DevToken(DevTokenEntry { + token: token.to_owned(), + logged_in_at: Utc::now(), + }), + ) + .unwrap_or_else(|err| panic!("failed to seed dev-token auth: {err}")); +} + pub(crate) fn no_redirect_browser_client() -> fabro_http::HttpClient { fabro_http::HttpClientBuilder::new() .cookie_store(true) diff --git a/lib/crates/fabro-cli/tests/it/support/mod.rs b/lib/crates/fabro-cli/tests/it/support/mod.rs index 067cddb94..27acbb8d1 100644 --- a/lib/crates/fabro-cli/tests/it/support/mod.rs +++ b/lib/crates/fabro-cli/tests/it/support/mod.rs @@ -4,7 +4,7 @@ mod auth_tokens; use assert_cmd::Command; pub(crate) use auth_harness::{ RealAuthHarness, TEST_DEV_TOKEN, complete_login_via_browser, expire_saved_access_token, - no_redirect_browser_client, run_detached, saved_auth_entry, + no_redirect_browser_client, run_detached, saved_auth_entry, seed_dev_token_auth, }; pub(crate) use auth_tokens::{TEST_SESSION_SECRET, issue_test_github_jwt}; use fabro_store::EventEnvelope; @@ -60,6 +60,10 @@ impl LightweightCli { } } + pub(crate) fn home(&self) -> &std::path::Path { + self.home_dir.path() + } + #[expect( clippy::disallowed_methods, reason = "Lightweight CLI test harness reconstructs a minimal process env for subprocesses."