From 8d33be2ee71362ee969998580815654aefb3652a Mon Sep 17 00:00:00 2001 From: Piotr Chabros Date: Tue, 8 Sep 2026 23:28:27 +0200 Subject: [PATCH 1/5] fix clone flow for GitHub SAML authorization --- .../components/clone-project-dialog.test.tsx | 39 +++++++++++++++++ .../src/components/clone-project-dialog.tsx | 43 +++++++++++++++++-- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/packages/web/src/components/clone-project-dialog.test.tsx b/packages/web/src/components/clone-project-dialog.test.tsx index ebd572aba..40b1f8dc3 100644 --- a/packages/web/src/components/clone-project-dialog.test.tsx +++ b/packages/web/src/components/clone-project-dialog.test.tsx @@ -190,6 +190,45 @@ describe('CloneProjectDialog', () => { expect(cloneButton().disabled).toBe(false) }) + it('links GitHub SAML authorization and explains that the failed clone must be retried', async () => { + const ssoUrl = + 'https://github.com/orgs/Bug-Bounty-Switzerland/sso?authorization_request=AHBV4IUOPSOOJDNNTYUU4ALKUCFKNA5P' + serve(async () => + json( + { + error: + `GraphQL: Resource protected by organization SAML enforcement. ` + + `You must grant your OAuth token access to this organization. (repository) ` + + `Authorize in your web browser: ${ssoUrl}`, + }, + 500, + ), + ) + renderDialog() + fireEvent.change(urlInput(), { target: { value: 'Bug-Bounty-Switzerland/private-repo' } }) + fireEvent.click(cloneButton()) + + await waitFor(() => expect(slot('clone-sso-link')).toBeTruthy()) + const link = slot('clone-sso-link') as HTMLAnchorElement + expect(link.textContent).toBe('Authorize this GitHub organization') + expect(link.href).toBe(ssoUrl) + expect(link.target).toBe('_blank') + expect(link.rel).toBe('noreferrer') + expect(slot('clone-error')?.textContent).toContain('then return here and retry the clone') + expect(cloneButton().textContent).toBe('Retry clone') + }) + + it('does not turn an arbitrary URL in clone output into a link', async () => { + serve(async () => json({ error: 'remote: visit https://evil.example/authorize' }, 500)) + renderDialog() + fireEvent.change(urlInput(), { target: { value: 'open-mercato/cezar' } }) + fireEvent.click(cloneButton()) + + await waitFor(() => expect(slot('clone-error')?.textContent).toContain('evil.example')) + expect(slot('clone-sso-link')).toBeNull() + expect(cloneButton().textContent).toBe('Retry clone') + }) + it('surfaces the existing-folder 409 as an error rather than navigating anywhere', async () => { serve(async () => json({ error: 'folder already exists: /home/me/cezar/projects/cezar' }, 409)) renderDialog() diff --git a/packages/web/src/components/clone-project-dialog.tsx b/packages/web/src/components/clone-project-dialog.tsx index 5ca4c27ce..72a59e4b9 100644 --- a/packages/web/src/components/clone-project-dialog.tsx +++ b/packages/web/src/components/clone-project-dialog.tsx @@ -17,6 +17,24 @@ import { import { Input } from '@/components/ui/input' import { Label } from '@/components/ui/label' +/** + * GitHub prints this URL when an otherwise-valid OAuth token still needs SAML + * authorization for the repository's organization. The failed `gh` process + * cannot resume after the browser flow, so the dialog turns the URL into a + * trusted link and makes the required retry explicit. + * + * Keep this deliberately narrower than "find a URL": clone errors include + * output influenced by the remote, and rendering arbitrary output as a link + * would make the local cockpit an excellent phishing surface. + */ +export function githubSsoUrl(error: unknown): string | null { + if (!(error instanceof Error)) return null + const match = error.message.match( + /https:\/\/github\.com\/orgs\/[A-Za-z0-9._-]+\/sso\?authorization_request=[A-Za-z0-9._~-]+/, + ) + return match?.[0] ?? null +} + /** * "Add project → Clone from GitHub" (multi-project spec, "Add project" option B / step 4.3). * @@ -49,6 +67,7 @@ export function CloneProjectDialog({ const projects = useProjects() const checkout = useCheckoutProject() const navigate = useNavigate() + const ssoUrl = githubSsoUrl(checkout.error) // One id per mounted dialog. The dialog is mounted only while open (AddProjectMenu), so a // second clone attempt in a second opening is a second id — which is the point: a stale @@ -180,9 +199,25 @@ export function CloneProjectDialog({ ) : null} {checkout.isError ? ( -

- {checkout.error instanceof Error ? checkout.error.message : 'could not clone that repository'} -

+
+

+ {checkout.error instanceof Error ? checkout.error.message : 'could not clone that repository'} +

+ {ssoUrl ? ( +

+ + Authorize this GitHub organization + + {' — then return here and retry the clone.'} +

+ ) : null} +
) : null} @@ -194,7 +229,7 @@ export function CloneProjectDialog({ disabled={url.trim() === '' || effectiveName === '' || checkout.isPending} onClick={clone} > - {checkout.isPending ? 'Cloning…' : 'Clone'} + {checkout.isPending ? 'Cloning…' : checkout.isError ? 'Retry clone' : 'Clone'} From cb3a2e34614992299b5c7f67a48fb12368c82d31 Mon Sep 17 00:00:00 2001 From: Piotr Chabros Date: Tue, 8 Sep 2026 23:45:04 +0200 Subject: [PATCH 2/5] fix SAML clone continuation and layout --- .../components/clone-project-dialog.test.tsx | 41 ++++++---- .../src/components/clone-project-dialog.tsx | 81 ++++++++++++++----- 2 files changed, 88 insertions(+), 34 deletions(-) diff --git a/packages/web/src/components/clone-project-dialog.test.tsx b/packages/web/src/components/clone-project-dialog.test.tsx index 40b1f8dc3..b8d0ee2c8 100644 --- a/packages/web/src/components/clone-project-dialog.test.tsx +++ b/packages/web/src/components/clone-project-dialog.test.tsx @@ -190,21 +190,25 @@ describe('CloneProjectDialog', () => { expect(cloneButton().disabled).toBe(false) }) - it('links GitHub SAML authorization and explains that the failed clone must be retried', async () => { + it('keeps GitHub SAML authorization compact and retries when the user returns', async () => { const ssoUrl = 'https://github.com/orgs/Bug-Bounty-Switzerland/sso?authorization_request=AHBV4IUOPSOOJDNNTYUU4ALKUCFKNA5P' - serve(async () => - json( - { - error: - `GraphQL: Resource protected by organization SAML enforcement. ` + - `You must grant your OAuth token access to this organization. (repository) ` + - `Authorize in your web browser: ${ssoUrl}`, - }, - 500, - ), - ) - renderDialog() + let attempts = 0 + serve(async () => { + attempts += 1 + return attempts === 1 + ? json( + { + error: + `GraphQL: Resource protected by organization SAML enforcement. ` + + `You must grant your OAuth token access to this organization. (repository) ` + + `Authorize in your web browser: ${ssoUrl}`, + }, + 500, + ) + : json({ project: PROJECT }) + }) + const { onOpenChange } = renderDialog() fireEvent.change(urlInput(), { target: { value: 'Bug-Bounty-Switzerland/private-repo' } }) fireEvent.click(cloneButton()) @@ -214,8 +218,16 @@ describe('CloneProjectDialog', () => { expect(link.href).toBe(ssoUrl) expect(link.target).toBe('_blank') expect(link.rel).toBe('noreferrer') - expect(slot('clone-error')?.textContent).toContain('then return here and retry the clone') + expect(slot('clone-error')?.textContent).toContain('cezar will retry when you return') + expect(slot('clone-error')?.textContent).not.toContain('authorization_request=') expect(cloneButton().textContent).toBe('Retry clone') + + fireEvent.click(link) + expect(slot('clone-error')?.textContent).toContain('waiting for you to return') + fireEvent.focus(window) + + await waitFor(() => expect(posted).toHaveLength(2)) + await waitFor(() => expect(onOpenChange).toHaveBeenCalledWith(false)) }) it('does not turn an arbitrary URL in clone output into a link', async () => { @@ -226,6 +238,7 @@ describe('CloneProjectDialog', () => { await waitFor(() => expect(slot('clone-error')?.textContent).toContain('evil.example')) expect(slot('clone-sso-link')).toBeNull() + expect(slot('clone-error')?.querySelector('p')?.className).toContain('break-all') expect(cloneButton().textContent).toBe('Retry clone') }) diff --git a/packages/web/src/components/clone-project-dialog.tsx b/packages/web/src/components/clone-project-dialog.tsx index 72a59e4b9..e88cc3cad 100644 --- a/packages/web/src/components/clone-project-dialog.tsx +++ b/packages/web/src/components/clone-project-dialog.tsx @@ -1,5 +1,5 @@ import { SettingsIcon } from 'lucide-react' -import { useEffect, useMemo, useRef, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { Link as RouterLink, useNavigate } from 'react-router' import { onWorkspaceEvent } from '@/api/global-events' @@ -64,6 +64,8 @@ export function CloneProjectDialog({ const [url, setUrl] = useState('') const [name, setName] = useState('') const [progress, setProgress] = useState(null) + const [awaitingSso, setAwaitingSso] = useState(false) + const ssoRetryArmed = useRef(false) const projects = useProjects() const checkout = useCheckoutProject() const navigate = useNavigate() @@ -103,8 +105,10 @@ export function CloneProjectDialog({ const projectsDir = projects.data?.projectsDir ?? '' const target = effectiveName === '' ? '' : `${projectsDir.replace(/\/+$/, '')}/${effectiveName}` - const clone = () => { + const clone = useCallback(() => { if (url.trim() === '' || checkout.isPending) return + ssoRetryArmed.current = false + setAwaitingSso(false) setProgress(null) checkout.mutate( { url: url.trim(), checkoutId, ...(name.trim() === '' ? {} : { name: name.trim() }) }, @@ -117,11 +121,38 @@ export function CloneProjectDialog({ }, }, ) - } + }, [checkout, checkoutId, name, navigate, onOpenChange, url]) + + // Authorizing an existing OAuth token changes GitHub's server-side token + // grant; it cannot wake the `gh repo clone` process that already exited. + // Returning to this tab is the only browser signal available to the local + // cockpit, so retry once on focus/visibility after the user follows the SSO + // link. The ref prevents browsers that emit both events from cloning twice. + useEffect(() => { + if (!awaitingSso) return + const retry = (): void => { + if (!ssoRetryArmed.current) return + ssoRetryArmed.current = false + setAwaitingSso(false) + clone() + } + const retryWhenVisible = (): void => { + if (document.visibilityState === 'visible') retry() + } + window.addEventListener('focus', retry) + document.addEventListener('visibilitychange', retryWhenVisible) + return () => { + window.removeEventListener('focus', retry) + document.removeEventListener('visibilitychange', retryWhenVisible) + } + }, [awaitingSso, clone]) return ( (checkout.isPending ? undefined : onOpenChange(next))}> - + Clone from GitHub @@ -199,24 +230,34 @@ export function CloneProjectDialog({ ) : null} {checkout.isError ? ( -
-

- {checkout.error instanceof Error ? checkout.error.message : 'could not clone that repository'} -

+
{ssoUrl ? ( -

- - Authorize this GitHub organization - - {' — then return here and retry the clone.'} + <> +

GitHub requires SAML authorization for this organization.

+

+ { + ssoRetryArmed.current = true + setAwaitingSso(true) + }} + > + Authorize this GitHub organization + + {awaitingSso + ? ' — waiting for you to return; cezar will retry automatically.' + : ' — cezar will retry when you return to this tab.'} +

+ + ) : ( +

+ {checkout.error instanceof Error ? checkout.error.message : 'could not clone that repository'}

- ) : null} + )}
) : null} From 44186c6905d858dc5a51a1b7f690ff702e1d7ae3 Mon Sep 17 00:00:00 2001 From: Piotr Chabros Date: Tue, 8 Sep 2026 23:59:25 +0200 Subject: [PATCH 3/5] fix SAML clones when gh prefers SSH --- packages/cezar/src/server/checkout.test.ts | 23 ++++++++++++++++++- packages/cezar/src/server/checkout.ts | 26 +++++++++++++++++----- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/packages/cezar/src/server/checkout.test.ts b/packages/cezar/src/server/checkout.test.ts index f9981ef8e..8101f7279 100644 --- a/packages/cezar/src/server/checkout.test.ts +++ b/packages/cezar/src/server/checkout.test.ts @@ -16,7 +16,14 @@ import { RunStore } from '../runs/store.ts'; import type { RunManager } from '../workflows/run.ts'; import { mergeWriteWorkspaceConfig } from '../workspace/config.ts'; import { clearProjectProbeCache, registerProject } from '../workspace/projects.ts'; -import { checkoutRepo, cleanupCheckout, isValidCheckoutName, parseRepoRef, type CloneRunner } from './checkout.ts'; +import { + checkoutRepo, + cleanupCheckout, + ghCloneArgs, + isValidCheckoutName, + parseRepoRef, + type CloneRunner, +} from './checkout.ts'; import { apiRequest } from './loopback-request.testkit.ts'; import { WorkspaceEventBus, @@ -54,6 +61,7 @@ describe('checkout — repo reference parsing', () => { owner: 'open-mercato', repo: 'cezar', slug: 'open-mercato/cezar', + cloneUrl: 'https://github.com/open-mercato/cezar.git', }); } }); @@ -83,6 +91,19 @@ describe('checkout — repo reference parsing', () => { expect(isValidCheckoutName(name), name).toBe(false); } }); + + it('forces the validated HTTPS URL so a global SSH preference cannot bypass the OAuth grant', () => { + const ref = parseRepoRef('git@github.com:open-mercato/cezar.git'); + expect(ref).not.toBeNull(); + expect(ghCloneArgs(ref!, '/checkouts/cezar')).toEqual([ + 'repo', + 'clone', + 'https://github.com/open-mercato/cezar.git', + '/checkouts/cezar', + '--', + '--progress', + ]); + }); }); describe('checkout — the cleanup guard', () => { diff --git a/packages/cezar/src/server/checkout.ts b/packages/cezar/src/server/checkout.ts index c216f7791..44b9c8423 100644 --- a/packages/cezar/src/server/checkout.ts +++ b/packages/cezar/src/server/checkout.ts @@ -63,9 +63,14 @@ export type CheckoutResult = { ok: true; target: string; name: string } | Checko export interface RepoRef { owner: string; repo: string; - /** What `gh repo clone` is handed — always the normalized `owner/repo`, so a - * URL spelling can never smuggle flags or a different host past `gh`. */ + /** Normalized identity used in messages and dry-run output. */ slug: string; + /** What `gh repo clone` is handed. Always reconstructed from validated + * segments rather than preserving user input. Forcing HTTPS is load-bearing: + * a machine configured with `gh config set git_protocol ssh` may have an + * OAuth token authorized for an organization's SAML policy while its SSH key + * is not. Passing only `owner/repo` silently selects that rejected key. */ + cloneUrl: string; } /** `owner` and `repo` as GitHub itself allows them: alphanumerics, `-`, `_`, @@ -103,7 +108,12 @@ export function parseRepoRef(input: string): RepoRef | null { if (parts.length !== 2) return null; const [owner, repo] = parts; if (!owner || !repo || !NAME_SEGMENT.test(owner) || !NAME_SEGMENT.test(repo)) return null; - return { owner, repo, slug: `${owner}/${repo}` }; + return { + owner, + repo, + slug: `${owner}/${repo}`, + cloneUrl: `https://github.com/${owner}/${repo}.git`, + }; } /** @@ -170,8 +180,14 @@ export type CloneRunner = ( signal: AbortSignal | undefined, ) => Promise<{ ok: true } | { ok: false; error: string; notFound?: boolean }>; +/** Kept pure so the SAML-safe transport choice is pinned without spawning a + * real GitHub process in the unit suite. */ +export function ghCloneArgs(ref: RepoRef, dir: string): string[] { + return ['repo', 'clone', ref.cloneUrl, dir, '--', '--progress']; +} + /** - * `gh repo clone -- --progress`. + * `gh repo clone -- --progress`. * * `spawn`, not `execFile`, because the whole point of this route is that the * dialog sees progress while it happens: `git clone --progress` writes its @@ -181,7 +197,7 @@ export type CloneRunner = ( */ export const ghCloneRunner: CloneRunner = (ref, dir, onLine, signal) => new Promise((resolvePromise) => { - const child = spawn('gh', ['repo', 'clone', ref.slug, dir, '--', '--progress'], { + const child = spawn('gh', ghCloneArgs(ref, dir), { stdio: ['ignore', 'pipe', 'pipe'], timeout: CLONE_TIMEOUT_MS, // No inherited stdin and `GH_PROMPT_DISABLED`: an unauthenticated `gh` From d8113bab8ff646be1fa897dd05bd364e41a68b26 Mon Sep 17 00:00:00 2001 From: Piotr Chabros Date: Mon, 14 Sep 2026 16:41:30 +0200 Subject: [PATCH 4/5] cezar autosave (run finalize) --- packages/cezar/src/server/checkout.ts | 49 +++++++++++++++++-- packages/cezar/src/server/forge/github.ts | 12 ++++- .../src/components/clone-project-dialog.tsx | 21 +++++++- 3 files changed, 76 insertions(+), 6 deletions(-) diff --git a/packages/cezar/src/server/checkout.ts b/packages/cezar/src/server/checkout.ts index 44b9c8423..aa50cf86c 100644 --- a/packages/cezar/src/server/checkout.ts +++ b/packages/cezar/src/server/checkout.ts @@ -1,4 +1,4 @@ -import { spawn } from 'node:child_process'; +import { execFile, spawn } from 'node:child_process'; import { lstat, mkdir, realpath, rm, writeFile } from 'node:fs/promises'; import { dirname, join, resolve } from 'node:path'; @@ -69,7 +69,9 @@ export interface RepoRef { * segments rather than preserving user input. Forcing HTTPS is load-bearing: * a machine configured with `gh config set git_protocol ssh` may have an * OAuth token authorized for an organization's SAML policy while its SSH key - * is not. Passing only `owner/repo` silently selects that rejected key. */ + * is not. Passing only `owner/repo` silently selects that rejected key. + * The resulting HTTPS `origin` needs a credential path of its own after the + * clone — see `persistGhCredentialHelper`. */ cloneUrl: string; } @@ -186,6 +188,41 @@ export function ghCloneArgs(ref: RepoRef, dir: string): string[] { return ['repo', 'clone', ref.cloneUrl, dir, '--', '--progress']; } +/** How long the post-clone config write may take. It is a local file edit; a + * `git` that has not answered in ten seconds is not going to. */ +const CONFIG_TIMEOUT_MS = 10_000; + +/** + * The second half of forcing HTTPS — and the half that is easy to miss. + * + * `gh repo clone` authenticates by injecting its credential helper *for the + * clone command only* (`git -c credential.https://github.com.helper=… clone …`); + * it does not persist into the repository it leaves behind. So a clone that + * `ghCloneArgs` pinned to HTTPS lands a checkout with a bare HTTPS `origin` and + * no credential path of its own, falling back on a GLOBAL helper that a user + * who answered `gh auth login` with SSH never had installed. cezar pushes task + * branches with raw `git` (`forge/github.ts`), so that user's first "Create PR" + * would block on a credential prompt until the push timeout — the SAML users + * this flow exists for are exactly that population. + * + * Writing the helper into the new repo's LOCAL config persists what the clone + * borrowed. It is the same line `gh auth setup-git` writes globally for HTTPS + * users, scoped to the one repository we just created. + */ +export function ghCredentialArgs(dir: string): string[] { + return ['-C', dir, 'config', '--local', 'credential.https://github.com.helper', '!gh auth git-credential']; +} + +/** Best effort by design: the clone already succeeded and the repository is on + * disk, so a failed config write is reported nowhere and fails nothing. The + * worst case is the pre-existing behaviour (git falls back on the global + * helper), never a lost checkout. */ +export function persistGhCredentialHelper(dir: string): Promise { + return new Promise((resolvePromise) => { + execFile('git', ghCredentialArgs(dir), { timeout: CONFIG_TIMEOUT_MS }, (err) => resolvePromise(!err)); + }); +} + /** * `gh repo clone -- --progress`. * @@ -253,7 +290,13 @@ export const ghCloneRunner: CloneRunner = (ref, dir, onLine, signal) => }); child.on('close', (code) => { signal?.removeEventListener('abort', onAbort); - if (code === 0) return finish({ ok: true }); + // A successful clone is not done until the checkout can authenticate on + // its own — see `persistGhCredentialHelper`. It never fails the clone, so + // the result is the same either way. + if (code === 0) { + void persistGhCredentialHelper(dir).then(() => finish({ ok: true })); + return; + } // The tail of gh/git's own output IS the error message — `gh` writes // "could not find repository", "authentication required" and the network // errors itself, and paraphrasing them would only lose detail. diff --git a/packages/cezar/src/server/forge/github.ts b/packages/cezar/src/server/forge/github.ts index 8533cc320..26a189c94 100644 --- a/packages/cezar/src/server/forge/github.ts +++ b/packages/cezar/src/server/forge/github.ts @@ -2547,7 +2547,17 @@ function execTool(args: string[], cwd: string, bin: string, timeoutMs = 30_000): execFile( bin, args, - { cwd, timeout: timeoutMs, maxBuffer: 4 * 1024 * 1024, encoding: 'utf8' }, + { + cwd, + timeout: timeoutMs, + maxBuffer: 4 * 1024 * 1024, + encoding: 'utf8', + // Nobody can answer a prompt here: git opens /dev/tty directly, so + // piped stdio is not enough to stop it. Without this a `git push` that + // cannot authenticate hangs for the whole timeout and surfaces as a + // blank one-minute stall instead of git's own "could not read Username". + env: { ...process.env, GIT_TERMINAL_PROMPT: '0' }, + }, (err, stdout, stderr) => resolve({ ok: !err, diff --git a/packages/web/src/components/clone-project-dialog.tsx b/packages/web/src/components/clone-project-dialog.tsx index e88cc3cad..2e0c3f142 100644 --- a/packages/web/src/components/clone-project-dialog.tsx +++ b/packages/web/src/components/clone-project-dialog.tsx @@ -26,13 +26,30 @@ import { Label } from '@/components/ui/label' * Keep this deliberately narrower than "find a URL": clone errors include * output influenced by the remote, and rendering arbitrary output as a link * would make the local cockpit an excellent phishing surface. + * + * Anchored at the END too, which matters as much as the `https://github.com/orgs/` + * prefix: the token charset cannot spell `%2F` or `=` padding, and an unanchored + * match would silently CUT such a token short and hand back a link + * indistinguishable from a good one. Refusing is the better failure — the caller + * then shows the raw error, which still contains the real URL to open by hand. */ export function githubSsoUrl(error: unknown): string | null { if (!(error instanceof Error)) return null const match = error.message.match( - /https:\/\/github\.com\/orgs\/[A-Za-z0-9._-]+\/sso\?authorization_request=[A-Za-z0-9._~-]+/, + /https:\/\/github\.com\/orgs\/[A-Za-z0-9._-]+\/sso\?authorization_request=[A-Za-z0-9._~-]+(?![^\s'"<>])/, ) - return match?.[0] ?? null + // `.` is in the charset (it has to be — hostnames above, tokens below), so a + // URL ending a sentence swallows the full stop. GitHub's nonce never ends in + // one, so trailing dots are punctuation, not payload. + return match ? match[0].replace(/\.+$/, '') : null +} + +/** The raw `gh` text, minus the one part of it that is a credential-shaped + * nonce. Shown alongside the SSO link so the message a user pastes into a bug + * report is never lost, without putting `authorization_request=` in the DOM as + * anything but the link's own `href`. */ +export function redactSsoToken(message: string): string { + return message.replace(/(authorization_request=)[A-Za-z0-9._~-]+/g, '$1…') } /** From 90738b4704ce0297e103f9acbd05b345744eaccd Mon Sep 17 00:00:00 2001 From: Piotr Chabros Date: Mon, 14 Sep 2026 17:22:02 +0200 Subject: [PATCH 5/5] fix(clone): complete GitHub SAML review fixes Persist GitHub HTTPS credentials for later pushes and preserve SSO error details. Reject truncated authorization links and cover visibility, focus, and manual retries. Validation: typecheck, 51 focused tests, 36 unit checks, build, and 16 package tests pass. Full suite: 6409 passed, 1 skipped; five workflow cleanup ENOENT rejections also reproduce on unchanged PR head 44186c69. Nine new regression cases fail without the fixes. --- packages/cezar/src/server/checkout.test.ts | 34 +++++++++++- packages/cezar/src/server/checkout.ts | 55 ++++++------------- .../server/forge/draft-pr-autosave.test.ts | 18 ++++++ .../components/clone-project-dialog.test.tsx | 42 +++++++++++--- .../src/components/clone-project-dialog.tsx | 33 +++++------ 5 files changed, 119 insertions(+), 63 deletions(-) diff --git a/packages/cezar/src/server/checkout.test.ts b/packages/cezar/src/server/checkout.test.ts index 8101f7279..4e13cefd6 100644 --- a/packages/cezar/src/server/checkout.test.ts +++ b/packages/cezar/src/server/checkout.test.ts @@ -1,3 +1,4 @@ +import { execFileSync } from 'node:child_process'; import { existsSync, mkdirSync, @@ -11,7 +12,7 @@ import { import { mkdir, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { RunStore } from '../runs/store.ts'; import type { RunManager } from '../workflows/run.ts'; import { mergeWriteWorkspaceConfig } from '../workspace/config.ts'; @@ -20,6 +21,7 @@ import { checkoutRepo, cleanupCheckout, ghCloneArgs, + ghCloneRunner, isValidCheckoutName, parseRepoRef, type CloneRunner, @@ -92,6 +94,9 @@ describe('checkout — repo reference parsing', () => { } }); +}); + +describe('checkout — GitHub transport', () => { it('forces the validated HTTPS URL so a global SSH preference cannot bypass the OAuth grant', () => { const ref = parseRepoRef('git@github.com:open-mercato/cezar.git'); expect(ref).not.toBeNull(); @@ -106,6 +111,33 @@ describe('checkout — repo reference parsing', () => { }); }); +describe('checkout — persisted GitHub credentials', () => { + it('leaves HTTPS origin and a local helper usable from task worktrees', async () => { + const root = mkdtempSync(join(realpathSync(tmpdir()), 'cez-credentials-')); + const bin = join(root, 'bin'); + const repo = join(root, 'repo'); + mkdirSync(bin); + // Substitute only gh: the runner and post-clone git configuration are real. + writeFileSync(join(bin, 'gh'), '#!/bin/sh\ngit init -q "$4" && git -C "$4" remote add origin "$3"\n', { mode: 0o755 }); + vi.stubEnv('PATH', `${bin}:${process.env.PATH}`); + vi.stubEnv('GIT_CONFIG_GLOBAL', '/dev/null'); + vi.stubEnv('GIT_CONFIG_NOSYSTEM', '1'); + try { + expect(await ghCloneRunner(parseRepoRef('owner/repo')!, repo, () => {}, undefined)).toEqual({ ok: true }); + const git = (...args: string[]) => execFileSync('git', ['-C', repo, ...args], { encoding: 'utf8' }); + expect(git('remote', 'get-url', 'origin').trim()).toBe('https://github.com/owner/repo.git'); + expect(git('config', '--local', '--get-all', 'credential.https://github.com.helper')).toBe('\n!gh auth git-credential\n'); + git('-c', 'user.name=Test', '-c', 'user.email=test@example.com', 'commit', '--allow-empty', '-qm', 'initial'); + const worktree = join(root, 'task'); + git('worktree', 'add', '-qb', 'task', worktree); + expect(execFileSync('git', ['-C', worktree, 'config', '--get-all', 'credential.https://github.com.helper'], { encoding: 'utf8' })).toBe('\n!gh auth git-credential\n'); + } finally { + vi.unstubAllEnvs(); + rmSync(root, { recursive: true, force: true }); + } + }); +}); + describe('checkout — the cleanup guard', () => { let root: string; diff --git a/packages/cezar/src/server/checkout.ts b/packages/cezar/src/server/checkout.ts index aa50cf86c..a997cd21a 100644 --- a/packages/cezar/src/server/checkout.ts +++ b/packages/cezar/src/server/checkout.ts @@ -188,39 +188,21 @@ export function ghCloneArgs(ref: RepoRef, dir: string): string[] { return ['repo', 'clone', ref.cloneUrl, dir, '--', '--progress']; } -/** How long the post-clone config write may take. It is a local file edit; a - * `git` that has not answered in ten seconds is not going to. */ -const CONFIG_TIMEOUT_MS = 10_000; - -/** - * The second half of forcing HTTPS — and the half that is easy to miss. - * - * `gh repo clone` authenticates by injecting its credential helper *for the - * clone command only* (`git -c credential.https://github.com.helper=… clone …`); - * it does not persist into the repository it leaves behind. So a clone that - * `ghCloneArgs` pinned to HTTPS lands a checkout with a bare HTTPS `origin` and - * no credential path of its own, falling back on a GLOBAL helper that a user - * who answered `gh auth login` with SSH never had installed. cezar pushes task - * branches with raw `git` (`forge/github.ts`), so that user's first "Create PR" - * would block on a credential prompt until the push timeout — the SAML users - * this flow exists for are exactly that population. - * - * Writing the helper into the new repo's LOCAL config persists what the clone - * borrowed. It is the same line `gh auth setup-git` writes globally for HTTPS - * users, scoped to the one repository we just created. - */ -export function ghCredentialArgs(dir: string): string[] { - return ['-C', dir, 'config', '--local', 'credential.https://github.com.helper', '!gh auth git-credential']; -} - -/** Best effort by design: the clone already succeeded and the repository is on - * disk, so a failed config write is reported nowhere and fails nothing. The - * worst case is the pre-existing behaviour (git falls back on the global - * helper), never a lost checkout. */ -export function persistGhCredentialHelper(dir: string): Promise { - return new Promise((resolvePromise) => { - execFile('git', ghCredentialArgs(dir), { timeout: CONFIG_TIMEOUT_MS }, (err) => resolvePromise(!err)); - }); +/** PR #968: gh injects credentials only for the clone command. Persist the + * helper locally so subsequent raw git pushes (including task worktrees) use + * the same OAuth grant. Reset inherited helpers first, as gh setup-git does. */ +async function persistGhCredentialHelper(dir: string): Promise { + for (const args of [ + ['--replace-all', 'credential.https://github.com.helper', ''], + ['--add', 'credential.https://github.com.helper', '!gh auth git-credential'], + ]) { + const ok = await new Promise((resolvePromise) => { + execFile('git', ['-C', dir, 'config', '--local', ...args], + { timeout: 10_000 }, (err) => resolvePromise(!err)); + }); + if (!ok) return false; + } + return true; } /** @@ -290,11 +272,10 @@ export const ghCloneRunner: CloneRunner = (ref, dir, onLine, signal) => }); child.on('close', (code) => { signal?.removeEventListener('abort', onAbort); - // A successful clone is not done until the checkout can authenticate on - // its own — see `persistGhCredentialHelper`. It never fails the clone, so - // the result is the same either way. if (code === 0) { - void persistGhCredentialHelper(dir).then(() => finish({ ok: true })); + void persistGhCredentialHelper(dir).then((ok) => finish(ok + ? { ok: true } + : { ok: false, error: 'Could not configure GitHub credentials for the checkout. Check directory permissions and retry.' })); return; } // The tail of gh/git's own output IS the error message — `gh` writes diff --git a/packages/cezar/src/server/forge/draft-pr-autosave.test.ts b/packages/cezar/src/server/forge/draft-pr-autosave.test.ts index 8772ca8ed..7175390ca 100644 --- a/packages/cezar/src/server/forge/draft-pr-autosave.test.ts +++ b/packages/cezar/src/server/forge/draft-pr-autosave.test.ts @@ -44,6 +44,24 @@ describe('createDraftPr pre-PR autosave (#471 follow-up)', () => { rmSync(repo, { recursive: true, force: true }); }); + it('disables terminal credential prompts for raw git pushes', async () => { + vi.stubEnv('CEZ_DRY_RUN', '0'); + vi.stubEnv('GIT_TERMINAL_PROMPT', '1'); + const remote = mkdtempSync(join(tmpdir(), 'cez-push-remote-')); + try { + await run('git', ['init', '--bare', '-q', remote]); + await git(['remote', 'add', 'origin', remote]); + await git(['branch', 'cez/abc123']); + writeFileSync(join(repo, '.git', 'hooks', 'pre-push'), + '#!/bin/sh\necho "prompt-setting=$GIT_TERMINAL_PROMPT" >&2\nexit 1\n', { mode: 0o755 }); + const outcome = await createDraftPr(input()); + expect(outcome.ok).toBe(false); + expect(outcome.ok === false && outcome.error).toContain('prompt-setting=0'); + } finally { + rmSync(remote, { recursive: true, force: true }); + } + }); + it('refuses to publish a worktree holding conflict markers', async () => { writeFileSync( join(repo, 'a.txt'), diff --git a/packages/web/src/components/clone-project-dialog.test.tsx b/packages/web/src/components/clone-project-dialog.test.tsx index b8d0ee2c8..1d3aabb42 100644 --- a/packages/web/src/components/clone-project-dialog.test.tsx +++ b/packages/web/src/components/clone-project-dialog.test.tsx @@ -5,7 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { createQueryClient } from '@/api/query-client' import type { ProjectListEntry } from '@open-mercato/cezar-api-client' -import { CloneProjectDialog } from '@/components/clone-project-dialog' +import { CloneProjectDialog, githubSsoUrl } from '@/components/clone-project-dialog' /** * The clone-from-GitHub dialog (multi-project spec, "Add project" option B / step 4.3). @@ -35,6 +35,7 @@ beforeEach(() => { }) afterEach(() => { + vi.restoreAllMocks() cleanup() fetchMock.mockReset() emitWorkspaceEvent = null @@ -190,7 +191,7 @@ describe('CloneProjectDialog', () => { expect(cloneButton().disabled).toBe(false) }) - it('keeps GitHub SAML authorization compact and retries when the user returns', async () => { + it.each(['focus', 'visibility', 'manual'])('keeps SAML details and retries once via %s', async (mode) => { const ssoUrl = 'https://github.com/orgs/Bug-Bounty-Switzerland/sso?authorization_request=AHBV4IUOPSOOJDNNTYUU4ALKUCFKNA5P' let attempts = 0 @@ -218,13 +219,26 @@ describe('CloneProjectDialog', () => { expect(link.href).toBe(ssoUrl) expect(link.target).toBe('_blank') expect(link.rel).toBe('noreferrer') - expect(slot('clone-error')?.textContent).toContain('cezar will retry when you return') - expect(slot('clone-error')?.textContent).not.toContain('authorization_request=') + expect(slot('clone-error')?.textContent).toContain('return to this tab to retry') + expect(slot('clone-error')?.querySelector('details')?.textContent).toContain(ssoUrl) + expect(slot('clone-error')?.querySelector('details')?.open).toBe(false) expect(cloneButton().textContent).toBe('Retry clone') - fireEvent.click(link) - expect(slot('clone-error')?.textContent).toContain('waiting for you to return') - fireEvent.focus(window) + fireEvent.click(link, { ctrlKey: mode === 'manual' }) + expect(slot('clone-error')?.textContent).toContain('or choose Retry clone') + if (mode === 'visibility') { + const visibility = vi.spyOn(document, 'visibilityState', 'get').mockReturnValue('hidden') + fireEvent(document, new Event('visibilitychange')) + expect(posted).toHaveLength(1) + visibility.mockReturnValue('visible') + fireEvent(document, new Event('visibilitychange')) + fireEvent.focus(window) + } else if (mode === 'manual') { + expect(posted).toHaveLength(1) + fireEvent.click(cloneButton()) + } else { + fireEvent.focus(window) + } await waitFor(() => expect(posted).toHaveLength(2)) await waitFor(() => expect(onOpenChange).toHaveBeenCalledWith(false)) @@ -261,3 +275,17 @@ describe('CloneProjectDialog', () => { await waitFor(() => expect(cloneButton().disabled).toBe(false)) }) }) + + +describe('githubSsoUrl', () => { + const url = 'https://github.com/orgs/Acme/sso?authorization_request=ABC' + it.each(['==', '%2FDEF', '&extra=1', ')'])('rejects a partial token before %s', (suffix) => { + expect(githubSsoUrl(new Error(url + suffix))).toBeNull() + }) + it.each(['', '\nnext line', '"', "'"])('accepts a complete URL before %s', (suffix) => { + expect(githubSsoUrl(new Error(url + suffix))).toBe(url) + }) + it.each(['https://github.com.evil.example', 'http://github.com', 'https://evil.example@github.com'])('rejects %s', (host) => { + expect(githubSsoUrl(new Error(url.replace('https://github.com', host)))).toBeNull() + }) +}) diff --git a/packages/web/src/components/clone-project-dialog.tsx b/packages/web/src/components/clone-project-dialog.tsx index 2e0c3f142..27a4933b0 100644 --- a/packages/web/src/components/clone-project-dialog.tsx +++ b/packages/web/src/components/clone-project-dialog.tsx @@ -38,18 +38,7 @@ export function githubSsoUrl(error: unknown): string | null { const match = error.message.match( /https:\/\/github\.com\/orgs\/[A-Za-z0-9._-]+\/sso\?authorization_request=[A-Za-z0-9._~-]+(?![^\s'"<>])/, ) - // `.` is in the charset (it has to be — hostnames above, tokens below), so a - // URL ending a sentence swallows the full stop. GitHub's nonce never ends in - // one, so trailing dots are punctuation, not payload. - return match ? match[0].replace(/\.+$/, '') : null -} - -/** The raw `gh` text, minus the one part of it that is a credential-shaped - * nonce. Shown alongside the SSO link so the message a user pastes into a bug - * report is never lost, without putting `authorization_request=` in the DOM as - * anything but the link's own `href`. */ -export function redactSsoToken(message: string): string { - return message.replace(/(authorization_request=)[A-Za-z0-9._~-]+/g, '$1…') + return match?.[0] ?? null } /** @@ -69,7 +58,8 @@ export function redactSsoToken(message: string): string { * Errors are shown verbatim (`{ error }`): a clone fails for reasons — `gh` missing, not * authenticated, no such repo, target folder exists, DNS down — that only the server can name, * and paraphrasing them into "could not clone" is exactly the silent-spinner failure this - * dialog exists to avoid. + * dialog exists to avoid. SAML errors add an authorization link and keep the original + * message available under Error details. */ export function CloneProjectDialog({ open, @@ -85,6 +75,7 @@ export function CloneProjectDialog({ const ssoRetryArmed = useRef(false) const projects = useProjects() const checkout = useCheckoutProject() + const { mutate, isPending } = checkout const navigate = useNavigate() const ssoUrl = githubSsoUrl(checkout.error) @@ -123,11 +114,11 @@ export function CloneProjectDialog({ const target = effectiveName === '' ? '' : `${projectsDir.replace(/\/+$/, '')}/${effectiveName}` const clone = useCallback(() => { - if (url.trim() === '' || checkout.isPending) return + if (url.trim() === '' || isPending) return ssoRetryArmed.current = false setAwaitingSso(false) setProgress(null) - checkout.mutate( + mutate( { url: url.trim(), checkoutId, ...(name.trim() === '' ? {} : { name: name.trim() }) }, { onSuccess: ({ project }) => { @@ -138,7 +129,7 @@ export function CloneProjectDialog({ }, }, ) - }, [checkout, checkoutId, name, navigate, onOpenChange, url]) + }, [mutate, isPending, checkoutId, name, navigate, onOpenChange, url]) // Authorizing an existing OAuth token changes GitHub's server-side token // grant; it cannot wake the `gh repo clone` process that already exited. @@ -266,9 +257,15 @@ export function CloneProjectDialog({ Authorize this GitHub organization {awaitingSso - ? ' — waiting for you to return; cezar will retry automatically.' - : ' — cezar will retry when you return to this tab.'} + ? ' — return here after authorizing, or choose Retry clone.' + : ' — return to this tab to retry, or choose Retry clone.'}

+
+ Error details +

+ {checkout.error instanceof Error ? checkout.error.message : null} +

+
) : (