From 60cf287c11d1bf3ec22ba49dccffffa8793140ab Mon Sep 17 00:00:00 2001 From: hugocasa Date: Wed, 1 Jul 2026 18:53:53 +0200 Subject: [PATCH] fix(git-sync): address PR review findings - webhook_secret: redact from the settings API response and Debug output (still persisted encrypted); it's a server-only HMAC key the UI never needs. - poller: honor each repo's effective poll interval (relaxed ~10 min when a webhook is live) instead of probing every ~60s tick. - settings save: roll back a just-created webhook if the settings transaction doesn't commit, so a failed save can't orphan a hook. - auto-pull head check: fail SSH remotes with an actionable message (background polling has no SSH identity) instead of a confusing ls-remote error. - deploy/PR check summary: a pull result carrying neither changes nor a settings diff now falls back to the unsummarized path instead of a false "in sync". - UI: reset isGithubApp on resource change / failed fetch so webhook + fork controls can't show for the wrong repo. - tests: cover parse_git_sync_changes and format_change_list edge cases. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PP5gBSPfo1YtkL1sWVAjJm --- backend/src/monitor.rs | 28 +++++++ .../windmill-api-workspaces/src/workspaces.rs | 74 ++++++++++++++++--- backend/windmill-common/src/workspaces.rs | 21 +++++- backend/windmill-store/src/resources.rs | 11 +++ .../windmill-worker/src/result_processor.rs | 55 +++++++++++++- .../git_sync/GitSyncRepositoryCard.svelte | 4 + 6 files changed, 177 insertions(+), 16 deletions(-) diff --git a/backend/src/monitor.rs b/backend/src/monitor.rs index dbb71b5808..4062d49c9e 100644 --- a/backend/src/monitor.rs +++ b/backend/src/monitor.rs @@ -3092,6 +3092,19 @@ pub async fn poll_git_auto_pull(db: &Pool) { } #[cfg(feature = "private")] +lazy_static::lazy_static! { + /// Last auto-pull poll time (unix secs) per `workspace|repo_path`, so each repo + /// is only probed once per its effective interval instead of every ~60s tick. + /// Bounded by the number of auto-pull repos; stale entries for removed repos are + /// harmless. + static ref AUTO_PULL_LAST_POLL: std::sync::Mutex> = + std::sync::Mutex::new(std::collections::HashMap::new()); +} + +/// Slack (seconds) subtracted from the effective interval so a repo whose interval +/// equals the ~60s tick isn't skipped by tick jitter. +const AUTO_PULL_POLL_SLACK_S: i64 = 30; + async fn poll_git_auto_pull_inner(db: &Pool) -> error::Result<()> { use windmill_common::workspaces::{AutoPullMode, WorkspaceGitSyncSettings}; @@ -3127,6 +3140,21 @@ async fn poll_git_auto_pull_inner(db: &Pool) -> error::Result<()> { continue; } + // Honor the repo's effective poll interval (relaxed to ~10 min when a + // webhook is live) instead of probing every ~60s tick. + let interval_s = auto_pull.effective_poll_interval_s() as i64; + let poll_key = format!("{}|{}", row.workspace_id, repo.git_repo_resource_path); + let now = chrono::Utc::now().timestamp(); + { + let mut last = AUTO_PULL_LAST_POLL.lock().unwrap(); + if let Some(&t) = last.get(&poll_key) { + if now - t < interval_s - AUTO_PULL_POLL_SLACK_S { + continue; + } + } + last.insert(poll_key, now); + } + let head = match windmill_store::resources::get_git_repo_head_for_autopull( db, &row.workspace_id, diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index a00f2bfe13..6f15428fc3 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -708,6 +708,21 @@ async fn list_workspaces( Ok(Json(workspaces)) } +/// Strip the server-only webhook HMAC secret from a `git_sync` blob before it is +/// returned to a client. The UI never needs it; it stays (encrypted) in the DB. +fn redact_git_sync_webhook_secrets(git_sync: &mut serde_json::Value) { + if let Some(repos) = git_sync + .get_mut("repositories") + .and_then(|r| r.as_array_mut()) + { + for repo in repos { + if let Some(auto_pull) = repo.get_mut("auto_pull").and_then(|a| a.as_object_mut()) { + auto_pull.remove("webhook_secret"); + } + } + } +} + async fn get_settings( authed: ApiAuthed, Path(w_id): Path, @@ -764,9 +779,13 @@ async fn get_settings( .await .map_err(|e| Error::internal_err(format!("getting settings: {e:#}")))?; - let settings = not_found_if_none(settings, "workspace settings", &w_id)?; + let mut settings = not_found_if_none(settings, "workspace settings", &w_id)?; tx.commit().await?; + if let Some(git_sync) = settings.git_sync.as_mut() { + redact_git_sync_webhook_secrets(git_sync); + } + Ok(Json(settings)) } @@ -2775,20 +2794,29 @@ async fn edit_git_sync_repository( // Create or remove the repo's GitHub webhook to match its auto-pull config // (phase 2). Best-effort: a failure falls back to polling and never fails the - // settings save. The hook id/secret it writes are persisted by the UPDATE below. + // settings save. The hook id/secret it writes are persisted by the UPDATE + // below; if that save doesn't commit, the just-created hook is rolled back so a + // settings save can never leave an orphaned webhook. #[cfg(feature = "enterprise")] - { + let created_webhook_id: Option = { + let mut created = None; if let Some(repo) = git_sync_settings .repositories .iter_mut() .find(|r| r.git_repo_resource_path == new_config.git_repo_resource_path) { + let before = repo.auto_pull.as_ref().and_then(|a| a.webhook_id); if let Err(e) = windmill_common::git_sync_ee::sync_repo_webhook(&db, &w_id, repo).await { tracing::warn!("git auto-pull: webhook sync error: {}", e); } + // A hook that existed before wasn't created by this call, so don't roll it back. + if before.is_none() { + created = repo.auto_pull.as_ref().and_then(|a| a.webhook_id); + } } - } + created + }; // Clean up legacy workspace-level settings if all repos are migrated cleanup_legacy_git_sync_settings_in_memory(&mut git_sync_settings, &w_id); @@ -2797,15 +2825,37 @@ async fn edit_git_sync_repository( let serialized_config = serde_json::to_value::(git_sync_settings) .map_err(|err| Error::internal_err(err.to_string()))?; - sqlx::query!( - "UPDATE workspace_settings SET git_sync = $1 WHERE workspace_id = $2", - serialized_config, - &w_id - ) - .execute(&mut *tx) - .await?; + let save_result: std::result::Result<(), sqlx::Error> = async { + sqlx::query!( + "UPDATE workspace_settings SET git_sync = $1 WHERE workspace_id = $2", + serialized_config, + &w_id + ) + .execute(&mut *tx) + .await?; + tx.commit().await + } + .await; - tx.commit().await?; + if let Err(e) = save_result { + // Settings never persisted — delete any webhook we just created so its + // id/secret (which were never saved) don't leave an orphan on GitHub. + #[cfg(feature = "enterprise")] + if let Some(hook_id) = created_webhook_id { + if let Ok(url) = windmill_common::git_sync_ee::resolve_repo_url( + &db, + &w_id, + &new_config.git_repo_resource_path, + ) + .await + { + let _ = + windmill_common::git_sync_ee::delete_repo_webhook(&db, &w_id, &url, hook_id) + .await; + } + } + return Err(e.into()); + } // Trigger git sync for individual repository update/add handle_deployment_metadata( diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index f0113aea5c..ce167648cd 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -375,7 +375,7 @@ pub struct AutoPullStatus { /// Stored inside `GitRepositorySettings` (workspace_settings.git_sync JSONB). /// Webhook fields are populated in phase 2; phase 1 exercises the polling path /// only, but the full shape is defined up front to avoid a second schema change. -#[derive(Serialize, Deserialize, Debug, Clone)] +#[derive(Serialize, Deserialize, Clone)] pub struct AutoPullSettings { pub enabled: bool, #[serde(default)] @@ -403,6 +403,25 @@ pub struct AutoPullSettings { pub last_pull_status: Option, } +// Manual Debug so the HMAC `webhook_secret` (even encrypted) never lands in logs. +impl std::fmt::Debug for AutoPullSettings { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("AutoPullSettings") + .field("enabled", &self.enabled) + .field("mode", &self.mode) + .field("poll_interval_s", &self.poll_interval_s) + .field("webhook_id", &self.webhook_id) + .field( + "webhook_secret", + &self.webhook_secret.as_ref().map(|_| ""), + ) + .field("webhook_error", &self.webhook_error) + .field("last_synced_sha", &self.last_synced_sha) + .field("last_pull_status", &self.last_pull_status) + .finish() + } +} + /// Default polling interval when a webhook is not active. pub const DEFAULT_AUTO_PULL_POLL_INTERVAL_S: u32 = 60; diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index ad00567639..a35e66ac83 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -2850,6 +2850,17 @@ pub async fn get_git_repo_head_for_autopull( let 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://") { + 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(), + )); + } + let ref_spec = git_resource .branch .as_deref() diff --git a/backend/windmill-worker/src/result_processor.rs b/backend/windmill-worker/src/result_processor.rs index 461b1cfb2b..a839313adb 100644 --- a/backend/windmill-worker/src/result_processor.rs +++ b/backend/windmill-worker/src/result_processor.rs @@ -750,18 +750,23 @@ fn parse_git_sync_changes(result_raw: &str) -> Option<(Vec<(String, String)>, bo } #[derive(Deserialize)] struct SyncResponse { - #[serde(default)] - changes: Vec, - #[serde(default, rename = "settingsDiffResult")] + changes: Option>, + #[serde(rename = "settingsDiffResult")] settings_diff_result: Option, } let resp = serde_json::from_str::(result_raw).ok()?; + // A result carrying neither field isn't a recognizable diff; return None so the + // caller falls back to the unsummarized path instead of a false "in sync". + if resp.changes.is_none() && resp.settings_diff_result.is_none() { + return None; + } let settings_changed = resp .settings_diff_result .map(|s| s.has_changes) .unwrap_or(false); Some(( resp.changes + .unwrap_or_default() .into_iter() .map(|c| (c.change_type, c.path)) .collect(), @@ -781,6 +786,50 @@ fn format_change_list(changes: &[(String, String)]) -> Vec { lines } +#[cfg(all(test, feature = "enterprise", feature = "private"))] +mod git_sync_check_tests { + use super::{format_change_list, parse_git_sync_changes}; + + #[test] + fn parse_empty_changes_is_in_sync() { + // Present-but-empty diff → a real "in sync" result, not None. + let (changes, settings) = parse_git_sync_changes(r#"{"changes":[]}"#).unwrap(); + assert!(changes.is_empty()); + assert!(!settings); + } + + #[test] + fn parse_missing_fields_is_none() { + // Neither field present → unrecognizable, falls back to the caller's path. + assert!(parse_git_sync_changes("{}").is_none()); + } + + #[test] + fn parse_unparseable_is_none() { + assert!(parse_git_sync_changes("not json").is_none()); + } + + #[test] + fn parse_changes_and_settings() { + let (changes, settings) = parse_git_sync_changes( + r#"{"changes":[{"type":"edited","path":"f/a"}],"settingsDiffResult":{"hasChanges":true}}"#, + ) + .unwrap(); + assert_eq!(changes, vec![("edited".to_string(), "f/a".to_string())]); + assert!(settings); + } + + #[test] + fn format_truncates_over_100() { + let changes: Vec<(String, String)> = (0..150) + .map(|i| ("edited".to_string(), format!("f/{i}"))) + .collect(); + let lines = format_change_list(&changes); + assert_eq!(lines.len(), 101); + assert_eq!(lines.last().unwrap(), "- ... and 50 more"); + } +} + /// When a git-sync pull job carrying a check marker completes, post the outcome /// to its GitHub check run: the PR diff preview (`__git_sync_pr_check`, phase 4) /// or the live deploy status (`__git_sync_deploy_check`, phase 6). diff --git a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte index 2cca8038cb..23d83b7efd 100644 --- a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte +++ b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte @@ -129,6 +129,9 @@ if (repo?.git_repo_resource_path && !repo.isUnsavedConnection && $workspaceStore) { loadingResourceInfo = true resourceInfo = null + // Clear stale app state up front so a resource change or a failed + // fetch can't leave webhook/fork controls showing for the wrong repo. + isGithubApp = false try { const resource = await ResourceService.getResource({ workspace: $workspaceStore, @@ -201,6 +204,7 @@ } } else { resourceInfo = null + isGithubApp = false } }