From 03a8a0edf26226ebc30447a08ccf15ee9e44090d Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 7 Sep 2026 18:19:17 +0200 Subject: [PATCH] fix: read the provider for url-token repos, await the origin before defaulting, and visit unchecked repos last The maintenance pass sorted repositories with no recorded check first on the premise that they cost nothing, but a token-in-URL remote on a host that is not GitLab is probed every pass and never records a check, so it held the head of the list ahead of the tokens that expire. Such repositories now sort last. The card decided its delivery defaults before the origin lookup landed, so a freshly picked GitLab repository never got webhook delivery; the two lookups are awaited together. The resource editor offers to replace a token only where it is held, not in a fork that borrows it, and the replace flow refuses a URL it cannot parse instead of keying the token to it. Attaching a stored credential to a commit-hash probe now requires admin, matching the installation credential beside it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01C1xHmkxuxYb1GYvth1BS75 --- ...d33a26bb2c4804c8045d09bed2de03996ecc.json} | 6 +-- backend/ee-repo-ref.txt | 2 +- backend/src/monitor.rs | 16 ++++---- .../fixtures/git_sync_fork_credential.sql | 5 ++- backend/tests/git_sync_fork_credential.rs | 23 +++++++++-- backend/windmill-store/src/resources.rs | 11 +++-- .../src/lib/components/ResourceEditor.svelte | 22 +++++----- .../git_sync/GitSyncRepositoryCard.svelte | 40 +++++++++---------- .../git_sync/ReplaceGitCredential.svelte | 35 +++++++++------- 9 files changed, 95 insertions(+), 65 deletions(-) rename backend/.sqlx/{query-2184f787e6dffc76bfc28f2f2cefc244505996be71d9caec9c6d69e32a9c3d2c.json => query-85730ae5368b0942c256909efab6d33a26bb2c4804c8045d09bed2de03996ecc.json} (87%) diff --git a/backend/.sqlx/query-2184f787e6dffc76bfc28f2f2cefc244505996be71d9caec9c6d69e32a9c3d2c.json b/backend/.sqlx/query-85730ae5368b0942c256909efab6d33a26bb2c4804c8045d09bed2de03996ecc.json similarity index 87% rename from backend/.sqlx/query-2184f787e6dffc76bfc28f2f2cefc244505996be71d9caec9c6d69e32a9c3d2c.json rename to backend/.sqlx/query-85730ae5368b0942c256909efab6d33a26bb2c4804c8045d09bed2de03996ecc.json index 26098b3c09..d3bb2517b7 100644 --- a/backend/.sqlx/query-2184f787e6dffc76bfc28f2f2cefc244505996be71d9caec9c6d69e32a9c3d2c.json +++ b/backend/.sqlx/query-85730ae5368b0942c256909efab6d33a26bb2c4804c8045d09bed2de03996ecc.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT ws.workspace_id, ws.git_sync\n FROM workspace_settings ws\n JOIN workspace w ON w.id = ws.workspace_id\n WHERE NOT w.deleted\n AND ws.git_sync IS NOT NULL\n AND jsonb_typeof(ws.git_sync->'repositories') = 'array'\n ORDER BY (\n SELECT min((elem->'credential'->>'checked_at')::bigint)\n FROM jsonb_array_elements(ws.git_sync->'repositories') AS elem\n ) ASC NULLS FIRST", + "query": "SELECT ws.workspace_id, ws.git_sync\n FROM workspace_settings ws\n JOIN workspace w ON w.id = ws.workspace_id\n WHERE NOT w.deleted\n AND ws.git_sync IS NOT NULL\n AND jsonb_typeof(ws.git_sync->'repositories') = 'array'\n ORDER BY (\n SELECT min((elem->'credential'->>'checked_at')::bigint)\n FROM jsonb_array_elements(ws.git_sync->'repositories') AS elem\n ) ASC NULLS LAST", "describe": { "columns": [ { @@ -22,5 +22,5 @@ true ] }, - "hash": "2184f787e6dffc76bfc28f2f2cefc244505996be71d9caec9c6d69e32a9c3d2c" -} + "hash": "85730ae5368b0942c256909efab6d33a26bb2c4804c8045d09bed2de03996ecc" +} \ No newline at end of file diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 5b3eef469a..05a7ed1d1e 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -746033ec5f4d77343d39f47a839ddbd12c0f5f54 \ No newline at end of file +333dadfd8bdab3102fd23df6c4b2fb31675c69cd diff --git a/backend/src/monitor.rs b/backend/src/monitor.rs index 6ca0d2520f..86fbed4c04 100644 --- a/backend/src/monitor.rs +++ b/backend/src/monitor.rs @@ -4774,11 +4774,10 @@ async fn maintain_git_credentials_inner(db: &Pool) -> error::Result<() // settings row survives, and rotating a token for one would be pure damage. // Least-recently-checked first, so a pass that runs out of budget resumes // where it stopped instead of re-checking the same head of the list forever. - // A repository with no recorded credential sorts first and stays there, - // which is deliberate: it has no token to introspect, so it costs a few - // database queries and nothing else. Tens of thousands of them would have to - // exist in one instance before they consumed the pass budget ahead of a - // repository that does have a token. + // A repository with no recorded credential sorts last: it has nothing to + // rotate, yet a remote whose URL carries a token on a host that is not + // GitLab still costs a probe every pass and never records a check, so put + // first it would hold the head of the list ahead of the tokens that expire. let rows = sqlx::query!( r#"SELECT ws.workspace_id, ws.git_sync FROM workspace_settings ws @@ -4789,7 +4788,7 @@ async fn maintain_git_credentials_inner(db: &Pool) -> error::Result<() ORDER BY ( SELECT min((elem->'credential'->>'checked_at')::bigint) FROM jsonb_array_elements(ws.git_sync->'repositories') AS elem - ) ASC NULLS FIRST"# + ) ASC NULLS LAST"# ) .fetch_all(db) .await?; @@ -4815,7 +4814,10 @@ async fn maintain_git_credentials_inner(db: &Pool) -> error::Result<() // within one, the repositories need the same least-recently-checked // order or the tail of a large workspace never gets its turn. let mut repositories: Vec<_> = settings.repositories.iter().collect(); - repositories.sort_by_key(|r| r.credential.as_ref().map(|c| c.checked_at)); + repositories.sort_by_key(|r| { + let checked_at = r.credential.as_ref().map(|c| c.checked_at); + (checked_at.is_none(), checked_at) + }); for repo in repositories { if started.elapsed() >= GIT_CREDENTIAL_PASS_BUDGET { diff --git a/backend/tests/fixtures/git_sync_fork_credential.sql b/backend/tests/fixtures/git_sync_fork_credential.sql index 403fd78cc4..5f860129c0 100644 --- a/backend/tests/fixtures/git_sync_fork_credential.sql +++ b/backend/tests/fixtures/git_sync_fork_credential.sql @@ -45,9 +45,12 @@ INSERT INTO workspace_settings (workspace_id, git_sync) VALUES ('orphan-ws', '{"repositories":[{"git_repo_resource_path":"$res:u/admin/repo"}]}'); -- The resource each repository entry names, all pointing at the same repository. +-- The errored fork carries its token in the URL, the way a repository configured +-- by hand does: nothing stores a credential for it, so only its recorded check +-- knows which host it talks to. INSERT INTO resource (workspace_id, path, value, resource_type) VALUES ('parent-ws', 'u/admin/repo', '{"url":"https://gitlab.com/grp/proj.git"}', 'git_repository'), ('fork-ws', 'u/admin/repo', '{"url":"https://gitlab.com/grp/proj.git"}', 'git_repository'), ('deep-fork-ws', 'u/admin/repo', '{"url":"https://gitlab.com/grp/proj.git"}', 'git_repository'), - ('errored-fork-ws', 'u/admin/repo', '{"url":"https://gitlab.com/grp/proj.git"}', 'git_repository'), + ('errored-fork-ws', 'u/admin/repo', '{"url":"https://oauth2:glpat-inline@gitlab.com/grp/proj.git"}', 'git_repository'), ('orphan-ws', 'u/admin/repo', '{"url":"https://gitlab.com/grp/proj.git"}', 'git_repository'); diff --git a/backend/tests/git_sync_fork_credential.rs b/backend/tests/git_sync_fork_credential.rs index c1add83676..8bde262c99 100644 --- a/backend/tests/git_sync_fork_credential.rs +++ b/backend/tests/git_sync_fork_credential.rs @@ -39,19 +39,28 @@ async fn credential_status_is_a_workspaces_own(db: Pool) -> anyhow::Re } /// The host a repository talks to is declared when its credential is stored, and -/// travels with the credential down the fork chain. +/// travels with the credential down the fork chain. A repository whose token +/// rides in its URL has no stored credential, so its host is known only from the +/// check that introspected the token. /// -/// Read from the recorded status instead, a fork answered with the default +/// Read from the recorded status alone, a fork answered with the default /// provider until its own check ran, which is long enough to register a webhook -/// against the wrong receiver. +/// against the wrong receiver. Read from the credential alone, a URL-token +/// repository answered with the default forever. #[sqlx::test(fixtures("git_sync_fork_credential"))] async fn the_provider_comes_from_the_credential_and_reaches_forks( db: Pool, ) -> anyhow::Result<()> { assert_eq!( repo_provider(&db, "parent-ws", REPO).await, + GitProvider::GitLab, + "with nothing stored, the host the check recorded is the answer" + ); + assert_eq!( + repo_provider(&db, "fork-ws", REPO).await, GitProvider::GitHub, - "with nothing stored there is no declaration to read, so the default stands" + "a fork with neither a credential to resolve nor a check of its own \ + answers the default" ); set_git_credential( @@ -83,6 +92,12 @@ async fn the_provider_comes_from_the_credential_and_reaches_forks( GitProvider::GitHub, "a workspace outside the chain resolves no credential and no declaration" ); + assert_eq!( + repo_provider(&db, "errored-fork-ws", REPO).await, + GitProvider::GitLab, + "a token carried in the URL is held by nobody, so the parent's credential \ + is not consulted and the recorded check alone names the host" + ); Ok(()) } diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index d2721693c2..7bc4453d09 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -3268,11 +3268,16 @@ async fn get_git_commit_hash( })?; git_resource.url = resolve_azure_devops_url(&db_with_opt_authed, &w_id, &git_resource.url, false).await?; - // Safe to attach for a caller-named resource because a credential is stored - // under the repository it was issued for: a resource repointed at another - // host asks for that repository's credential and finds none. + // A credential is stored under the repository it was issued for, so a + // resource repointed elsewhere finds none. Which credential can be attached + // is bounded by that; who may use it is bounded here, on the same terms as + // the installation credential above. + let plain_url = git_resource.url.clone(); git_resource.url = windmill_common::git_sync_oss::with_stored_credential(&db, &w_id, git_resource.url).await?; + if git_resource.url != plain_url { + require_admin(authed.is_admin, &authed.username)?; + } let identities: Vec = query .git_ssh_identity diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index cc6f380083..803da67d51 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -159,20 +159,20 @@ // The deployed path, for the same reason as the deployed URL: the server // answers about what is stored, and an unsaved rename names nothing yet. let deployedPath = $derived(selected ? (initialStates[selected]?.path ?? initialPath) : undefined) - // Asked of the server rather than read off the resource: the marker this - // replaced was a copy of a server fact kept in a client-editable, exported - // object, so it went stale on a URL edit, a workspace import, and in a fork. - // Re-asked when the saved URL moves, since that is a different repository. + // Asked of the server rather than read off the resource: the resource is + // client-editable, exported and copied into forks, so nothing written on it + // stays true. Re-asked when the saved URL moves, since that is a different + // repository. const credentialOrigin = resource( - [() => selected, () => deployedPath, () => deployedUrl], - async ([ws, path]) => - ws && path + [() => selected, () => deployedPath, () => deployedUrl, () => resource_type], + async ([ws, path, _url, type]) => + ws && path && type === 'git_repository' ? await GitSyncService.getCredentialOrigin({ workspace: ws, path }).catch(() => undefined) : undefined ) - let managedHost = $derived( - credentialOrigin.current?.origin ? credentialOrigin.current.provider : undefined - ) + // Only a credential this workspace holds is its to replace: a fork borrows + // its ancestor's, and storing a replacement here would split it in two. + let holdsCredential = $derived(credentialOrigin.current?.origin === 'held') // Only an unsaved *URL* blocks replacing the token, not any unsaved change: // opening the drawer materialises schema defaults (`folder: ""`), so a whole- // resource dirty check would disable it the moment the drawer opens. @@ -423,7 +423,7 @@ {/if} - {#if managedHost && selected} + {#if holdsCredential && selected}
diff --git a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte index 0f08cbb7aa..5428776220 100644 --- a/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte +++ b/frontend/src/lib/components/git_sync/GitSyncRepositoryCard.svelte @@ -278,25 +278,25 @@ credentialOrigin = undefined try { // The server answers whether it holds this repository's credential; - // the resource cannot, because the marker that used to claim it was - // a copy that went stale on a URL edit, an import, and in a fork. - // Best-effort: a failure here must not hide the URL below. - GitSyncService.getCredentialOrigin({ - workspace: $workspaceStore, - path: repo.git_repo_resource_path - }) - .then((r) => { - if (!abortController.signal.aborted) { - credentialOrigin = r?.origin - managedCredential = r?.origin ? (r.provider ?? 'gitlab') : undefined - } + // the resource cannot, being client-editable, exported and copied + // into forks. Best-effort: a failure here must not hide the URL + // below. Awaited alongside the resource because the defaults below + // read the answer. + const [origin, resource] = await Promise.all([ + GitSyncService.getCredentialOrigin({ + workspace: $workspaceStore, + path: repo.git_repo_resource_path + }).catch(() => undefined), + ResourceService.getResource({ + workspace: $workspaceStore, + path: repo.git_repo_resource_path }) - .catch(() => {}) - const resource = await ResourceService.getResource({ - workspace: $workspaceStore, - path: repo.git_repo_resource_path - }) + ]) + if (!abortController.signal.aborted) { + credentialOrigin = origin?.origin + managedCredential = origin?.origin ? (origin.provider ?? 'gitlab') : undefined + } if (!abortController.signal.aborted && resource?.value) { // Extract git URL from resource value const value = resource.value as Record @@ -651,15 +651,15 @@
{#if credentialDaysLeft === undefined} Repository token does not expire. + {:else if credentialOrigin === 'borrowed'} + Repository token expires on {repo.credential.expires_at}, and the workspace that holds it + manages renewal. {:else if repo.credential.rotatable && $enterpriseLicense} Repository token expires on {repo.credential.expires_at}, and Windmill renews it automatically. {:else if repo.credential.rotatable} Repository token expires on {repo.credential.expires_at}. Renewing it automatically requires an enterprise license. - {:else if credentialOrigin === 'borrowed'} - Repository token expires on {repo.credential.expires_at}, and the workspace that holds it - manages renewal. {:else} Repository token expires on {repo.credential.expires_at}, and Windmill does not renew it. {/if} diff --git a/frontend/src/lib/components/git_sync/ReplaceGitCredential.svelte b/frontend/src/lib/components/git_sync/ReplaceGitCredential.svelte index a9528fd8c1..cfeaee20ad 100644 --- a/frontend/src/lib/components/git_sync/ReplaceGitCredential.svelte +++ b/frontend/src/lib/components/git_sync/ReplaceGitCredential.svelte @@ -52,23 +52,28 @@ // repository but only refuses it when something tries to use it, so a // wrong token would otherwise be accepted here and surface as a failed // sync later. + // A URL that does not parse names no project to check, and would be + // stored as the credential's key verbatim: a `$var:` reference here + // keys the token to a repository that does not exist. const parts = repoParts(forRepo) - if (parts) { - const projects = await GitSyncService.listGitlabProjects({ - workspace: inWorkspace, - // Searched by name rather than listed whole: the listing is one - // capped page, so a token that reaches more projects than fit - // would not show this one and a working token would be refused. - requestBody: { - base_url: parts.base, - token: candidate, - search: parts.project.split('/').pop() - } - }) - if (!projects.some((p) => p.path_with_namespace === parts.project)) { - error = `That token cannot push to ${parts.project}. Check its role and that it belongs to this project.` - return + if (!parts) { + error = 'The resource URL must name the repository directly to replace its token here.' + return + } + const projects = await GitSyncService.listGitlabProjects({ + workspace: inWorkspace, + // Searched by name rather than listed whole: the listing is one + // capped page, so a token that reaches more projects than fit + // would not show this one and a working token would be refused. + requestBody: { + base_url: parts.base, + token: candidate, + search: parts.project.split('/').pop() } + }) + if (!projects.some((p) => p.path_with_namespace === parts.project)) { + error = `That token cannot push to ${parts.project}. Check its role and that it belongs to this project.` + return } await GitSyncService.setGitCredential({ workspace: inWorkspace,