From c55940e61eb5b7e809cdfcc419d6151b48646d8c Mon Sep 17 00:00:00 2001 From: Brendan Kellam Date: Thu, 13 Aug 2026 17:36:51 -0700 Subject: [PATCH 1/4] fix(worker): keep Git credentials out of process arguments --- packages/backend/src/git.test.ts | 273 +++++++++++++++++- packages/backend/src/git.ts | 160 +++++++--- .../backend/src/gitCredentialSession.test.ts | 262 +++++++++++++++++ packages/backend/src/gitCredentialSession.ts | 218 ++++++++++++++ packages/backend/src/repoCompileUtils.ts | 4 +- packages/backend/src/repoIndexManager.ts | 11 +- packages/backend/src/types.ts | 10 +- packages/backend/src/utils.test.ts | 44 ++- packages/backend/src/utils.ts | 121 +++----- 9 files changed, 961 insertions(+), 142 deletions(-) create mode 100644 packages/backend/src/gitCredentialSession.test.ts create mode 100644 packages/backend/src/gitCredentialSession.ts diff --git a/packages/backend/src/git.test.ts b/packages/backend/src/git.test.ts index c9c32cf57..29a8983c0 100644 --- a/packages/backend/src/git.test.ts +++ b/packages/backend/src/git.test.ts @@ -1,9 +1,11 @@ -import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { execFileSync, spawn } from "node:child_process"; +import { randomUUID } from "node:crypto"; +import { createServer, type Server } from "node:http"; +import { mkdtemp, readFile, readdir, rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { execFileSync } from "node:child_process"; import { afterEach, describe, expect, test } from "vitest"; -import { getBranches, getTags } from "./git.js"; +import { cloneRepository, fetchRepository, getBranches, getRemoteDefaultBranch, getTags } from "./git.js"; const runGit = ( repoPath: string, @@ -32,6 +34,154 @@ const createTempRepo = async () => { return repoPath; }; +const createAuthenticatedGitServer = async ({ + projectRoot, + username, + password, +}: { + projectRoot: string; + username: string; + password: string; +}) => { + const expectedAuthorization = `Basic ${Buffer.from(`${username}:${password}`).toString('base64')}`; + let authenticatedRequestCount = 0; + let unauthenticatedRequestCount = 0; + + const server = createServer((request, response) => { + if (request.headers.authorization !== expectedAuthorization) { + unauthenticatedRequestCount++; + response.writeHead(401, { + 'WWW-Authenticate': 'Basic realm="Sourcebot Git Test"', + }); + response.end(); + return; + } + + authenticatedRequestCount++; + const requestUrl = new URL(request.url ?? '/', 'http://127.0.0.1'); + const backend = spawn('git', ['http-backend'], { + env: { + ...process.env, + GIT_HTTP_EXPORT_ALL: '1', + GIT_PROJECT_ROOT: projectRoot, + PATH_INFO: requestUrl.pathname, + QUERY_STRING: requestUrl.searchParams.toString(), + REQUEST_METHOD: request.method ?? 'GET', + CONTENT_TYPE: request.headers['content-type'] ?? '', + CONTENT_LENGTH: request.headers['content-length'] ?? '', + REMOTE_USER: username, + SERVER_PROTOCOL: 'HTTP/1.1', + }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + let headerBuffer = Buffer.alloc(0); + let headersSent = false; + const stderr: Buffer[] = []; + + backend.stderr.on('data', (chunk: Buffer) => stderr.push(chunk)); + backend.stdout.on('data', (chunk: Buffer) => { + if (headersSent) { + response.write(chunk); + return; + } + + headerBuffer = Buffer.concat([headerBuffer, chunk]); + const crlfTerminatorIndex = headerBuffer.indexOf('\r\n\r\n'); + const lfTerminatorIndex = headerBuffer.indexOf('\n\n'); + const terminatorIndex = crlfTerminatorIndex >= 0 + ? crlfTerminatorIndex + : lfTerminatorIndex; + if (terminatorIndex < 0) { + return; + } + + const terminatorLength = crlfTerminatorIndex >= 0 ? 4 : 2; + const rawHeaders = headerBuffer.subarray(0, terminatorIndex).toString('utf8'); + const responseHeaders: Record = {}; + let statusCode = 200; + for (const line of rawHeaders.split(/\r?\n/)) { + const separatorIndex = line.indexOf(':'); + if (separatorIndex < 0) { + continue; + } + + const name = line.slice(0, separatorIndex).trim(); + const value = line.slice(separatorIndex + 1).trim(); + if (name.toLowerCase() === 'status') { + statusCode = Number.parseInt(value, 10); + } else { + responseHeaders[name] = value; + } + } + + response.writeHead(statusCode, responseHeaders); + headersSent = true; + response.write(headerBuffer.subarray(terminatorIndex + terminatorLength)); + headerBuffer = Buffer.alloc(0); + }); + backend.once('error', (error) => { + if (!response.headersSent) { + response.writeHead(500); + } + response.end(error.message); + }); + backend.once('close', (code) => { + if (!headersSent) { + response.writeHead(500); + response.end(Buffer.concat(stderr)); + return; + } + if (code !== 0) { + response.destroy(new Error(Buffer.concat(stderr).toString('utf8'))); + return; + } + response.end(); + }); + + request.pipe(backend.stdin); + }); + + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', () => resolve()); + }); + const address = server.address(); + if (!address || typeof address === 'string') { + throw new Error('Git test server did not bind to a TCP port'); + } + + return { + cloneUrl: `http://127.0.0.1:${address.port}/repo.git`, + getAuthenticatedRequestCount: () => authenticatedRequestCount, + getUnauthenticatedRequestCount: () => unauthenticatedRequestCount, + server, + }; +}; + +const closeServer = async (server: Server) => { + await new Promise((resolve, reject) => { + server.close((error) => error ? reject(error) : resolve()); + }); +}; + +const directoryContains = async (directory: string, value: string): Promise => { + const entries = await readdir(directory, { withFileTypes: true }); + for (const entry of entries) { + const path = join(directory, entry.name); + if (entry.isDirectory()) { + if (await directoryContains(path, value)) { + return true; + } + } else if (entry.isFile()) { + const contents = await readFile(path); + if (contents.includes(Buffer.from(value))) { + return true; + } + } + } + return false; +}; + const commitFile = async ({ repoPath, fileName, @@ -134,3 +284,120 @@ describe("git ref ordering", () => { ); }); }); + +describe('authenticated Git operations', () => { + const repoPaths: string[] = []; + + afterEach(async () => { + await Promise.all( + repoPaths + .splice(0) + .map((repoPath) => rm(repoPath, { recursive: true, force: true })), + ); + }); + + test('clone, fetch, and ls-remote authenticate without exposing the credential', async () => { + const sourcePath = await createTempRepo(); + repoPaths.push(sourcePath); + await commitFile({ + repoPath: sourcePath, + fileName: 'README.md', + content: 'initial\n', + message: 'initial commit', + timestamp: '2024-01-01T00:00:00Z', + }); + + const projectRoot = await mkdtemp(join(tmpdir(), 'sourcebot-git-http-root-')); + repoPaths.push(projectRoot); + const bareRepoPath = join(projectRoot, 'repo.git'); + runGit(projectRoot, ['clone', '--bare', sourcePath, bareRepoPath]); + + const username = 'sourcebot-test-user'; + const token = `sourcebot-test-token-${randomUUID()}`; + const gitServer = await createAuthenticatedGitServer({ + projectRoot, + username, + password: token, + }); + const clonePath = await mkdtemp(join(tmpdir(), 'sourcebot-git-auth-clone-')); + repoPaths.push(clonePath); + const tracePath = join(projectRoot, 'git-trace.json'); + const previousTrace = process.env.GIT_TRACE2_EVENT; + process.env.GIT_TRACE2_EVENT = tracePath; + let unauthenticatedRequestsBeforeProactiveAuth: number | undefined; + let proactiveAuthDefaultBranch: string | undefined; + + try { + await cloneRepository({ + cloneUrl: gitServer.cloneUrl, + credentials: { + username, + password: token, + }, + path: clonePath, + }); + + await commitFile({ + repoPath: sourcePath, + fileName: 'new.txt', + content: 'new commit\n', + message: 'new commit', + timestamp: '2024-01-02T00:00:00Z', + }); + runGit(sourcePath, ['push', bareRepoPath, 'main']); + + await fetchRepository({ + cloneUrl: gitServer.cloneUrl, + credentials: { + username, + password: token, + }, + path: clonePath, + }); + + unauthenticatedRequestsBeforeProactiveAuth = gitServer.getUnauthenticatedRequestCount(); + proactiveAuthDefaultBranch = await getRemoteDefaultBranch({ + path: clonePath, + cloneUrl: gitServer.cloneUrl, + credentials: { + username, + password: token, + proactiveAuth: 'basic', + }, + }); + } finally { + if (previousTrace === undefined) { + delete process.env.GIT_TRACE2_EVENT; + } else { + process.env.GIT_TRACE2_EVENT = previousTrace; + } + await closeServer(gitServer.server); + } + + const expectedHead = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: sourcePath, + encoding: 'utf8', + }).trim(); + const fetchedHead = execFileSync('git', ['rev-parse', 'refs/heads/main'], { + cwd: clonePath, + encoding: 'utf8', + }).trim(); + const repositoryConfig = execFileSync('git', ['config', '--local', '--list', '--show-origin'], { + cwd: clonePath, + encoding: 'utf8', + }); + const trace = await readFile(tracePath, 'utf8'); + + expect(fetchedHead).toBe(expectedHead); + expect(gitServer.getAuthenticatedRequestCount()).toBeGreaterThan(0); + expect(gitServer.getUnauthenticatedRequestCount()).toBeGreaterThan(0); + expect(proactiveAuthDefaultBranch).toBe('main'); + expect(gitServer.getUnauthenticatedRequestCount()).toBe(unauthenticatedRequestsBeforeProactiveAuth); + expect(repositoryConfig).not.toContain('remote.origin.url'); + expect(repositoryConfig).not.toContain('http.extraHeader'); + expect(repositoryConfig).not.toContain(token); + expect(trace).not.toContain(token); + expect(trace).not.toContain(Buffer.from(`${username}:${token}`).toString('base64')); + expect(await directoryContains(clonePath, token)).toBe(false); + }, 20_000); +}); diff --git a/packages/backend/src/git.ts b/packages/backend/src/git.ts index d827a5714..8d2615358 100644 --- a/packages/backend/src/git.ts +++ b/packages/backend/src/git.ts @@ -1,9 +1,11 @@ -import { env, createLogger } from "@sourcebot/shared"; +import { createLogger } from "@sourcebot/shared"; import { existsSync } from 'node:fs'; import { mkdir } from 'node:fs/promises'; import { dirname, resolve } from 'node:path'; import { CheckRepoActions, GitConfigScope, simpleGit, SimpleGitProgressEvent } from 'simple-git'; import { parseEnv } from '@simple-git/argv-parser'; +import { withGitCredentialSession } from './gitCredentialSession.js'; +import type { GitHttpCredentials } from './types.js'; type onProgressFn = (event: SimpleGitProgressEvent) => void; @@ -34,20 +36,29 @@ if (envVulnerabilities.length > 0) { * Creates a simple-git client that has it's working directory * set to the given path. */ -const createGitClientForPath = (path: string, onProgress?: onProgressFn, signal?: AbortSignal) => { +const createGitClientForPath = ( + path: string, + onProgress?: onProgressFn, + signal?: AbortSignal, + environment: NodeJS.ProcessEnv = {}, + additionalUnsafe: typeof unsafe = {}, +) => { if (!existsSync(path)) { throw new Error(`Path ${path} does not exist`); } const parentPath = resolve(dirname(path)); - const git = simpleGit({ progress: onProgress, abort: signal, - unsafe, + unsafe: { + ...unsafe, + ...additionalUnsafe, + }, }) .env({ ...process.env, + ...environment, /** * @note on some inside-baseball on why this is necessary: The specific * issue we saw was that a `git clone` would fail without throwing, and @@ -73,16 +84,56 @@ const createGitClientForPath = (path: string, onProgress?: onProgressFn, signal? return git; } +/** + * Runs a remote Git operation with a client scoped to an isolated credential session. + */ +const withRemoteGitClient = async ({ + path, + cloneUrl, + credentials, + onProgress, + signal, + operation, +}: { + path: string; + cloneUrl: string; + credentials?: GitHttpCredentials; + onProgress?: onProgressFn; + signal?: AbortSignal; + operation: (git: ReturnType) => Promise; +}): Promise => withGitCredentialSession({ + cloneUrl, + credentials, + signal, + operation: async (environment) => { + const credentialSessionUnsafe: typeof unsafe = credentials === undefined + ? {} + : { + allowUnsafeConfigEnvCount: true, + allowUnsafeCredentialHelper: true, + allowUnsafeAskPass: true, + }; + const git = createGitClientForPath( + path, + onProgress, + signal, + environment, + credentialSessionUnsafe, + ); + return operation(git); + }, +}); + export const cloneRepository = async ( { cloneUrl, - authHeader, + credentials, path, onProgress, signal, }: { cloneUrl: string, - authHeader?: string, + credentials?: GitHttpCredentials, path: string, onProgress?: onProgressFn signal?: AbortSignal @@ -91,14 +142,16 @@ export const cloneRepository = async ( try { await mkdir(path, { recursive: true }); - const git = createGitClientForPath(path, onProgress, signal); - - const cloneArgs = [ - "--bare", - ...(authHeader ? ["-c", `http.extraHeader=${authHeader}`] : []) - ]; - - await git.clone(cloneUrl, path, cloneArgs); + await withRemoteGitClient({ + path, + cloneUrl, + credentials, + onProgress, + signal, + operation: async (git) => { + await git.clone(cloneUrl, path, ['--bare']); + }, + }); await unsetGitConfig({ path, @@ -108,10 +161,7 @@ export const cloneRepository = async ( } catch (error: unknown) { const baseLog = `Failed to clone repository: ${path}`; - if (env.SOURCEBOT_LOG_LEVEL !== "debug") { - // Avoid printing the remote URL (that may contain credentials) to logs by default. - throw new Error(`${baseLog}. Set environment variable SOURCEBOT_LOG_LEVEL=debug to see the full error message.`); - } else if (error instanceof Error) { + if (error instanceof Error) { throw new Error(`${baseLog}. Reason: ${error.message}`); } else { throw new Error(`${baseLog}. Error: ${error}`); @@ -122,55 +172,55 @@ export const cloneRepository = async ( export const fetchRepository = async ( { cloneUrl, - authHeader, + credentials, path, onProgress, signal, }: { cloneUrl: string, - authHeader?: string, + credentials?: GitHttpCredentials, path: string, onProgress?: onProgressFn, signal?: AbortSignal } ) => { - const git = createGitClientForPath(path, onProgress, signal); try { - if (authHeader) { - await git.addConfig("http.extraHeader", authHeader); - } - - await git.fetch([ + await withRemoteGitClient({ + path, cloneUrl, - "+refs/heads/*:refs/heads/*", - "--prune", - "--progress", - ]); + credentials, + onProgress, + signal, + operation: async (git) => { + await git.fetch([ + cloneUrl, + "+refs/heads/*:refs/heads/*", + "--prune", + "--progress", + ]); + }, + }); // Update HEAD to match the remote's default branch. This handles the case where the remote's // default branch changes. const remoteDefaultBranch = await getRemoteDefaultBranch({ path, cloneUrl, + credentials, + signal, }); if (remoteDefaultBranch) { + const git = createGitClientForPath(path, onProgress, signal); await git.raw(['symbolic-ref', 'HEAD', `refs/heads/${remoteDefaultBranch}`]); } } catch (error: unknown) { const baseLog = `Failed to fetch repository: ${path}`; - if (env.SOURCEBOT_LOG_LEVEL !== "debug") { - // Avoid printing the remote URL (that may contain credentials) to logs by default. - throw new Error(`${baseLog}. Set environment variable SOURCEBOT_LOG_LEVEL=debug to see the full error message.`); - } else if (error instanceof Error) { + if (error instanceof Error) { throw new Error(`${baseLog}. Reason: ${error.message}`); } else { throw new Error(`${baseLog}. Error: ${error}`); } - } finally { - if (authHeader) { - await git.raw(["config", "--unset", "http.extraHeader", authHeader]); - } } } @@ -272,12 +322,23 @@ export const isPathAValidGitRepoRoot = async ({ } } -export const isUrlAValidGitRepo = async (url: string) => { - const git = simpleGit(); - +export const isUrlAValidGitRepo = async ({ + cloneUrl, + credentials, +}: { + cloneUrl: string; + credentials?: GitHttpCredentials; +}) => { // List the remote heads. If an exception is thrown, the URL is not a valid git repo. try { - const result = await git.listRemote(['--heads', url]); + const result = await withRemoteGitClient({ + path: process.cwd(), + cloneUrl, + credentials, + operation: async (git) => { + return git.listRemote(['--heads', cloneUrl]); + }, + }); return result.trim().length > 0; } catch (error: unknown) { return false; @@ -376,13 +437,24 @@ export const getCommitHashForRefName = async ({ export const getRemoteDefaultBranch = async ({ path, cloneUrl, + credentials, + signal, }: { path: string, cloneUrl: string, + credentials?: GitHttpCredentials, + signal?: AbortSignal, }) => { - const git = createGitClientForPath(path); try { - const remoteHead = await git.raw(['ls-remote', '--symref', cloneUrl, 'HEAD']); + const remoteHead = await withRemoteGitClient({ + path, + cloneUrl, + credentials, + signal, + operation: async (git) => { + return git.raw(['ls-remote', '--symref', cloneUrl, 'HEAD']); + }, + }); const match = remoteHead.match(/^ref: refs\/heads\/(\S+)\s+HEAD/m); if (match) { return match[1]; diff --git a/packages/backend/src/gitCredentialSession.test.ts b/packages/backend/src/gitCredentialSession.test.ts new file mode 100644 index 000000000..c17859761 --- /dev/null +++ b/packages/backend/src/gitCredentialSession.test.ts @@ -0,0 +1,262 @@ +import { execFileSync, spawn } from 'node:child_process'; +import { randomUUID } from 'node:crypto'; +import { access, chmod, mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { afterEach, describe, expect, test } from 'vitest'; +import { withGitCredentialSession } from './gitCredentialSession.js'; + +const temporaryPaths: string[] = []; + +const runGitWithInput = async ({ + args, + environment, + input, +}: { + args: string[]; + environment: NodeJS.ProcessEnv; + input: string; +}) => { + return new Promise((resolve, reject) => { + const child = spawn('git', args, { + env: { + ...process.env, + ...environment, + GIT_TERMINAL_PROMPT: '0', + }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + const stdout: Buffer[] = []; + const stderr: Buffer[] = []; + + child.stdout.on('data', (chunk: Buffer) => stdout.push(chunk)); + child.stderr.on('data', (chunk: Buffer) => stderr.push(chunk)); + child.once('error', reject); + child.once('close', (code) => { + if (code === 0) { + resolve(Buffer.concat(stdout).toString('utf8')); + return; + } + reject(new Error(Buffer.concat(stderr).toString('utf8'))); + }); + child.stdin.end(input); + }); +}; + +const getSocketPath = (environment: NodeJS.ProcessEnv) => { + const count = Number(environment.GIT_CONFIG_COUNT); + for (let index = 0; index < count; index++) { + if (environment[`GIT_CONFIG_KEY_${index}`] !== 'credential.helper') { + continue; + } + + const helper = environment[`GIT_CONFIG_VALUE_${index}`]; + const match = helper?.match(/--socket='([^']+)'$/); + if (match) { + return match[1]; + } + } + throw new Error('Credential-cache socket was not configured'); +}; + +const fillCredential = async ({ + environment, + cloneUrl, +}: { + environment: NodeJS.ProcessEnv; + cloneUrl: string; +}) => { + return runGitWithInput({ + args: ['credential', 'fill'], + environment, + input: `url=${cloneUrl}\n\n`, + }); +}; + +afterEach(async () => { + await Promise.all( + temporaryPaths.splice(0).map((path) => rm(path, { recursive: true, force: true })), + ); +}); + +describe('withGitCredentialSession', () => { + test('stores a credential in an isolated memory cache and removes the session afterward', async () => { + const cloneUrl = 'https://example.com/org/repo.git'; + const token = `sourcebot-test-token-${randomUUID()}`; + let sessionDirectory: string | undefined; + + const result = await withGitCredentialSession({ + cloneUrl, + credentials: { + username: 'test-user', + password: token, + }, + operation: async (environment) => { + expect(JSON.stringify(environment)).not.toContain(token); + expect(environment.GIT_ASKPASS).toBe('/bin/false'); + expect(environment.SSH_ASKPASS).toBe('/bin/false'); + expect(environment.GIT_TERMINAL_PROMPT).toBe('0'); + + const socketPath = getSocketPath(environment); + sessionDirectory = dirname(socketPath); + expect((await stat(sessionDirectory)).mode & 0o777).toBe(0o700); + + const entries = await readdir(sessionDirectory, { withFileTypes: true }); + expect(entries.some((entry) => entry.isFile())).toBe(false); + + const credential = await fillCredential({ environment, cloneUrl }); + expect(credential).toContain('username=test-user'); + expect(credential).toContain(`password=${token}`); + + return 'completed'; + }, + }); + + expect(result).toBe('completed'); + expect(sessionDirectory).toBeDefined(); + await expect(access(sessionDirectory!)).rejects.toThrow(); + }); + + test('uses independent caches for concurrent operations', async () => { + const cloneUrl = 'https://example.com/org/repo.git'; + const tokens = [ + `sourcebot-test-token-${randomUUID()}`, + `sourcebot-test-token-${randomUUID()}`, + ]; + const socketPaths: string[] = []; + let releaseOperations!: () => void; + const operationsReady = new Promise((resolve) => { + releaseOperations = resolve; + }); + let operationCount = 0; + + const credentials = await Promise.all(tokens.map((token, tokenIndex) => + withGitCredentialSession({ + cloneUrl, + credentials: { + username: `test-user-${tokenIndex}`, + password: token, + }, + operation: async (environment) => { + socketPaths.push(getSocketPath(environment)); + operationCount++; + if (operationCount === tokens.length) { + releaseOperations(); + } + await operationsReady; + return fillCredential({ environment, cloneUrl }); + }, + }), + )); + + expect(new Set(socketPaths).size).toBe(tokens.length); + credentials.forEach((credential, tokenIndex) => { + expect(credential).toContain(`username=test-user-${tokenIndex}`); + expect(credential).toContain(`password=${tokens[tokenIndex]}`); + expect(credential).not.toContain(tokens[1 - tokenIndex]); + }); + }); + + test('removes the session when the operation fails', async () => { + const expectedError = new Error('operation failed'); + let sessionDirectory: string | undefined; + + await expect(withGitCredentialSession({ + cloneUrl: 'https://example.com/org/repo.git', + credentials: { + username: 'test-user', + password: `sourcebot-test-token-${randomUUID()}`, + }, + operation: async (environment) => { + sessionDirectory = dirname(getSocketPath(environment)); + throw expectedError; + }, + })).rejects.toBe(expectedError); + + expect(sessionDirectory).toBeDefined(); + await expect(access(sessionDirectory!)).rejects.toThrow(); + }); + + test('does not place the credential in Git command arguments', async () => { + const wrapperDirectory = await mkdtemp(join(tmpdir(), 'sourcebot-git-wrapper-')); + temporaryPaths.push(wrapperDirectory); + const wrapperPath = join(wrapperDirectory, 'git'); + const argvLogPath = join(wrapperDirectory, 'argv.log'); + // Resolve Git before changing PATH so the wrapper can delegate to it. + const resolvedGit = execFileSync('which', ['git'], { encoding: 'utf8' }).trim(); + + await writeFile(wrapperPath, [ + '#!/bin/sh', + 'for argument in "$@"; do', + ' printf "%s\\n" "$argument" >> "$SOURCEBOT_TEST_GIT_ARGV_LOG"', + 'done', + 'exec "$SOURCEBOT_TEST_REAL_GIT" "$@"', + '', + ].join('\n')); + await chmod(wrapperPath, 0o700); + + const previousPath = process.env.PATH; + const previousRealGit = process.env.SOURCEBOT_TEST_REAL_GIT; + const previousArgvLog = process.env.SOURCEBOT_TEST_GIT_ARGV_LOG; + process.env.PATH = `${wrapperDirectory}:${previousPath ?? ''}`; + process.env.SOURCEBOT_TEST_REAL_GIT = resolvedGit; + process.env.SOURCEBOT_TEST_GIT_ARGV_LOG = argvLogPath; + + const token = `sourcebot-test-token-${randomUUID()}`; + try { + await withGitCredentialSession({ + cloneUrl: 'https://example.com/org/repo.git', + credentials: { + username: 'test-user', + password: token, + }, + operation: async (environment) => { + await fillCredential({ + environment, + cloneUrl: 'https://example.com/org/repo.git', + }); + }, + }); + } finally { + if (previousPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = previousPath; + } + if (previousRealGit === undefined) { + delete process.env.SOURCEBOT_TEST_REAL_GIT; + } else { + process.env.SOURCEBOT_TEST_REAL_GIT = previousRealGit; + } + if (previousArgvLog === undefined) { + delete process.env.SOURCEBOT_TEST_GIT_ARGV_LOG; + } else { + process.env.SOURCEBOT_TEST_GIT_ARGV_LOG = previousArgvLog; + } + } + + const argvLog = await readFile(argvLogPath, 'utf8'); + expect(argvLog).not.toContain(token); + expect(argvLog).not.toContain('Authorization: Basic'); + expect(argvLog).not.toContain('@example.com'); + }); + + test('rejects clone URLs that already contain credentials', async () => { + await expect(withGitCredentialSession({ + cloneUrl: 'https://embedded:secret@example.com/org/repo.git', + credentials: { + username: 'test-user', + password: 'test-password', + }, + operation: async () => undefined, + })).rejects.toThrow('clone URL without embedded credentials'); + }); + + test('preserves a user-configured URL when no separate credential is provided', async () => { + const cloneUrl = 'https://embedded:secret@example.com/org/repo.git'; + await expect(withGitCredentialSession({ + cloneUrl, + operation: async () => cloneUrl, + })).resolves.toBe(cloneUrl); + }); +}); diff --git a/packages/backend/src/gitCredentialSession.ts b/packages/backend/src/gitCredentialSession.ts new file mode 100644 index 000000000..066703a93 --- /dev/null +++ b/packages/backend/src/gitCredentialSession.ts @@ -0,0 +1,218 @@ +import { spawn } from 'node:child_process'; +import { chmod, mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { GitHttpCredentials } from './types.js'; + +// Explicit cleanup normally ends the daemon immediately. This timeout is only +// a crash backstop and must be long enough for large clones and fetches. +const CACHE_TIMEOUT_SECONDS = 60 * 60; +const CACHE_EXIT_TIMEOUT_MS = 5_000; +const CREDENTIAL_CACHE_DIRECTORY_PREFIX = 'sourcebot-git-credential-'; + +type GitCredentialSessionOptions = { + cloneUrl: string; + credentials?: GitHttpCredentials; + signal?: AbortSignal; + operation: (environment: NodeJS.ProcessEnv) => Promise; +}; + +const assertNoEmbeddedHttpCredentials = (cloneUrl: string) => { + let url: URL; + try { + url = new URL(cloneUrl); + } catch { + return; + } + if ( + (url.protocol === 'http:' || url.protocol === 'https:') && + (url.username || url.password) + ) { + throw new Error('Authenticated Git operations require a clone URL without embedded credentials'); + } +}; + +const validateCredentialField = (name: string, value: string) => { + if (value.includes('\n') || value.includes('\r') || value.includes('\0')) { + throw new Error(`Git credential ${name} contains an unsupported control character`); + } +}; + +const quoteCredentialHelperArgument = (value: string) => { + return `'${value.replaceAll("'", "'\\''")}'`; +}; + +// @see: https://git-scm.com/docs/git-credential#IOFMT +const getCredentialDescription = (cloneUrl: string, credentials: GitHttpCredentials) => { + const url = new URL(cloneUrl); + if (url.protocol !== 'http:' && url.protocol !== 'https:') { + throw new Error('Git HTTP credentials can only be used with HTTP(S) clone URLs'); + } + assertNoEmbeddedHttpCredentials(cloneUrl); + + validateCredentialField('username', credentials.username); + validateCredentialField('password', credentials.password); + + const path = decodeURIComponent(url.pathname).replace(/^\//, ''); + const fields = [ + `protocol=${url.protocol.slice(0, -1)}`, + `host=${url.host}`, + `path=${path}`, + `username=${credentials.username}`, + `password=${credentials.password}`, + '', + ]; + + return `${fields.join('\n')}\n`; +}; + +const createCredentialEnvironment = ({ + socketPath, + proactiveAuth, +}: { + socketPath: string; + proactiveAuth?: 'basic'; +}): NodeJS.ProcessEnv => { + const configEntries: [string, string][] = [ + // discard previously configured credential helpers + ['credential.helper', ''], + // Hold credentials in this operation's isolated in-memory cache. + // @see: https://git-scm.com/docs/git-credential-cache + [ + 'credential.helper', + `cache --timeout=${CACHE_TIMEOUT_SECONDS} --socket=${quoteCredentialHelperArgument(socketPath)}`, + ], + // Include the repository path when matching cached credentials. + ['credential.useHttpPath', 'true'], + // Prevent credential helpers from requesting user interaction. + ['credential.interactive', 'false'], + ]; + + if (proactiveAuth === 'basic') { + configEntries.push( + // Disable credential-free negotiation so proactive authentication takes effect. + ['http.emptyAuth', 'false'], + // Send Basic credentials on the first request instead of waiting for a 401. + ['http.proactiveAuth', 'basic'], + ); + } + + return Object.fromEntries([ + // prevents Git from launching a graphical or scripted password prompt. + ['GIT_ASKPASS', '/bin/false'], + // prevents Git from launching a graphical or scripted password prompt for ssh auth. + ['SSH_ASKPASS', '/bin/false'], + // prevents Git from prompting through the terminal if the credential cache cannot provide a credential + ['GIT_TERMINAL_PROMPT', '0'], + ['GIT_CONFIG_COUNT', configEntries.length.toString()], + ...configEntries.flatMap(([key, value], index) => [ + [`GIT_CONFIG_KEY_${index}`, key], + [`GIT_CONFIG_VALUE_${index}`, value], + ]), + ]); +}; + +const runGit = async ({ + args, + environment, + input, + signal, + timeoutMs, +}: { + args: string[]; + environment: NodeJS.ProcessEnv; + input?: string; + signal?: AbortSignal; + timeoutMs?: number; +}) => { + await new Promise((resolve, reject) => { + const child = spawn('git', args, { + env: { + ...process.env, + ...environment, + GIT_TERMINAL_PROMPT: '0', + }, + signal, + stdio: ['pipe', 'ignore', 'ignore'], + }); + let settled = false; + const timeout = timeoutMs === undefined + ? undefined + : setTimeout(() => child.kill('SIGKILL'), timeoutMs); + + const settle = (callback: () => void) => { + if (settled) { + return; + } + settled = true; + if (timeout) { + clearTimeout(timeout); + } + callback(); + }; + + child.once('error', (error) => settle(() => reject(error))); + child.once('close', (code, childSignal) => settle(() => { + if (code === 0) { + resolve(); + return; + } + reject(new Error(`Git credential command failed with ${childSignal ? `signal ${childSignal}` : `exit code ${code}`}`)); + })); + + child.stdin.on('error', () => { + // The child process error/close handlers report the actionable failure. + }); + child.stdin.end(input); + }); +}; + +/** + * Runs one authenticated Git network operation with credentials held by an + * isolated in-memory credential-cache daemon. The credential is sent to Git + * only through stdin; the returned environment contains cache configuration, + * but no secret values. + */ +export const withGitCredentialSession = async ({ + cloneUrl, + credentials, + signal, + operation, +}: GitCredentialSessionOptions): Promise => { + if (!credentials) { + return operation({}); + } + + const credentialDescription = getCredentialDescription(cloneUrl, credentials); + const sessionDirectory = await mkdtemp(join(tmpdir(), CREDENTIAL_CACHE_DIRECTORY_PREFIX)); + await chmod(sessionDirectory, 0o700); + const socketPath = join(sessionDirectory, 'socket'); + const environment = createCredentialEnvironment({ + socketPath, + proactiveAuth: credentials.proactiveAuth, + }); + + try { + // Ask Git's credential subsystem to store the credential in this session's cache. + // The cache helper lazily starts its daemon, and the secret enters Git only via stdin. + // @see: https://git-scm.com/docs/git-credential + await runGit({ + args: ['credential', 'approve'], + environment, + input: credentialDescription, + signal, + }); + return await operation(environment); + } finally { + try { + await runGit({ + args: ['credential-cache', `--socket=${socketPath}`, 'exit'], + environment: {}, + timeoutMs: CACHE_EXIT_TIMEOUT_MS, + }); + } catch { + // The daemon may never have started or may already have exited. + } + await rm(sessionDirectory, { recursive: true, force: true }); + } +}; diff --git a/packages/backend/src/repoCompileUtils.ts b/packages/backend/src/repoCompileUtils.ts index 6888f2510..1e65565f1 100644 --- a/packages/backend/src/repoCompileUtils.ts +++ b/packages/backend/src/repoCompileUtils.ts @@ -711,7 +711,9 @@ export const compileGenericGitHostConfig_url = async ( const warnings: string[] = []; // Validate that we are dealing with a valid git repo. - const isGitRepo = await isUrlAValidGitRepo(remoteUrl.toString()); + const isGitRepo = await isUrlAValidGitRepo({ + cloneUrl: remoteUrl.toString(), + }); if (!isGitRepo) { const warning = `Skipping ${remoteUrl.toString()} - not a git repository.`; logger.warn(warning); diff --git a/packages/backend/src/repoIndexManager.ts b/packages/backend/src/repoIndexManager.ts index aea1291dc..5a6354e16 100644 --- a/packages/backend/src/repoIndexManager.ts +++ b/packages/backend/src/repoIndexManager.ts @@ -352,8 +352,7 @@ export class RepoIndexManager { const metadata = repoMetadataSchema.parse(repo.metadata); const credentials = await getAuthCredentialsForRepo(repo, logger); - const cloneUrlMaybeWithToken = credentials?.cloneUrlWithToken ?? repo.cloneUrl; - const authHeader = credentials?.authHeader ?? undefined; + const gitHttpCredentials = credentials?.gitHttpCredentials; // If the repo path exists but it is not a valid git repository root, this indicates // that the repository is in a bad state. To fix, we remove the directory and perform @@ -385,8 +384,8 @@ export class RepoIndexManager { logger.debug(`Fetching ${repo.name} (id: ${repo.id})...`); const { durationMs } = await measure(() => fetchRepository({ - cloneUrl: cloneUrlMaybeWithToken, - authHeader, + cloneUrl: repo.cloneUrl, + credentials: gitHttpCredentials, path: repoPath, onProgress: ({ method, stage, progress }) => { logger.debug(`git.${method} ${stage} stage ${progress}% complete for ${repo.name} (id: ${repo.id})`) @@ -413,8 +412,8 @@ export class RepoIndexManager { logger.debug(`Cloning ${repo.name} (id: ${repo.id})...`); const { durationMs } = await measure(() => cloneRepository({ - cloneUrl: cloneUrlMaybeWithToken, - authHeader, + cloneUrl: repo.cloneUrl, + credentials: gitHttpCredentials, path: repoPath, onProgress: ({ method, stage, progress }) => { logger.debug(`git.${method} ${stage} stage ${progress}% complete for ${repo.name} (id: ${repo.id})`) diff --git a/packages/backend/src/types.ts b/packages/backend/src/types.ts index 8803b48b9..3018b9dca 100644 --- a/packages/backend/src/types.ts +++ b/packages/backend/src/types.ts @@ -14,14 +14,18 @@ export type WithRequired = T & { [P in K]-?: T[P] }; export type RepoWithConnections = Repo & { connections: (RepoToConnection & { connection: Connection })[] }; +export type GitHttpCredentials = { + username: string; + password: string; + proactiveAuth?: 'basic'; +}; export type RepoAuthCredentials = { hostUrl?: string; token: string; - cloneUrlWithToken?: string; - authHeader?: string; + gitHttpCredentials?: GitHttpCredentials; /** The connection that configured the * credentials for this repo. */ connectionConfig?: ConnectionConfig; -} \ No newline at end of file +} diff --git a/packages/backend/src/utils.test.ts b/packages/backend/src/utils.test.ts index 7977c4b60..83abbb0eb 100644 --- a/packages/backend/src/utils.test.ts +++ b/packages/backend/src/utils.test.ts @@ -18,9 +18,11 @@ const createMockLogger = (): Logger => ({ describe('getAuthCredentialsForRepo', () => { const originalAskGithubToken = env.EXPERIMENT_ASK_GH_GITHUB_TOKEN; + const azureDevOpsTokenEnvironment = 'SOURCEBOT_TEST_AZURE_DEVOPS_TOKEN'; afterEach(() => { env.EXPERIMENT_ASK_GH_GITHUB_TOKEN = originalAskGithubToken; + delete process.env[azureDevOpsTokenEnvironment]; }); test('uses the Ask GitHub PAT before other GitHub credentials', async () => { @@ -31,14 +33,52 @@ describe('getAuthCredentialsForRepo', () => { external_codeHostUrl: 'https://github.com', cloneUrl: 'https://github.com/codemirror/dev.git', connections: [], - } as RepoWithConnections; + } as unknown as RepoWithConnections; const credentials = await getAuthCredentialsForRepo(repo); expect(credentials).toEqual({ hostUrl: 'https://github.com', token: 'github-pat-token', - cloneUrlWithToken: 'https://x-access-token:github-pat-token@github.com/codemirror/dev.git', + gitHttpCredentials: { + username: 'x-access-token', + password: 'github-pat-token', + }, + }); + }); + + test('uses proactive Basic authentication for Azure DevOps Server', async () => { + const connectionConfig = { + type: 'azuredevops', + url: 'https://azure-devops.example.com', + deploymentType: 'server', + token: { + env: azureDevOpsTokenEnvironment, + }, + } as const; + process.env[azureDevOpsTokenEnvironment] = 'azure-devops-test-token'; + const repo = { + external_codeHostType: 'azuredevops', + cloneUrl: 'https://azure-devops.example.com/project/_git/repo', + connections: [{ + connection: { + connectionType: 'azuredevops', + config: connectionConfig, + }, + }], + } as unknown as RepoWithConnections; + + const credentials = await getAuthCredentialsForRepo(repo); + + expect(credentials).toEqual({ + hostUrl: 'https://azure-devops.example.com', + token: 'azure-devops-test-token', + gitHttpCredentials: { + username: 'user', + password: 'azure-devops-test-token', + proactiveAuth: 'basic', + }, + connectionConfig, }); }); }); diff --git a/packages/backend/src/utils.ts b/packages/backend/src/utils.ts index 7b999ecc9..1d4f655ed 100644 --- a/packages/backend/src/utils.ts +++ b/packages/backend/src/utils.ts @@ -122,13 +122,10 @@ export const getAuthCredentialsForRepo = async (repo: RepoWithConnections, logge return { hostUrl: repo.external_codeHostUrl, token, - cloneUrlWithToken: createGitCloneUrlWithToken( - repo.cloneUrl, - { - username: 'x-access-token', - password: token, - } - ), + gitHttpCredentials: { + username: 'x-access-token', + password: token, + }, }; } @@ -154,13 +151,10 @@ export const getAuthCredentialsForRepo = async (repo: RepoWithConnections, logge return { hostUrl: repo.external_codeHostUrl, token, - cloneUrlWithToken: createGitCloneUrlWithToken( - repo.cloneUrl, - { - username: 'x-access-token', - password: token - } - ), + gitHttpCredentials: { + username: 'x-access-token', + password: token, + }, } } } @@ -174,12 +168,10 @@ export const getAuthCredentialsForRepo = async (repo: RepoWithConnections, logge return { hostUrl: config.url, token, - cloneUrlWithToken: createGitCloneUrlWithToken( - repo.cloneUrl, - { - password: token, - } - ), + gitHttpCredentials: { + username: 'x-access-token', + password: token, + }, connectionConfig: config, } } @@ -190,13 +182,10 @@ export const getAuthCredentialsForRepo = async (repo: RepoWithConnections, logge return { hostUrl: config.url, token, - cloneUrlWithToken: createGitCloneUrlWithToken( - repo.cloneUrl, - { - username: 'oauth2', - password: token - } - ), + gitHttpCredentials: { + username: 'oauth2', + password: token, + }, connectionConfig: config, } } @@ -207,12 +196,10 @@ export const getAuthCredentialsForRepo = async (repo: RepoWithConnections, logge return { hostUrl: config.url, token, - cloneUrlWithToken: createGitCloneUrlWithToken( - repo.cloneUrl, - { - password: token - } - ), + gitHttpCredentials: { + username: token, + password: '', + }, connectionConfig: config, } } @@ -224,13 +211,10 @@ export const getAuthCredentialsForRepo = async (repo: RepoWithConnections, logge return { hostUrl: config.url, token, - cloneUrlWithToken: createGitCloneUrlWithToken( - repo.cloneUrl, - { - username, - password: token - } - ), + gitHttpCredentials: { + username, + password: token, + }, connectionConfig: config, } } @@ -239,32 +223,20 @@ export const getAuthCredentialsForRepo = async (repo: RepoWithConnections, logge if (config.token) { const token = await getTokenFromConfig(config.token); - // For ADO server, multiple auth schemes may be supported. If the ADO deployment supports NTLM, the git clone will default - // to this over basic auth. As a result, we cannot embed the token in the clone URL and must force basic auth by passing in the token - // appropriately in the header. To do this, we set the authHeader field here - if (config.deploymentType === 'server') { - return { - hostUrl: config.url, - token, - authHeader: "Authorization: Basic " + Buffer.from(`:${token}`).toString('base64') - } - } else { - return { - hostUrl: config.url, - token, - cloneUrlWithToken: createGitCloneUrlWithToken( - repo.cloneUrl, - { - // @note: If we don't provide a username, the password will be set as the username. This seems to work - // for ADO cloud but not for ADO server. To fix this, we set a placeholder username to ensure the password - // is set correctly - username: 'user', - password: token - } - ), - connectionConfig: config, - } - } + // ADO Server may advertise NTLM alongside Basic authentication. Force + // proactive Basic auth there so libcurl does not select NTLM first. + return { + hostUrl: config.url, + token, + gitHttpCredentials: { + username: 'user', + password: token, + ...(config.deploymentType === 'server' ? { + proactiveAuth: 'basic' as const, + } : {}), + }, + connectionConfig: config, + }; } } } @@ -272,23 +244,6 @@ export const getAuthCredentialsForRepo = async (repo: RepoWithConnections, logge return undefined; } -const createGitCloneUrlWithToken = (cloneUrl: string, credentials: { username?: string, password: string }) => { - const url = new URL(cloneUrl); - // @note: URL has a weird behavior where if you set the password but - // _not_ the username, the ":" delimiter will still be present in the - // URL (e.g., https://:password@example.com). To get around this, if - // we only have a password, we set the username to the password. - // @see: https://www.typescriptlang.org/play/?#code/MYewdgzgLgBArgJwDYwLwzAUwO4wKoBKAMgBQBEAFlFAA4QBcA9I5gB4CGAtjUpgHShOZADQBKANwAoREj412ECNhAIAJmhhl5i5WrJTQkELz5IQAcxIy+UEAGUoCAJZhLo0UA - if (!credentials.username) { - url.username = credentials.password; - } else { - url.username = credentials.username; - url.password = credentials.password; - } - return url.toString(); -} - - // setInterval wrapper that ensures async callbacks are not executed concurrently. // @see: https://mottaquikarim.github.io/dev/posts/setinterval-that-blocks-on-await/ export const setIntervalAsync = (target: () => Promise, pollingIntervalMs: number): NodeJS.Timeout => { From 7d935880d4a97dd6735685d4713b2774762d8250 Mon Sep 17 00:00:00 2001 From: Brendan Kellam Date: Thu, 13 Aug 2026 17:37:40 -0700 Subject: [PATCH 2/4] chore: add changelog entry for PR 1584 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c679ae54..944f10305 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed memory leak attributed to CodeMirror allocating objects on heap that were never freed. [#1580](https://github.com/sourcebot-dev/sourcebot/pull/1580) +- Kept Git provider credentials out of subprocess arguments and on-disk configuration by using isolated in-memory credential caches. [#1584](https://github.com/sourcebot-dev/sourcebot/pull/1584) ## [5.1.7] - 2026-08-13 From f49e6fb421466f88c92458d4db39bd3b4f55c26d Mon Sep 17 00:00:00 2001 From: Brendan Kellam Date: Thu, 13 Aug 2026 18:12:36 -0700 Subject: [PATCH 3/4] feedback --- packages/backend/src/git.test.ts | 3 ++ .../backend/src/gitCredentialSession.test.ts | 42 +++++++++++++++++++ packages/backend/src/gitCredentialSession.ts | 38 +++++++++++++---- 3 files changed, 74 insertions(+), 9 deletions(-) diff --git a/packages/backend/src/git.test.ts b/packages/backend/src/git.test.ts index 29a8983c0..167dd3731 100644 --- a/packages/backend/src/git.test.ts +++ b/packages/backend/src/git.test.ts @@ -138,6 +138,9 @@ const createAuthenticatedGitServer = async ({ response.end(); }); + backend.stdin.on('error', () => { + // The child process error and close handlers report the actionable failure. + }); request.pipe(backend.stdin); }); diff --git a/packages/backend/src/gitCredentialSession.test.ts b/packages/backend/src/gitCredentialSession.test.ts index c17859761..25c169850 100644 --- a/packages/backend/src/gitCredentialSession.test.ts +++ b/packages/backend/src/gitCredentialSession.test.ts @@ -241,6 +241,48 @@ describe('withGitCredentialSession', () => { expect(argvLog).not.toContain('@example.com'); }); + test('reports credential command stderr without exposing the password', async () => { + const wrapperDirectory = await mkdtemp(join(tmpdir(), 'sourcebot-git-wrapper-')); + temporaryPaths.push(wrapperDirectory); + const wrapperPath = join(wrapperDirectory, 'git'); + await writeFile(wrapperPath, [ + '#!/bin/sh', + 'input=$(cat)', + 'printf "%s\\n" "$input" >&2', + 'exit 1', + '', + ].join('\n')); + await chmod(wrapperPath, 0o700); + + const previousPath = process.env.PATH; + process.env.PATH = `${wrapperDirectory}:${previousPath ?? ''}`; + const token = `sourcebot-test-token-${randomUUID()}`; + let error: unknown; + try { + await withGitCredentialSession({ + cloneUrl: 'https://example.com/org/repo.git', + credentials: { + username: 'test-user', + password: token, + }, + operation: async () => undefined, + }); + } catch (caughtError) { + error = caughtError; + } finally { + if (previousPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = previousPath; + } + } + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toContain('protocol=https'); + expect((error as Error).message).toContain('password=[REDACTED]'); + expect((error as Error).message).not.toContain(token); + }); + test('rejects clone URLs that already contain credentials', async () => { await expect(withGitCredentialSession({ cloneUrl: 'https://embedded:secret@example.com/org/repo.git', diff --git a/packages/backend/src/gitCredentialSession.ts b/packages/backend/src/gitCredentialSession.ts index 066703a93..4b5d82c3e 100644 --- a/packages/backend/src/gitCredentialSession.ts +++ b/packages/backend/src/gitCredentialSession.ts @@ -8,6 +8,7 @@ import type { GitHttpCredentials } from './types.js'; // a crash backstop and must be long enough for large clones and fetches. const CACHE_TIMEOUT_SECONDS = 60 * 60; const CACHE_EXIT_TIMEOUT_MS = 5_000; +const GIT_CREDENTIAL_COMMAND_TIMEOUT_MS = 30_000; const CREDENTIAL_CACHE_DIRECTORY_PREFIX = 'sourcebot-git-credential-'; type GitCredentialSessionOptions = { @@ -117,13 +118,15 @@ const runGit = async ({ environment, input, signal, - timeoutMs, + timeoutMs = GIT_CREDENTIAL_COMMAND_TIMEOUT_MS, + sensitiveValues = [], }: { args: string[]; environment: NodeJS.ProcessEnv; input?: string; signal?: AbortSignal; timeoutMs?: number; + sensitiveValues?: string[]; }) => { await new Promise((resolve, reject) => { const child = spawn('git', args, { @@ -133,21 +136,24 @@ const runGit = async ({ GIT_TERMINAL_PROMPT: '0', }, signal, - stdio: ['pipe', 'ignore', 'ignore'], + stdio: ['pipe', 'ignore', 'pipe'], }); + const stderr: Buffer[] = []; let settled = false; - const timeout = timeoutMs === undefined - ? undefined - : setTimeout(() => child.kill('SIGKILL'), timeoutMs); + let timedOut = false; + const timeout = setTimeout(() => { + timedOut = true; + child.kill('SIGKILL'); + }, timeoutMs); + + child.stderr.on('data', (chunk: Buffer) => stderr.push(chunk)); const settle = (callback: () => void) => { if (settled) { return; } settled = true; - if (timeout) { - clearTimeout(timeout); - } + clearTimeout(timeout); callback(); }; @@ -157,7 +163,20 @@ const runGit = async ({ resolve(); return; } - reject(new Error(`Git credential command failed with ${childSignal ? `signal ${childSignal}` : `exit code ${code}`}`)); + const failure = timedOut + ? `timed out after ${timeoutMs}ms` + : childSignal + ? `signal ${childSignal}` + : `exit code ${code}`; + const diagnostic = sensitiveValues + .filter(Boolean) + .reduce( + (value, sensitiveValue) => value.replaceAll(sensitiveValue, '[REDACTED]'), + Buffer.concat(stderr).toString('utf8').trim(), + ); + reject(new Error( + `Git credential command failed with ${failure}${diagnostic ? `: ${diagnostic}` : ''}`, + )); })); child.stdin.on('error', () => { @@ -201,6 +220,7 @@ export const withGitCredentialSession = async ({ environment, input: credentialDescription, signal, + sensitiveValues: [credentials.password], }); return await operation(environment); } finally { From 6e40d49706850fb124b0b0a8a474f860e617d23a Mon Sep 17 00:00:00 2001 From: Brendan Kellam Date: Thu, 13 Aug 2026 18:25:34 -0700 Subject: [PATCH 4/4] feedback --- .../backend/src/gitCredentialSession.test.ts | 18 ++++++++++++------ packages/backend/src/gitCredentialSession.ts | 16 ---------------- 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/packages/backend/src/gitCredentialSession.test.ts b/packages/backend/src/gitCredentialSession.test.ts index 25c169850..13b912f79 100644 --- a/packages/backend/src/gitCredentialSession.test.ts +++ b/packages/backend/src/gitCredentialSession.test.ts @@ -283,15 +283,21 @@ describe('withGitCredentialSession', () => { expect((error as Error).message).not.toContain(token); }); - test('rejects clone URLs that already contain credentials', async () => { - await expect(withGitCredentialSession({ - cloneUrl: 'https://embedded:secret@example.com/org/repo.git', + test('supports a clone URL with an embedded username', async () => { + const cloneUrl = 'https://test-user@example.com/org/repo.git'; + const token = `sourcebot-test-token-${randomUUID()}`; + + const credential = await withGitCredentialSession({ + cloneUrl, credentials: { username: 'test-user', - password: 'test-password', + password: token, }, - operation: async () => undefined, - })).rejects.toThrow('clone URL without embedded credentials'); + operation: async (environment) => fillCredential({ environment, cloneUrl }), + }); + + expect(credential).toContain('username=test-user'); + expect(credential).toContain(`password=${token}`); }); test('preserves a user-configured URL when no separate credential is provided', async () => { diff --git a/packages/backend/src/gitCredentialSession.ts b/packages/backend/src/gitCredentialSession.ts index 4b5d82c3e..d66ea7fe1 100644 --- a/packages/backend/src/gitCredentialSession.ts +++ b/packages/backend/src/gitCredentialSession.ts @@ -18,21 +18,6 @@ type GitCredentialSessionOptions = { operation: (environment: NodeJS.ProcessEnv) => Promise; }; -const assertNoEmbeddedHttpCredentials = (cloneUrl: string) => { - let url: URL; - try { - url = new URL(cloneUrl); - } catch { - return; - } - if ( - (url.protocol === 'http:' || url.protocol === 'https:') && - (url.username || url.password) - ) { - throw new Error('Authenticated Git operations require a clone URL without embedded credentials'); - } -}; - const validateCredentialField = (name: string, value: string) => { if (value.includes('\n') || value.includes('\r') || value.includes('\0')) { throw new Error(`Git credential ${name} contains an unsupported control character`); @@ -49,7 +34,6 @@ const getCredentialDescription = (cloneUrl: string, credentials: GitHttpCredenti if (url.protocol !== 'http:' && url.protocol !== 'https:') { throw new Error('Git HTTP credentials can only be used with HTTP(S) clone URLs'); } - assertNoEmbeddedHttpCredentials(cloneUrl); validateCredentialField('username', credentials.username); validateCredentialField('password', credentials.password);