refactor: key a stored git credential by its repository, not its resource

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C1xHmkxuxYb1GYvth1BS75
This commit is contained in:
hugocasa
2026-09-07 11:17:28 +02:00
co-authored by Claude Opus 5
parent 95c6438d67
commit fa841de6ec
12 changed files with 167 additions and 129 deletions
+1 -1
View File
@@ -1 +1 @@
46ccf72d93cf5f7ceceb0959bc07c14aa76e21fa
dbd426a1bce34e9d07d5fdf4aa6d57e1d17d1176
+74 -12
View File
@@ -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<Postgres>,
) -> 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<Postgres>) -> 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<Postgres>,
) -> 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(())
}
+4 -7
View File
@@ -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:
@@ -21,7 +21,6 @@ pub async fn get_github_app_token_internal(
pub async fn with_stored_credential(
_db: &Pool<Postgres>,
_w_id: &str,
_resource_path: &str,
url: String,
) -> crate::error::Result<String> {
Ok(url)
+6 -11
View File
@@ -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<String> = 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();
+19 -11
View File
@@ -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`
@@ -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)
@@ -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
@@ -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<string, any>) => 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)
}
</script>
@@ -174,12 +190,18 @@
clearable={false}
/>
</div>
{#if applyError}
<Alert type="error" title="Could not store the token" size="xs">{applyError}</Alert>
{/if}
<div class="flex justify-end">
<Button
variant="accent"
unifiedSize="sm"
disabled={!project || !token}
startIcon={{ icon: GitBranch }}
disabled={!project || !token || applying}
startIcon={{
icon: applying ? Loader2 : GitBranch,
classes: applying ? 'animate-spin' : ''
}}
onclick={() => apply(close)}
>
Use this project
@@ -1,7 +1,6 @@
<script lang="ts">
import type { Schema } from '$lib/common'
import {
GitSyncService,
ResourceService,
WorkspaceService,
type Resource,
@@ -162,11 +161,6 @@
// opening the drawer materialises schema defaults (`folder: ""`), so a whole-
// resource dirty check would disable it the moment the drawer opens.
let urlDirty = $derived(!!deployedUrl && current?.args?.url !== deployedUrl)
// The saved path, for the same reason as the saved URL: a credential filed
// under an unsaved rename would sit at a path nothing points at, while the
// repository kept authenticating with the token it already had.
let deployedPath = $derived(selected ? (initialStates[selected]?.path ?? initialPath) : undefined)
let pathDirty = $derived(!!deployedPath && current?.path !== deployedPath)
let resourceToEdit: Resource | undefined = $derived(
selected ? fetchedResources[selected] : undefined
)
@@ -347,11 +341,6 @@
current.path = npath
}
// Carries the workspace it was picked in: `save()` loops over every dirty
// workspace, so an unkeyed token would be filed in all of them.
let pendingGitCredential: { workspace: string; token: string; repoUrl: string } | undefined =
$state(undefined)
/** Whether the write landed. It toasts its own failure, so most callers ignore this;
* one that follows the save with bookkeeping of its own has to know not to. */
export async function save(): Promise<boolean> {
@@ -360,23 +349,6 @@
for (const ws of dirty) {
const s = states[ws].draft!
const ini = initialStates[ws]
// Before the resource write, and only for the workspace the token was
// picked in. Ordered this way so a failure is always the recoverable
// one: if this throws, nothing else has happened and the workspace is
// still dirty, so saving again retries it. Were it to run after, the
// baseline would already be reset and the retry would find nothing to
// do while discarding the token — a saved repository with a managed
// marker and no credential.
if (pendingGitCredential?.workspace === ws) {
await GitSyncService.setGitCredential({
workspace: ws,
requestBody: {
repo_path: s.path,
repo_url: pendingGitCredential.repoUrl,
token: pendingGitCredential.token
}
})
}
if (existedInitially[ws]) {
await ResourceService.updateResource({
workspace: ws,
@@ -415,7 +387,6 @@
// it shows up immediately instead of after the 60s TTL.
invalidateWorkspacePaths(ws)
}
pendingGitCredential = undefined
sendUserToast(
dirty.length > 1 ? `Saved resource in ${dirty.length} workspaces` : `Saved resource`
)
@@ -443,15 +414,14 @@
The URL below carries no credential. Windmill renews the token before it expires and
hands it to this workspace's sync jobs, and forks of this workspace use it without
storing their own copy.
{#if urlDirty || pathDirty}
Save your {urlDirty ? 'URL' : 'path'} change to replace the token.
{#if urlDirty}
Save your URL change to replace the token.
{/if}
</div>
<ReplaceGitCredential
workspace={selected}
resourcePath={deployedPath ?? ''}
repoUrl={deployedUrl ?? ''}
disabled={urlDirty || pathDirty || !deployedUrl || !deployedPath}
disabled={urlDirty || !deployedUrl}
/>
</div>
</Alert>
@@ -479,8 +449,6 @@
{resourceToEdit}
onLoadResourceType={() => resourceTypeResource.refetch()}
workspace={selected}
onCredentialSelected={(c) =>
(pendingGitCredential = selected ? { ...c, workspace: selected } : undefined)}
/>
{/key}
{/if}
@@ -50,7 +50,7 @@
workspace?: string | undefined
/** A git credential the picker chose, for the editor to store once it has
* saved the resource and its path is final. */
onCredentialSelected?: (credential: { token: string; repoUrl: string }) => void
onCredentialStored?: () => void
}
let {
@@ -73,7 +73,7 @@
resourceToEdit,
onLoadResourceType,
workspace = undefined,
onCredentialSelected
onCredentialStored
}: Props = $props()
let ws = $derived(workspace ?? $workspaceStore)
@@ -266,7 +266,7 @@
resourceType={resource_type}
{args}
workspace={ws}
{onCredentialSelected}
{onCredentialStored}
onArgsUpdate={(newArgs) => {
args = newArgs
// The raw editor is also what a workspace missing the resource type
@@ -9,17 +9,16 @@
interface Props {
workspace: string
/** Resource the credential is filed under. */
resourcePath: string
/** The repository as currently saved. The new token is bound to it, so a
* URL edited but not yet saved would bind the credential to something the
* resource does not point at; the caller disables this until it is saved. */
/** The repository as currently saved, which is the key the token is stored
* under. A URL edited but not yet saved would file the token against a
* repository the resource does not point at, so the caller disables this
* until it is saved. */
repoUrl: string
disabled?: boolean
onReplaced?: () => void
}
let { workspace, resourcePath, repoUrl, disabled = false, onReplaced }: Props = $props()
let { workspace, repoUrl, disabled = false, onReplaced }: Props = $props()
let token = $state('')
let saving = $state(false)
@@ -50,7 +49,14 @@
if (parts) {
const projects = await GitSyncService.listGitlabProjects({
workspace,
requestBody: { base_url: parts.base, token }
// 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,
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.`
@@ -59,7 +65,7 @@
}
await GitSyncService.setGitCredential({
workspace,
requestBody: { repo_path: resourcePath, repo_url: repoUrl, token }
requestBody: { repo_url: repoUrl, token }
})
token = ''
sendUserToast('Token replaced')