From 582712633558f77e39ff2824576231e8a2d31cd0 Mon Sep 17 00:00:00 2001 From: Azalea <22280294+hykilpikonna@users.noreply.github.com> Date: Sat, 9 May 2026 23:44:18 +0000 Subject: [PATCH] [F] Webhook issues --- Cargo.lock | 1 + Cargo.toml | 1 + README.md | 4 +- src/config.rs | 35 +++++++-- src/interactive.rs | 24 ++---- src/provider.rs | 42 +++++++++-- src/sync.rs | 27 ++++--- src/webhook.rs | 32 +------- tests/e2e/sequential.rs | 161 +++++++++++++++++++++++++++++++++++++--- tests/unit/config.rs | 53 ++++++++++++- tests/unit/provider.rs | 68 ++++++++++++++++- tests/unit/sync.rs | 71 ++++++++++++------ tests/unit/webhook.rs | 4 +- 13 files changed, 411 insertions(+), 112 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 07ad363..8265b04 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1125,6 +1125,7 @@ dependencies = [ "console", "dialoguer", "directories", + "getrandom 0.3.4", "hmac", "regex", "reqwest", diff --git a/Cargo.toml b/Cargo.toml index 67f0ad4..f937bb4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,6 +9,7 @@ clap = { version = "4.5", features = ["derive"] } console = "0.16" dialoguer = "0.12" directories = "6.0" +getrandom = "0.3" hmac = "0.13" reqwest = { version = "0.13", default-features = false, features = ["blocking", "json", "rustls"] } regex = "1.11" diff --git a/README.md b/README.md index f023756..fb68040 100644 --- a/README.md +++ b/README.md @@ -58,13 +58,13 @@ jobs = 8 name = "github" provider = "github" base_url = "https://github.com" -token = { env = "GITHUB_TOKEN" } +token = { value = "github_pat_..." } [[sites]] name = "gitea" provider = "gitea" base_url = "https://gitea.example.com" -token = { env = "GITEA_TOKEN" } +token = { value = "gitea_pat_..." } [[mirrors]] name = "personal" diff --git a/src/config.rs b/src/config.rs index 5e1135d..36604ac 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,4 +1,4 @@ -use std::env; +use std::collections::BTreeSet; use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; @@ -48,7 +48,6 @@ pub enum ProviderKind { #[serde(rename_all = "snake_case")] pub enum TokenConfig { Value(String), - Env(String), } #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] @@ -217,7 +216,7 @@ impl Config { .with_context(|| format!("failed to create {}", parent.display()))?; } let contents = toml::to_string_pretty(self)?; - let mut file = fs::File::create(path) + let mut file = create_private_file(path) .with_context(|| format!("failed to create {}", path.display()))?; file.write_all(contents.as_bytes()) .with_context(|| format!("failed to write {}", path.display()))?; @@ -295,11 +294,9 @@ impl WebhookConfig { } impl TokenConfig { - pub fn value(&self, label: &str) -> Result { + pub fn value(&self, _label: &str) -> Result { match self { TokenConfig::Value(value) => Ok(value.clone()), - TokenConfig::Env(name) => env::var(name) - .with_context(|| format!("environment variable {name} for {label} is not set")), } } } @@ -326,6 +323,24 @@ fn trim_end(value: &str) -> &str { value.trim_end_matches('/') } +#[cfg(unix)] +fn create_private_file(path: &Path) -> Result { + use std::os::unix::fs::OpenOptionsExt; + + fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path) + .with_context(|| format!("failed to create {}", path.display())) +} + +#[cfg(not(unix))] +fn create_private_file(path: &Path) -> Result { + fs::File::create(path).with_context(|| format!("failed to create {}", path.display())) +} + #[cfg(unix)] fn protect_file(path: &Path) -> Result<()> { use std::os::unix::fs::PermissionsExt; @@ -358,7 +373,15 @@ pub fn validate_config(config: &Config) -> Result<()> { mirror.name ); } + let mut endpoints = BTreeSet::new(); for endpoint in &mirror.endpoints { + if !endpoints.insert(endpoint) { + bail!( + "mirror '{}' contains duplicate endpoint {}", + mirror.name, + endpoint.label() + ); + } config.site(&endpoint.site).ok_or_else(|| { anyhow!( "mirror '{}' references unknown site '{}'", diff --git a/src/interactive.rs b/src/interactive.rs index e2f1bab..43e84ff 100644 --- a/src/interactive.rs +++ b/src/interactive.rs @@ -1,10 +1,8 @@ use std::fmt::Display; -use std::fs::File; -use std::io::Read; use std::path::Path; use std::sync::mpsc; use std::thread; -use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use std::time::Duration; use anyhow::{Context, Result, anyhow}; use console::{Term, style}; @@ -284,7 +282,7 @@ fn prompt_webhook_setup_styled(config: &mut Config, theme: &ColorfulTheme) -> Re config.webhook = Some(WebhookConfig { install: true, url, - secret: TokenConfig::Value(generate_webhook_secret()), + secret: TokenConfig::Value(generate_webhook_secret()?), full_sync_interval_minutes, reachability_check_interval_minutes: Some(15), }); @@ -1340,25 +1338,15 @@ fn validate_url(value: &str) -> std::result::Result<(), String> { } } -fn generate_webhook_secret() -> String { +fn generate_webhook_secret() -> Result { let mut bytes = [0_u8; 32]; - if File::open("/dev/urandom") - .and_then(|mut file| file.read_exact(&mut bytes)) - .is_err() - { - let nanos = SystemTime::now() - .duration_since(UNIX_EPOCH) - .map(|duration| duration.as_nanos()) - .unwrap_or_default(); - for (index, byte) in bytes.iter_mut().enumerate() { - *byte = ((nanos >> ((index % 16) * 8)) & 0xff) as u8; - } - } + getrandom::fill(&mut bytes) + .map_err(|error| anyhow!("failed to generate webhook secret: {error}"))?; let mut output = String::with_capacity(bytes.len() * 2); for byte in bytes { output.push_str(&format!("{byte:02x}")); } - output + Ok(output) } #[cfg(test)] diff --git a/src/provider.rs b/src/provider.rs index 62c40cf..4f6ab34 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -787,9 +787,10 @@ impl<'a> ProviderClient<'a> { fn find_existing_hook(&self, hooks_url: &str, target_url: &str) -> Result> { let hooks: Vec = self.paged_get(hooks_url)?; - Ok(hooks - .into_iter() - .find(|hook| hook.url() == Some(target_url))) + Ok(hooks.into_iter().find(|hook| { + hook.url() + .is_some_and(|hook_url| webhook_urls_match(hook_url, target_url)) + })) } fn upsert_hook( @@ -981,6 +982,34 @@ fn is_conflict_error(error: &anyhow::Error) -> bool { error.to_string().contains("409 Conflict") } +fn webhook_urls_match(left: &str, right: &str) -> bool { + if left == right { + return true; + } + match (normalize_webhook_url(left), normalize_webhook_url(right)) { + (Some(left), Some(right)) => left == right, + _ => false, + } +} + +fn normalize_webhook_url(value: &str) -> Option { + let url = Url::parse(value).ok()?; + let scheme = url.scheme().to_ascii_lowercase(); + if scheme != "http" && scheme != "https" { + return None; + } + let host = url.host_str()?.to_ascii_lowercase(); + let port = url.port_or_known_default()?; + let username = url.username(); + let password = url.password().unwrap_or_default(); + let path = url.path().trim_end_matches('/'); + let path = if path.is_empty() { "/" } else { path }; + let query = url.query().unwrap_or_default(); + Some(format!( + "{scheme}://{username}:{password}@{host}:{port}{path}?{query}" + )) +} + fn next_link(headers: &HeaderMap) -> Option { let header = headers.get("link")?.to_str().ok()?; for part in header.split(',') { @@ -1131,9 +1160,10 @@ struct PullRequestHead { impl RepoHook { fn url(&self) -> Option<&str> { - self.url - .as_deref() - .or_else(|| self.config.get("url").map(String::as_str)) + self.config + .get("url") + .map(String::as_str) + .or(self.url.as_deref()) } } diff --git a/src/sync.rs b/src/sync.rs index c6200c9..7d8ff54 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -9,8 +9,8 @@ use console::style; use regex::Regex; use crate::config::{ - Config, ConflictResolutionStrategy, DEFAULT_JOBS, EndpointConfig, MirrorConfig, RepoNameFilter, - SyncVisibility, default_work_dir, validate_config, + Config, ConflictResolutionStrategy, DEFAULT_JOBS, EndpointConfig, MirrorConfig, NamespaceKind, + RepoNameFilter, SyncVisibility, default_work_dir, validate_config, }; use crate::git::{ BranchConflict, BranchDeletion, BranchUpdate, GitMirror, Redactor, RemoteSpec, @@ -846,12 +846,8 @@ fn remote_specs(context: &RepoSyncContext<'_>, repos: &[EndpointRepo]) -> Result } let site = context.config.site(&endpoint_repo.endpoint.site).unwrap(); let client = ProviderClient::new(site)?; - let remote_name = safe_remote_name(&format!( - "{}_{}", - endpoint_repo.endpoint.site, endpoint_repo.endpoint.namespace - )); remotes.push(RemoteSpec { - name: remote_name, + name: remote_name_for_endpoint(&endpoint_repo.endpoint), url: client.authenticated_clone_url(&endpoint_repo.repo.clone_url)?, display: endpoint_repo.endpoint.label(), }); @@ -1274,7 +1270,20 @@ fn remote_name_for_endpoint_repo(endpoint_repo: &EndpointRepo) -> String { } fn remote_name_for_endpoint(endpoint: &EndpointConfig) -> String { - safe_remote_name(&format!("{}_{}", endpoint.site, endpoint.namespace)) + format!( + "r{}_{}_{}", + hex_component(&endpoint.site), + namespace_kind_key(&endpoint.kind), + hex_component(&endpoint.namespace) + ) +} + +fn namespace_kind_key(kind: &NamespaceKind) -> &'static str { + match kind { + NamespaceKind::User => "user", + NamespaceKind::Org => "org", + NamespaceKind::Group => "group", + } } fn branch_names(branches: &[crate::git::BranchDecision]) -> BTreeSet { @@ -1329,7 +1338,7 @@ fn hex_component(value: &str) -> String { } fn decode_hex_component(value: &str) -> Option { - if value.len() % 2 != 0 { + if !value.len().is_multiple_of(2) { return None; } let mut bytes = Vec::with_capacity(value.len() / 2); diff --git a/src/webhook.rs b/src/webhook.rs index 3a289ac..d838202 100644 --- a/src/webhook.rs +++ b/src/webhook.rs @@ -215,7 +215,7 @@ pub fn install_webhooks(config: &Config, options: WebhookInstallOptions) -> Resu }); } } - run_install_tasks(tasks, options.jobs, Arc::clone(&state), false)?; + run_install_tasks(tasks, options.jobs, Arc::clone(&state))?; } if !options.dry_run { let state = state @@ -345,7 +345,7 @@ pub fn ensure_configured_webhooks( dry_run: false, }); } - run_install_tasks(tasks, jobs, Arc::clone(&state), true)?; + run_install_tasks(tasks, jobs, Arc::clone(&state))?; let state = state .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); @@ -480,7 +480,6 @@ fn run_install_tasks( tasks: Vec, jobs: usize, state: Arc>, - use_state_cache: bool, ) -> Result<()> { if tasks.is_empty() { return Ok(()); @@ -510,7 +509,7 @@ fn run_install_tasks( break; }; if result_sender - .send(install_webhook_task(task, &state, use_state_cache)) + .send(install_webhook_task(task, &state)) .is_err() { break; @@ -598,31 +597,8 @@ fn run_uninstall_tasks(tasks: Vec, jobs: usize) -> Result< Ok(removed_keys) } -fn install_webhook_task( - task: WebhookInstallTask, - state: &Arc>, - use_state_cache: bool, -) -> Result<()> { +fn install_webhook_task(task: WebhookInstallTask, state: &Arc>) -> Result<()> { let key = webhook_installation_key(&task.group, &task.endpoint, &task.repo.name); - if use_state_cache { - let state = state - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - if state - .installations - .get(&key) - .is_some_and(|installation| installation.url == task.url) - { - return Ok(()); - } - if state - .skipped - .get(&key) - .is_some_and(|skipped| skipped.url == task.url) - { - return Ok(()); - } - } crate::logln!( " {} {} {}", style(if task.dry_run { diff --git a/tests/e2e/sequential.rs b/tests/e2e/sequential.rs index d23d10b..02f87ac 100644 --- a/tests/e2e/sequential.rs +++ b/tests/e2e/sequential.rs @@ -613,11 +613,18 @@ namespace = "{}" fn webhook_commands_and_receiver_work(&self) -> Result<()> { let repo = self.repo_name("webhook"); let source = self.primary_provider(); + let webhook_url = format!("https://example.com/refray-e2e/{}/{repo}", self.run_id); + let webhook_url_with_slash = format!("{webhook_url}/"); self.seed_all_main(&repo, "webhook base", 1_700_001_601)?; self.sync_repo(&repo, [])?; + self.set_webhook_url(&webhook_url)?; self.refray(["webhook", "install", "--dry-run"])?; self.refray(["webhook", "uninstall", "--dry-run"])?; + self.refray(["webhook", "install"])?; + self.assert_webhook_exists_all(&repo, &webhook_url)?; + self.refray(["webhook", "uninstall", &webhook_url_with_slash])?; + self.assert_webhook_absent_all(&repo, &webhook_url)?; self.refray([ "webhook", "update", @@ -783,6 +790,41 @@ namespace = "{}" .with_context(|| format!("failed to write {}", self.config_path.display())) } + fn set_webhook_url(&self, url: &str) -> Result<()> { + let contents = fs::read_to_string(&self.config_path) + .with_context(|| format!("failed to read {}", self.config_path.display()))?; + let escaped_url = url.replace('"', "\\\""); + let mut in_webhook = false; + let mut replaced = false; + let mut updated = contents + .lines() + .map(|line| { + if line.trim() == "[webhook]" { + in_webhook = true; + return line.to_string(); + } + if line.starts_with('[') { + in_webhook = false; + } + if in_webhook && line.starts_with("url = ") { + replaced = true; + format!("url = \"{escaped_url}\"") + } else { + line.to_string() + } + }) + .collect::>() + .join("\n"); + if contents.ends_with('\n') { + updated.push('\n'); + } + if !replaced { + bail!("config is missing [webhook].url"); + } + fs::write(&self.config_path, updated) + .with_context(|| format!("failed to write {}", self.config_path.display())) + } + fn sync(&self, args: [&str; N]) -> Result<()> { let mut command = vec!["sync"]; command.extend(args); @@ -1014,6 +1056,42 @@ namespace = "{}" }) } + fn assert_webhook_exists_all(&self, repo: &str, url: &str) -> Result<()> { + retry("webhook installed", || { + for provider in &self.settings.providers { + let urls = provider.list_webhook_urls(repo)?; + if !urls + .iter() + .any(|candidate| webhook_urls_match(candidate, url)) + { + bail!( + "webhook {url} missing on {} for {repo}; found {urls:?}", + provider.site_name + ); + } + } + Ok(()) + }) + } + + fn assert_webhook_absent_all(&self, repo: &str, url: &str) -> Result<()> { + retry("webhook uninstalled", || { + for provider in &self.settings.providers { + let urls = provider.list_webhook_urls(repo)?; + if urls + .iter() + .any(|candidate| webhook_urls_match(candidate, url)) + { + bail!( + "webhook {url} still exists on {} for {repo}", + provider.site_name + ); + } + } + Ok(()) + }) + } + fn refs_by_provider(&self, repo: &str) -> Result> { let mut output = BTreeMap::new(); for provider in &self.settings.providers { @@ -1081,17 +1159,17 @@ impl ProviderAccount { ) -> Self { let mut base_url = trim_url(&base_url).to_string(); let mut username = username; - if let Ok(url) = Url::parse(&username) { - if let Some(host) = url.host_str() { - let path = url.path().trim_matches('/'); - if !path.is_empty() { - let mut profile_base_url = format!("{}://{}", url.scheme(), host); - if let Some(port) = url.port() { - profile_base_url.push_str(&format!(":{port}")); - } - base_url = profile_base_url; - username = path.to_string(); + if let Ok(url) = Url::parse(&username) + && let Some(host) = url.host_str() + { + let path = url.path().trim_matches('/'); + if !path.is_empty() { + let mut profile_base_url = format!("{}://{}", url.scheme(), host); + if let Some(port) = url.port() { + profile_base_url.push_str(&format!(":{port}")); } + base_url = profile_base_url; + username = path.to_string(); } } Self { @@ -1454,6 +1532,33 @@ impl ProviderAccount { .collect()) } + fn list_webhook_urls(&self, repo: &str) -> Result> { + let url = match self.kind { + ProviderKind::Github => format!( + "{}/repos/{}/{}/hooks?per_page=100", + self.api_base(), + self.username, + repo + ), + ProviderKind::Gitlab => format!( + "{}/projects/{}/hooks?per_page=100", + self.api_base(), + urlencoding(&format!("{}/{}", self.username, repo)) + ), + ProviderKind::Gitea | ProviderKind::Forgejo => format!( + "{}/repos/{}/{}/hooks?limit=50", + self.api_base(), + self.username, + repo + ), + }; + Ok(self + .paged_get_values(&url)? + .into_iter() + .filter_map(|value| webhook_url_from_json(&value)) + .collect()) + } + fn paged_get(&self, first_url: &str) -> Result> { Ok(self .paged_get_values(first_url)? @@ -1808,6 +1913,42 @@ fn trim_url(value: &str) -> &str { value.trim_end_matches('/') } +fn webhook_url_from_json(value: &Value) -> Option { + value + .pointer("/config/url") + .or_else(|| value.get("url")) + .and_then(Value::as_str) + .map(ToOwned::to_owned) +} + +fn webhook_urls_match(left: &str, right: &str) -> bool { + if left == right { + return true; + } + match (normalize_webhook_url(left), normalize_webhook_url(right)) { + (Some(left), Some(right)) => left == right, + _ => false, + } +} + +fn normalize_webhook_url(value: &str) -> Option { + let url = Url::parse(value).ok()?; + let scheme = url.scheme().to_ascii_lowercase(); + if scheme != "http" && scheme != "https" { + return None; + } + let host = url.host_str()?.to_ascii_lowercase(); + let port = url.port_or_known_default()?; + let username = url.username(); + let password = url.password().unwrap_or_default(); + let path = url.path().trim_end_matches('/'); + let path = if path.is_empty() { "/" } else { path }; + let query = url.query().unwrap_or_default(); + Some(format!( + "{scheme}://{username}:{password}@{host}:{port}{path}?{query}" + )) +} + fn urlencoding(value: &str) -> String { url::form_urlencoded::byte_serialize(value.as_bytes()).collect() } diff --git a/tests/unit/config.rs b/tests/unit/config.rs index 24f6171..085415b 100644 --- a/tests/unit/config.rs +++ b/tests/unit/config.rs @@ -1,7 +1,7 @@ use super::*; #[test] -fn parses_token_forms() { +fn parses_value_tokens() { let config: Config = toml::from_str( r#" jobs = 8 @@ -9,7 +9,7 @@ fn parses_token_forms() { [webhook] install = true url = "https://mirror.example.test/webhook" - secret = { env = "WEBHOOK_SECRET" } + secret = { value = "webhook-secret" } full_sync_interval_minutes = 60 reachability_check_interval_minutes = 15 @@ -17,7 +17,7 @@ fn parses_token_forms() { name = "github" provider = "github" base_url = "https://github.com" - token = { env = "GITHUB_TOKEN" } + token = { value = "github-token" } [[mirrors]] name = "personal" @@ -62,11 +62,28 @@ fn parses_token_forms() { assert_eq!(webhook.url, "https://mirror.example.test/webhook"); assert_eq!( webhook.secret, - TokenConfig::Env("WEBHOOK_SECRET".to_string()) + TokenConfig::Value("webhook-secret".to_string()) ); assert_eq!(webhook.full_sync_interval_minutes, Some(60)); } +#[test] +fn env_token_form_is_rejected() { + let err = toml::from_str::( + r#" + [[sites]] + name = "github" + provider = "github" + base_url = "https://github.com" + token = { env = "GITHUB_TOKEN" } + "#, + ) + .unwrap_err() + .to_string(); + + assert!(err.contains("unknown variant") || err.contains("expected")); +} + #[test] fn config_defaults_jobs() { let config: Config = toml::from_str("").unwrap(); @@ -206,6 +223,34 @@ fn validation_rejects_invalid_repo_filter_regex() { assert!(err.contains("invalid repo_whitelist regex")); } +#[test] +fn validation_rejects_duplicate_mirror_endpoints() { + let duplicate = EndpointConfig { + site: "github".to_string(), + kind: NamespaceKind::User, + namespace: "alice".to_string(), + }; + let config = Config { + jobs: crate::config::DEFAULT_JOBS, + sites: vec![site("github", ProviderKind::Github)], + mirrors: vec![MirrorConfig { + name: "broken".to_string(), + endpoints: vec![duplicate.clone(), duplicate], + sync_visibility: SyncVisibility::All, + repo_whitelist: Vec::new(), + repo_blacklist: Vec::new(), + create_missing: true, + visibility: Visibility::Private, + conflict_resolution: ConflictResolutionStrategy::Fail, + }], + webhook: None, + }; + + let err = validate_config(&config).unwrap_err().to_string(); + + assert!(err.contains("duplicate endpoint")); +} + #[test] fn validation_rejects_zero_jobs() { let mut config = Config { diff --git a/tests/unit/provider.rs b/tests/unit/provider.rs index 8e9a2ef..41d001d 100644 --- a/tests/unit/provider.rs +++ b/tests/unit/provider.rs @@ -62,6 +62,26 @@ fn group_paths_are_url_encoded_for_gitlab() { assert_eq!(urlencoding("parent/child group"), "parent%2Fchild+group"); } +#[test] +fn webhook_url_matching_tolerates_trailing_slash_and_default_port() { + assert!(webhook_urls_match( + "https://example.test", + "https://example.test/" + )); + assert!(webhook_urls_match( + "https://example.test:443/webhook/", + "https://EXAMPLE.test/webhook" + )); + assert!(!webhook_urls_match( + "https://example.test/webhook", + "https://example.test/" + )); + assert!(!webhook_urls_match( + "https://example.test/webhook?token=one", + "https://example.test/webhook?token=two" + )); +} + #[test] fn validate_token_checks_user_endpoint_with_provider_auth_header() { let (api_url, handle) = one_request_server("200 OK", "{}", |request| { @@ -217,7 +237,7 @@ fn uninstall_webhook_deletes_matching_github_hook() { vec![ ( "200 OK", - r#"[{"id":42,"config":{"url":"https://mirror.example.test/webhook"}}]"#, + r#"[{"id":42,"url":"https://api.github.com/repos/alice/repo/hooks/42","config":{"url":"https://mirror.example.test/webhook"}}]"#, ), ("204 No Content", ""), ], @@ -255,11 +275,55 @@ fn uninstall_webhook_deletes_matching_github_hook() { handle.join().unwrap(); } +#[test] +fn uninstall_webhook_matches_github_hook_url_without_trailing_slash() { + let (api_url, handle) = request_server( + vec![ + ( + "200 OK", + r#"[{"id":42,"url":"https://api.github.com/repos/alice/repo/hooks/42","config":{"url":"https://mirror.example.test"}}]"#, + ), + ("204 No Content", ""), + ], + |index, request| match index { + 0 => assert!( + request.starts_with("GET /repos/alice/repo/hooks "), + "request was {request}" + ), + 1 => assert!( + request.starts_with("DELETE /repos/alice/repo/hooks/42 "), + "request was {request}" + ), + _ => unreachable!(), + }, + ); + let site = SiteConfig { + api_url: Some(api_url), + ..site(ProviderKind::Github, None) + }; + let client = ProviderClient::new(&site).unwrap(); + + let removed = client + .uninstall_webhook( + &EndpointConfig { + site: "github".to_string(), + kind: NamespaceKind::User, + namespace: "alice".to_string(), + }, + "repo", + "https://mirror.example.test/", + ) + .unwrap(); + + assert!(removed); + handle.join().unwrap(); +} + #[test] fn uninstall_webhook_reports_missing_github_hook() { let (api_url, handle) = one_request_server( "200 OK", - r#"[{"id":42,"config":{"url":"https://old.example.test/webhook"}}]"#, + r#"[{"id":42,"url":"https://api.github.com/repos/alice/repo/hooks/42","config":{"url":"https://old.example.test/webhook"}}]"#, |request| { assert!( request.starts_with("GET /repos/alice/repo/hooks "), diff --git a/tests/unit/sync.rs b/tests/unit/sync.rs index e673ed1..f1aad56 100644 --- a/tests/unit/sync.rs +++ b/tests/unit/sync.rs @@ -59,11 +59,11 @@ fn ref_state_persists_and_requires_exact_remote_ref_match() { let temp = tempfile::TempDir::new().unwrap(); let mut refs = BTreeMap::new(); refs.insert( - "github_alice".to_string(), + remote_key("github"), remote_ref_state("abc", &[("main", "111")]), ); refs.insert( - "gitea_alice".to_string(), + remote_key("gitea"), remote_ref_state("def", &[("main", "111")]), ); let mut state = RefState::default(); @@ -76,13 +76,13 @@ fn ref_state_persists_and_requires_exact_remote_ref_match() { let mut changed_hash = refs.clone(); changed_hash.insert( - "github_alice".to_string(), + remote_key("github"), remote_ref_state("changed", &[("main", "111")]), ); assert!(!loaded.repo_matches("sync-1", "repo-a", &changed_hash)); let mut missing_remote = refs; - missing_remote.remove("gitea_alice"); + missing_remote.remove(&remote_key("gitea")); assert!(!loaded.repo_matches("sync-1", "repo-a", &missing_remote)); } @@ -179,16 +179,16 @@ fn repo_deletion_decision_propagates_previous_synced_repo_deletion() { let mirror = test_mirror(); let mut previous = BTreeMap::new(); previous.insert( - "github_alice".to_string(), + remote_key("github"), remote_ref_state("a", &[("main", "111")]), ); previous.insert( - "gitea_alice".to_string(), + remote_key("gitea"), remote_ref_state("b", &[("main", "111")]), ); let mut current = BTreeMap::new(); current.insert( - "gitea_alice".to_string(), + remote_key("gitea"), remote_ref_state("b", &[("main", "111")]), ); @@ -202,8 +202,8 @@ fn repo_deletion_decision_propagates_previous_synced_repo_deletion() { assert_eq!( decision, RepoDeletionDecision::Propagate { - deleted_remotes: vec!["github_alice".to_string()], - target_remotes: vec!["gitea_alice".to_string()], + deleted_remotes: vec![remote_key("github")], + target_remotes: vec![remote_key("gitea")], } ); } @@ -213,16 +213,16 @@ fn repo_deletion_decision_conflicts_when_remaining_repo_changed() { let mirror = test_mirror(); let mut previous = BTreeMap::new(); previous.insert( - "github_alice".to_string(), + remote_key("github"), remote_ref_state("a", &[("main", "111")]), ); previous.insert( - "gitea_alice".to_string(), + remote_key("gitea"), remote_ref_state("b", &[("main", "111")]), ); let mut current = BTreeMap::new(); current.insert( - "gitea_alice".to_string(), + remote_key("gitea"), remote_ref_state("changed", &[("main", "222")]), ); @@ -236,8 +236,8 @@ fn repo_deletion_decision_conflicts_when_remaining_repo_changed() { assert_eq!( decision, RepoDeletionDecision::Conflict { - deleted_remotes: vec!["github_alice".to_string()], - changed_remotes: vec!["gitea_alice".to_string()], + deleted_remotes: vec![remote_key("github")], + changed_remotes: vec![remote_key("gitea")], } ); } @@ -247,11 +247,11 @@ fn repo_deletion_decision_removes_state_when_deleted_everywhere() { let mirror = test_mirror(); let mut previous = BTreeMap::new(); previous.insert( - "github_alice".to_string(), + remote_key("github"), remote_ref_state("a", &[("main", "111")]), ); previous.insert( - "gitea_alice".to_string(), + remote_key("gitea"), remote_ref_state("b", &[("main", "111")]), ); @@ -260,7 +260,7 @@ fn repo_deletion_decision_removes_state_when_deleted_everywhere() { assert_eq!( decision, RepoDeletionDecision::DeletedEverywhere { - deleted_remotes: vec!["github_alice".to_string(), "gitea_alice".to_string()], + deleted_remotes: vec![remote_key("github"), remote_key("gitea")], } ); } @@ -270,7 +270,7 @@ fn repo_deletion_decision_removes_partial_state_when_deleted_everywhere() { let mirror = test_mirror(); let mut previous = BTreeMap::new(); previous.insert( - "gitea_alice".to_string(), + remote_key("gitea"), remote_ref_state("b", &[("main", "111")]), ); @@ -279,7 +279,7 @@ fn repo_deletion_decision_removes_partial_state_when_deleted_everywhere() { assert_eq!( decision, RepoDeletionDecision::DeletedEverywhere { - deleted_remotes: vec!["github_alice".to_string(), "gitea_alice".to_string()], + deleted_remotes: vec![remote_key("github"), remote_key("gitea")], } ); } @@ -289,12 +289,12 @@ fn repo_deletion_decision_ignores_repos_not_previously_synced_everywhere() { let mirror = test_mirror(); let mut previous = BTreeMap::new(); previous.insert( - "gitea_alice".to_string(), + remote_key("gitea"), remote_ref_state("b", &[("main", "111")]), ); let mut current = BTreeMap::new(); current.insert( - "gitea_alice".to_string(), + remote_key("gitea"), remote_ref_state("b", &[("main", "111")]), ); @@ -316,7 +316,7 @@ fn filtered_sync_visibility_does_not_treat_state_only_repos_as_deleted() { ref_state.set_repo( &mirror.name, "private-repo", - BTreeMap::from([("github_alice".to_string(), remote_ref_state("a", &[]))]), + BTreeMap::from([(remote_key("github"), remote_ref_state("a", &[]))]), ); let repo_filter = mirror.repo_filter().unwrap(); @@ -332,7 +332,7 @@ fn all_visibility_keeps_state_only_repos_for_deletion_detection() { ref_state.set_repo( &mirror.name, "deleted-repo", - BTreeMap::from([("github_alice".to_string(), remote_ref_state("a", &[]))]), + BTreeMap::from([(remote_key("github"), remote_ref_state("a", &[]))]), ); let repo_filter = mirror.repo_filter().unwrap(); @@ -350,7 +350,7 @@ fn repo_name_filters_do_not_treat_state_only_repos_as_deleted() { ref_state.set_repo( &mirror.name, "private-repo", - BTreeMap::from([("github_alice".to_string(), remote_ref_state("a", &[]))]), + BTreeMap::from([(remote_key("github"), remote_ref_state("a", &[]))]), ); let names = sync_candidate_repo_names(&HashMap::new(), &ref_state, &mirror, &repo_filter); @@ -377,6 +377,25 @@ fn conflict_branch_prefixes_are_reversible_not_slug_collisions() { ); } +#[test] +fn endpoint_remote_names_do_not_slug_collide() { + let slash = EndpointConfig { + site: "gitlab".to_string(), + kind: crate::config::NamespaceKind::Group, + namespace: "parent/child".to_string(), + }; + let underscore = EndpointConfig { + site: "gitlab".to_string(), + kind: crate::config::NamespaceKind::Group, + namespace: "parent_child".to_string(), + }; + + assert_ne!( + remote_name_for_endpoint(&slash), + remote_name_for_endpoint(&underscore) + ); +} + fn remote_ref_state(hash: &str, branches: &[(&str, &str)]) -> RemoteRefState { RemoteRefState { hash: hash.to_string(), @@ -425,6 +444,10 @@ fn endpoint(site: &str) -> EndpointConfig { } } +fn remote_key(site: &str) -> String { + remote_name_for_endpoint(&endpoint(site)) +} + fn endpoint_repo(site: &str) -> EndpointRepo { EndpointRepo { endpoint: endpoint(site), diff --git a/tests/unit/webhook.rs b/tests/unit/webhook.rs index 438ab3b..5e07453 100644 --- a/tests/unit/webhook.rs +++ b/tests/unit/webhook.rs @@ -342,7 +342,6 @@ fn blocked_webhook_install_is_skipped_and_recorded() { dry_run: false, }, &state, - false, ) .unwrap(); @@ -401,7 +400,7 @@ fn duplicate_webhook_error_records_existing_installation() { } #[test] -fn install_task_state_cache_is_only_used_for_sync() { +fn install_task_rechecks_cached_installation() { let (api_url, handle) = request_server( vec![("200 OK", "[]"), ("201 Created", r#"{"id":1}"#)], |index, request| match index { @@ -442,7 +441,6 @@ fn install_task_state_cache_is_only_used_for_sync() { dry_run: false, }, &state, - false, ) .unwrap();