refactor(client): dedupe helpers and fix TOCTOU in auth store

- Expose apply_bearer_token_auth and ensure_refresh_target_transport
  from fabro-client; drop the CLI's duplicate copies.
- Collapse AuthStore's two read paths into one NotFound-tolerant
  reader and drop the pre-existence checks in get/remove/list.
- Avoid rewriting auth.json when remove found nothing.
- Inline the one-line user_config::build_public_http_client wrapper.
- Trim unused pub use fabro_api::types re-export and the narrating
  doc comment in fabro-client/src/lib.rs.
- Clean up pre-existing unused imports in run/create.rs and
  loopback.rs tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-20 21:44:09 -04:00
parent f0f04abf44
commit eb8ea317ec
No known key found for this signature in database
9 changed files with 30 additions and 77 deletions

View file

@ -3,7 +3,7 @@ use std::time::Duration;
use anyhow::{Context as _, Result, anyhow, bail};
use chrono::{DateTime, Utc};
use fabro_api::types;
use fabro_client::{AuthEntry, AuthStore, StoredSubject};
use fabro_client::{AuthEntry, AuthStore, StoredSubject, ensure_refresh_target_transport};
use fabro_http::header::CONTENT_TYPE;
use fabro_types::settings::CliSettings;
use fabro_types::settings::cli::CliLayer;
@ -96,14 +96,7 @@ pub(super) async fn login_command(
}
};
match target.loopback_classification()? {
fabro_client::LoopbackClassification::Https
| fabro_client::LoopbackClassification::LoopbackHttp
| fabro_client::LoopbackClassification::UnixSocket => {}
fabro_client::LoopbackClassification::Rejected => {
bail!("{}", token_transport_error(&target));
}
}
ensure_refresh_target_transport(&target)?;
let tokens = exchange_cli_token(&target, &code, &pkce.verifier, &redirect_uri).await?;
let entry = AuthEntry {
@ -129,7 +122,7 @@ pub(super) async fn login_command(
#[cfg(unix)]
async fn fetch_cli_auth_config(target: &ServerTarget) -> Result<types::CliAuthConfig> {
let (http_client, base_url) = user_config::build_public_http_client(target)?;
let (http_client, base_url) = target.build_public_http_client()?;
let client = fabro_api::ApiClient::new_with_client(&base_url, http_client);
client
.get_cli_auth_config()
@ -146,7 +139,7 @@ async fn exchange_cli_token(
code_verifier: &str,
redirect_uri: &str,
) -> Result<CliTokenResponse> {
let (http_client, base_url) = user_config::build_public_http_client(target)?;
let (http_client, base_url) = target.build_public_http_client()?;
let response = http_client
.post(format!("{base_url}/auth/cli/token"))
.header(CONTENT_TYPE, "application/json")
@ -241,12 +234,6 @@ fn login_failure_message(error_code: &str, error_description: Option<&str>) -> S
}
}
fn token_transport_error(target: &ServerTarget) -> String {
format!(
"Refusing to send refresh-token credentials over plaintext HTTP to a non-loopback host ({target}). Use HTTPS, or bind the server to 127.0.0.1 / ::1."
)
}
fn identity_summary(subject: &StoredSubject) -> String {
if !subject.name.is_empty() && !subject.email.is_empty() {
format!("{} ({} <{}>)", subject.login, subject.name, subject.email)

View file

@ -58,7 +58,7 @@ pub(super) async fn logout_command(
}
async fn revoke_remote_session(target: &ServerTarget, entry: &AuthEntry) -> Result<()> {
let (http_client, base_url) = user_config::build_public_http_client(target)?;
let (http_client, base_url) = target.build_public_http_client()?;
let response = http_client
.post(format!("{base_url}/auth/cli/logout"))
.header(AUTHORIZATION, format!("Bearer {}", entry.refresh_token))

View file

@ -13,7 +13,7 @@ use super::overrides::run_args_layer;
use crate::args::RunArgs;
use crate::command_context::CommandContext;
use crate::manifest_builder::{ManifestBuildInput, build_run_manifest, run_manifest_args};
use crate::user_config::{self, ServerTarget};
use crate::user_config;
pub(crate) struct CreatedRun {
pub(crate) run_id: RunId,

View file

@ -5,10 +5,10 @@ use std::time::Duration;
use anyhow::{Context as _, Result, anyhow, bail};
use fabro_client::{
AuthStore, Credential, CredentialFallback, OAuthSession, ServerTarget, TransportConnector,
apply_bearer_token_auth,
};
pub(crate) use fabro_client::{Client, RunEventStream};
use fabro_config::Storage;
use fabro_http::header::AUTHORIZATION;
use fabro_server::bind::Bind;
pub(crate) use fabro_types::RunProjection;
use fabro_types::settings::SettingsLayer;
@ -264,19 +264,6 @@ async fn wait_for_local_dev_token(storage_dir: &Path) -> Result<String> {
);
}
fn apply_bearer_token_auth(
builder: fabro_http::HttpClientBuilder,
token: &str,
) -> Result<fabro_http::HttpClientBuilder> {
let mut headers = fabro_http::HeaderMap::new();
headers.insert(
AUTHORIZATION,
fabro_http::HeaderValue::from_str(&format!("Bearer {token}"))
.context("invalid dev token header value")?,
);
Ok(builder.default_headers(headers))
}
async fn build_authed_unix_socket_http_client(
path: &Path,
storage_dir: Option<&Path>,

View file

@ -61,12 +61,6 @@ pub(crate) fn apply_storage_dir_override(
layer
}
pub(crate) fn build_public_http_client(
target: &ServerTarget,
) -> Result<(fabro_http::HttpClient, String)> {
target.build_public_http_client()
}
/// Pull the resolved CLI target configuration out of `[cli.target]`.
/// Returns either an http(s) URL or a unix socket path.
fn cli_target_from_settings(settings: &CliSettings) -> Option<String> {

View file

@ -120,10 +120,6 @@ impl AuthStore {
}
pub fn get(&self, target: &ServerTarget) -> Result<Option<AuthEntry>, AuthStoreError> {
if !self.path.exists() {
return Ok(None);
}
let key = key_for_target(target);
self.with_shared_lock(|| {
let file = self.read_auth_file()?;
@ -143,7 +139,7 @@ impl AuthStore {
let key = key_for_target(target);
self.ensure_parent_dir()?;
self.with_exclusive_lock(|| {
let mut file = self.read_auth_file_if_exists()?;
let mut file = self.read_auth_file()?;
file.servers.insert(key, entry);
self.write_auth_file(&file)
})
@ -159,26 +155,20 @@ impl AuthStore {
#[cfg(unix)]
{
if !self.path.exists() {
return Ok(false);
}
let key = key_for_target(target);
self.ensure_parent_dir()?;
self.with_exclusive_lock(|| {
let mut file = self.read_auth_file_if_exists()?;
let mut file = self.read_auth_file()?;
let removed = file.servers.remove(&key).is_some();
self.write_auth_file(&file)?;
if removed {
self.write_auth_file(&file)?;
}
Ok(removed)
})
}
}
pub fn list(&self) -> Result<Vec<(ServerTarget, AuthEntry)>, AuthStoreError> {
if !self.path.exists() {
return Ok(Vec::new());
}
self.with_shared_lock(|| {
let file = self.read_auth_file()?;
file.servers
@ -189,21 +179,19 @@ impl AuthStore {
}
fn read_auth_file(&self) -> Result<AuthFile, AuthStoreError> {
let contents = fs::read_to_string(&self.path).map_err(|source| AuthStoreError::Read {
path: self.path.clone(),
source,
})?;
serde_json::from_str(&contents).map_err(|source| AuthStoreError::Corrupt {
path: self.path.clone(),
source,
})
}
fn read_auth_file_if_exists(&self) -> Result<AuthFile, AuthStoreError> {
if !self.path.exists() {
return Ok(AuthFile::default());
match fs::read_to_string(&self.path) {
Ok(contents) => {
serde_json::from_str(&contents).map_err(|source| AuthStoreError::Corrupt {
path: self.path.clone(),
source,
})
}
Err(source) if source.kind() == std::io::ErrorKind::NotFound => Ok(AuthFile::default()),
Err(source) => Err(AuthStoreError::Read {
path: self.path.clone(),
source,
}),
}
self.read_auth_file()
}
#[cfg(unix)]

View file

@ -1294,7 +1294,7 @@ fn connect_target_transport(
Ok((http_client, "http://fabro".to_string()))
}
fn apply_bearer_token_auth(
pub fn apply_bearer_token_auth(
builder: fabro_http::HttpClientBuilder,
token: &str,
) -> Result<fabro_http::HttpClientBuilder> {
@ -1307,7 +1307,7 @@ fn apply_bearer_token_auth(
Ok(builder.default_headers(headers))
}
fn ensure_refresh_target_transport(target: &ServerTarget) -> Result<()> {
pub fn ensure_refresh_target_transport(target: &ServerTarget) -> Result<()> {
match target.loopback_classification()? {
LoopbackClassification::Https
| LoopbackClassification::LoopbackHttp

View file

@ -1,7 +1,4 @@
//! Typed HTTP client for the Fabro API.
//!
//! This crate hosts the reusable client and auth/session plumbing that was
//! previously embedded in `fabro-cli`.
pub mod auth_store;
pub mod client;
@ -13,14 +10,16 @@ pub mod sse;
pub mod target;
pub use auth_store::{AuthEntry, AuthStore, AuthStoreError, LockError, StoredSubject};
pub use client::{Client, RunEventStream, TransportConnector};
pub use client::{
Client, RunEventStream, TransportConnector, apply_bearer_token_auth,
ensure_refresh_target_transport,
};
pub use credential::{Credential, CredentialFallback};
pub use error::{
ApiError, ApiFailure, StructuredApiError, classify_api_error, classify_http_response,
convert_type, is_not_found_error, map_api_error, parse_error_response_value,
raw_response_failure_error,
};
pub use fabro_api::types;
pub use loopback::{LoopbackClassification, TargetSchemeError};
pub use session::OAuthSession;
pub use target::ServerTarget;

View file

@ -122,8 +122,6 @@ fn ip_is_loopback(ip: &IpAddr) -> bool {
#[cfg(test)]
mod tests {
use std::path::PathBuf;
use super::LoopbackClassification;
use crate::target::ServerTarget;