refactor(auth): deduplicate session secret, remove dead code, tighten test helpers

- Extract generate_session_secret and validate_session_secret to
  fabro_util::session_secret, removing duplicate implementations in
  install.rs (with private hex module) and start.rs
- Remove dead run_auth_method_for_config/run_auth_method_for_method
  from jwt_auth.rs (zero callers)
- Replace test read_dev_token helper with dev_token::read_dev_token_file
  which validates the fabro_dev_ prefix rather than just non-empty
- Extract build_unix_socket_probe_client to deduplicate probe client
  construction in try_connect/connect_unix_socket_api_client_bundle

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-13 12:18:01 -04:00
parent 47de0914f3
commit c7a9f1e30b
8 changed files with 49 additions and 87 deletions

View file

@ -91,16 +91,6 @@ async fn run_openssl_with_stdin(
Ok(output.stdout)
}
// ---------------------------------------------------------------------------
// Session secret
// ---------------------------------------------------------------------------
fn generate_session_secret() -> String {
let mut rng = rand::thread_rng();
let bytes: [u8; 32] = rng.gen();
hex::encode(&bytes)
}
// ---------------------------------------------------------------------------
// JWT keypair generation
// ---------------------------------------------------------------------------
@ -938,7 +928,7 @@ pub(crate) async fn run_install(
s.dim.apply_to("Generating secrets and auth material...")
);
let session_secret = generate_session_secret();
let session_secret = fabro_util::session_secret::generate_session_secret();
fabro_util::printerr!(
printer,
" {} Session secret generated",
@ -1036,22 +1026,6 @@ pub(crate) async fn run_install(
Ok(())
}
// ---------------------------------------------------------------------------
// Hex encoding (used by generate_session_secret)
// ---------------------------------------------------------------------------
mod hex {
use std::fmt::Write as _;
pub(super) fn encode(bytes: &[u8]) -> String {
let mut encoded = String::with_capacity(bytes.len() * 2);
for byte in bytes {
write!(&mut encoded, "{byte:02x}").expect("writing to String should not fail");
}
encoded
}
}
// ---------------------------------------------------------------------------
// Tests
// ---------------------------------------------------------------------------
@ -1078,19 +1052,19 @@ mod tests {
#[test]
fn session_secret_length() {
let secret = generate_session_secret();
let secret = fabro_util::session_secret::generate_session_secret();
assert_eq!(secret.len(), 64);
}
#[test]
fn session_secret_is_hex() {
let secret = generate_session_secret();
let secret = fabro_util::session_secret::generate_session_secret();
assert!(secret.chars().all(|c| c.is_ascii_hexdigit()));
}
#[test]
fn session_secret_is_lowercase() {
let secret = generate_session_secret();
let secret = fabro_util::session_secret::generate_session_secret();
assert!(secret.chars().all(|c| !c.is_ascii_uppercase()));
}

View file

@ -11,7 +11,6 @@ use fabro_server::serve::{DEFAULT_TCP_PORT, ServeArgs};
use fabro_util::printer::Printer;
use fabro_util::terminal::Styles;
use fabro_util::{Home, dev_token};
use rand::Rng;
use tokio::process::Command as TokioCommand;
use tokio::time;
@ -149,19 +148,7 @@ fn load_or_create_local_dev_token(storage_dir: &Path, home: &Home) -> Result<Str
}
fn valid_session_secret(secret: &str) -> bool {
secret.len() >= 64 && secret.chars().all(|ch| ch.is_ascii_hexdigit())
}
fn generate_session_secret() -> String {
let mut rng = rand::thread_rng();
let bytes: [u8; 32] = rng.gen();
let mut output = String::with_capacity(bytes.len() * 2);
for byte in bytes {
use std::fmt::Write as _;
write!(&mut output, "{byte:02x}").expect("writing to string should not fail");
}
output
fabro_util::session_secret::validate_session_secret(secret).is_ok()
}
fn load_or_create_local_session_secret(storage_dir: &Path) -> Result<String> {
@ -182,7 +169,7 @@ fn load_or_create_local_session_secret(storage_dir: &Path) -> Result<String> {
return Ok(secret);
}
let secret = generate_session_secret();
let secret = fabro_util::session_secret::generate_session_secret();
envfile::merge_env_file(&server_env_path, [("SESSION_SECRET", secret.as_str())])?;
Ok(secret)
}

View file

@ -380,16 +380,19 @@ async fn build_authed_unix_socket_client(
Ok(unix_socket_api_client_bundle(http_client))
}
fn build_unix_socket_probe_client(path: &Path) -> Result<fabro_http::HttpClient> {
cli_http_client_builder()
.unix_socket(path)
.no_proxy()
.build()
.context("Failed to build Unix-socket HTTP client for fabro server")
}
async fn try_connect_unix_socket_api_client_bundle(
path: &Path,
storage_dir: Option<&Path>,
) -> Result<ServerStoreClient> {
let probe_client = cli_http_client_builder()
.unix_socket(path)
.no_proxy()
.build()
.context("Failed to build Unix-socket HTTP client for fabro server")?;
check_server_ready(&probe_client).await?;
check_server_ready(&build_unix_socket_probe_client(path)?).await?;
build_authed_unix_socket_client(path, storage_dir).await
}
@ -397,12 +400,7 @@ async fn connect_unix_socket_api_client_bundle(
path: &Path,
storage_dir: Option<&Path>,
) -> Result<ServerStoreClient> {
let probe_client = cli_http_client_builder()
.unix_socket(path)
.no_proxy()
.build()
.context("Failed to build Unix-socket HTTP client for fabro server")?;
wait_for_server_ready(&probe_client).await?;
wait_for_server_ready(&build_unix_socket_probe_client(path)?).await?;
build_authed_unix_socket_client(path, storage_dir).await
}

View file

@ -659,23 +659,16 @@ struct TestServerRecord {
dev_token_path: Option<PathBuf>,
}
fn read_dev_token(path: &Path) -> Option<String> {
std::fs::read_to_string(path)
.ok()
.map(|token| token.trim().to_string())
.filter(|token| !token.is_empty())
}
pub(crate) fn local_dev_token(storage_dir: &Path) -> Option<String> {
let server_state = Storage::new(storage_dir).server_state();
read_dev_token(&server_state.dev_token_path()).or_else(|| {
fabro_util::dev_token::read_dev_token_file(&server_state.dev_token_path()).or_else(|| {
std::fs::read_to_string(server_state.record_path())
.ok()
.and_then(|content| serde_json::from_str::<TestServerRecord>(&content).ok())
.and_then(|record| record.dev_token_path)
.as_deref()
.and_then(read_dev_token)
.and_then(fabro_util::dev_token::read_dev_token_file)
})
}

View file

@ -144,16 +144,7 @@ fn validate_tls_private_key(pem: &str) -> Result<(), String> {
}
fn validate_session_secret(value: &str) -> Result<(), String> {
if value.len() < 64 {
return Err(format!(
"too short ({} chars, need at least 64 hex chars for 256-bit entropy)",
value.len()
));
}
if !value.chars().all(|c| c.is_ascii_hexdigit()) {
return Err("contains non-hex characters".to_string());
}
Ok(())
fabro_util::session_secret::validate_session_secret(value)
}
pub async fn run_all(state: &AppState) -> DiagnosticsReport {

View file

@ -110,13 +110,6 @@ pub(crate) fn dev_token_matches(provided: &str, expected: &str) -> bool {
expected_mac.verify_slice(&provided_mac).is_ok()
}
fn run_auth_method_for_config(method: ServerAuthMethod) -> RunAuthMethod {
match method {
ServerAuthMethod::DevToken => RunAuthMethod::DevToken,
ServerAuthMethod::Github => RunAuthMethod::Github,
}
}
fn config_allows_run_auth_method(config: &ConfiguredAuth, method: RunAuthMethod) -> bool {
match method {
RunAuthMethod::Disabled => false,
@ -242,10 +235,6 @@ pub fn auth_method_name(method: ServerAuthMethod) -> &'static str {
}
}
pub fn run_auth_method_for_method(method: ServerAuthMethod) -> RunAuthMethod {
run_auth_method_for_config(method)
}
#[cfg(test)]
mod tests {
use axum::body::{Body, to_bytes};

View file

@ -8,6 +8,7 @@ pub mod path;
pub mod printer;
pub mod redact;
pub mod run_log;
pub mod session_secret;
pub mod terminal;
pub mod text;
pub mod version;

View file

@ -0,0 +1,29 @@
use rand::Rng;
const MIN_SESSION_SECRET_LEN: usize = 64;
pub fn generate_session_secret() -> String {
let mut rng = rand::thread_rng();
let bytes: [u8; 32] = rng.gen();
let mut output = String::with_capacity(bytes.len() * 2);
for byte in bytes {
use std::fmt::Write as _;
write!(&mut output, "{byte:02x}").expect("writing to string should not fail");
}
output
}
pub fn validate_session_secret(value: &str) -> Result<(), String> {
if value.len() < MIN_SESSION_SECRET_LEN {
return Err(format!(
"too short ({} chars, need at least {} hex chars for 256-bit entropy)",
value.len(),
MIN_SESSION_SECRET_LEN,
));
}
if !value.chars().all(|ch| ch.is_ascii_hexdigit()) {
return Err("contains non-hex characters".to_string());
}
Ok(())
}