diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index e01fab6059..2a98be2811 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -6ebc78675dc3510a8abc56a957b96160a4dc7f5f +64567911809a2feb4cb0383153939d25585d1f7b diff --git a/backend/windmill-worker/src/result_processor.rs b/backend/windmill-worker/src/result_processor.rs index d6e3dca911..821514d862 100644 --- a/backend/windmill-worker/src/result_processor.rs +++ b/backend/windmill-worker/src/result_processor.rs @@ -1396,19 +1396,24 @@ async fn maybe_post_git_sync_check( // identity existed keeps using the URL it captured at enqueue, which cannot // have been repointed since. let repo_url = match (check.repo.is_some(), row.repo_path.as_deref(), check.repo_url.clone()) { + // The resource path is mutable, so following it is only safe when the + // marker also carries the identity to check the result against. (true, Some(path), _) => { windmill_common::git_sync_ee::resolve_repo_url_interpolated(db, workspace_id, path) .await } + // A marker written before that identity existed captured the URL itself, + // which cannot have been repointed since. (_, _, Some(url)) => { windmill_common::variables::get_variable_or_self(url, db, workspace_id).await } - (false, Some(path), None) => { - windmill_common::git_sync_ee::resolve_repo_url_interpolated(db, workspace_id, path) - .await - } - (_, None, None) => { - tracing::error!("git sync-check: marker names no repository"); + // Neither: nothing here can prove which repository this check belongs to, + // and resolving the path anyway is how a preview reaches the wrong one. + // Leaving the check unfinished is the safe failure. + _ => { + tracing::error!( + "git sync-check: the marker carries neither a repository identity nor a url; not acting on it" + ); return; } }; diff --git a/frontend/src/lib/components/GitLabIntegration.svelte b/frontend/src/lib/components/GitLabIntegration.svelte index 137aafcb1f..25462496eb 100644 --- a/frontend/src/lib/components/GitLabIntegration.svelte +++ b/frontend/src/lib/components/GitLabIntegration.svelte @@ -69,23 +69,54 @@ } }) - // Applying replaces whatever is at this path, and anything else pointing at it - // would silently start using a different repository. - let variableExists = $state(false) + // Host and path of a git remote, lowercased with `.git` dropped: the same + // identity the backend compares, so "is this the same repository?" gets one + // answer on both sides. + function repoIdentity(url: string): string | undefined { + try { + const u = new URL(url.trim()) + const path = u.pathname + .toLowerCase() + .replace(/^\/+|\/+$/g, '') + .replace(/\.git$/, '') + return path ? `${u.host.toLowerCase()}/${path}` : undefined + } catch { + return undefined + } + } + + // What already lives at this path decides whether applying is safe. A + // suggested path can collide with another project's, and the field is free + // text, so the name alone proves nothing: only the repository the stored + // value points at does. + type Occupant = 'free' | 'same-repo' | 'other' + let occupant: Occupant = $state('free') $effect(() => { const path = variablePath const workspace = ws - if (!workspace || !path) { - variableExists = false + const target = project ? repoIdentity(project.http_url_to_repo) : undefined + if (!workspace || !path || !target) { + occupant = 'free' return } let cancelled = false VariableService.existsVariable({ workspace, path }) - .then((e) => { - if (!cancelled) variableExists = e + .then(async (exists) => { + if (cancelled) return + if (!exists) { + occupant = 'free' + return + } + // Reading it decrypts a secret, which is why this is admin-only. + const current = await VariableService.getVariableValue({ workspace, path }).catch( + () => undefined + ) + if (cancelled) return + // Unreadable counts as occupied: it is someone else's until proven otherwise. + occupant = current && repoIdentity(current) === target ? 'same-repo' : 'other' }) .catch(() => { - if (!cancelled) variableExists = false + if (!cancelled) occupant = 'other' }) return () => { cancelled = true @@ -126,8 +157,10 @@ async function apply(close: (_: any) => void) { // The token is cleared once stored, and re-applying without one would - // overwrite the stored credential with an empty password. - if (!ws || !project || !token) return + // overwrite the stored credential with an empty password. A path holding + // another repository's remote is never written: that would repoint every + // resource using it, silently, at this project. + if (!ws || !project || !token || occupant === 'other') return applying = true try { const value = repositoryUrl(project) @@ -244,10 +277,14 @@ size="sm" inputProps={{ oninput: () => (variablePathTouched = true) }} /> - {#if variableExists} + {#if occupant === 'other'} +
+ This variable already holds something else. Choose another path, or anything using + it would start pointing at this project. +
+ {:else if occupant === 'same-repo'}
- This variable already exists and will be replaced. Anything else using it will - point at this repository. + This variable already points at this project, and its token will be replaced.
{/if} @@ -255,7 +292,7 @@