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)); + } +}