diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index b18b3ded04..5901aafb2a 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -46ccf72d93cf5f7ceceb0959bc07c14aa76e21fa +dbd426a1bce34e9d07d5fdf4aa6d57e1d17d1176 diff --git a/backend/tests/git_sync_fork_credential.rs b/backend/tests/git_sync_fork_credential.rs index 14a398f794..91b2eaa364 100644 --- a/backend/tests/git_sync_fork_credential.rs +++ b/backend/tests/git_sync_fork_credential.rs @@ -45,48 +45,110 @@ async fn a_fork_qualifies_through_the_nearest_ancestors_credential( Ok(()) } -/// The stored credential is shared with forks and bound to one repository. +/// The stored credential is shared with forks and keyed by one repository. /// -/// Both properties are the point of keeping it in `workspace_settings`: sharing -/// is what stops a rotation from stranding every fork on a revoked token, and -/// the binding is what stops a rewritten resource URL from carrying the token to -/// a host of the writer's choosing. +/// Both properties are the point of keeping it in `workspace_settings` under the +/// repository's identity: sharing is what stops a rotation from stranding every +/// fork on a revoked token, and the key is what stops a rewritten resource URL +/// from carrying the token to a host of the writer's choosing. #[sqlx::test(fixtures("git_sync_fork_credential"))] async fn a_fork_reads_an_ancestors_credential_for_the_bound_repository_only( db: Pool, ) -> anyhow::Result<()> { - set_git_credential(&db, "parent-ws", REPO, URL, "glpat-secret").await?; + set_git_credential(&db, "parent-ws", URL, "glpat-secret").await?; assert_eq!( - git_credential_for_url(&db, "parent-ws", REPO, URL) + git_credential_for_url(&db, "parent-ws", URL) .await? .as_deref(), Some("glpat-secret"), "the workspace that stored it reads it back" ); assert_eq!( - git_credential_for_url(&db, "fork-ws", REPO, URL) + git_credential_for_url(&db, "fork-ws", URL) .await? .as_deref(), Some("glpat-secret"), "a fork stores none of its own and resolves the parent's" ); assert_eq!( - git_credential_for_url(&db, "deep-fork-ws", REPO, URL) + git_credential_for_url(&db, "deep-fork-ws", URL) .await? .as_deref(), Some("glpat-secret"), "a fork of a fork resolves the root's, two levels up" ); assert_eq!( - git_credential_for_url(&db, "fork-ws", REPO, "https://evil.example/grp/proj.git").await?, + git_credential_for_url(&db, "fork-ws", "https://evil.example/grp/proj.git").await?, None, - "a resource repointed at another repository resolves to no credential" + "a resource repointed at another repository asks for that one's \ + credential and finds none" ); assert_eq!( - git_credential_for_url(&db, "orphan-ws", REPO, URL).await?, + git_credential_for_url(&db, "orphan-ws", URL).await?, None, "a workspace with no credential and no parent resolves nothing" ); Ok(()) } + +/// One repository's credential is untouched by another's. +/// +/// The key is the repository, so picking a second repository stores beside the +/// first rather than over it. Keyed by the resource instead, a workspace editing +/// one repository's resource to point somewhere else would replace the token the +/// original repository was still syncing with. +#[sqlx::test(fixtures("git_sync_fork_credential"))] +async fn each_repository_keeps_its_own_credential(db: Pool) -> anyhow::Result<()> { + const OTHER_URL: &str = "https://gitlab.com/grp/other.git"; + + set_git_credential(&db, "parent-ws", URL, "glpat-first").await?; + set_git_credential(&db, "parent-ws", OTHER_URL, "glpat-second").await?; + + assert_eq!( + git_credential_for_url(&db, "parent-ws", URL) + .await? + .as_deref(), + Some("glpat-first"), + "storing a second repository's token leaves the first's in place" + ); + assert_eq!( + git_credential_for_url(&db, "parent-ws", OTHER_URL) + .await? + .as_deref(), + Some("glpat-second") + ); + + set_git_credential(&db, "parent-ws", URL, "glpat-replacement").await?; + assert_eq!( + git_credential_for_url(&db, "parent-ws", URL) + .await? + .as_deref(), + Some("glpat-replacement"), + "storing the same repository again replaces rather than duplicates" + ); + assert_eq!( + git_credential_for_url(&db, "parent-ws", OTHER_URL) + .await? + .as_deref(), + Some("glpat-second"), + "and still leaves the other repository alone" + ); + Ok(()) +} + +/// A credential issued for `https` is not served for the `http` spelling. +/// +/// The resource holding the URL is writable by anyone with write on its path, so +/// without the scheme in the key that edit would send the token over cleartext. +#[sqlx::test(fixtures("git_sync_fork_credential"))] +async fn a_credential_is_not_served_over_a_downgraded_transport( + db: Pool, +) -> anyhow::Result<()> { + set_git_credential(&db, "parent-ws", URL, "glpat-secret").await?; + assert_eq!( + git_credential_for_url(&db, "parent-ws", "http://gitlab.com/grp/proj.git").await?, + None + ); + Ok(()) +} diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 275f4e2957..2eeed211df 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -2940,20 +2940,17 @@ paths: schema: type: object properties: - repo_path: - type: string - description: Path of the git repository resource the credential authenticates repo_url: type: string description: >- - The repository the credential is for. The token is only used - against this repository, so repointing the resource elsewhere - cannot carry it along. + The repository the credential is for, and the key it is + stored under. It is served for this repository and no other, + so repointing a resource elsewhere cannot carry the token + along. token: type: string description: The access token, as pasted required: - - repo_path - repo_url - token responses: diff --git a/backend/windmill-common/src/git_sync_oss.rs b/backend/windmill-common/src/git_sync_oss.rs index 1a3d5f8a23..8b9062ec5a 100644 --- a/backend/windmill-common/src/git_sync_oss.rs +++ b/backend/windmill-common/src/git_sync_oss.rs @@ -21,7 +21,6 @@ pub async fn get_github_app_token_internal( pub async fn with_stored_credential( _db: &Pool, _w_id: &str, - _resource_path: &str, url: String, ) -> crate::error::Result { Ok(url) diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 43205936bf..228b1bea37 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -3268,11 +3268,11 @@ 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 the stored credential - // is bound to the repository it was issued for: a resource repointed at - // another host resolves to no credential rather than carrying this one there. + // 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. git_resource.url = - windmill_common::git_sync_oss::with_stored_credential(&db, &w_id, path, git_resource.url) + windmill_common::git_sync_oss::with_stored_credential(&db, &w_id, git_resource.url) .await?; let identities: Vec = query @@ -3954,13 +3954,8 @@ pub async fn get_git_repo_head_for_autopull( resolve_azure_devops_url(&git_sync_system_dba(db), w_id, &git_resource.url, true).await?; // A repo whose credential Windmill holds carries none in its URL, so the // poller has to attach it here or every probe would be unauthenticated. - git_resource.url = windmill_common::git_sync_oss::with_stored_credential( - db, - w_id, - git_repo_resource_path, - git_resource.url, - ) - .await?; + git_resource.url = + windmill_common::git_sync_oss::with_stored_credential(db, w_id, git_resource.url).await?; if let Some(branch) = git_resource.branch.as_deref().filter(|s| !s.is_empty()) { let branch = branch.to_string(); diff --git a/docs/git-sync-gitlab-setup.md b/docs/git-sync-gitlab-setup.md index 14bbdae34b..649d2ba678 100644 --- a/docs/git-sync-gitlab-setup.md +++ b/docs/git-sync-gitlab-setup.md @@ -57,15 +57,18 @@ paste the instance URL and the token, pick a project from the list, and Windmill 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. -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. +The token is stored encrypted on the workspace, keyed by the repository it was +issued for rather than by the resource naming it, the same way a GitHub App +installation is held against the account it covers. 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. Repointing a resource's `url` asks +for a different repository's token and finds none, so the edit carries nothing +with it; 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. +Because the repository is the key, the token is stored the moment you pick the +project, before the resource is saved. Renaming the resource later keeps it, and +cancelling the edit leaves a stored token that nothing uses until some resource +points at that repository again. Forks of the workspace read this one copy rather than getting their own, so renewal reaches all of them at once and the token is not duplicated into every @@ -93,9 +96,14 @@ 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. +Only the workspace that issued a credential rotates it, so one rotation serves +the whole family instead of each fork racing to renew the same token. A fork +either reads the parent's stored credential or, for a repository whose token +lives in the URL, holds a copy of it that forking made; either way the token is +the parent's to renew. A fork that carries a genuinely different token rotates +it itself. The fork reports the same expiry from its own first check onwards; a +fork created since the parent's last check shows none until the maintenance pass +reaches it. 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 2deb075e34..19642219e3 100644 --- a/frontend/src/lib/components/ApiConnectForm.svelte +++ b/frontend/src/lib/components/ApiConnectForm.svelte @@ -35,7 +35,7 @@ workspace?: string /** A git credential the picker chose, for the drawer to store once it has * saved the resource and its path is final. */ - onCredentialSelected?: (credential: { token: string; repoUrl: string }) => void + onCredentialStored?: () => void onSynced?: () => void } @@ -48,7 +48,7 @@ linkedSecretCandidates = undefined, description = $bindable(undefined), workspace = undefined, - onCredentialSelected, + onCredentialStored, onSynced = undefined }: Props = $props() @@ -265,7 +265,7 @@ {resourceType} {args} {workspace} - {onCredentialSelected} + {onCredentialStored} 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 2b537af7ff..72940612f9 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -12,7 +12,6 @@ sortResourceTypesByMatch } from './resourceTypeDisplay' import { - GitSyncService, OauthService, ResourceService, WorkspaceService, @@ -76,7 +75,6 @@ }: Props = $props() let effectiveWorkspace = $derived(workspace ?? $workspaceStore!) - let pendingGitCredential: { token: string; repoUrl: string } | undefined = $state(undefined) let isValid = $state(true) @@ -933,21 +931,6 @@ } } - // Before the resource is written, so a failure here leaves nothing behind - // and the whole save can simply be retried. After it, a failed credential - // write would leave a created resource that the retry cannot create again, - // with a managed marker and no token behind it. - if (pendingGitCredential) { - await GitSyncService.setGitCredential({ - workspace: effectiveWorkspace, - requestBody: { - repo_path: path, - repo_url: pendingGitCredential.repoUrl, - token: pendingGitCredential.token - } - }) - } - if (filling) { // The stub the import made carries no description, so this is the one chance to // give it one; its resource_type and path are already what we want. @@ -969,7 +952,6 @@ } }) } - pendingGitCredential = undefined dispatch('refresh', path) dispatch('close') sendUserToast( @@ -1351,13 +1333,12 @@ {resourceType} {resourceTypeInfo} workspace={effectiveWorkspace} - onCredentialSelected={(c) => { - pendingGitCredential = c + onCredentialStored={() => { // `forceSecretValue` files a git_repository's `url` in a secret // variable, for the URLs that carry a token in them. The picker's - // does not — the token is stored separately — so that variable - // would hold nothing secret and add a second place to keep in - // step with the resource. + // does not: the token is stored separately, so that variable would + // hold nothing secret and add a second place to keep in step with + // the resource. linkedSecrets = linkedSecrets.filter((f) => f !== 'url') }} bind:args diff --git a/frontend/src/lib/components/GitLabIntegration.svelte b/frontend/src/lib/components/GitLabIntegration.svelte index 0bb8d10b20..7ad9b4fcef 100644 --- a/frontend/src/lib/components/GitLabIntegration.svelte +++ b/frontend/src/lib/components/GitLabIntegration.svelte @@ -16,9 +16,9 @@ * one being navigated: the credential has to land where the resource will * look for it. */ workspace?: string - /** The picked project's token, handed over for the form to store once the - * resource is saved and its path is final. */ - onCredentialSelected?: (credential: { token: string; repoUrl: string }) => void + /** Fired once the picked project's token is stored, so a form that would + * otherwise file the URL as a secret knows it no longer holds one. */ + onCredentialStored?: () => void onArgsUpdate?: (args: Record) => void } @@ -26,7 +26,7 @@ resourceType, args = {}, workspace = undefined, - onCredentialSelected, + onCredentialStored, onArgsUpdate }: Props = $props() @@ -39,6 +39,8 @@ let selectedProject: string | undefined = $state(undefined) let loading = $state(false) let listError: string | undefined = $state(undefined) + let applying = $state(false) + let applyError: string | undefined = $state(undefined) // Shown alongside the GitHub App button and on the same terms, so the two // read as one choice rather than one option and one absence. @@ -77,15 +79,29 @@ } } - function apply(close: (_: any) => void) { - if (!project || !token) return + async function apply(close: (_: any) => void) { + if (!project || !token || applying) return const chosen = project const url = chosen.http_url_to_repo - // Handed to the form instead of stored now. The credential is filed under - // the resource's path, which is not settled until the resource is saved, - // and writing here would outlive an edit the user then cancels: picking a - // different project and backing out would have replaced a working token. - onCredentialSelected?.({ token, repoUrl: url }) + applying = true + applyError = undefined + try { + // Stored against the project it was issued for, the way a GitHub App + // installation is stored against the account it covers. Nothing waits on + // the resource: its path is not settled while it is being created, and + // deferring the write would tie one repository's token to another + // repository's save succeeding. + await GitSyncService.setGitCredential({ + workspace: ws!, + requestBody: { repo_url: url, token } + }) + } catch (err) { + applyError = err?.body ?? err?.message ?? String(err) + return + } finally { + applying = false + } + onCredentialStored?.() onArgsUpdate?.({ ...args, url, @@ -100,7 +116,7 @@ token = '' projects = [] selectedProject = undefined - sendUserToast(`${chosen.path_with_namespace} selected. Its token is stored when you save.`) + sendUserToast(`${chosen.path_with_namespace} selected and its token stored`) close(null) } @@ -174,12 +190,18 @@ clearable={false} /> + {#if applyError} + {applyError} + {/if}