diff --git a/src/git-clone.ts b/src/git-clone.ts index 3523156e..369b6c83 100644 --- a/src/git-clone.ts +++ b/src/git-clone.ts @@ -479,14 +479,23 @@ export function gitNonInteractiveEnv(base: NodeJS.ProcessEnv = process.env): Nod // ─── Pure: output handling ─────────────────────────────────────────────────── +/** + * Redact any `scheme://user:secret@host` credential pair embedded in text — a + * remote URL stored with an inline token, or git stderr echoing such a URL + * back. Shared by the clone error path (`sanitizeGitOutput`) and the + * repository-status card fields (`web/repo-status.ts`). + */ +export function redactGitCredentials(text: string): string { + return text.replace(/([a-zA-Z][a-zA-Z0-9+.-]*:\/\/)[^/@\s]*:[^/@\s]*@/g, '$1***:***@'); +} + /** * Make git's stderr safe to show in the browser: strip ANSI/control bytes, * redact any `scheme://user:secret@host` that a credential helper echoed back, * and keep only the tail (the last lines are the ones that say why it failed). */ export function sanitizeGitOutput(text: string, maxBytes = MAX_STDERR_BYTES): string { - const redacted = text - .replace(/([a-zA-Z][a-zA-Z0-9+.-]*:\/\/)[^/@\s]*:[^/@\s]*@/g, '$1***:***@') + const redacted = redactGitCredentials(text) // eslint-disable-next-line no-control-regex -- deliberate: strip C0/C1 and DEL. .replace(/[\u0000-\u0008\u000b\u000c\u000e-\u001f\u007f-\u009f]/g, '') .trim(); diff --git a/src/web/repo-status.ts b/src/web/repo-status.ts index c4858675..2456a772 100644 --- a/src/web/repo-status.ts +++ b/src/web/repo-status.ts @@ -21,22 +21,39 @@ * Split PURE helpers (parsing + remote-set/role/compare-ref decisions, unit * tested) from the IO wrapper `getRepositoryStatus()` (touches git). * + * PERFORMANCE: every git invocation is ASYNC (`execFile`), never `execFileSync` + * — the fetch path can spend `2 × FETCH_TIMEOUT_MS` per remote on the network, + * and a synchronous version froze the whole event loop (SSE, PTY streaming) for + * up to a minute per request. The computation is additionally single-flight + * with a short TTL cache: concurrent requests share one in-flight promise, and + * a fresh-enough result is served without spawning git at all. + * + * SECURITY: remote URLs (and git stderr echoing them) can embed + * `scheme://user:token@host` credentials, so every `url`/`error` field is + * passed through `redactGitCredentials()` (shared with `git-clone.ts`) before + * it reaches a client. + * * @module web/repo-status */ -import { execFileSync } from 'node:child_process'; +import { execFile } from 'node:child_process'; import { createRequire } from 'node:module'; +import { promisify } from 'node:util'; import { EXEC_TIMEOUT_MS } from '../config/exec-timeout.js'; +import { gitNonInteractiveEnv, redactGitCredentials } from '../git-clone.js'; import { getInstallInfo } from './self-update.js'; import type { RepoIncomingCommit, RepoRemoteRole, RepoRemoteStatus, RepositoryStatusResult } from '../types/update.js'; const require = createRequire(import.meta.url); const { version: APP_VERSION } = require('../../package.json') as { version: string }; +const execFileAsync = promisify(execFile); /** Network/git timeout for the fetch path (longer than EXEC_TIMEOUT_MS — hits network). */ const FETCH_TIMEOUT_MS = 15_000; /** Max incoming commit subjects to list per remote. */ const MAX_INCOMING = 10; +/** Fresh-enough window for a cached status — repeated UI polls reuse it instead of re-running git. */ +const STATUS_CACHE_TTL_MS = 45_000; // ───────────────────────────────────────────────────────────────────────────── // PURE helpers (unit tested, no IO) @@ -144,6 +161,69 @@ function dedupe(names: string[]): string[] { return [...new Set(names)]; } +/** + * Extract the remote name from an `@{upstream}` short ref like `origin/master`. + * + * A ref with NO slash is a LOCAL-branch upstream (`git branch -u otherbranch`): + * `rev-parse --abbrev-ref @{upstream}` prints just the branch name, and there + * is no remote in it — the old `slice(0, indexOf('/'))` became `slice(0, -1)` + * there and yielded garbage like `maste`. Returns null for that case (treated + * as "no tracking remote"), and for empty/degenerate refs. + */ +export function parseTrackingRemote(trackingRef: string | null | undefined): string | null { + if (!trackingRef) return null; + const idx = trackingRef.indexOf('/'); + if (idx <= 0) return null; + return trackingRef.slice(0, idx); +} + +/** + * Redact embedded `scheme://user:secret@host` credentials from the fields of + * one remote's status that can carry them: the remote URL itself, and the + * error string (which may include git stderr echoing that URL back). + */ +export function redactRemoteStatus(status: RepoRemoteStatus): RepoRemoteStatus { + const out: RepoRemoteStatus = { ...status, url: redactGitCredentials(status.url) }; + if (out.error !== undefined) out.error = redactGitCredentials(out.error); + return out; +} + +/** + * Single-flight TTL cache around an async compute: concurrent callers await + * the SAME in-flight promise, and a result younger than `ttlMs` is returned + * without recomputing. A rejected compute is never cached, so the next call + * retries. `now` is injectable for deterministic TTL tests. + */ +export function createSingleFlightCache( + ttlMs: number, + compute: () => Promise +): { get(now?: number): Promise } { + let cachedAt = 0; + let cachedValue: T | undefined; + let hasValue = false; + let inFlight: Promise | null = null; + return { + get(now = Date.now()): Promise { + if (hasValue && now - cachedAt < ttlMs) return Promise.resolve(cachedValue as T); + if (inFlight) return inFlight; + inFlight = compute().then( + (value) => { + cachedValue = value; + hasValue = true; + cachedAt = Date.now(); + inFlight = null; + return value; + }, + (err: unknown) => { + inFlight = null; + throw err; + } + ); + return inFlight; + }, + }; +} + // ───────────────────────────────────────────────────────────────────────────── // IO wrapper // ───────────────────────────────────────────────────────────────────────────── @@ -156,20 +236,18 @@ interface GitResult { /** * Run git non-interactively (no credential or SSH prompts — a missing key/cred - * fails fast instead of hanging). Returns captured stdout/stderr and ok flag. + * fails fast instead of hanging; env via the shared `gitNonInteractiveEnv()`, + * which also closes the askpass/GCM/DISPLAY prompt paths). ASYNC on purpose: + * the fetch path hits the network for up to `FETCH_TIMEOUT_MS`, and a sync + * spawn would block the event loop for the whole wait. */ -function runGit(args: string[], cwd: string, timeout = EXEC_TIMEOUT_MS): GitResult { +async function runGit(args: string[], cwd: string, timeout = EXEC_TIMEOUT_MS): Promise { try { - const stdout = execFileSync('git', args, { + const { stdout } = await execFileAsync('git', args, { cwd, encoding: 'utf-8', timeout, - stdio: ['ignore', 'pipe', 'pipe'], - env: { - ...process.env, - GIT_TERMINAL_PROMPT: '0', - GIT_SSH_COMMAND: process.env.GIT_SSH_COMMAND ?? 'ssh -oBatchMode=yes', - }, + env: gitNonInteractiveEnv(), }); return { ok: true, stdout: stdout.trim(), stderr: '' }; } catch (err: unknown) { @@ -193,14 +271,14 @@ function firstLine(s: string): string { } /** Compute ahead/behind + incoming commits for one already-selected remote. */ -function statusForRemote( +async function statusForRemote( dir: string, name: string, trackingRemote: string, trackingRef: string | null -): RepoRemoteStatus { +): Promise { const role = roleForRemote(name, trackingRemote); - const url = runGit(['remote', 'get-url', name], dir).stdout || ''; + const url = (await runGit(['remote', 'get-url', name], dir)).stdout || ''; const base: RepoRemoteStatus = { name, url, role, compareRef: '', ahead: 0, behind: 0, incoming: [] }; // SECURITY: the remote name reaches git as a positional; reject a `-` prefix @@ -215,7 +293,7 @@ function statusForRemote( // e.g. trackingRef = "bitbucket/local" → branch = "local" branch = trackingRef.slice(name.length + 1) || null; } else { - const symref = runGit(['ls-remote', '--symref', name, 'HEAD'], dir, FETCH_TIMEOUT_MS); + const symref = await runGit(['ls-remote', '--symref', name, 'HEAD'], dir, FETCH_TIMEOUT_MS); branch = symref.ok ? parseSymrefDefaultBranch(symref.stdout) : null; if (!branch && !symref.ok) { return { ...base, error: `Could not reach ${name}: ${firstLine(symref.stderr)}` }; @@ -232,30 +310,26 @@ function statusForRemote( // Read-only fetch of just that ref so the local rev-list/log can see it. // `--` ends option parsing so neither `name` nor `branch` can be read as a flag. - const fetched = runGit(['fetch', '--no-tags', name, '--', branch], dir, FETCH_TIMEOUT_MS); + const fetched = await runGit(['fetch', '--no-tags', name, '--', branch], dir, FETCH_TIMEOUT_MS); if (!fetched.ok) { return { ...base, compareRef, error: `Could not fetch ${compareRef}: ${firstLine(fetched.stderr)}` }; } - const counts = runGit(['rev-list', '--left-right', '--count', `HEAD...${compareRef}`], dir); + const counts = await runGit(['rev-list', '--left-right', '--count', `HEAD...${compareRef}`], dir); if (!counts.ok) { return { ...base, compareRef, error: `Could not compare against ${compareRef}: ${firstLine(counts.stderr)}` }; } const ab = parseAheadBehind(counts.stdout); if (!ab) return { ...base, compareRef, error: `Unexpected git output comparing ${compareRef}.` }; - const log = runGit(['log', '--oneline', '-n', String(MAX_INCOMING), `HEAD..${compareRef}`], dir); + const log = await runGit(['log', '--oneline', '-n', String(MAX_INCOMING), `HEAD..${compareRef}`], dir); const incoming = log.ok ? parseLogLines(log.stdout) : []; return { ...base, compareRef, ahead: ab.ahead, behind: ab.behind, incoming }; } -/** - * Inspect how the local checkout sits relative to the configured remotes. - * Each remote is fetched + compared independently; a single unreachable remote - * surfaces as that card's `error` and never fails the whole call. - */ -export function getRepositoryStatus(): RepositoryStatusResult { +/** The uncached computation behind `getRepositoryStatus()`. */ +async function computeRepositoryStatus(): Promise { const checkedAt = Date.now(); const info = getInstallInfo(); const base: RepositoryStatusResult = { @@ -271,10 +345,14 @@ export function getRepositoryStatus(): RepositoryStatusResult { const dir = info.installDir; // Tracking ref of the current branch, e.g. "bitbucket/local" (empty if none). - const trackingRef = runGit(['rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{upstream}'], dir).stdout || null; - const trackingRemote = trackingRef ? trackingRef.slice(0, trackingRef.indexOf('/')) || null : null; + const trackingRef = + (await runGit(['rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{upstream}'], dir)).stdout || null; + const trackingRemote = parseTrackingRemote(trackingRef); + // A local-branch upstream (no slash) carries no remote — drop the ref too, so + // nothing downstream can mistake a bare branch name for a remote-tracking ref. + const remoteTrackingRef = trackingRemote ? trackingRef : null; - const remotesOut = runGit(['remote'], dir); + const remotesOut = await runGit(['remote'], dir); const existingRemotes = remotesOut.ok ? remotesOut.stdout .split('\n') @@ -292,6 +370,28 @@ export function getRepositoryStatus(): RepositoryStatusResult { return { ...base, error: 'No comparable remotes found (set CODEMAN_UPDATE_REMOTES to choose).' }; } - const remotes = selected.map((name) => statusForRemote(dir, name, trackingRemote ?? '', trackingRef)); + // Sequential on purpose: concurrent `git fetch` in one repo can contend on + // ref locks, and the event loop no longer cares how long this takes. + const remotes: RepoRemoteStatus[] = []; + for (const name of selected) { + // SECURITY: redact `scheme://user:token@host` credentials from the URL and + // any error string (git stderr echoes the URL back) before they leave the server. + remotes.push(redactRemoteStatus(await statusForRemote(dir, name, trackingRemote ?? '', remoteTrackingRef))); + } return { ...base, remotes }; } + +const statusCache = createSingleFlightCache(STATUS_CACHE_TTL_MS, computeRepositoryStatus); + +/** + * Inspect how the local checkout sits relative to the configured remotes. + * Each remote is fetched + compared independently; a single unreachable remote + * surfaces as that card's `error` and never fails the whole call. + * + * Single-flight + TTL-cached: concurrent requests share one in-flight + * computation, and a result younger than `STATUS_CACHE_TTL_MS` is served + * without spawning git. + */ +export function getRepositoryStatus(): Promise { + return statusCache.get(); +} diff --git a/test/repo-status.test.ts b/test/repo-status.test.ts index 3308e254..2a8e3435 100644 --- a/test/repo-status.test.ts +++ b/test/repo-status.test.ts @@ -1,6 +1,8 @@ /** * @fileoverview Unit tests for the repository-status pure helpers: ahead/behind - * + log + symref parsing, env parsing, and the remote-set / role decisions. + * + log + symref parsing, env parsing, the remote-set / role / tracking-remote + * decisions, credential redaction of the returned fields, and the single-flight + * TTL cache that keeps the async status computation off the git hot path. * No IO, no git, no port — safe to run individually. * * npm test -- test/repo-status.test.ts @@ -12,10 +14,15 @@ import { parseLogLines, parseSymrefDefaultBranch, parseRemotesEnv, + parseTrackingRemote, resolveRemoteSet, roleForRemote, isSafeGitPositional, + redactRemoteStatus, + createSingleFlightCache, } from '../src/web/repo-status.js'; +import { redactGitCredentials } from '../src/git-clone.js'; +import type { RepoRemoteStatus } from '../src/types/update.js'; describe('parseAheadBehind', () => { it('parses tab-separated left/right counts as ahead/behind', () => { @@ -157,3 +164,118 @@ describe('roleForRemote', () => { expect(roleForRemote('fork', 'bitbucket')).toBe('other'); }); }); + +describe('parseTrackingRemote', () => { + it('extracts the remote name from a remote-tracking short ref', () => { + expect(parseTrackingRemote('origin/master')).toBe('origin'); + expect(parseTrackingRemote('bitbucket/feature/nested')).toBe('bitbucket'); + }); + it('returns null for a LOCAL-branch upstream (ref with no slash)', () => { + // `git branch -u otherbranch` makes @{upstream} a bare branch name; the old + // slice(0, indexOf('/')) turned "master" into "maste" here. + expect(parseTrackingRemote('master')).toBeNull(); + expect(parseTrackingRemote('main')).toBeNull(); + }); + it('returns null for null/empty/degenerate refs', () => { + expect(parseTrackingRemote(null)).toBeNull(); + expect(parseTrackingRemote(undefined)).toBeNull(); + expect(parseTrackingRemote('')).toBeNull(); + expect(parseTrackingRemote('/leading-slash')).toBeNull(); + }); +}); + +describe('credential redaction', () => { + it('redactGitCredentials masks scheme://user:secret@host pairs', () => { + expect(redactGitCredentials('https://user:ghp_token123@github.com/o/r.git')).toBe( + 'https://***:***@github.com/o/r.git' + ); + expect(redactGitCredentials('plain text, no url')).toBe('plain text, no url'); + expect(redactGitCredentials('https://github.com/o/r.git')).toBe('https://github.com/o/r.git'); + }); + + it('redactRemoteStatus masks the url field', () => { + const status: RepoRemoteStatus = { + name: 'origin', + url: 'https://alice:s3cret@example.com/repo.git', + role: 'upstream', + compareRef: 'origin/master', + ahead: 1, + behind: 2, + incoming: [{ sha: 'abc', subject: 'hi' }], + }; + const out = redactRemoteStatus(status); + expect(out.url).toBe('https://***:***@example.com/repo.git'); + // Everything else passes through untouched. + expect(out.name).toBe('origin'); + expect(out.compareRef).toBe('origin/master'); + expect(out.ahead).toBe(1); + expect(out.behind).toBe(2); + expect(out.incoming).toEqual([{ sha: 'abc', subject: 'hi' }]); + expect(out.error).toBeUndefined(); + }); + + it('redactRemoteStatus masks credentials echoed into the error string by git stderr', () => { + const status: RepoRemoteStatus = { + name: 'origin', + url: 'https://alice:s3cret@example.com/repo.git', + role: 'upstream', + compareRef: '', + ahead: 0, + behind: 0, + incoming: [], + error: "Could not reach origin: fatal: unable to access 'https://alice:s3cret@example.com/repo.git/'", + }; + const out = redactRemoteStatus(status); + expect(out.error).toBe("Could not reach origin: fatal: unable to access 'https://***:***@example.com/repo.git/'"); + expect(out.error).not.toContain('s3cret'); + expect(out.url).not.toContain('s3cret'); + }); +}); + +describe('createSingleFlightCache', () => { + it('shares one in-flight computation across concurrent callers', async () => { + let calls = 0; + let release!: (v: string) => void; + const cache = createSingleFlightCache(60_000, () => { + calls++; + return new Promise((resolve) => { + release = resolve; + }); + }); + const a = cache.get(); + const b = cache.get(); + release('result'); + expect(await a).toBe('result'); + expect(await b).toBe('result'); + expect(calls).toBe(1); + }); + + it('serves a fresh-enough cached result without recomputing', async () => { + let calls = 0; + const cache = createSingleFlightCache(60_000, async () => ++calls); + expect(await cache.get()).toBe(1); + expect(await cache.get()).toBe(1); + expect(calls).toBe(1); + }); + + it('recomputes once the TTL has elapsed', async () => { + let calls = 0; + const cache = createSingleFlightCache(60_000, async () => ++calls); + expect(await cache.get()).toBe(1); + // Inject a "now" past the TTL instead of sleeping. + expect(await cache.get(Date.now() + 60_001)).toBe(2); + expect(calls).toBe(2); + }); + + it('does not cache a rejected computation — the next call retries', async () => { + let calls = 0; + const cache = createSingleFlightCache(60_000, async () => { + calls++; + if (calls === 1) throw new Error('boom'); + return 'ok'; + }); + await expect(cache.get()).rejects.toThrow('boom'); + expect(await cache.get()).toBe('ok'); + expect(calls).toBe(2); + }); +});