From cdc88b315889186682363e36dedb17b9777ed408 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Mon, 3 Aug 2026 14:53:46 -0400 Subject: [PATCH] Move GitHub coordinate validation to shared types GitHub repository slug and git ref selector syntax now has one owner: fabro-types::repository defines GitHubRepositorySlug with a try_new constructor and the is_valid_github_ref_selector predicate. fabro-automation keeps its public type path as a re-export of the same type and delegates its existing parser and ref validation to the shared grammar, preserving its exact error variants and messages. Server checkout and materialization code imports the type from its canonical owner. No wire, API, or behavior change. Co-Authored-By: Claude Fable 5 --- Cargo.lock | 1 + .../src/automation_materializer.rs | 4 +- lib/apps/fabro-server/src/git_checkout.rs | 4 +- lib/components/fabro-automation/Cargo.toml | 1 + lib/components/fabro-automation/src/lib.rs | 4 +- lib/components/fabro-automation/src/model.rs | 118 ++++------- lib/foundation/fabro-types/src/lib.rs | 2 +- lib/foundation/fabro-types/src/repository.rs | 187 ++++++++++++++++++ 8 files changed, 234 insertions(+), 87 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 384eac09b..9071dca61 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2374,6 +2374,7 @@ dependencies = [ "chrono", "croner", "fabro-db", + "fabro-types", "hex", "serde", "sha2 0.10.9", diff --git a/lib/apps/fabro-server/src/automation_materializer.rs b/lib/apps/fabro-server/src/automation_materializer.rs index 3cd7007f6..ffd64b564 100644 --- a/lib/apps/fabro-server/src/automation_materializer.rs +++ b/lib/apps/fabro-server/src/automation_materializer.rs @@ -4,10 +4,10 @@ use std::sync::Arc; use anyhow::Context as _; use async_trait::async_trait; use fabro_api::types::RunManifest; -use fabro_automation::{AutomationId, AutomationTarget, GitHubRepositorySlug}; +use fabro_automation::{AutomationId, AutomationTarget}; use fabro_config::{EnvironmentLayer, MergeMap}; use fabro_manifest::ManifestBuildInput; -use fabro_types::{DirtyStatus, GitContext, PreRunPushOutcome, RunId}; +use fabro_types::{DirtyStatus, GitContext, GitHubRepositorySlug, PreRunPushOutcome, RunId}; use fabro_util::error::collect_chain; use tokio::{fs, task}; diff --git a/lib/apps/fabro-server/src/git_checkout.rs b/lib/apps/fabro-server/src/git_checkout.rs index 7220a829b..8bdb4f869 100644 --- a/lib/apps/fabro-server/src/git_checkout.rs +++ b/lib/apps/fabro-server/src/git_checkout.rs @@ -3,8 +3,8 @@ use std::time::Duration; use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; -use fabro_automation::GitHubRepositorySlug; use fabro_store::KeyedMutex; +use fabro_types::GitHubRepositorySlug; use tokio::process::Command; use tokio::{fs, time}; @@ -465,7 +465,7 @@ mod tests { use super::*; fn repository_slug(value: &str) -> GitHubRepositorySlug { - fabro_automation::parse_github_repository_slug(value).expect("slug should parse") + GitHubRepositorySlug::try_new(value).expect("slug should parse") } #[test] diff --git a/lib/components/fabro-automation/Cargo.toml b/lib/components/fabro-automation/Cargo.toml index 9b1b75575..798af66b2 100644 --- a/lib/components/fabro-automation/Cargo.toml +++ b/lib/components/fabro-automation/Cargo.toml @@ -16,6 +16,7 @@ workspace = true chrono.workspace = true croner.workspace = true fabro-db = { path = "../../foundation/fabro-db" } +fabro-types = { path = "../../foundation/fabro-types" } hex.workspace = true serde.workspace = true sha2.workspace = true diff --git a/lib/components/fabro-automation/src/lib.rs b/lib/components/fabro-automation/src/lib.rs index 7498e2878..90d356a5f 100644 --- a/lib/components/fabro-automation/src/lib.rs +++ b/lib/components/fabro-automation/src/lib.rs @@ -5,11 +5,11 @@ mod model; mod store; pub use error::{AutomationStoreError, AutomationValidationError}; +pub use fabro_types::GitHubRepositorySlug; pub use id::{AutomationId, AutomationRevision, AutomationRevisionParseError, AutomationTriggerId}; pub use migrations::{ImportReport, import_legacy_directory_once}; pub use model::{ ApiTrigger, Automation, AutomationDraft, AutomationReplace, AutomationTarget, - AutomationTrigger, GitHubRepositorySlug, ScheduleTrigger, parse_github_repository_slug, - parse_schedule_expression, + AutomationTrigger, ScheduleTrigger, parse_github_repository_slug, parse_schedule_expression, }; pub use store::AutomationStore; diff --git a/lib/components/fabro-automation/src/model.rs b/lib/components/fabro-automation/src/model.rs index 90be56d8b..1aa68d1f7 100644 --- a/lib/components/fabro-automation/src/model.rs +++ b/lib/components/fabro-automation/src/model.rs @@ -4,6 +4,7 @@ use std::sync::LazyLock; use croner::Cron; use croner::errors::CronError; use croner::parser::{CronParser, Seconds, Year}; +use fabro_types::{GitHubRepositorySlug, repository}; use serde::{Deserialize, Serialize}; use crate::{ @@ -158,24 +159,6 @@ pub struct AutomationTarget { pub workflow: String, } -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct GitHubRepositorySlug { - owner: String, - repo: String, -} - -impl GitHubRepositorySlug { - #[must_use] - pub fn owner(&self) -> &str { - &self.owner - } - - #[must_use] - pub fn repo(&self) -> &str { - &self.repo - } -} - #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "snake_case", deny_unknown_fields)] pub enum AutomationTrigger { @@ -367,19 +350,10 @@ fn normalize_replace( pub fn parse_github_repository_slug( value: &str, ) -> Result { - let Some((owner, repo)) = value.split_once('/') else { - return Err(AutomationValidationError::InvalidRepositorySlug { + GitHubRepositorySlug::try_new(value).ok_or_else(|| { + AutomationValidationError::InvalidRepositorySlug { value: value.to_string(), - }); - }; - if repo.contains('/') || !valid_github_owner(owner) || !valid_github_repo(repo) { - return Err(AutomationValidationError::InvalidRepositorySlug { - value: value.to_string(), - }); - } - Ok(GitHubRepositorySlug { - owner: owner.to_string(), - repo: repo.to_string(), + } }) } @@ -387,47 +361,8 @@ fn validate_repository_slug(value: &str) -> Result<(), AutomationValidationError parse_github_repository_slug(value).map(|_| ()) } -fn valid_github_owner(value: &str) -> bool { - if value.is_empty() || value.len() > 39 { - return false; - } - let bytes = value.as_bytes(); - let first = bytes[0]; - let last = bytes[bytes.len() - 1]; - (first.is_ascii_alphanumeric() && last.is_ascii_alphanumeric()) - && bytes - .iter() - .all(|byte| byte.is_ascii_alphanumeric() || *byte == b'-') -} - -fn valid_github_repo(value: &str) -> bool { - !value.is_empty() - && value.len() <= 100 - && value != "." - && value != ".." - && value - .bytes() - .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b'-')) -} - fn validate_git_ref_selector(value: &str) -> Result<(), AutomationValidationError> { - let valid = !value.is_empty() - && value.len() <= 255 - && value.trim() == value - && !value.starts_with(['/', '-', '.']) - && !value.ends_with(['/', '.']) - && !has_lock_suffix(value) - && value != "@" - && !value.contains("..") - && !value.contains("//") - && !value.contains("@{") - && value - .bytes() - .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'/' | b'.' | b'_' | b'-')) - && value - .split('/') - .all(|part| !part.is_empty() && !part.starts_with('.') && !has_lock_suffix(part)); - if valid { + if repository::is_valid_github_ref_selector(value) { Ok(()) } else { Err(AutomationValidationError::InvalidGitRefSelector { @@ -458,12 +393,6 @@ fn validate_workflow_selector(value: &str) -> Result<(), AutomationValidationErr } } -fn has_lock_suffix(value: &str) -> bool { - value - .rsplit_once('.') - .is_some_and(|(_, extension)| extension == "lock") -} - fn validate_triggers(triggers: &[AutomationTrigger]) -> Result<(), AutomationValidationError> { let mut seen = HashSet::new(); let mut has_api_trigger = false; @@ -505,7 +434,7 @@ fn validate_triggers(triggers: &[AutomationTrigger]) -> Result<(), AutomationVal mod tests { use crate::{ ApiTrigger, Automation, AutomationId, AutomationReplace, AutomationTarget, - AutomationTrigger, AutomationTriggerId, ScheduleTrigger, + AutomationTrigger, AutomationTriggerId, AutomationValidationError, ScheduleTrigger, }; fn target() -> AutomationTarget { @@ -616,12 +545,41 @@ enabled = true } #[test] - fn repository_slug_parser_returns_validated_parts() { - let slug = crate::parse_github_repository_slug("owner/.github").unwrap(); + fn repository_slug_parser_returns_the_shared_type() { + let slug: fabro_types::GitHubRepositorySlug = + crate::parse_github_repository_slug("owner/.github").unwrap(); assert_eq!(slug.owner(), "owner"); assert_eq!(slug.repo(), ".github"); - assert!(crate::parse_github_repository_slug("not/github/slug").is_err()); + } + + #[test] + fn invalid_repository_slug_preserves_the_automation_error() { + let error = crate::parse_github_repository_slug("not/github/slug").unwrap_err(); + + assert!(matches!( + &error, + AutomationValidationError::InvalidRepositorySlug { value } + if value == "not/github/slug" + )); + assert_eq!( + error.to_string(), + "repository slug \"not/github/slug\" must be a GitHub owner/repo slug" + ); + } + + #[test] + fn invalid_git_ref_selector_preserves_the_automation_error() { + let error = super::validate_git_ref_selector("main;rm").unwrap_err(); + + assert!(matches!( + &error, + AutomationValidationError::InvalidGitRefSelector { value } if value == "main;rm" + )); + assert_eq!( + error.to_string(), + "git ref selector \"main;rm\" is not safe" + ); } #[test] diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 309505c4a..c77cc2cd5 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -105,7 +105,7 @@ pub use pull_request::{ PullRequestRef, PullRequestResponse, PullRequestTimestamps, PullRequestUser, }; pub use reasoning::ReasoningOutput; -pub use repository::{RepositoryProvider, RepositoryRef}; +pub use repository::{GitHubRepositorySlug, RepositoryProvider, RepositoryRef}; pub use run::{ DirtyStatus, ForkSourceRef, GitContext, PreRunPushOutcome, RunClientProvenance, RunProvenance, RunServerProvenance, RunSpec, diff --git a/lib/foundation/fabro-types/src/repository.rs b/lib/foundation/fabro-types/src/repository.rs index 75167296d..778b3ebf0 100644 --- a/lib/foundation/fabro-types/src/repository.rs +++ b/lib/foundation/fabro-types/src/repository.rs @@ -21,6 +21,97 @@ impl RepositoryRef { } } +/// A validated GitHub `owner/repo` coordinate. +/// +/// Construction enforces GitHub's owner and repository name syntax on the +/// exact submitted bytes; no trimming, case folding, or other normalization +/// is performed. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct GitHubRepositorySlug { + owner: String, + repo: String, +} + +impl GitHubRepositorySlug { + /// Validates `value` as a GitHub `owner/repo` slug, returning `None` when + /// it is not exactly one `/`-separated pair of a valid owner and + /// repository name. + #[must_use] + pub fn try_new(value: &str) -> Option { + let (owner, repo) = value.split_once('/')?; + if repo.contains('/') || !valid_github_owner(owner) || !valid_github_repo(repo) { + return None; + } + Some(Self { + owner: owner.to_string(), + repo: repo.to_string(), + }) + } + + #[must_use] + pub fn owner(&self) -> &str { + &self.owner + } + + #[must_use] + pub fn repo(&self) -> &str { + &self.repo + } +} + +fn valid_github_owner(value: &str) -> bool { + if value.is_empty() || value.len() > 39 { + return false; + } + let bytes = value.as_bytes(); + let first = bytes[0]; + let last = bytes[bytes.len() - 1]; + (first.is_ascii_alphanumeric() && last.is_ascii_alphanumeric()) + && bytes + .iter() + .all(|byte| byte.is_ascii_alphanumeric() || *byte == b'-') +} + +fn valid_github_repo(value: &str) -> bool { + !value.is_empty() + && value.len() <= 100 + && value != "." + && value != ".." + && value + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b'-')) +} + +/// Reports whether `value` is a safe GitHub git ref selector. +/// +/// The grammar checks the exact untrimmed ASCII byte content; no +/// normalization is performed. +#[must_use] +pub fn is_valid_github_ref_selector(value: &str) -> bool { + !value.is_empty() + && value.len() <= 255 + && value.trim() == value + && !value.starts_with(['/', '-', '.']) + && !value.ends_with(['/', '.']) + && !has_lock_suffix(value) + && value != "@" + && !value.contains("..") + && !value.contains("//") + && !value.contains("@{") + && value + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'/' | b'.' | b'_' | b'-')) + && value + .split('/') + .all(|part| !part.is_empty() && !part.starts_with('.') && !has_lock_suffix(part)) +} + +fn has_lock_suffix(value: &str) -> bool { + value + .rsplit_once('.') + .is_some_and(|(_, extension)| extension == "lock") +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum RepositoryProvider { @@ -91,3 +182,99 @@ fn repository_name_from_path(path: &str) -> Option<&str> { fn path_basename(path: &str) -> Option<&str> { path.rsplit(['/', '\\']).find(|segment| !segment.is_empty()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn valid_slugs_preserve_exact_parts() { + let cases = [ + ("fabro-sh/fabro", "fabro-sh", "fabro"), + ("owner/.github", "owner", ".github"), + ]; + for (input, owner, repo) in cases { + let slug = GitHubRepositorySlug::try_new(input).expect(input); + assert_eq!(slug.owner(), owner, "{input}"); + assert_eq!(slug.repo(), repo, "{input}"); + } + + let max_owner = "a".repeat(39); + let max_repo = "b".repeat(100); + let boundary = format!("{max_owner}/{max_repo}"); + let slug = GitHubRepositorySlug::try_new(&boundary).expect("boundary-length slug"); + assert_eq!(slug.owner(), max_owner); + assert_eq!(slug.repo(), max_repo); + } + + #[test] + fn invalid_slugs_are_rejected() { + let cases = [ + "", + "fabro", + "not/github/slug", + "/repo", + "owner/", + "-owner/repo", + "owner-/repo", + "own_er/repo", + "owner/.", + "owner/..", + "owner/re po", + "öwner/repo", + "owner/rëpo", + ]; + for input in cases { + assert!(GitHubRepositorySlug::try_new(input).is_none(), "{input}"); + } + + let over_owner = format!("{}/repo", "a".repeat(40)); + let over_repo = format!("owner/{}", "b".repeat(101)); + assert!(GitHubRepositorySlug::try_new(&over_owner).is_none()); + assert!(GitHubRepositorySlug::try_new(&over_repo).is_none()); + } + + #[test] + fn valid_ref_selectors_are_accepted() { + let max = "a".repeat(255); + let cases = [ + "main", + "feature/release-1.2_rc", + "refs/tags/v1.0.0", + max.as_str(), + ]; + for input in cases { + assert!(is_valid_github_ref_selector(input), "{input}"); + } + } + + #[test] + fn invalid_ref_selectors_are_rejected() { + let cases = [ + "", + " main", + "main ", + "/main", + "-main", + ".main", + "main/", + "main.", + "feature//x", + "feature/.hidden", + "main.lock", + "branch.lock/x", + "@", + "a..b", + "a@{b", + "main;rm", + "ma\tin", + "mäin", + ]; + for input in cases { + assert!(!is_valid_github_ref_selector(input), "{input:?}"); + } + + let over = "a".repeat(256); + assert!(!is_valid_github_ref_selector(&over)); + } +}