From 38962207e5e34648a4e23c0a84b880fde4730679 Mon Sep 17 00:00:00 2001 From: estib Date: Tue, 24 Feb 2026 13:37:07 +0100 Subject: [PATCH 1/2] forge storage: Don't migrate the old github accounts anymore Old GitHub secrets can now stop being migrated. --- Cargo.lock | 1 - crates/but-forge-storage/Cargo.toml | 1 - crates/but-forge-storage/src/storage.rs | 124 ++---------------------- 3 files changed, 6 insertions(+), 120 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5b01c14ec7f..0a4ea2a2976 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1145,7 +1145,6 @@ version = "0.0.0" dependencies = [ "anyhow", "but-fs", - "but-secret", "serde", "serde_json", ] diff --git a/crates/but-forge-storage/Cargo.toml b/crates/but-forge-storage/Cargo.toml index f212e25c4a4..c11b7e5e424 100644 --- a/crates/but-forge-storage/Cargo.toml +++ b/crates/but-forge-storage/Cargo.toml @@ -14,7 +14,6 @@ description = "Store and retrieve information about forges" doctest = false [dependencies] -but-secret.workspace = true but-fs = { workspace = true, features = ["legacy"] } serde.workspace = true diff --git a/crates/but-forge-storage/src/storage.rs b/crates/but-forge-storage/src/storage.rs index 8143719beee..20d47f292cb 100644 --- a/crates/but-forge-storage/src/storage.rs +++ b/crates/but-forge-storage/src/storage.rs @@ -1,9 +1,6 @@ -use std::{path::PathBuf, sync::Mutex}; - +use crate::settings::ForgeSettings; use anyhow::Result; -use serde::{Deserialize, Serialize}; - -use crate::settings::{ForgeSettings, GitHubAccount}; +use std::path::PathBuf; const FORGE_SETTINGS_FILE: &str = "forge_settings.json"; @@ -20,15 +17,13 @@ impl Storage { } pub fn read(&self) -> Result { - let settings = match self.inner.read(FORGE_SETTINGS_FILE)? { + match self.inner.read(FORGE_SETTINGS_FILE)? { Some(settings) => { let settings: ForgeSettings = serde_json::from_str(&settings)?; - settings + Ok(settings) } - None => Default::default(), - }; - - self.migrate_accounts(settings) + None => Ok(Default::default()), + } } pub fn save(&self, settings: &ForgeSettings) -> Result<()> { @@ -36,111 +31,4 @@ impl Storage { self.inner.write(FORGE_SETTINGS_FILE, &data)?; Ok(()) } - - /// Migrate known GitHub accounts from secret storage to the new settings structure. - /// This can be removed after the stable release. - fn migrate_accounts(&self, settings: ForgeSettings) -> Result { - let old_handle = "github_known_accounts"; - let namespace = but_secret::secret::Namespace::BuildKind; - - if settings.github.known_accounts.is_empty() - && let Some(known_accounts) = but_secret::secret::retrieve(old_handle, namespace)? - { - // Migrate old known accounts from secret storage - let known_account_keys: Vec = serde_json::from_str(&known_accounts)?; - let mut new_settings = settings.clone(); - for account_key in known_account_keys { - if let Some(migrated_account) = migrate_account(&account_key)? { - new_settings.github.known_accounts.push(migrated_account); - } - } - self.save(&new_settings)?; - return Ok(new_settings); - } - - Ok(settings) - } -} - -// =============================== -// Migration of GitHub accounts. -// -// All of the code below this line is only needed to migrate old GitHub accounts, -// and can be removed after the stable release. -// =============================== - -#[derive(Debug, Clone, Serialize, Deserialize)] -#[serde(tag = "type")] -pub enum DeprecatedSerializableGitHubAccount { - OAuth { - username: String, - access_token: String, - }, - Pat { - username: String, - access_token: String, - }, -} - -fn read_deprecated_github_secret( - account_secret_handle: &str, -) -> Result> { - static FAIR_QUEUE: Mutex<()> = Mutex::new(()); - let _one_at_a_time_to_prevent_races = FAIR_QUEUE.lock().unwrap(); - if let Some(secret_value) = but_secret::secret::retrieve( - account_secret_handle, - but_secret::secret::Namespace::BuildKind, - )? { - let account: DeprecatedSerializableGitHubAccount = serde_json::from_str(&secret_value)?; - Ok(Some(account)) - } else { - Ok(None) - } -} - -fn store_access_token_in_secret(account_secret_handle: &str, access_token: &str) -> Result<()> { - static FAIR_QUEUE: Mutex<()> = Mutex::new(()); - let _one_at_a_time_to_prevent_races = FAIR_QUEUE.lock().unwrap(); - but_secret::secret::persist( - account_secret_handle, - &but_secret::Sensitive(access_token.to_string()), - but_secret::secret::Namespace::BuildKind, - ) -} - -fn migrate_account(account_secret_handle: &str) -> Result> { - // Read the deprecated account structure. - // If it fails to parse it we assume there is nothing to migrate. - if let Some(deprecated_account) = read_deprecated_github_secret(account_secret_handle) - .ok() - .flatten() - { - let new_account = match deprecated_account { - DeprecatedSerializableGitHubAccount::OAuth { - username, - access_token, - } => { - // Store the plain access token in the same secret - store_access_token_in_secret(account_secret_handle, &access_token)?; - GitHubAccount::OAuth { - username: username.clone(), - access_token_key: account_secret_handle.to_string(), - } - } - DeprecatedSerializableGitHubAccount::Pat { - username, - access_token, - } => { - // Store the plain access token in the same secret - store_access_token_in_secret(account_secret_handle, &access_token)?; - GitHubAccount::Pat { - username: username.clone(), - access_token_key: account_secret_handle.to_string(), - } - } - }; - - return Ok(Some(new_account)); - } - Ok(None) } From 2dd3a383717e9e224b485a020855a51aaa21ef7b Mon Sep 17 00:00:00 2001 From: estib Date: Tue, 24 Feb 2026 12:54:01 +0100 Subject: [PATCH 2/2] but-forge: Improve forge determination Try to determine the forge type for a given URL, by matching it naively (as we were always doing) and fallback to matching it to the custom hosts for the self-hosted or enterprise accounts. --- Cargo.lock | 1 + crates/but-forge/Cargo.toml | 1 + crates/but-forge/src/lib.rs | 277 +++++++++++++++++++- crates/but-github/src/token.rs | 9 + crates/but-gitlab/src/token.rs | 8 + crates/gitbutler-branch-actions/src/base.rs | 3 +- 6 files changed, 296 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0a4ea2a2976..4521ba1bec4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1130,6 +1130,7 @@ dependencies = [ "but-fs", "but-github", "but-gitlab", + "but-path", "chrono", "git-url-parse", "schemars 1.2.1", diff --git a/crates/but-forge/Cargo.toml b/crates/but-forge/Cargo.toml index c9af7ff270c..9146e0fa01e 100644 --- a/crates/but-forge/Cargo.toml +++ b/crates/but-forge/Cargo.toml @@ -24,6 +24,7 @@ but-github.workspace = true but-gitlab.workspace = true but-forge-storage.workspace = true but-db.workspace = true +but-path.workspace = true chrono.workspace = true tokio = { workspace = true, features = ["rt-multi-thread"] } diff --git a/crates/but-forge/src/lib.rs b/crates/but-forge/src/lib.rs index e884475c04d..9cc92ac2d58 100644 --- a/crates/but-forge/src/lib.rs +++ b/crates/but-forge/src/lib.rs @@ -31,17 +31,290 @@ fn determine_forge_from_host(host: &str) -> Option { } /// Derive the forge repository information from a remote URL. -pub fn derive_forge_repo_info(url: &str) -> Option { +pub fn derive_forge_repo_info(url: &str, accounts: &[ForgeUser]) -> Option { let git_url = GitUrl::parse(url).ok()?; let host = git_url.host()?; let protocol = git_url.scheme()?; let provider_info: GenericProvider = git_url.provider_info().ok()?; + // Attempt to figure out the forge by looking at the host string and + // falling back to matching it to the known accounts custom host URL. + let forge = determine_forge_from_host(host) + .or_else(|| match_host_to_accounts_custom_host(host, accounts))?; Some(ForgeRepoInfo { - forge: determine_forge_from_host(host)?, + forge, owner: provider_info.owner().to_string(), repo: provider_info.repo().to_string(), protocol: protocol.to_string(), }) } + +/// Look for the best matching account by comparing the repository host to the +/// account custom host string. +fn match_host_to_accounts_custom_host(host: &str, accounts: &[ForgeUser]) -> Option { + let user = accounts.iter().find(|account| match account { + ForgeUser::GitHub(gh_account) => gh_account + .custom_host() + .as_deref() + .is_some_and(|custom_host| custom_host_matches_repository_host(host, custom_host)), + ForgeUser::GitLab(gl_account) => gl_account + .custom_host() + .as_deref() + .is_some_and(|custom_host| custom_host_matches_repository_host(host, custom_host)), + }); + + match user { + Some(ForgeUser::GitHub(_)) => Some(ForgeName::GitHub), + Some(ForgeUser::GitLab(_)) => Some(ForgeName::GitLab), + None => None, + } +} + +/// Compare a repository host to an account custom-host string. +/// +/// Motivation: +/// account custom hosts may be stored as full API endpoints (for example +/// `https://api.repository.com/v1/api`), while repository remotes usually +/// provide only the repository host (`repository.com`). +/// +/// Behavior: +/// - both inputs are normalized (scheme, path/query/fragment, user-info, and +/// numeric port are removed; casing is ignored) +/// - exact host matches return `true` +/// - subdomain custom-hosts match their root repository host +/// (`api.repository.com` matches `repository.com`) +/// - partial suffixes do not match (`api.notrepository.com` does not match +/// `repository.com`) +fn custom_host_matches_repository_host(repository_host: &str, account_custom_host: &str) -> bool { + let normalized_repository_host = normalize_host_for_comparison(repository_host); + let normalized_account_host = normalize_host_for_comparison(account_custom_host); + + if normalized_repository_host.is_empty() || normalized_account_host.is_empty() { + return false; + } + + normalized_account_host == normalized_repository_host + || normalized_account_host.ends_with(&format!(".{normalized_repository_host}")) +} + +fn normalize_host_for_comparison(value: &str) -> String { + let without_scheme = value.split_once("://").map_or(value, |(_, rest)| rest); + let without_path = without_scheme + .split(['/', '?', '#']) + .next() + .unwrap_or_default(); + let without_user_info = without_path + .rsplit_once('@') + .map_or(without_path, |(_, host)| host); + + let without_port = match without_user_info.rsplit_once(':') { + Some((host, port)) if port.chars().all(|c| c.is_ascii_digit()) => host, + _ => without_user_info, + }; + + without_port + .trim() + .trim_end_matches('.') + .to_ascii_lowercase() +} + +/// Get all known forge accounts +pub fn get_all_forge_accounts() -> anyhow::Result> { + if let Ok(handle) = tokio::runtime::Handle::try_current() { + tokio::task::block_in_place(|| handle.block_on(get_all_forge_accounts_async())) + } else { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build()?; + runtime.block_on(get_all_forge_accounts_async()) + } +} + +async fn get_all_forge_accounts_async() -> anyhow::Result> { + let storage = but_forge_storage::Controller::from_path(but_path::app_data_dir()?); + let gh_accounts = but_github::list_known_github_accounts(&storage).await?; + let gl_accounts = but_gitlab::list_known_gitlab_accounts(&storage).await?; + + let mut forge_users = vec![]; + for gh_account in gh_accounts { + forge_users.push(ForgeUser::GitHub(gh_account)); + } + + for gl_account in gl_accounts { + forge_users.push(ForgeUser::GitLab(gl_account)); + } + + Ok(forge_users) +} + +#[cfg(test)] +mod tests { + use super::{ + ForgeName, ForgeUser, derive_forge_repo_info, match_host_to_accounts_custom_host, + normalize_host_for_comparison, + }; + + #[test] + fn matches_github_enterprise_custom_host() { + let accounts = vec![ForgeUser::GitHub( + but_github::GithubAccountIdentifier::enterprise("alice", "gh.example.com"), + )]; + + assert_eq!( + match_host_to_accounts_custom_host("gh.example.com", &accounts), + Some(ForgeName::GitHub) + ); + } + + #[test] + fn matches_gitlab_self_hosted_custom_host() { + let accounts = vec![ForgeUser::GitLab( + but_gitlab::GitlabAccountIdentifier::selfhosted("bob", "gl.example.com"), + )]; + + assert_eq!( + match_host_to_accounts_custom_host("gl.example.com", &accounts), + Some(ForgeName::GitLab) + ); + } + + #[test] + fn does_not_match_accounts_without_custom_host() { + let accounts = vec![ + ForgeUser::GitHub(but_github::GithubAccountIdentifier::oauth("alice")), + ForgeUser::GitHub(but_github::GithubAccountIdentifier::pat("charlie")), + ForgeUser::GitLab(but_gitlab::GitlabAccountIdentifier::pat("bob")), + ]; + + assert_eq!( + match_host_to_accounts_custom_host("gh.example.com", &accounts), + None + ); + } + + #[test] + fn returns_none_when_custom_hosts_do_not_match() { + let accounts = vec![ + ForgeUser::GitHub(but_github::GithubAccountIdentifier::enterprise( + "alice", + "gh.example.com", + )), + ForgeUser::GitLab(but_gitlab::GitlabAccountIdentifier::selfhosted( + "bob", + "gl.example.com", + )), + ]; + + assert_eq!( + match_host_to_accounts_custom_host("no-match.example.com", &accounts), + None + ); + } + + #[test] + fn matches_repository_host_against_custom_host_with_subdomain_and_path() { + let accounts = vec![ForgeUser::GitLab( + but_gitlab::GitlabAccountIdentifier::selfhosted( + "bob", + "https://api.repository.com/v1/api", + ), + )]; + + assert_eq!( + match_host_to_accounts_custom_host("repository.com", &accounts), + Some(ForgeName::GitLab) + ); + } + + #[test] + fn matches_repository_host_against_custom_host_with_scheme_port_and_path() { + let accounts = vec![ForgeUser::GitHub( + but_github::GithubAccountIdentifier::enterprise( + "alice", + "https://api.repository.com:8443/v1/api", + ), + )]; + + assert_eq!( + match_host_to_accounts_custom_host("repository.com", &accounts), + Some(ForgeName::GitHub) + ); + } + + #[test] + fn does_not_match_partial_domain_suffixes() { + let accounts = vec![ForgeUser::GitLab( + but_gitlab::GitlabAccountIdentifier::selfhosted("bob", "api.notrepository.com/v1"), + )]; + + assert_eq!( + match_host_to_accounts_custom_host("repository.com", &accounts), + None + ); + } + + #[test] + fn matches_repository_host_case_insensitively_against_custom_host() { + let accounts = vec![ForgeUser::GitLab( + but_gitlab::GitlabAccountIdentifier::selfhosted( + "bob", + "HTTPS://API.REPOSITORY.COM/v1/api", + ), + )]; + + assert_eq!( + match_host_to_accounts_custom_host("Repository.COM", &accounts), + Some(ForgeName::GitLab) + ); + } + + #[test] + fn normalize_host_for_comparison_strips_url_parts_and_normalizes_case() { + assert_eq!( + normalize_host_for_comparison("HTTPS://user@API.Repository.com:8443/v1/api?x=1#frag"), + "api.repository.com" + ); + } + + #[test] + fn normalize_host_for_comparison_trims_whitespace_and_trailing_dot() { + assert_eq!( + normalize_host_for_comparison(" repository.com. "), + "repository.com" + ); + } + + #[test] + fn derive_forge_repo_info_uses_custom_host_when_known_forge_is_not_detected() { + let accounts = vec![ForgeUser::GitLab( + but_gitlab::GitlabAccountIdentifier::selfhosted( + "bob", + "https://api.repository.com/v1/api", + ), + )]; + + let info = derive_forge_repo_info("https://repository.com/org/repo.git", &accounts) + .expect("expected forge info"); + + assert_eq!(info.forge, ForgeName::GitLab); + assert_eq!(info.owner, "org"); + assert_eq!(info.repo, "repo"); + assert_eq!(info.protocol, "https"); + } + + #[test] + fn derive_forge_repo_info_prefers_known_forge_from_repository_host() { + let accounts = vec![ForgeUser::GitLab( + but_gitlab::GitlabAccountIdentifier::selfhosted( + "bob", + "https://api.repository.com/v1/api", + ), + )]; + + let info = derive_forge_repo_info("https://github.com/org/repo.git", &accounts) + .expect("expected forge info"); + + assert_eq!(info.forge, ForgeName::GitHub); + } +} diff --git a/crates/but-github/src/token.rs b/crates/but-github/src/token.rs index 44193ae6acd..13359b82680 100644 --- a/crates/but-github/src/token.rs +++ b/crates/but-github/src/token.rs @@ -99,6 +99,15 @@ impl GithubAccountIdentifier { } } } + + /// Retrieve the custom forge host, if this is an Enterprise account. + pub fn custom_host(&self) -> Option { + match self { + GithubAccountIdentifier::Enterprise { host, .. } => Some(host.to_string()), + GithubAccountIdentifier::OAuthUsername { .. } => None, + GithubAccountIdentifier::PatUsername { .. } => None, + } + } } impl std::fmt::Display for GithubAccountIdentifier { diff --git a/crates/but-gitlab/src/token.rs b/crates/but-gitlab/src/token.rs index c9951883fba..b4ee43ccdd5 100644 --- a/crates/but-gitlab/src/token.rs +++ b/crates/but-gitlab/src/token.rs @@ -91,6 +91,14 @@ impl GitlabAccountIdentifier { } } } + + /// Retrieve the custom forge host, if this is a Self-Hosted account. + pub fn custom_host(&self) -> Option { + match self { + GitlabAccountIdentifier::SelfHosted { host, .. } => Some(host.to_string()), + GitlabAccountIdentifier::PatUsername { .. } => None, + } + } } impl std::fmt::Display for GitlabAccountIdentifier { diff --git a/crates/gitbutler-branch-actions/src/base.rs b/crates/gitbutler-branch-actions/src/base.rs index ee999ce45ca..6f0a7cfc254 100644 --- a/crates/gitbutler-branch-actions/src/base.rs +++ b/crates/gitbutler-branch-actions/src/base.rs @@ -362,7 +362,8 @@ pub(crate) fn target_to_base_branch(ctx: &Context, target: &Target) -> Result