diff --git a/src/orchestrator/orchestrator.ts b/src/orchestrator/orchestrator.ts index d6e9df91..3129a651 100644 --- a/src/orchestrator/orchestrator.ts +++ b/src/orchestrator/orchestrator.ts @@ -61,7 +61,7 @@ function buildAuthenticatedCloneUrl( // Backward-compatibility: older rows may still contain raw PAT strings. // Use the raw value as a last resort so private HTTPS clones keep working. token = encryptedToken; - log.warn({ integrationType, cloneUrl: rawCloneUrl, err }, "failed to decrypt token for authenticated clone URL; falling back to raw token value"); + log.warn({ integrationType, cloneUrl: redactUrls(rawCloneUrl), err }, "failed to decrypt token for authenticated clone URL; falling back to raw token value"); } if (/^\*{4,}$/.test(token)) return undefined; const usernamePrefix = integrationType === "github" ? "x-access-token" : "oauth2"; diff --git a/src/utils/redactUrl.ts b/src/utils/redactUrl.ts index c4c9ce0d..d1868b54 100644 --- a/src/utils/redactUrl.ts +++ b/src/utils/redactUrl.ts @@ -1,9 +1,45 @@ +/** Token-shaped values that must be masked regardless of the surrounding key. */ +const TOKEN_VALUE = /(gh[opusr]_[A-Za-z0-9]{16,})|(github_pat_[A-Za-z0-9_]{16,})/g; + /** - * Redact credentials embedded in HTTP(S) URLs. + * Redact credentials embedded in HTTP(S) URLs and GitHub token-shaped values. * * Replaces `https://user:secret@host/...` with `https://@host/...` * so tokens never appear in logs, error messages, or external comments. */ export function redactUrls(text: string): string { - return text.replace(/(https?:\/\/)[^@\s/][^@\s]*@/g, "$1@"); + return text + .replace(/(https?:\/\/)[^/\s?#@]+@/gi, "$1@") + .replace(TOKEN_VALUE, ""); +} + +/** Env-var names whose values must never be logged. */ +const SENSITIVE_KEY = + /(?:^|[_-])(?:TOKEN|SECRET|PASSWORD|PASSWD|PASSPHRASE|CREDENTIALS?|PRIVATE[_-]?KEY|API[_-]?KEY|KEY|AUTH|PAT)(?:$|[_-])/i; + +function redactSensitiveAssignment(arg: string): string | undefined { + const prefix = arg.startsWith("--env=") ? "--env=" : ""; + const assignment = prefix ? arg.slice(prefix.length) : arg; + const separator = assignment.indexOf("="); + if (separator <= 0 || !SENSITIVE_KEY.test(assignment.slice(0, separator))) { + return undefined; + } + return `${prefix}${assignment.slice(0, separator)}=`; +} + +/** + * Redact secrets from a Docker argv array before logging. + * + * Masks the value of any `NAME=VALUE` argument whose name looks sensitive + * (e.g. `GITHUB_TOKEN=...`, `COPILOT_SDK_AUTH_TOKEN=...`), strips credentials + * embedded in URLs, and masks any stray GitHub token-shaped values. + */ +export function redactDockerArgs(args: readonly string[]): string[] { + return args.map((arg) => { + const redactedAssignment = redactSensitiveAssignment(arg); + if (redactedAssignment !== undefined) { + return redactedAssignment; + } + return redactUrls(arg); + }); } diff --git a/src/vcs/githubVcsConnector.ts b/src/vcs/githubVcsConnector.ts index 724430fb..2310f7b2 100644 --- a/src/vcs/githubVcsConnector.ts +++ b/src/vcs/githubVcsConnector.ts @@ -49,7 +49,10 @@ export class GitHubVcsConnector implements VcsConnector { /** Clone a GitHub repository via HTTPS into the target directory. */ async clone(repoUrl: string, branch: string, targetDir: string): Promise { - log.info({ repoUrl, branch, targetDir }, "cloning repository from GitHub via HTTPS"); + log.info( + { repoUrl: redactUrls(repoUrl), branch, targetDir }, + "cloning repository from GitHub via HTTPS" + ); try { execFileSync("git", ["clone", "--branch", branch, "--depth", "1", repoUrl, targetDir], { @@ -60,7 +63,9 @@ export class GitHubVcsConnector implements VcsConnector { log.info({ targetDir }, "repository cloned successfully"); } catch (err: unknown) { const error = err instanceof Error ? err : new Error(String(err)); - throw new Error(`Failed to clone ${repoUrl}: ${error.message.slice(0, 500)}`); + throw new Error( + redactUrls(`Failed to clone ${repoUrl}: ${error.message.slice(0, 500)}`) + ); } } diff --git a/src/vcs/gitlabVcsConnector.ts b/src/vcs/gitlabVcsConnector.ts index 59e93e62..5e3970a6 100644 --- a/src/vcs/gitlabVcsConnector.ts +++ b/src/vcs/gitlabVcsConnector.ts @@ -46,7 +46,7 @@ export class GitLabVcsConnector implements VcsConnector { /** Clone a GitLab repository via HTTP into the target directory. */ async clone(repoUrl: string, branch: string, targetDir: string): Promise { log.info( - { repoUrl, branch, targetDir }, + { repoUrl: redactUrls(repoUrl), branch, targetDir }, "cloning repository from GitLab via HTTP" ); @@ -61,7 +61,7 @@ export class GitLabVcsConnector implements VcsConnector { } catch (err: unknown) { const error = err instanceof Error ? err : new Error(String(err)); throw new Error( - `Failed to clone GitLab repository: ${error.message.slice(0, 300)}` + redactUrls(`Failed to clone GitLab repository: ${error.message.slice(0, 300)}`) ); } } diff --git a/src/workspace/workspaceRunner.ts b/src/workspace/workspaceRunner.ts index f3211956..d16c1894 100644 --- a/src/workspace/workspaceRunner.ts +++ b/src/workspace/workspaceRunner.ts @@ -14,6 +14,7 @@ import type { import type { AgentAdapter } from "../interfaces.js"; import { createVolume, removeVolume, execInVolume, stopContainersUsingVolume } from "./dockerVolume.js"; import { getLogger } from "../logger.js"; +import { redactUrls, redactDockerArgs } from "../utils/redactUrl.js"; import { buildSkillsCliArgs, isSshSkillSource, parseRemoteSkillSources, skillsAgentId, sshSkillSourceCommandPort } from "./skillSources.js"; import type { AgentProvider } from "./skillSources.js"; @@ -220,7 +221,7 @@ export class DockerWorkspaceRunner implements WorkspaceRunner { dockerArgs.push(spec.image, ...spec.command); - log.debug({ taskId: context.taskId, dockerArgs }, "agent container command"); + log.debug({ taskId: context.taskId, dockerArgs: redactDockerArgs(dockerArgs) }, "agent container command"); const stdoutChunks: string[] = []; const stderrChunks: string[] = []; @@ -322,7 +323,7 @@ export class DockerWorkspaceRunner implements WorkspaceRunner { ): Promise { try { log.info( - { taskId: handle.taskId, repoUrl, branch, volume: handle.volumeName }, + { taskId: handle.taskId, repoUrl: redactUrls(repoUrl), branch, volume: handle.volumeName }, "cloning repository into volume" ); const result = await execInVolume({ @@ -337,9 +338,9 @@ export class DockerWorkspaceRunner implements WorkspaceRunner { } return { success: true, localPath: "/workspace" }; } catch (err) { - const errorMsg = err instanceof Error ? err.message : String(err); + const errorMsg = redactUrls(err instanceof Error ? err.message : String(err)); log.error( - { taskId: handle.taskId, repoUrl, branch, error: errorMsg }, + { taskId: handle.taskId, repoUrl: redactUrls(repoUrl), branch, error: errorMsg }, "failed to clone repository" ); return { success: false, localPath: "/workspace", error: errorMsg }; @@ -392,7 +393,7 @@ export class DockerWorkspaceRunner implements WorkspaceRunner { taskId: handle.taskId, targetCount: sorted.length, rootRepoKey: root.repoKey, - rootCloneUrl: root.cloneUrl, + rootCloneUrl: redactUrls(root.cloneUrl), }, "preparing project workspace (multi push target via volume)" ); @@ -407,7 +408,7 @@ export class DockerWorkspaceRunner implements WorkspaceRunner { ...(sshKnownHostsPath !== undefined ? { sshKnownHostsPath } : {}), }); if (rootResult.exitCode !== 0) { - const errorMsg = rootResult.stderr.slice(0, 500); + const errorMsg = redactUrls(rootResult.stderr.slice(0, 500)); log.error( { taskId: handle.taskId, repoKey: root.repoKey, error: errorMsg }, "root push target clone failed" @@ -441,7 +442,7 @@ export class DockerWorkspaceRunner implements WorkspaceRunner { ...(sshKnownHostsPath !== undefined ? { sshKnownHostsPath } : {}), }); if (cloneResult.exitCode !== 0) { - const errorMsg = cloneResult.stderr.slice(0, 300); + const errorMsg = redactUrls(cloneResult.stderr.slice(0, 300)); log.warn( { taskId: handle.taskId, repoKey: target.repoKey, localPath: target.localPath, error: errorMsg }, "push target clone failed; continuing with remaining targets" @@ -677,7 +678,7 @@ export class DockerWorkspaceRunner implements WorkspaceRunner { dockerArgs.push(spec.image, ...spec.command); - log.debug({ taskId: handle.taskId, dockerArgs }, "review container command"); + log.debug({ taskId: handle.taskId, dockerArgs: redactDockerArgs(dockerArgs) }, "review container command"); const stdoutChunks: string[] = []; const stderrChunks: string[] = []; diff --git a/tests/unit/githubVcsConnector.test.ts b/tests/unit/githubVcsConnector.test.ts index 7ddedc98..b8e73713 100644 --- a/tests/unit/githubVcsConnector.test.ts +++ b/tests/unit/githubVcsConnector.test.ts @@ -1,7 +1,23 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { execFileSync } from "child_process"; import { GitHubVcsConnector } from "../../src/vcs/githubVcsConnector.js"; import { jsonResponse, errorResponse } from "./helpers/fixtures.js"; +const { logger } = vi.hoisted(() => ({ + logger: { + trace: vi.fn(), + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + fatal: vi.fn(), + }, +})); + +vi.mock("../../src/logger.js", () => ({ + getLogger: vi.fn(() => logger), +})); + const API_BASE_URL = "https://api.github.com"; const HOST = "github.com"; const OWNER = "octocat"; @@ -77,6 +93,37 @@ describe("GitHubVcsConnector", () => { }); }); + describe("clone", () => { + it("redacts credentials from logs without changing the Git command", async () => { + const repoUrl = + "https://x-access-token:ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ012345@github.com/octocat/hello-world.git"; + + await makeConnector().clone(repoUrl, "main", "/tmp/repo"); + + expect(vi.mocked(execFileSync)).toHaveBeenCalledWith( + "git", + ["clone", "--branch", "main", "--depth", "1", repoUrl, "/tmp/repo"], + expect.any(Object) + ); + expect(logger.info).toHaveBeenCalledWith( + expect.objectContaining({ repoUrl: "https://@github.com/octocat/hello-world.git" }), + "cloning repository from GitHub via HTTPS" + ); + }); + + it("redacts credentials from clone failures", async () => { + const repoUrl = + "https://x-access-token:ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ012345@github.com/octocat/hello-world.git"; + vi.mocked(execFileSync).mockImplementationOnce(() => { + throw new Error(`fatal: unable to access '${repoUrl}'`); + }); + + await expect(makeConnector().clone(repoUrl, "main", "/tmp/repo")).rejects.not.toThrow( + /ghp_/ + ); + }); + }); + describe("getChangeStatus", () => { it("returns OPEN for open PRs", async () => { fetchMock.mockResolvedValueOnce(jsonResponse(PR_RESPONSE)); diff --git a/tests/unit/gitlabVcsConnector.test.ts b/tests/unit/gitlabVcsConnector.test.ts index c16f2c48..f264f7fb 100644 --- a/tests/unit/gitlabVcsConnector.test.ts +++ b/tests/unit/gitlabVcsConnector.test.ts @@ -9,6 +9,21 @@ import { GitLabVcsConnector } from "../../src/vcs/gitlabVcsConnector.js"; import type { GitLabVcsConnectorConfig } from "../../src/vcs/gitlabVcsConnector.js"; import { ReviewApiError } from "../../src/interfaces.js"; +const { logger } = vi.hoisted(() => ({ + logger: { + trace: vi.fn(), + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + fatal: vi.fn(), + }, +})); + +vi.mock("../../src/logger.js", () => ({ + getLogger: vi.fn(() => logger), +})); + vi.mock("child_process"); vi.mock("../../src/connectors/gitlabHttpClient.js", () => { const GitLabHttpClient = vi.fn().mockImplementation(function () { @@ -59,6 +74,25 @@ describe("GitLabVcsConnector", () => { ); }); + it("redacts credentials from clone logs without changing the Git command", async () => { + const mockExecFileSync = vi.mocked(execFileSync); + mockExecFileSync.mockReturnValue(""); + const repoUrl = + "https://oauth2:glpat-secret@gitlab.example.com/my-project.git"; + + await connector.clone(repoUrl, "main", "/tmp/repo"); + + expect(mockExecFileSync).toHaveBeenCalledWith( + "git", + ["clone", "--branch", "main", "--depth", "1", repoUrl, "/tmp/repo"], + expect.any(Object) + ); + expect(logger.info).toHaveBeenCalledWith( + expect.objectContaining({ repoUrl: "https://@gitlab.example.com/my-project.git" }), + "cloning repository from GitLab via HTTP" + ); + }); + it("should throw on clone failure", async () => { const mockExecFileSync = vi.mocked(execFileSync); mockExecFileSync.mockImplementation(() => { @@ -69,6 +103,17 @@ describe("GitLabVcsConnector", () => { connector.clone("https://invalid.com/repo.git", "main", "/tmp/repo") ).rejects.toThrow("Failed to clone GitLab repository"); }); + + it("redacts credentials echoed in clone failures", async () => { + const repoUrl = "https://oauth2:glpat-secret@gitlab.example.com/my-project.git"; + vi.mocked(execFileSync).mockImplementationOnce(() => { + throw new Error(`fatal: unable to access '${repoUrl}'`); + }); + + await expect(connector.clone(repoUrl, "main", "/tmp/repo")).rejects.not.toThrow( + /glpat-secret/ + ); + }); }); describe("push", () => { diff --git a/tests/unit/redactUrl.test.ts b/tests/unit/redactUrl.test.ts new file mode 100644 index 00000000..bf2118ac --- /dev/null +++ b/tests/unit/redactUrl.test.ts @@ -0,0 +1,86 @@ +import { describe, it, expect } from "vitest"; +import { redactUrls, redactDockerArgs } from "../../src/utils/redactUrl.js"; + +describe("redactUrls", () => { + it("masks credentials embedded in an https clone URL", () => { + const url = + "https://x-access-token:gho_ABCDEFGHIJKLMNOPQRST@github.com/savoirfairelinux/virtual-engineer.git"; + const out = redactUrls(url); + expect(out).toBe("https://@github.com/savoirfairelinux/virtual-engineer.git"); + expect(out).not.toContain("gho_"); + }); + + it("leaves credential-free URLs untouched", () => { + const url = "https://github.com/savoirfairelinux/virtual-engineer.git"; + expect(redactUrls(url)).toBe(url); + }); + + it("handles uppercase schemes without redacting at-signs in URL paths", () => { + expect(redactUrls("HTTPS://user:secret@example.com/org/repo.git")).toBe( + "HTTPS://@example.com/org/repo.git" + ); + expect(redactUrls("https://example.com/users/dev@example.com")).toBe( + "https://example.com/users/dev@example.com" + ); + }); + + it("masks GitHub tokens outside URL userinfo", () => { + const text = + "fatal: unable to access 'https://github.com/org/repo.git?access_token=ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ012345'"; + const out = redactUrls(text); + expect(out).not.toContain("ghp_"); + expect(out).toContain("access_token="); + }); +}); + +describe("redactDockerArgs", () => { + it("masks the value of sensitive env-var assignments", () => { + const args = [ + "run", + "--rm", + "-e", + "HOME=/ve-home", + "-e", + "GITHUB_TOKEN=ghu_ABCDEFGHIJKLMNOPQRSTUVWXYZ123456", + "-e", + "COPILOT_SDK_AUTH_TOKEN=gho_ABCDEFGHIJKLMNOPQRSTUVWXYZ123456", + "image", + ]; + const out = redactDockerArgs(args); + expect(out).toContain("GITHUB_TOKEN="); + expect(out).toContain("COPILOT_SDK_AUTH_TOKEN="); + // Non-sensitive values stay readable. + expect(out).toContain("HOME=/ve-home"); + expect(out.join(" ")).not.toContain("ghu_"); + expect(out.join(" ")).not.toContain("gho_"); + }); + + it("masks credentials embedded in URL-bearing args", () => { + const args = [ + "git", + "clone", + "https://x-access-token:gho_ABCDEFGHIJKLMNOPQRST@github.com/org/repo.git", + "/workspace", + ]; + const out = redactDockerArgs(args); + expect(out.join(" ")).not.toContain("gho_"); + expect(out.join(" ")).toContain("@github.com/org/repo.git"); + }); + + it("masks stray token-shaped values even without a sensitive key name", () => { + const out = redactDockerArgs(["-e", "FOO=ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ012345"]); + expect(out.join(" ")).not.toContain("ghp_"); + expect(out.join(" ")).toContain(""); + }); + + it("does not mask benign env-var names containing sensitive substrings", () => { + const args = ["AUTHOR_NAME=alice", "TOKENIZER_MODE=strict", "PATH=/usr/bin"]; + expect(redactDockerArgs(args)).toEqual(args); + }); + + it("masks sensitive assignments in long Docker env syntax", () => { + expect(redactDockerArgs(["--env=GITHUB_TOKEN=opaque-value"])).toEqual([ + "--env=GITHUB_TOKEN=", + ]); + }); +}); diff --git a/tests/unit/workspaceRunner.multiTarget.test.ts b/tests/unit/workspaceRunner.multiTarget.test.ts index 27c3f81f..147912c8 100644 --- a/tests/unit/workspaceRunner.multiTarget.test.ts +++ b/tests/unit/workspaceRunner.multiTarget.test.ts @@ -1,5 +1,20 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; +const { logger } = vi.hoisted(() => ({ + logger: { + trace: vi.fn(), + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + fatal: vi.fn(), + }, +})); + +vi.mock("../../src/logger.js", () => ({ + getLogger: vi.fn(() => logger), +})); + vi.mock("child_process", () => ({ spawn: vi.fn(), })); @@ -108,6 +123,28 @@ describe("DockerWorkspaceRunner.prepareProjectWorkspace", () => { expect(result.error).toContain("root clone failed"); }); + it("redacts credentials echoed by a failed root clone", async () => { + const repoUrl = + "https://x-access-token:ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ012345@github.com/org/root.git"; + execInVolumeMock.mockResolvedValueOnce({ + stdout: "", + stderr: `fatal: unable to access '${repoUrl}'`, + exitCode: 1, + }); + const runner = new DockerWorkspaceRunner( + { agentContainerImage: "ve:latest", agentTimeoutMs: 1000 }, + makeAdapter(), + ); + const handle = await runner.createWorkspace(makeTaskId("t3-redaction")); + + const result = await runner.prepareProjectWorkspace(handle, [ + makeTarget({ id: 1, commitOrder: 1, localPath: ".", cloneUrl: repoUrl }), + ]); + + expect(result.error).not.toContain("ghp_"); + expect(JSON.stringify(logger.error.mock.calls)).not.toContain("ghp_"); + }); + it("continues when a non-root target fails (best-effort)", async () => { // Root succeeds, second fails, third succeeds execInVolumeMock diff --git a/tests/unit/workspaceRunner.test.ts b/tests/unit/workspaceRunner.test.ts index b2ab1304..b9d3b167 100644 --- a/tests/unit/workspaceRunner.test.ts +++ b/tests/unit/workspaceRunner.test.ts @@ -3,6 +3,21 @@ import { EventEmitter } from "events"; // ─── Mocks ──────────────────────────────────────────────────────────────────── +const { logger } = vi.hoisted(() => ({ + logger: { + trace: vi.fn(), + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + fatal: vi.fn(), + }, +})); + +vi.mock("../../src/logger.js", () => ({ + getLogger: vi.fn(() => logger), +})); + vi.mock("child_process", () => { const spawn = vi.fn(); return { spawn }; @@ -966,6 +981,23 @@ describe("DockerWorkspaceRunner", () => { expect(result.localPath).toBe("/workspace"); }); + it("redacts credentials from clone logs without changing the Git command", async () => { + const runner = makeRunner(); + const handle = await runner.createWorkspace(makeTaskId("task-1")); + const repoUrl = + "https://x-access-token:ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ012345@github.com/org/repo.git"; + + await runner.cloneRepo(handle, repoUrl, "main"); + + expect(mockExecInVolume).toHaveBeenCalledWith( + expect.objectContaining({ command: expect.arrayContaining([repoUrl]) }) + ); + expect(logger.info).toHaveBeenCalledWith( + expect.objectContaining({ repoUrl: "https://@github.com/org/repo.git" }), + "cloning repository into volume" + ); + }); + it("returns a CloneResult with error details if clone fails", async () => { mockExecInVolume.mockResolvedValueOnce({ stdout: "", stderr: "Network error", exitCode: 1 }); const runner = makeRunner(); @@ -977,5 +1009,22 @@ describe("DockerWorkspaceRunner", () => { expect(result.error).toContain("Network error"); expect(result.localPath).toBe("/workspace"); }); + + it("redacts credentials echoed in clone errors", async () => { + const repoUrl = + "https://x-access-token:ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ012345@github.com/org/repo.git"; + mockExecInVolume.mockResolvedValueOnce({ + stdout: "", + stderr: `fatal: unable to access '${repoUrl}'`, + exitCode: 1, + }); + const runner = makeRunner(); + const handle = await runner.createWorkspace(makeTaskId("task-1")); + + const result = await runner.cloneRepo(handle, repoUrl, "main"); + + expect(result.error).not.toContain("ghp_"); + expect(JSON.stringify(logger.error.mock.calls)).not.toContain("ghp_"); + }); }); });