diff --git a/backend/tests/asset_trigger_dispatch.rs b/backend/tests/asset_trigger_dispatch.rs index 7184d4810d..c165958a33 100644 --- a/backend/tests/asset_trigger_dispatch.rs +++ b/backend/tests/asset_trigger_dispatch.rs @@ -244,6 +244,7 @@ fn make_mini(id: Uuid, runnable_path: &str) -> MiniCompletedJob { cache_ttl: None, cache_ignore_s3_path: None, runnable_settings_handle: None, + build_binary_only: false, } } diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 1dcfa6429e..a3b5d2a1a7 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -1140,6 +1140,19 @@ async fn create_resource( } let authed = maybe_refresh_folders(&resource.path, &w_id, authed, &db).await; + authorize_azure_devops_reference( + &authed, + &db, + &user_db, + &w_id, + resource + .value + .as_deref() + .and_then(|v| serde_json::from_str::(v.get()).ok()) + .as_ref(), + ) + .await?; + let mut tx = user_db.begin(&authed).await?; let update_if_exists = q.update_if_exists.unwrap_or(false); @@ -1821,6 +1834,18 @@ async fn update_resource( sqlb.returning("path"); let authed = maybe_refresh_folders(path, &w_id, authed, &db).await; + authorize_azure_devops_reference( + &authed, + &db, + &user_db, + &w_id, + ns.value + .as_deref() + .and_then(|v| serde_json::from_str::(v.get()).ok()) + .as_ref(), + ) + .await?; + let mut tx = user_db.begin(&authed).await?; if let Some(npath) = ns.path.clone() { @@ -2098,6 +2123,8 @@ async fn set_resource_value( { return Err(Error::PermissionDenied(msg)); } + authorize_azure_devops_reference(authed, db, user_db, w_id, value.as_ref()).await?; + let mut tx = user_db.clone().begin(authed).await?; // `RETURNING resource_type` rather than a second lookup: the advisory below has to know the @@ -2954,6 +2981,50 @@ fn extract_host_from_git_url(url: &str) -> Option { None } +/// Strip the userinfo from a git URL. These probes run against URLs that embed a +/// credential (a `$var:` token, or one minted from an `AZURE_DEVOPS_TOKEN(...)` +/// placeholder), and their errors are persisted as the repository's sync status and +/// rendered in the UI. git's own redaction cannot be relied on — it drops the userinfo +/// from `unable to access ''` but echoes it in `could not read Password for +/// ''` — so anything that formats a probe URL has to strip it here. +fn redact_git_url_credentials(url: &str) -> String { + match git_url_userinfo_range(url) { + Some(r) => format!("{}***{}", &url[..r.start], &url[r.end..]), + None => url.to_string(), + } +} + +/// Byte range of a git URL's userinfo (the credentials before the authority's '@'), +/// for both `scheme://user[:pass]@host/path` and SCP-style `user@host:path`. +/// +/// The authority ends at the first '/', '?' or '#' and the credentials are the *last* +/// '@' within it, so an '@' planted in the path cannot mis-scope the split +/// (GHSA-p5cj-8cfh-mjv6). +fn git_url_userinfo_range(url: &str) -> Option> { + let (authority_start, authority) = match url.find("://") { + Some(scheme_sep) => { + let start = scheme_sep + 3; + let after = &url[start..]; + let end = after + .find(|c| c == '/' || c == '?' || c == '#') + .unwrap_or(after.len()); + (start, &after[..end]) + } + // SCP-style `[user@]host:path` has no scheme, and its authority is bounded by + // the first ':' — never by the last '@', which an '@' in the path would move + // (the same mis-scoping `extract_host_from_git_url` guards against). scp syntax + // has no password field, so bounding this way cannot cut a credential in half. + None if url.contains('@') => (0, url.split(':').next().unwrap_or(url)), + None => return None, + }; + let at = authority.rfind('@')?; + (at > 0).then(|| authority_start..authority_start + at) +} + +fn git_url_userinfo(url: &str) -> Option<&str> { + git_url_userinfo_range(url).map(|r| &url[r]) +} + /// Validates a git URL to prevent option injection, SSRF, and local file read. async fn validate_git_url(url: &str) -> Result<()> { let url = url.trim(); @@ -2979,6 +3050,14 @@ async fn validate_git_url(url: &str) -> Result<()> { "Git URL cannot contain '?' or '#' characters".to_string(), )); } + // Every probe URL is validated, so this catches a caller that reached git without + // expanding the placeholder — which git would otherwise report as an unresolvable + // host, the placeholder's own '/' having truncated the authority. + if url.contains(AZURE_DEVOPS_TOKEN_PLACEHOLDER) { + return Err(Error::BadRequest( + "Git URL still contains an unexpanded AZURE_DEVOPS_TOKEN(...) placeholder".to_string(), + )); + } let lower = url.to_lowercase(); @@ -3112,12 +3191,14 @@ async fn get_git_commit_hash( .await .map_err(|e| Error::NotFound(format!("Access to resource {} denied: ({e})", path)))?; - let git_resource: GitRepositoryResource = match git_repo_resource_value { + let mut git_resource: GitRepositoryResource = match git_repo_resource_value { Some(value) => serde_json::from_value(value).map_err(|e| { Error::BadRequest(format!("Invalid git repository resource format: {}", e)) })?, None => return Err(Error::NotFound(format!("Resource {} not found", path)).into()), }; + git_resource.url = + resolve_azure_devops_url(&db_with_opt_authed, &w_id, &git_resource.url, false).await?; let identities: Vec = query .git_ssh_identity @@ -3259,9 +3340,21 @@ fn is_refused_redirect(stderr: &str) -> bool { /// Decode a failed probe's stderr, naming the remedy when the remote redirected /// somewhere the `.git` retry could not reach (an `http://` URL upgraded to https, /// say) — `git_probe_command` refuses redirects, so nothing else explains the status. -fn git_probe_stderr(stderr: Vec) -> String { +/// +/// `probe_url` is the URL the probe ran against, and its userinfo is scrubbed from the +/// output: git strips credentials from some messages but not all — a token in the +/// username position comes back verbatim in `could not read Password for +/// 'https://@host'` — and these strings are persisted as a repository's sync +/// status and rendered in the UI. +fn git_probe_stderr(stderr: Vec, probe_url: &str) -> String { let stderr = String::from_utf8(stderr).unwrap_or_else(|_| "Failed to decode stderr".to_string()); + // Scrub the `@` form git prints, not the bare userinfo: a one-character + // username would otherwise be replaced everywhere it happens to occur. + let stderr = match git_url_userinfo(probe_url) { + Some(userinfo) => stderr.replace(&format!("{userinfo}@"), "***@"), + None => stderr, + }; if is_refused_redirect(&stderr) { format!( "{} (the remote redirects, and redirects are not followed; set the repository URL to the address it redirects to)", @@ -3333,6 +3426,317 @@ async fn run_git_probe(mut git_cmd: Command, what: &str) -> Result)` +/// where a credential belongs: an Azure DevOps access token minted at use time from +/// that `azure` resource's client credentials. The hub sync scripts expand it in +/// TypeScript before running git; the probes below shell out to git from the backend, +/// so they must expand it too or git is handed the literal placeholder (whose '/' +/// truncates the authority, and curl rejects the resulting hostname). +const AZURE_DEVOPS_TOKEN_PLACEHOLDER: &str = "AZURE_DEVOPS_TOKEN("; + +/// Azure DevOps resource id the token is minted for, and the endpoint that mints it — +/// both identical to the hub sync scripts', so a repository that authenticates for a +/// sync job authenticates for these probes too. +const AZURE_DEVOPS_RESOURCE_ID: &str = "499b84ac-1321-427f-aa17-267ca6975798/.default"; +const AZURE_LOGIN_HOST: &str = "https://login.microsoftonline.com"; + +/// Minted tokens, keyed by a digest of the credentials they came from — never by +/// resource path, so a cache hit cannot hand a token to a caller who was not able to +/// read the resource itself. Auto-pull probes every repository on an interval; without +/// this, every tick would mint a fresh token. +static AZURE_DEVOPS_TOKEN_CACHE: LazyLock> = + LazyLock::new(DashMap::new); + +/// Shaved off a token's advertised lifetime so one is never handed out as it expires. +const AZURE_TOKEN_EXPIRY_MARGIN_S: i64 = 60; + +/// Lifetime assumed when the token response omits `expires_in`. +const AZURE_TOKEN_FALLBACK_LIFETIME_S: i64 = 300; + +/// Whether the span `start..end` of `url` is the userinfo of an https authority. +/// The placeholder contains '/', which truncates the authority for any left-to-right +/// parse, so terminators falling inside the span are skipped rather than honored. +/// +/// https only: over plaintext an on-path attacker answers the probe's first request +/// with a Basic challenge, and git retries carrying the minted token. +fn span_is_https_userinfo(url: &str, start: usize, end: usize) -> bool { + let Some(scheme_sep) = url.find("://") else { + return false; + }; + if !url[..scheme_sep].eq_ignore_ascii_case("https") { + return false; + } + let body_start = scheme_sep + 3; + if start < body_start { + return false; + } + let authority_end = url[body_start..] + .char_indices() + .map(|(i, c)| (body_start + i, c)) + .find(|&(i, c)| (i < start || i >= end) && (c == '/' || c == '?' || c == '#')) + .map_or(url.len(), |(i, _)| i); + // Taking the authority's *last* '@' is what makes one inside the span harmless: + // such a match sits before `end` and fails the comparison. + url[body_start..authority_end] + .rfind('@') + .is_some_and(|rel| end <= body_start + rel) +} + +/// Locate the placeholder in a git URL, returning `(whole placeholder, resource path)`. +fn parse_azure_devops_placeholder(url: &str) -> Result> { + let Some(start) = url.find(AZURE_DEVOPS_TOKEN_PLACEHOLDER) else { + return Ok(None); + }; + let after = &url[start + AZURE_DEVOPS_TOKEN_PLACEHOLDER.len()..]; + // Greedy to the last ')', matching the hub scripts' `AZURE_DEVOPS_TOKEN\((.+)\)`. + let end = after.rfind(')').ok_or_else(|| { + Error::BadRequest( + "Git repository URL has an unterminated AZURE_DEVOPS_TOKEN(...) placeholder" + .to_string(), + ) + })?; + let end = start + AZURE_DEVOPS_TOKEN_PLACEHOLDER.len() + end + 1; + // Anywhere but the userinfo, the minted token would be spliced into a part of the + // URL that git echoes verbatim in its failure messages (which are persisted as the + // repository's sync status) and that credential redaction does not cover. + if !span_is_https_userinfo(url, start, end) { + return Err(Error::BadRequest( + "The AZURE_DEVOPS_TOKEN(...) placeholder must be the credentials of an https git URL, i.e. directly before the '@'".to_string(), + )); + } + Ok(Some(( + &url[start..end], + &after[..end - start - AZURE_DEVOPS_TOKEN_PLACEHOLDER.len() - 1], + ))) +} + +/// Hosts an Azure DevOps token may be sent to. The minted token is an AAD token for +/// the Azure DevOps resource id, so Microsoft is the only party it is meaningful to. +fn is_azure_devops_host(host: &str) -> bool { + let host = host.trim_end_matches('.'); + host == "dev.azure.com" + || host.ends_with(".dev.azure.com") + || host == "visualstudio.com" + || host.ends_with(".visualstudio.com") +} + +/// Expand an `AZURE_DEVOPS_TOKEN(...)` placeholder in a git URL, or return the URL +/// unchanged when it has none. The referenced resource is read through `dba`, so an +/// authed caller only reaches credentials they can already read. +async fn resolve_azure_devops_url( + dba: &DbWithOptAuthed<'_, ApiAuthed>, + w_id: &str, + url: &str, + allow_cache: bool, +) -> Result { + // Trim first: the http(s) gates the callers apply trim too, so a stored URL with + // leading whitespace must not reach the scheme check here as a non-http one. + let url = url.trim(); + let Some((placeholder, resource_path)) = parse_azure_devops_placeholder(url)? else { + return Ok(url.to_string()); + }; + + // Vet the destination before minting: a URL the host checks would reject must not + // cost a live credential (nor cache one), and whoever can edit the URL would + // otherwise drive a token mint per poll tick. + let probe_url = url.replace(placeholder, "windmill"); + validate_git_url(&probe_url).await?; + + // The background poller reads the referenced resource under the system identity, + // which bypasses RLS. Confining the destination is what keeps that from becoming an + // exfiltration primitive: whoever can write this URL picks both the resource path + // and the host, so an unconfined splice would hand a credential they cannot read to + // a host they choose. Unlike `$var:`, which substitutes a whole value and so cannot + // place a secret inside a caller-chosen URL, this placeholder is a substring. + let host = extract_host_from_git_url(&probe_url) + .ok_or_else(|| Error::BadRequest("Could not parse hostname from git URL".to_string()))?; + if !is_azure_devops_host(&host) { + return Err(Error::BadRequest(format!( + "An AZURE_DEVOPS_TOKEN(...) placeholder is only allowed on an Azure DevOps URL (dev.azure.com or visualstudio.com), not '{host}'" + ))); + } + + let value = + get_resource_value_interpolated_internal(dba, w_id, resource_path, None, None, allow_cache) + .await + .map_err(|e| { + Error::BadRequest(format!( + "Azure resource '{resource_path}' referenced by the git repository URL could not be read: {e}" + )) + })? + .ok_or_else(|| { + Error::NotFound(format!( + "Azure resource '{resource_path}' referenced by the git repository URL was not found" + )) + })?; + + let field = |name: &str| -> Result { + value + .get(name) + .and_then(|v| v.as_str()) + .filter(|s| !s.is_empty()) + .map(|s| s.to_string()) + .ok_or_else(|| { + Error::BadRequest(format!( + "Azure resource '{resource_path}' referenced by the git repository URL has no '{name}'" + )) + }) + }; + let token = mint_azure_devops_token( + &field("azureTenantId")?, + &field("azureClientId")?, + &field("azureClientSecret")?, + allow_cache, + ) + .await?; + + Ok(url.replace(placeholder, &token)) +} + +/// Gate writing an `AZURE_DEVOPS_TOKEN(...)` reference into a resource value. +/// +/// The background probes mint from the named `azure` resource under the system identity, +/// which bypasses RLS, and no principal exists at that point to authorize against — so +/// authorization cannot be enforced where the credential is used, only where the +/// reference is introduced. A read check alone would not survive that gap: the reference +/// names a resource whose own value stays mutable, and repointing it at `$res:`/`$var:` +/// the writer cannot read would be a later write this never sees. +/// +/// Hence workspace admin, who can already read every resource in the workspace: the +/// escalation a mutable reference would otherwise buy is one the configurer already has. +/// The read check stays as a typo guard, so a reference to a nonexistent resource fails +/// at configuration time rather than as a puzzling sync error later. +/// +/// Only the `url` field is inspected, and with the same parser the probes use, so the +/// path checked here is exactly the path they will resolve. +pub async fn authorize_azure_devops_reference( + authed: &ApiAuthed, + db: &DB, + user_db: &UserDB, + w_id: &str, + value: Option<&serde_json::Value>, +) -> Result<()> { + let Some(url) = value.and_then(|v| v.get("url")).and_then(|u| u.as_str()) else { + return Ok(()); + }; + let Some((_, resource_path)) = parse_azure_devops_placeholder(url.trim())? else { + return Ok(()); + }; + + if !authed.is_admin { + return Err(Error::PermissionDenied(format!( + "Only a workspace admin can point a git repository URL at AZURE_DEVOPS_TOKEN({resource_path}): background sync mints that credential under an identity that bypasses resource permissions" + ))); + } + + let dba = DbWithOptAuthed::from_authed(authed, db.clone(), Some(user_db.clone())); + let readable = + get_resource_value_interpolated_internal(&dba, w_id, resource_path, None, None, false) + .await + .unwrap_or(None); + if readable.is_none() { + return Err(Error::PermissionDenied(format!( + "Cannot reference AZURE_DEVOPS_TOKEN({resource_path}) in a git repository URL: no such resource" + ))); + } + Ok(()) +} + +/// `allow_cache` carries the caller's freshness requirement through to the token, not +/// just to the resource read: an on-demand check must not succeed on a token minted +/// before the Azure app's permissions were last changed. +async fn mint_azure_devops_token( + tenant_id: &str, + client_id: &str, + client_secret: &str, + allow_cache: bool, +) -> Result { + use sha2::{Digest, Sha256}; + + let mut hasher = Sha256::new(); + for part in [tenant_id, client_id, client_secret] { + hasher.update(part.as_bytes()); + hasher.update([0u8]); + } + let cache_key = hex::encode(hasher.finalize()); + let now = chrono::Utc::now().timestamp(); + // Entries are only ever replaced by a later mint for the same credentials, so a + // rotated secret's entry would otherwise sit here for the process's lifetime. + AZURE_DEVOPS_TOKEN_CACHE.retain(|_, (_, expires_at)| *expires_at > now); + if allow_cache { + let cached = AZURE_DEVOPS_TOKEN_CACHE + .get(&cache_key) + .map(|e| e.value().0.clone()); + if let Some(token) = cached { + return Ok(token); + } + } + + let response = windmill_common::utils::HTTP_CLIENT + .post(format!("{AZURE_LOGIN_HOST}/{tenant_id}/oauth2/token")) + .form(&[ + ("client_id", client_id), + ("client_secret", client_secret), + ("grant_type", "client_credentials"), + ("resource", AZURE_DEVOPS_RESOURCE_ID), + ]) + .send() + .await + .map_err(|e| { + Error::BadRequest(format!("Failed to request an Azure DevOps token: {e:#}")) + })?; + + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + if !status.is_success() { + return Err(Error::BadRequest(format!( + "Azure DevOps token request failed ({status}): {}", + windmill_common::utils::truncate_with_ellipsis(&body, 500) + ))); + } + + #[derive(Deserialize)] + struct AzureTokenResponse { + access_token: String, + expires_in: Option, + } + let parsed: AzureTokenResponse = serde_json::from_str(&body) + .map_err(|e| Error::BadRequest(format!("Unexpected Azure DevOps token response: {e}")))?; + + // The v1 token endpoint returns `expires_in` as a string, the v2 one as a number. + let lifetime = parsed + .expires_in + .as_ref() + .and_then(|v| { + v.as_i64() + .or_else(|| v.as_str().and_then(|s| s.parse::().ok())) + }) + .unwrap_or(AZURE_TOKEN_FALLBACK_LIFETIME_S); + AZURE_DEVOPS_TOKEN_CACHE.insert( + cache_key, + ( + parsed.access_token.clone(), + now + (lifetime - AZURE_TOKEN_EXPIRY_MARGIN_S).max(0), + ), + ); + + Ok(parsed.access_token) +} + +/// System identity used by background git-sync polling. SECURITY: bypasses resource +/// RLS — see [`resolve_git_repository_resource`] for the caller obligations. +fn git_sync_system_dba(db: &DB) -> DbWithOptAuthed<'static, ApiAuthed> { + DbWithOptAuthed::DB { + db: db.clone(), + audit_author: windmill_common::audit::AuditAuthor { + username: "git_sync_auto_pull".to_string(), + email: windmill_common::users::SUPERADMIN_SYNC_EMAIL.to_string(), + username_override: None, + token_prefix: None, + }, + } +} + async fn get_repo_latest_commit_hash( git_resource: &GitRepositoryResource, git_ssh_command: Option, @@ -3363,7 +3767,7 @@ async fn get_repo_latest_commit_hash( .await?; if !output.status.success() { - let stderr = git_probe_stderr(output.stderr); + let stderr = git_probe_stderr(output.stderr, &git_resource.url); return Err(Error::BadRequest(format!( "Error getting git repo commit hash: {}", stderr @@ -3378,7 +3782,8 @@ async fn get_repo_latest_commit_hash( if lines.is_empty() { return Err(Error::BadRequest(format!( "No commits found for reference '{}' in repository '{}'", - ref_spec, git_resource.url + ref_spec, + redact_git_url_credentials(&git_resource.url) ))); } @@ -3399,7 +3804,9 @@ async fn get_repo_latest_commit_hash( /// /// SECURITY: reads under the system identity (`SUPERADMIN_SYNC_EMAIL`), so it /// **bypasses resource RLS** and returns fully-interpolated JSON that **may -/// contain credentials** (an embedded `$var:` token in the URL). Callers must +/// contain credentials** — an embedded `$var:` token in the URL, or the `azure` +/// resource named by an `AZURE_DEVOPS_TOKEN(...)` placeholder. Both name a path +/// chosen by whoever can write the repository URL, not by the reader. Callers must /// have already authorized access to `w_id`, must use it only for git-sync /// `git_repository` resources, and must **not** return the resolved value to a /// client — derive and return only non-sensitive facts. Pass `allow_cache=true` @@ -3411,24 +3818,19 @@ pub async fn resolve_git_repository_resource( git_repo_resource_path: &str, allow_cache: bool, ) -> Result> { - use windmill_common::db::DbWithOptAuthed; - let resource_path = git_repo_resource_path .strip_prefix("$res:") .unwrap_or(git_repo_resource_path); - let dba: DbWithOptAuthed<'_, ApiAuthed> = DbWithOptAuthed::DB { - db: db.clone(), - audit_author: windmill_common::audit::AuditAuthor { - username: "git_sync_auto_pull".to_string(), - email: windmill_common::users::SUPERADMIN_SYNC_EMAIL.to_string(), - username_override: None, - token_prefix: None, - }, - }; - - get_resource_value_interpolated_internal(&dba, w_id, resource_path, None, None, allow_cache) - .await + get_resource_value_interpolated_internal( + &git_sync_system_dba(db), + w_id, + resource_path, + None, + None, + allow_cache, + ) + .await } /// Resolve a workspace git-sync repository and return its current head commit @@ -3459,19 +3861,22 @@ pub async fn get_git_repo_head_for_autopull( return Ok(None); } - let git_resource: GitRepositoryResource = serde_json::from_value(value) + let mut git_resource: GitRepositoryResource = serde_json::from_value(value) .map_err(|e| Error::BadRequest(format!("Invalid git repository resource: {}", e)))?; // The SSH identity is supplied per-call in the authed commit-hash path; the // background poller has none, so an SSH remote can't authenticate here. Fail // with an actionable message instead of a confusing ls-remote auth error — // these repos should use an HTTPS token URL or the GitHub App for auto-pull. - let url = git_resource.url.trim_start(); - if !url.starts_with("http://") && !url.starts_with("https://") { + if !git_resource.url.trim_start().starts_with("http://") + && !git_resource.url.trim_start().starts_with("https://") + { return Err(Error::BadRequest( "Automatic pull can't authenticate an SSH git remote in the background. Use an HTTPS URL with an embedded token, or connect the repository through the GitHub App.".to_string(), )); } + git_resource.url = + resolve_azure_devops_url(&git_sync_system_dba(db), w_id, &git_resource.url, true).await?; if let Some(branch) = git_resource.branch.as_deref().filter(|s| !s.is_empty()) { let branch = branch.to_string(); @@ -3491,7 +3896,7 @@ pub async fn get_git_repo_head_for_autopull( }) .await?; if !output.status.success() { - let stderr = git_probe_stderr(output.stderr); + let stderr = git_probe_stderr(output.stderr, &git_resource.url); return Err(Error::BadRequest(format!( "Error resolving git repo HEAD: {}", stderr @@ -3503,7 +3908,7 @@ pub async fn get_git_repo_head_for_autopull( let sha = sha.ok_or_else(|| { Error::BadRequest(format!( "No HEAD found in repository '{}'", - git_resource.url + redact_git_url_credentials(&git_resource.url) )) })?; Ok(Some((branch.unwrap_or_else(|| "HEAD".to_string()), sha))) @@ -3545,21 +3950,11 @@ pub async fn get_git_repo_fork_heads_for_autopull( base_branch: &str, extra_refs: &[String], ) -> Result>> { - use windmill_common::db::DbWithOptAuthed; - let resource_path = git_repo_resource_path .strip_prefix("$res:") .unwrap_or(git_repo_resource_path); - let dba: DbWithOptAuthed<'_, ApiAuthed> = DbWithOptAuthed::DB { - db: db.clone(), - audit_author: windmill_common::audit::AuditAuthor { - username: "git_sync_auto_pull".to_string(), - email: windmill_common::users::SUPERADMIN_SYNC_EMAIL.to_string(), - username_override: None, - token_prefix: None, - }, - }; + let dba = git_sync_system_dba(db); let value = get_resource_value_interpolated_internal(&dba, w_id, resource_path, None, None, true) .await? @@ -3578,14 +3973,16 @@ pub async fn get_git_repo_fork_heads_for_autopull( return Ok(None); } - let git_resource: GitRepositoryResource = serde_json::from_value(value) + let mut git_resource: GitRepositoryResource = serde_json::from_value(value) .map_err(|e| Error::BadRequest(format!("Invalid git repository resource: {}", e)))?; - let url = git_resource.url.trim_start(); - if !url.starts_with("http://") && !url.starts_with("https://") { + if !git_resource.url.trim_start().starts_with("http://") + && !git_resource.url.trim_start().starts_with("https://") + { return Err(Error::BadRequest( "Automatic pull can't authenticate an SSH git remote in the background. Use an HTTPS URL with an embedded token, or connect the repository through the GitHub App.".to_string(), )); } + git_resource.url = resolve_azure_devops_url(&dba, w_id, &git_resource.url, true).await?; validate_git_url(&git_resource.url).await?; validate_git_ref(base_branch)?; @@ -3608,7 +4005,7 @@ pub async fn get_git_repo_fork_heads_for_autopull( }) .await?; if !output.status.success() { - let stderr = git_probe_stderr(output.stderr); + let stderr = git_probe_stderr(output.stderr, &git_resource.url); return Err(Error::BadRequest(format!( "Error listing fork branches: {}", stderr @@ -4081,7 +4478,7 @@ mod tests { target_requests.lock().unwrap().is_empty(), "git followed the redirect to the unvalidated target" ); - let stderr = git_probe_stderr(output.stderr); + let stderr = git_probe_stderr(output.stderr, ""); assert!( stderr.contains("301") && stderr.contains("redirects are not followed"), "the failure should name the refused redirect and its remedy, got: {stderr}" @@ -4169,6 +4566,94 @@ mod tests { assert!(validate_git_url("--upload-pack=evil").await.is_err()); } + #[test] + fn test_parse_azure_devops_placeholder() { + // The resource path holds '/', so the placeholder must be cut at its own + // closing ')' rather than at the first path separator. + let url = "https://AZURE_DEVOPS_TOKEN(f/azure/devops)@dev.azure.com/org/proj/_git/repo"; + assert_eq!( + parse_azure_devops_placeholder(url).unwrap(), + Some(("AZURE_DEVOPS_TOKEN(f/azure/devops)", "f/azure/devops")) + ); + assert_eq!( + parse_azure_devops_placeholder("https://token@github.com/user/repo.git").unwrap(), + None + ); + assert!(parse_azure_devops_placeholder( + "https://AZURE_DEVOPS_TOKEN(f/azure@dev.azure.com/o" + ) + .is_err()); + // Outside the userinfo the minted token would land in a URL component that git + // echoes back in its errors and redaction does not cover. + assert!(parse_azure_devops_placeholder( + "https://dev.azure.com/org/AZURE_DEVOPS_TOKEN(f/azure)/repo" + ) + .is_err()); + assert!(parse_azure_devops_placeholder( + "ssh://AZURE_DEVOPS_TOKEN(f/azure)@dev.azure.com/o" + ) + .is_err()); + // Plaintext would let an on-path Basic challenge harvest the minted token. + assert!(parse_azure_devops_placeholder( + "http://AZURE_DEVOPS_TOKEN(f/azure)@dev.azure.com/o" + ) + .is_err()); + // A `user:token` userinfo is still the credentials position. + assert_eq!( + parse_azure_devops_placeholder("https://u:AZURE_DEVOPS_TOKEN(f/azure)@dev.azure.com/o") + .unwrap(), + Some(("AZURE_DEVOPS_TOKEN(f/azure)", "f/azure")) + ); + } + + #[test] + fn test_is_azure_devops_host() { + assert!(is_azure_devops_host("dev.azure.com")); + assert!(is_azure_devops_host("vssps.dev.azure.com")); + assert!(is_azure_devops_host("myorg.visualstudio.com")); + // The whole point: a token must never be splice-able onto a chosen host. + assert!(!is_azure_devops_host("attacker.example")); + assert!(!is_azure_devops_host("dev.azure.com.attacker.example")); + assert!(!is_azure_devops_host("notvisualstudio.com")); + assert!(!is_azure_devops_host("github.com")); + } + + #[test] + fn test_redact_git_url_credentials() { + assert_eq!( + redact_git_url_credentials("https://tok@dev.azure.com/o/p"), + "https://***@dev.azure.com/o/p" + ); + assert_eq!( + redact_git_url_credentials("https://user:tok@github.com/u/r.git"), + "https://***@github.com/u/r.git" + ); + // SCP-style `[user@]host:path` carries its credential in the user position. + assert_eq!( + redact_git_url_credentials("tok@github.com:u/r.git"), + "***@github.com:u/r.git" + ); + // A '@' in the path must not be mistaken for the credentials separator. + assert_eq!( + redact_git_url_credentials("https://github.com/u/r@v1.git"), + "https://github.com/u/r@v1.git" + ); + // A userinfo that also occurs in the scheme must not be redacted there. + assert_eq!( + redact_git_url_credentials("https://s@https.com/r"), + "https://***@https.com/r" + ); + } + + #[test] + fn test_git_probe_stderr_scrubs_the_probe_url_credentials() { + // git echoes a username-position token verbatim in this message, and the result + // is persisted as the repository's sync status. + let stderr = b"fatal: could not read Password for 'https://SECRET@dev.azure.com'".to_vec(); + let out = git_probe_stderr(stderr, "https://SECRET@dev.azure.com/o/p"); + assert!(!out.contains("SECRET"), "token survived redaction: {out}"); + } + #[tokio::test] async fn test_validate_git_url_blocks_fragment_query_ssrf() { // GHSA-p5cj-8cfh-mjv6: a loopback authority must stay blocked, and the