diff --git a/packages/cezar/src/server/checkout.test.ts b/packages/cezar/src/server/checkout.test.ts index f9981ef8e..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,12 +12,20 @@ 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'; import { clearProjectProbeCache, registerProject } from '../workspace/projects.ts'; -import { checkoutRepo, cleanupCheckout, isValidCheckoutName, parseRepoRef, type CloneRunner } from './checkout.ts'; +import { + checkoutRepo, + cleanupCheckout, + ghCloneArgs, + ghCloneRunner, + isValidCheckoutName, + parseRepoRef, + type CloneRunner, +} from './checkout.ts'; import { apiRequest } from './loopback-request.testkit.ts'; import { WorkspaceEventBus, @@ -54,6 +63,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 +93,49 @@ describe('checkout — repo reference parsing', () => { expect(isValidCheckoutName(name), name).toBe(false); } }); + +}); + +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(); + expect(ghCloneArgs(ref!, '/checkouts/cezar')).toEqual([ + 'repo', + 'clone', + 'https://github.com/open-mercato/cezar.git', + '/checkouts/cezar', + '--', + '--progress', + ]); + }); +}); + +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', () => { diff --git a/packages/cezar/src/server/checkout.ts b/packages/cezar/src/server/checkout.ts index c216f7791..a997cd21a 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'; @@ -63,9 +63,16 @@ 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. + * The resulting HTTPS `origin` needs a credential path of its own after the + * clone — see `persistGhCredentialHelper`. */ + cloneUrl: string; } /** `owner` and `repo` as GitHub itself allows them: alphanumerics, `-`, `_`, @@ -103,7 +110,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 +182,31 @@ 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']; +} + +/** 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; +} + /** - * `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 +216,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` @@ -237,7 +272,12 @@ export const ghCloneRunner: CloneRunner = (ref, dir, onLine, signal) => }); child.on('close', (code) => { signal?.removeEventListener('abort', onAbort); - if (code === 0) return finish({ ok: true }); + if (code === 0) { + 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 // "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/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/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.test.tsx b/packages/web/src/components/clone-project-dialog.test.tsx index ebd572aba..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,6 +191,71 @@ describe('CloneProjectDialog', () => { expect(cloneButton().disabled).toBe(false) }) + 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 + 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()) + + 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('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, { 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)) + }) + + 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(slot('clone-error')?.querySelector('p')?.className).toContain('break-all') + 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() @@ -209,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 5ca4c27ce..27a4933b0 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' @@ -17,6 +17,30 @@ 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. + * + * 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._~-]+(?![^\s'"<>])/, + ) + return match?.[0] ?? null +} + /** * "Add project → Clone from GitHub" (multi-project spec, "Add project" option B / step 4.3). * @@ -34,7 +58,8 @@ import { Label } from '@/components/ui/label' * 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, @@ -46,9 +71,13 @@ 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 { mutate, isPending } = checkout 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 @@ -84,10 +113,12 @@ export function CloneProjectDialog({ const projectsDir = projects.data?.projectsDir ?? '' const target = effectiveName === '' ? '' : `${projectsDir.replace(/\/+$/, '')}/${effectiveName}` - const clone = () => { - if (url.trim() === '' || checkout.isPending) return + const clone = useCallback(() => { + 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 }) => { @@ -98,11 +129,38 @@ export function CloneProjectDialog({ }, }, ) - } + }, [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. + // 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 @@ -180,9 +238,41 @@ export function CloneProjectDialog({ ) : null} {checkout.isError ? ( -

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

+
+ {ssoUrl ? ( + <> +

GitHub requires SAML authorization for this organization.

+

+ { + ssoRetryArmed.current = true + setAwaitingSso(true) + }} + > + Authorize this GitHub organization + + {awaitingSso + ? ' — 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} +

+
+ + ) : ( +

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

+ )} +
) : null} @@ -194,7 +284,7 @@ export function CloneProjectDialog({ disabled={url.trim() === '' || effectiveName === '' || checkout.isPending} onClick={clone} > - {checkout.isPending ? 'Cloning…' : 'Clone'} + {checkout.isPending ? 'Cloning…' : checkout.isError ? 'Retry clone' : 'Clone'}