From 7377923cd5c39895cbb9ca021f174af09c1539bc Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 4 Sep 2026 16:06:01 +0200 Subject: [PATCH] [ee] fix: drop a removed repo's credential and honor the workspace override --- backend/ee-repo-ref.txt | 2 +- .../windmill-api-workspaces/src/workspaces.rs | 50 ++++++++++++++++--- docs/git-sync-gitlab-setup.md | 37 ++++++++++---- .../src/lib/components/ApiConnectForm.svelte | 8 ++- .../src/lib/components/AppConnectInner.svelte | 1 + 5 files changed, 79 insertions(+), 19 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 6f4eb45254..210e263fb5 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -66e4efa6c1b25b50071712a19d81b269b2b15c05 +c8deb48ca538225feca9853fce2f1afd0499c48f diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 8a908605d9..2b09c9652f 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3923,10 +3923,10 @@ async fn edit_git_sync_config( // The whole-config save only writes the DB below; the managed GitHub webhooks // are reconciled after the commit is durable (like the per-repository endpoint): - // `post_commit` carries the saved repos to reconcile + the hooks of repos this - // save removed, to delete. + // `post_commit` carries the saved repos to reconcile, the hooks of repos this + // save removed, and the paths of those repos, whose credentials go with them. #[cfg(all(feature = "enterprise", feature = "private"))] - let post_commit: Option<(WorkspaceGitSyncSettings, Vec<(String, i64)>)>; + let post_commit: Option<(WorkspaceGitSyncSettings, Vec<(String, i64)>, Vec)>; if let Some(mut git_sync_settings) = new_config.git_sync_settings { // Client-supplied server-owned auto-pull state is never trusted: strip it up @@ -4028,6 +4028,27 @@ async fn edit_git_sync_config( .collect() }) .unwrap_or_default(); + // Repos this save drops entirely: the workspace no longer syncs them, so the + // credential it holds for each goes with them. A separate list from the + // webhooks above, which only covers repos that had one — otherwise a dropped + // repo that never had a webhook would keep its token, and re-adding the same + // path and URL later would silently authenticate with it. + #[cfg(all(feature = "enterprise", feature = "private"))] + let removed_repos: Vec = existing + .as_ref() + .map(|e| { + e.repositories + .iter() + .filter(|old| { + !git_sync_settings + .repositories + .iter() + .any(|n| n.git_repo_resource_path == old.git_repo_resource_path) + }) + .map(|old| old.git_repo_resource_path.clone()) + .collect() + }) + .unwrap_or_default(); if let Some(existing) = &existing { for repo in git_sync_settings.repositories.iter_mut() { let Some(old) = existing @@ -4066,7 +4087,7 @@ async fn edit_git_sync_config( .await?; #[cfg(all(feature = "enterprise", feature = "private"))] { - post_commit = Some((git_sync_settings, removed_webhooks)); + post_commit = Some((git_sync_settings, removed_webhooks, removed_repos)); } } else { // Clearing the whole config removes every repo — delete all their webhooks. @@ -4084,6 +4105,7 @@ async fn edit_git_sync_config( .flatten() .and_then(|v| serde_json::from_value(v).ok()); let removed_webhooks: Vec<(String, i64)> = existing + .as_ref() .map(|e| { e.repositories .iter() @@ -4096,7 +4118,19 @@ async fn edit_git_sync_config( .collect() }) .unwrap_or_default(); - post_commit = Some((WorkspaceGitSyncSettings::default(), removed_webhooks)); + let removed_repos: Vec = existing + .map(|e| { + e.repositories + .iter() + .map(|r| r.git_repo_resource_path.clone()) + .collect() + }) + .unwrap_or_default(); + post_commit = Some(( + WorkspaceGitSyncSettings::default(), + removed_webhooks, + removed_repos, + )); } sqlx::query!( "UPDATE workspace_settings SET git_sync = NULL WHERE workspace_id = $1", @@ -4112,7 +4146,7 @@ async fn edit_git_sync_config( // and delete the webhooks of repos this save removed. Best-effort — a failure // leaves polling on. #[cfg(all(feature = "enterprise", feature = "private"))] - if let Some((mut settings, removed_webhooks)) = post_commit { + if let Some((mut settings, removed_webhooks, removed_repos)) = post_commit { for repo in settings.repositories.iter_mut() { // `sync_repo_webhook` writes back the webhook fields it changes itself: // the remote hook and the record of it have to move together, so @@ -4144,6 +4178,10 @@ async fn edit_git_sync_config( .await; } } + // After the webhook deletions above, which authenticate with these credentials. + for path in removed_repos { + let _ = windmill_common::git_sync_ee::delete_git_credential(&db, &w_id, &path).await; + } } // Trigger git sync for git sync settings changes diff --git a/docs/git-sync-gitlab-setup.md b/docs/git-sync-gitlab-setup.md index 6fca65ede7..e3d03e2e64 100644 --- a/docs/git-sync-gitlab-setup.md +++ b/docs/git-sync-gitlab-setup.md @@ -54,15 +54,26 @@ group. In the resource form for a `git_repository` resource, use the **GitLab** button: paste the instance URL and the token, pick a project from the list, and Windmill -stores the whole remote URL, credential included, in a **secret variable** and -points the resource at it (`"url": "$var:u/you/gitlab_host_group_project_url"`). -It refuses to write over a variable already holding a different repository, so a -path collision cannot silently repoint an existing resource. +keeps the token for you. The resource itself gets the plain remote URL +(`"url": "https://gitlab.com/group/project.git"`), with no credential in it. -Renewal rewrites whichever of the two holds the URL, so a URL pasted straight -into the resource is renewed as well. The variable is still the better place for -it: the credential stays out of the resource, and everything else that references -the variable keeps working when the token changes. What cannot be renewed is a +The token is stored encrypted on the workspace, keyed by the resource's path, and +recorded against the repository it was issued for. Nothing reads it back out over +the API: the server attaches it when it talks to GitLab, and a sync job receives +it only against its own job token. Because it is bound to one repository, +repointing the resource's `url` at somewhere else does not carry the token along; +a repository that genuinely moved needs its token entered again. + +Give the resource its final path before picking a project. The token is filed +under that path, so renaming afterwards leaves it behind. + +Forks of the workspace read this one copy rather than getting their own, so +renewal reaches all of them at once and no fork holds a credential a fork admin +could read. + +A URL with the token written into it keeps working, whether it sits in the +resource or in a secret variable the resource points at (`"url": "$var:..."`), +and renewal rewrites whichever of the two holds it. What cannot be renewed is a variable held in an external secret backend, which Windmill can read but does not own the write to; that is reported on the repository. @@ -71,9 +82,13 @@ own the write to; that is reported on the repository. Windmill reads `expires_at` from the token itself and shows it on the repository in the workspace's git sync settings. Within three weeks of expiry it rotates the token through GitLab's own `POST /personal_access_tokens/self/rotate`, writes the -replacement back to the variable, and verifies it. Only the token can rotate -itself, so a token without `api` (or `self_rotate`) is a permanent warning rather -than something Windmill can fix. +replacement back where the credential is stored, and verifies it. Only the token +can rotate itself, so a token without `api` (or `self_rotate`) is a permanent +warning rather than something Windmill can fix. + +Only the workspace that stores a credential rotates it. A fork reading its +parent's shows the same expiry but is not itself rotatable, so one rotation +serves the whole family instead of each fork racing to renew its own copy. Rotation is deliberately never retried. GitLab revokes the old token the instant it issues the replacement, and presenting an already-rotated token to `/rotate` diff --git a/frontend/src/lib/components/ApiConnectForm.svelte b/frontend/src/lib/components/ApiConnectForm.svelte index 253929a4d9..d339c779a2 100644 --- a/frontend/src/lib/components/ApiConnectForm.svelte +++ b/frontend/src/lib/components/ApiConnectForm.svelte @@ -30,8 +30,12 @@ linkedSecretCandidates?: string[] | undefined description?: string | undefined /** Path the resource is being saved at, passed through to the GitLab picker - * so the credential variable it creates takes the resource's own path. */ + * so the credential it stores is keyed by the resource's own path. */ resourcePath?: string + /** Workspace the resource is being saved into, which is not always the one + * being navigated. The GitLab picker has to store the credential where the + * resource will look for it. */ + workspace?: string onSynced?: () => void } @@ -44,6 +48,7 @@ linkedSecretCandidates = undefined, description = $bindable(undefined), resourcePath = undefined, + workspace = undefined, onSynced = undefined }: Props = $props() @@ -260,6 +265,7 @@ {resourceType} {args} {resourcePath} + {workspace} onArgsUpdate={(newArgs) => { args = newArgs rawCode = JSON.stringify(args, null, 2) diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index 39b76a501b..4beb3be850 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -1389,6 +1389,7 @@ {resourceType} {resourceTypeInfo} resourcePath={path} + workspace={effectiveWorkspace} bind:args bind:isValid onSynced={getResourceTypeInfo}