From f219411f8c6acd5e4c79043ebfb3ebf85111e8e4 Mon Sep 17 00:00:00 2001 From: YuliiaKovalova <95473390+YuliiaKovalova@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:04:35 +0200 Subject: [PATCH] Fix command-injection RCE in the CI integration `execCommand` passed `shell: true` on Windows. Node does not escape the `args` array when a shell is used - it concatenates the arguments into a command line handed to cmd.exe (DEP0190), so any argument containing `&`, `|`, `;`, `$`, `(` or `)` runs as a separate command. Every value that reached those arguments was attacker-influenced: - owner/repo from `parseGitHubRemote(git remote get-url origin)` - org/project from `parseAzdoRemote` - branch from `git rev-parse --abbrev-ref HEAD` - artifactName returned by the CI server Git permits shell metacharacters in ref names, and `parseGitHubRemote` matched `github.com` as a substring, so `https://evil-github.com/a/b` and `https://github.com/owner/repo&calc` both parsed successfully. Cloning a repo with a crafted remote URL or branch name and running "Download Binlog from CI" was remote code execution on Windows. Fixed in three independent layers. 1. No shell (src/commandResolver.ts, new) `shell: true` is gone. `resolveExecutable` searches PATH once per command name, honouring PATHEXT, restricted to `.exe/.com/.cmd/.bat`, never searching the working directory, and caches successful resolutions. Real executables are launched directly with `shell: false`. `.cmd`/`.bat` shims (az) go through `cmd.exe /d /s /c`. Note that `spawn('cmd.exe', ['/d','/s','/c', shim, ...args])` is NOT sufficient: libuv's `quote_cmd_arg` only quotes an argument that contains a space, tab or quote, so `main&calc` would arrive unquoted and cmd.exe would still split the command. Instead the command line is built and quoted here, then passed with `windowsVerbatimArguments: true` - still no shell. Characters that cannot be neutralised inside cmd.exe double quotes (`"`, `%`, `!`, CR, LF, NUL) are rejected rather than escaped. 2. Allow-list validation at the boundary (src/ciSafety.ts) `isValid*`/`assertValid*` for repo identifiers (`^[A-Za-z0-9._-]+$`), git refs (shell metacharacters, whitespace and control characters rejected), artifact names (no path separators or metacharacters) and run ids. Applied in every function that builds `gh`/`az` arguments and to manually entered org/project and owner/repo. Rejection surfaces a user-facing error naming the offending value. 3. Real URL parsing (src/gitRemoteParse.ts, new) The scp form `git@host:owner/repo` is handled explicitly and everything else goes through `new URL()`. GitHub requires hostname `github.com` or `*.github.com`; Azure DevOps requires `dev.azure.com`, `ssh.dev.azure.com` or `*.visualstudio.com`. Path segments are percent-decoded before validation so `%26`/`%2F` payloads are caught. GitHub Enterprise Server remains unsupported, as before, but the failure is now explicit ("GitHub Enterprise remotes are not supported.") instead of a generic "no CI provider found". `github.com.attacker.io` is deliberately classified as unrelated, not as Enterprise. Two further call sites had the same defect and are reachable from the same attack chain (a CI artifact may contain `evil&calc.binlog`): - buildCheck.ts `detectSdkVersion` used `shell: true`. - extension.ts `binlog.openInStructuredLogViewer` passed an attacker-influenced file path into `cmd /c start` unquoted. Tests - src/test/gitRemoteParse.test.ts (new): spoofed hosts, metacharacter payloads, legitimate remotes (dotted repo names, `.git` suffix, trailing slash, scp form), Enterprise classification, AzDO anchoring. - src/test/ciSafety.test.ts: the validators, `quoteForCmdExe`, `buildLaunchSpec` (direct exec vs. cmd shim vs. unresolvable, and that the working directory is never searched), plus a regression test that walks src/ and fails if any file ever gives child_process a truthy `shell` option. 316 tests pass. Behaviour changes worth noting: Azure DevOps project names containing spaces and CI artifact names containing parentheses are now rejected with an explicit message. Loosen `REPO_IDENTIFIER_RE` / `ARTIFACT_NAME_RE` in src/ciSafety.ts if those turn out to matter in practice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 64a43ea9-7f9c-4024-a1bf-cfe3c35a0ed2 --- src/buildCheck.ts | 12 +- src/ciIntegration.ts | 171 +++++++++++++----- src/ciSafety.ts | 99 +++++++++++ src/commandResolver.ts | 234 +++++++++++++++++++++++++ src/extension.ts | 21 ++- src/gitRemoteParse.ts | 211 ++++++++++++++++++++++ src/test/ciSafety.test.ts | 300 ++++++++++++++++++++++++++++++++ src/test/gitRemoteParse.test.ts | 242 ++++++++++++++++++++++++++ 8 files changed, 1244 insertions(+), 46 deletions(-) create mode 100644 src/commandResolver.ts create mode 100644 src/gitRemoteParse.ts create mode 100644 src/test/gitRemoteParse.test.ts diff --git a/src/buildCheck.ts b/src/buildCheck.ts index 7c726eb..13b9baa 100644 --- a/src/buildCheck.ts +++ b/src/buildCheck.ts @@ -2,6 +2,7 @@ import * as vscode from 'vscode'; import * as cp from 'child_process'; import * as path from 'path'; import * as fs from 'fs'; +import { buildLaunchSpec } from './commandResolver'; export interface BuildCheckResult { code: string; @@ -43,7 +44,16 @@ export function getBuildCheckDiagnostics(): vscode.DiagnosticCollection | undefi export async function detectSdkVersion(): Promise<{ supported: boolean; sdkVersion: string }> { return new Promise((resolve) => { - cp.execFile('dotnet', ['--version'], { timeout: 10000, shell: true }, (err, stdout) => { + // No `shell` option: `shell: true` makes Node concatenate `args` into a + // cmd.exe command line without escaping (DEP0190). `buildLaunchSpec` + // resolves the real executable (or its .cmd shim) instead. + const spec = buildLaunchSpec('dotnet', ['--version']); + cp.execFile(spec.file, spec.args, { + timeout: 10000, + encoding: 'utf8', + windowsHide: true, + windowsVerbatimArguments: spec.windowsVerbatimArguments, + }, (err, stdout) => { if (err) { resolve({ supported: false, sdkVersion: 'unknown' }); return; diff --git a/src/ciIntegration.ts b/src/ciIntegration.ts index d307209..fce4f02 100644 --- a/src/ciIntegration.ts +++ b/src/ciIntegration.ts @@ -10,7 +10,18 @@ import { safeBasename, psSingleQuote, assertNoZipSlip, + assertValidRepoIdentifier, + assertValidGitRef, + assertValidArtifactName, + assertValidRunId, } from './ciSafety'; +import { buildLaunchSpec, LaunchSpec } from './commandResolver'; +import { + parseAzdoRemote, + parseGitHubRemote, + parseGitHubRemoteDetailed, + GITHUB_ENTERPRISE_UNSUPPORTED_MESSAGE, +} from './gitRemoteParse'; // ─── Types ─────────────────────────────────────────────────────────────────── @@ -33,14 +44,40 @@ interface CiArtifact { // ─── Azure DevOps ──────────────────────────────────────────────────────────── async function execCommand(cmd: string, args: string[], cwd?: string, timeoutMs: number = 30000): Promise<{ stdout: string; stderr: string; code: number }> { + // Strip GITHUB_TOKEN from env to avoid interfering with gh CLI keyring auth + const env = { ...process.env }; + if (cmd === 'gh') { delete env.GITHUB_TOKEN; } + + // SECURITY: the `shell` option must never be enabled here. Node does not + // escape `args` when a shell is used — it concatenates them into a command + // line handed to cmd.exe, so any attacker-influenced argument containing + // `&`, `|`, `;`, `$`, `(` or `)` executes as a separate command (DEP0190). + // `buildLaunchSpec` resolves `.cmd`/`.bat` shims (az) explicitly instead, + // which is the only reason a shell was ever used. + let launch: LaunchSpec | undefined; + let launchError: string | undefined; + try { + launch = buildLaunchSpec(cmd, args); + } catch (err) { + launchError = err instanceof Error ? err.message : String(err); + } + if (!launch) { + return { stdout: '', stderr: launchError ?? `Could not resolve command '${cmd}'`, code: 1 }; + } + const spec = launch; + + const options: cp.ExecFileOptionsWithStringEncoding & { windowsVerbatimArguments?: boolean } = { + cwd, + timeout: timeoutMs, + maxBuffer: 5 * 1024 * 1024, + env, + encoding: 'utf8', + windowsHide: true, + windowsVerbatimArguments: spec.windowsVerbatimArguments, + }; + return new Promise((resolve) => { - // Strip GITHUB_TOKEN from env to avoid interfering with gh CLI keyring auth - const env = { ...process.env }; - if (cmd === 'gh') { delete env.GITHUB_TOKEN; } - // shell: true is needed on Windows for .cmd shims (az, gh, dotnet). - // execFile with shell: true still passes args as an array — Node.js - // handles proper escaping per-platform, so this is NOT a shell-injection risk. - cp.execFile(cmd, args, { cwd, timeout: timeoutMs, maxBuffer: 5 * 1024 * 1024, shell: true, env }, (err, stdout, stderr) => { + cp.execFile(spec.file, spec.args, options, (err, stdout, stderr) => { resolve({ stdout: stdout || '', stderr: stderr || '', code: err ? (err as any).code ?? 1 : 0 }); }); }); @@ -56,39 +93,6 @@ async function isGhCliAvailable(): Promise { return result.code === 0; } -/** Detect Azure DevOps org/project from any Azure DevOps URL */ -function parseAzdoRemote(remoteUrl: string): { org: string; project: string } | null { - // HTTPS: https://dev.azure.com/{org}/{project}/_git/{repo} - let m = remoteUrl.match(/dev\.azure\.com\/([^/]+)\/([^/]+)\/_git/); - if (m) { return { org: m[1], project: m[2] }; } - - // General AzDO URL: https://dev.azure.com/{org}/{project}/... - m = remoteUrl.match(/dev\.azure\.com\/([^/]+)\/([^/?]+)/); - if (m) { return { org: m[1], project: m[2] }; } - - // SSH: git@ssh.dev.azure.com:v3/{org}/{project}/{repo} - m = remoteUrl.match(/ssh\.dev\.azure\.com:v3\/([^/]+)\/([^/]+)\//); - if (m) { return { org: m[1], project: m[2] }; } - - // Old VSTS: https://{org}.visualstudio.com/{project}/_git/{repo} - m = remoteUrl.match(/([^/.]+)\.visualstudio\.com\/([^/]+)\/_git/); - if (m) { return { org: m[1], project: m[2] }; } - - // Old VSTS general: https://{org}.visualstudio.com/{project}/... - m = remoteUrl.match(/([^/.]+)\.visualstudio\.com\/([^/?]+)/); - if (m) { return { org: m[1], project: m[2] }; } - - return null; -} - -/** Detect GitHub owner/repo from git remote URL */ -function parseGitHubRemote(remoteUrl: string): { owner: string; repo: string } | null { - // HTTPS: https://github.com/{owner}/{repo}.git - let m = remoteUrl.match(/github\.com[/:]([^/]+)\/([^/.]+)/); - if (m) { return { owner: m[1], repo: m[2] }; } - return null; -} - async function getGitRemoteUrl(cwd?: string): Promise { const result = await execCommand('git', ['remote', 'get-url', 'origin'], cwd); return result.stdout.trim(); @@ -105,7 +109,39 @@ interface AzdoPipeline { folder: string; } +/** + * Validate Azure DevOps coordinates before they are interpolated into a URL or + * passed to `az`. Both values originate from a git remote URL or from user + * input, so neither is trustworthy. + */ +function assertAzdoCoordinates(org: string, project: string): void { + assertValidRepoIdentifier(org, 'Azure DevOps organization'); + assertValidRepoIdentifier(project, 'Azure DevOps project'); +} + +/** Same, for GitHub owner/repo. */ +function assertGitHubCoordinates(owner: string, repo: string): void { + assertValidRepoIdentifier(owner, 'GitHub owner'); + assertValidRepoIdentifier(repo, 'GitHub repository'); +} + +/** + * Run a validator and surface its message to the user. Returns `false` when + * validation failed, so callers can abort the flow with a clear explanation + * instead of letting a rejected value reach a CLI argument. + */ +function reportValidationFailure(check: () => void): boolean { + try { + check(); + return true; + } catch (err) { + vscode.window.showErrorMessage(err instanceof Error ? err.message : String(err)); + return false; + } +} + async function listAzdoPipelines(org: string, project: string): Promise { + assertAzdoCoordinates(org, project); const apiUrl = `https://dev.azure.com/${org}/${project}/_apis/pipelines?api-version=7.0`; try { const data = await httpsGetJson(apiUrl); @@ -133,6 +169,8 @@ async function listAzdoPipelines(org: string, project: string): Promise { + assertAzdoCoordinates(org, project); + if (opts?.branch) { assertValidGitRef(opts.branch, 'branch'); } const top = opts?.top ?? 20; let apiUrl = `https://dev.azure.com/${org}/${project}/_apis/build/builds?api-version=7.0&%24top=${top}&queryOrder=finishTimeDescending`; if (opts?.pipelineId !== undefined) { @@ -196,6 +234,9 @@ async function listAzdoBuilds(org: string, project: string, opts?: { pipelineId? } async function downloadAzdoArtifact(org: string, project: string, runId: string, artifactName: string, destDir: string): Promise { + assertAzdoCoordinates(org, project); + assertValidRunId(runId, 'build ID'); + assertValidArtifactName(artifactName); const safeName = safeBasename(artifactName); // Try direct ZIP download first (works for public projects without az CLI) const zipUrl = `https://dev.azure.com/${org}/${project}/_apis/build/builds/${runId}/artifacts?artifactName=${encodeURIComponent(artifactName)}&api-version=7.0&%24format=zip`; @@ -246,6 +287,8 @@ async function downloadAzdoArtifact(org: string, project: string, runId: string, } async function listAzdoArtifacts(org: string, project: string, runId: string): Promise { + assertAzdoCoordinates(org, project); + assertValidRunId(runId, 'build ID'); const apiUrl = `https://dev.azure.com/${org}/${project}/_apis/build/builds/${runId}/artifacts?api-version=7.0`; const errors: string[] = []; @@ -311,6 +354,8 @@ async function listAzdoArtifacts(org: string, project: string, runId: string): P // ─── GitHub Actions ────────────────────────────────────────────────────────── async function listGitHubRuns(owner: string, repo: string, top: number = 20, branch?: string): Promise { + assertGitHubCoordinates(owner, repo); + if (branch) { assertValidGitRef(branch, 'branch'); } const args = [ 'run', 'list', '--repo', `${owner}/${repo}`, @@ -343,6 +388,8 @@ async function listGitHubRuns(owner: string, repo: string, top: number = 20, bra } async function listGitHubArtifacts(owner: string, repo: string, runId: string): Promise { + assertGitHubCoordinates(owner, repo); + assertValidRunId(runId); // Use full JSON output instead of jq for robustness const result = await execCommand('gh', [ 'api', `repos/${owner}/${repo}/actions/runs/${runId}/artifacts`, @@ -368,6 +415,9 @@ async function listGitHubArtifacts(owner: string, repo: string, runId: string): } async function downloadGitHubArtifact(owner: string, repo: string, runId: string, artifactName: string, destDir: string): Promise { + assertGitHubCoordinates(owner, repo); + assertValidRunId(runId); + assertValidArtifactName(artifactName); const result = await execCommand('gh', [ 'run', 'download', runId, '--repo', `${owner}/${repo}`, @@ -587,7 +637,17 @@ async function doDownloadCiBinlog(): Promise { const remoteUrl = await getGitRemoteUrl(cwd); const azdoInfo = remoteUrl ? parseAzdoRemote(remoteUrl) : null; - const ghInfo = remoteUrl ? parseGitHubRemote(remoteUrl) : null; + const ghResult = remoteUrl ? parseGitHubRemoteDetailed(remoteUrl) : { kind: 'none' as const }; + const ghInfo = ghResult.kind === 'github' ? { owner: ghResult.owner, repo: ghResult.repo } : null; + + // GitHub Enterprise Server has never been supported. Say so explicitly + // instead of silently behaving as if no CI provider were detected. + if (ghResult.kind === 'enterprise') { + vscode.window.showWarningMessage( + `${GITHUB_ENTERPRISE_UNSUPPORTED_MESSAGE} The remote '${ghResult.host}' is a GitHub Enterprise host; ` + + `choose Azure DevOps, or enter a github.com owner/repo manually.` + ); + } // Always let user choose platform — repos often have CI on a different platform const platformPick = await vscode.window.showQuickPick( @@ -595,7 +655,9 @@ async function doDownloadCiBinlog(): Promise { { label: '$(github) GitHub Actions', value: 'github' as const, - description: ghInfo ? `${ghInfo.owner}/${ghInfo.repo}` : '', + description: ghInfo + ? `${ghInfo.owner}/${ghInfo.repo}` + : (ghResult.kind === 'enterprise' ? GITHUB_ENTERPRISE_UNSUPPORTED_MESSAGE : ''), }, { label: '$(cloud) Azure DevOps', @@ -649,6 +711,9 @@ async function doDownloadCiBinlog(): Promise { const parts = trimmed.split('/'); effectiveAzdoInfo = { org: parts[0], project: parts[1] }; } + if (!reportValidationFailure(() => assertAzdoCoordinates(effectiveAzdoInfo!.org, effectiveAzdoInfo!.project))) { + return; + } saveRecentAzdoOrg(`${effectiveAzdoInfo.org}/${effectiveAzdoInfo.project}`); // If URL contains buildId, skip straight to artifact selection const directBuildId = extractBuildId(trimmed); @@ -693,6 +758,9 @@ async function doDownloadCiBinlog(): Promise { const parts = trimmed.split('/'); effectiveGhInfo = { owner: parts[0], repo: parts[1] }; } + if (!reportValidationFailure(() => assertGitHubCoordinates(effectiveGhInfo!.owner, effectiveGhInfo!.repo))) { + return; + } saveRecentGhRepo(`${effectiveGhInfo.owner}/${effectiveGhInfo.repo}`); // If URL contains /actions/runs/{id}, skip straight to artifact selection const runMatch = trimmed.match(/\/actions\/runs\/(\d+)/); @@ -789,6 +857,8 @@ async function doDownloadCiBinlog(): Promise { } else { // GitHub: resolve PR number to head branch name try { + assertValidRunId(prNumber, 'PR number'); + assertGitHubCoordinates(effectiveGhInfo!.owner, effectiveGhInfo!.repo); const prResult = await execCommand('gh', [ 'pr', 'view', prNumber, '--repo', `${effectiveGhInfo!.owner}/${effectiveGhInfo!.repo}`, @@ -811,6 +881,14 @@ async function doDownloadCiBinlog(): Promise { branchFilter = filterPick.value; } + // Validate before the value can become a `--branch` argument. Git permits + // `&`, `|`, `;`, `$`, `(` and `)` in ref names, so a hostile branch name is + // an injection vector on its own. + const selectedBranch = branchFilter; + if (selectedBranch && !reportValidationFailure(() => assertValidGitRef(selectedBranch, 'branch'))) { + return; + } + // List builds let builds: CiBuild[]; try { @@ -1009,7 +1087,14 @@ async function pickAndDownloadArtifact( ); if (!artifactPick) { return; } - const destDir = path.join(getDownloadDir(), `${source}-${downloadArgs[2]}-${artifactPick.artifact.name}`); + // The artifact name comes from the CI server. Validate it before it becomes + // either a CLI argument or part of a filesystem path. + if (!reportValidationFailure(() => assertValidArtifactName(artifactPick.artifact.name))) { return; } + + const destDir = path.join( + getDownloadDir(), + safeBasename(`${source}-${downloadArgs[2]}-${artifactPick.artifact.name}`) + ); fs.mkdirSync(destDir, { recursive: true }); let binlogFiles: string[]; diff --git a/src/ciSafety.ts b/src/ciSafety.ts index bca9197..40f4b41 100644 --- a/src/ciSafety.ts +++ b/src/ciSafety.ts @@ -73,6 +73,105 @@ export function safeBasename(name: string): string { return base; } +// ─── Command-argument allow-lists ──────────────────────────────────────────── +// +// Every value below is attacker-influenced (git remote URL, branch name, or a +// string chosen by the CI server) and ends up as an argument to `gh` / `az`. +// The command runner no longer uses a shell, but these allow-lists are the +// second, independent layer: they make the values structurally incapable of +// carrying shell syntax, and they stop option injection (`--repo -x`). + +/** owner / repo / org / project. */ +const REPO_IDENTIFIER_RE = /^[A-Za-z0-9._-]+$/; + +/** Characters that are never legal in a value we hand to a CLI. */ +const CONTROL_CHARS_RE = /[\u0000-\u001F\u007F]/; + +/** + * Shell metacharacters plus whitespace. Git allows most of these in ref names, + * which is exactly why a branch name is a viable injection vector. + */ +const REF_FORBIDDEN_RE = /[\s&|;<>()$`'"\\!*?[\]{}~^%]/; + +/** Artifact names: letters, digits and a few punctuation characters only. */ +const ARTIFACT_NAME_RE = /^[A-Za-z0-9 ._+=-]+$/; + +const MAX_IDENTIFIER_LENGTH = 100; +const MAX_REF_LENGTH = 255; + +/** True for a value usable as a GitHub owner/repo or an Azure DevOps org/project. */ +export function isValidRepoIdentifier(value: unknown): value is string { + return typeof value === 'string' + && value.length > 0 + && value.length <= MAX_IDENTIFIER_LENGTH + && REPO_IDENTIFIER_RE.test(value) + && !value.startsWith('-') + && value !== '.' + && value !== '..'; +} + +/** True for a value usable as a git branch / ref argument. */ +export function isValidGitRef(value: unknown): value is string { + return typeof value === 'string' + && value.length > 0 + && value.length <= MAX_REF_LENGTH + && !CONTROL_CHARS_RE.test(value) + && !REF_FORBIDDEN_RE.test(value) + && !value.startsWith('-') + && !value.startsWith('/') + && !value.endsWith('/') + && !value.includes('..'); +} + +/** True for a CI artifact name we are willing to pass to a CLI. */ +export function isValidArtifactName(value: unknown): value is string { + return typeof value === 'string' + && value.length > 0 + && value.length <= MAX_REF_LENGTH + && ARTIFACT_NAME_RE.test(value) + && !value.startsWith('-') + && !value.includes('..') + && value.trim().length > 0; +} + +/** True for a CI build/run identifier. */ +export function isValidRunId(value: unknown): value is string { + return typeof value === 'string' && /^[0-9]{1,20}$/.test(value); +} + +function reject(label: string, value: unknown, requirement: string): never { + throw new Error( + `Refusing to run CI command: invalid ${label} ${JSON.stringify(String(value))}. ${requirement}` + ); +} + +/** + * Validate a GitHub owner/repo or Azure DevOps org/project. + * `label` is used verbatim in the error message shown to the user. + */ +export function assertValidRepoIdentifier(value: unknown, label: string): string { + if (isValidRepoIdentifier(value)) { return value; } + reject(label, value, `Only letters, digits, '.', '_' and '-' are allowed.`); +} + +/** Validate a branch name / git ref before it reaches a CLI argument. */ +export function assertValidGitRef(value: unknown, label: string = 'branch'): string { + if (isValidGitRef(value)) { return value; } + reject(label, value, `Shell metacharacters, whitespace and control characters are not allowed.`); +} + +/** Validate a CI artifact name before it reaches a CLI argument. */ +export function assertValidArtifactName(value: unknown, label: string = 'artifact name'): string { + if (isValidArtifactName(value)) { return value; } + reject(label, value, `Path separators and shell metacharacters are not allowed.`); +} + +/** Validate a CI build/run identifier before it reaches a CLI argument. */ +export function assertValidRunId(value: unknown, label: string = 'run ID'): string { + if (isValidRunId(value)) { return value; } + reject(label, value, `Only digits are allowed.`); +} + /** * Escape a string so it is safe to embed inside a single-quoted PowerShell * literal: PowerShell escapes `'` by doubling it (`''`). Returns the value diff --git a/src/commandResolver.ts b/src/commandResolver.ts new file mode 100644 index 0000000..31aa5c3 --- /dev/null +++ b/src/commandResolver.ts @@ -0,0 +1,234 @@ +/** + * Safe executable resolution for `child_process.execFile`. + * + * Historically `execCommand` in `ciIntegration.ts` passed `shell: true` on + * Windows so that the `.cmd` shims used by `az` (and previously `gh`) could be + * launched at all. That is a remote-code-execution hazard: Node does **not** + * escape the `args` array when a shell is used — it concatenates the arguments + * into a single command line that is handed to `cmd.exe`, so any argument + * containing `&`, `|`, `;`, `(`, `)` … starts a second command. Node reports + * this as DEP0190. + * + * This module solves the `.cmd` problem properly instead: + * + * - `resolveExecutable` searches `PATH` (honouring `PATHEXT`) once per command + * name and returns the absolute path of the real file. + * - `buildLaunchSpec` turns a command + argv into something that can be handed + * to `execFile` **without** a shell. Real executables (`.exe`, `.com`) are + * launched directly. `.cmd`/`.bat` shims are launched through `cmd.exe /d /s + * /c` with a command line we build and quote ourselves, and any argument + * that cannot be represented safely inside that command line is rejected + * outright rather than escaped. + * + * Nothing in this module imports `vscode`, so it is unit-testable. + */ +import * as fs from 'fs'; +import * as path from 'path'; + +const IS_WINDOWS = process.platform === 'win32'; + +/** Extensions that `CreateProcess` can launch directly. */ +const DIRECT_EXEC_EXTENSIONS = ['.exe', '.com']; + +/** Extensions that must be interpreted by `cmd.exe`. */ +const SHIM_EXTENSIONS = ['.cmd', '.bat']; + +/** + * Everything `execFile` needs in order to launch a command without a shell. + */ +export interface LaunchSpec { + /** Executable to launch (absolute path when resolution succeeded). */ + file: string; + /** Argument vector, already adapted for `file`. */ + args: string[]; + /** + * True only for the `cmd.exe` shim path, where `args` is a pre-built, + * fully quoted command line that must not be re-quoted by libuv. + */ + windowsVerbatimArguments: boolean; +} + +/** Successful resolutions only — a negative result is cheap to recompute and + * caching it would hide a CLI that the user installs mid-session. */ +const resolveCache = new Map(); + +/** Test seam: forget every cached resolution. */ +export function clearCommandResolutionCache(): void { + resolveCache.clear(); +} + +function isFile(candidate: string): boolean { + try { + return fs.statSync(candidate).isFile(); + } catch { + return false; + } +} + +function isExecutableFile(candidate: string): boolean { + if (!isFile(candidate)) { return false; } + if (IS_WINDOWS) { return true; } + try { + fs.accessSync(candidate, fs.constants.X_OK); + return true; + } catch { + return false; + } +} + +/** + * Extensions to probe, in `PATHEXT` order, restricted to the ones we know how + * to launch safely. `.vbs`, `.js`, `.wsf` … are deliberately excluded: they + * require a script host and would reintroduce an interpreter we do not control. + */ +function candidateExtensions(): string[] { + if (!IS_WINDOWS) { return ['']; } + const known = [...DIRECT_EXEC_EXTENSIONS, ...SHIM_EXTENSIONS]; + const fromEnv = (process.env.PATHEXT || '') + .split(';') + .map(ext => ext.trim().toLowerCase()) + .filter(ext => known.includes(ext)); + return fromEnv.length > 0 ? fromEnv : known; +} + +function searchDirectories(): string[] { + const raw = process.env.PATH ?? process.env.Path ?? ''; + return raw + .split(path.delimiter) + .map(dir => dir.trim()) + // Windows allows quoted PATH entries; strip them before use. + .map(dir => (dir.startsWith('"') && dir.endsWith('"') ? dir.slice(1, -1) : dir)) + .filter(dir => dir.length > 0); +} + +function probe(basePath: string): string | null { + if (IS_WINDOWS) { + // An explicit extension wins, but only if we know how to launch it. + const ext = path.extname(basePath).toLowerCase(); + if (ext && [...DIRECT_EXEC_EXTENSIONS, ...SHIM_EXTENSIONS].includes(ext)) { + return isExecutableFile(basePath) ? basePath : null; + } + } else if (path.extname(basePath) && isExecutableFile(basePath)) { + return basePath; + } + for (const ext of candidateExtensions()) { + const candidate = basePath + ext; + if (isExecutableFile(candidate)) { return candidate; } + } + return null; +} + +/** + * Resolve `cmd` to an absolute path by searching `PATH` (and `PATHEXT` on + * Windows). Returns `null` when the command cannot be found. The current + * working directory is intentionally *not* searched — that is a classic + * binary-planting vector. + */ +export function resolveExecutable(cmd: string): string | null { + if (!cmd) { return null; } + + const cached = resolveCache.get(cmd); + if (cached !== undefined) { return cached; } + + let resolved: string | null = null; + + if (cmd.includes('/') || cmd.includes('\\') || path.isAbsolute(cmd)) { + resolved = probe(path.resolve(cmd)); + } else { + for (const dir of searchDirectories()) { + resolved = probe(path.join(dir, cmd)); + if (resolved) { break; } + } + } + + if (resolved) { + resolved = path.resolve(resolved); + resolveCache.set(cmd, resolved); + } + return resolved; +} + +/** + * Characters that cannot be represented safely inside the `cmd.exe` command + * line we build: + * + * - `"` would unbalance the quoting we rely on to neutralise `& | < > ( ) ^`. + * - `%` is expanded by `cmd.exe` even inside double quotes. + * - `!` is expanded when delayed expansion is enabled machine-wide. + * - CR / LF / NUL terminate or split the command line. + * + * We reject rather than escape: escaping `cmd.exe` correctly is famously + * error-prone, and no legitimate CI identifier needs these characters. + */ +const CMD_UNREPRESENTABLE = /["%!\r\n\u0000]/; + +/** + * Quote a single argument for inclusion in a `cmd.exe /d /s /c "…"` command + * line. Throws for arguments that cannot be represented safely. + */ +export function quoteForCmdExe(arg: string): string { + if (typeof arg !== 'string') { + throw new TypeError('quoteForCmdExe: argument must be a string'); + } + const unsafe = CMD_UNREPRESENTABLE.exec(arg); + if (unsafe) { + throw new Error( + `Refusing to run command: argument contains a character that cannot be passed ` + + `safely through cmd.exe (${JSON.stringify(unsafe[0])} in ${JSON.stringify(arg)})` + ); + } + // Wrapping in quotes makes `& | < > ( ) ^` literal for cmd.exe. A run of + // trailing backslashes would otherwise escape the closing quote when the + // target process re-parses the command line, so double it. + const escaped = arg.replace(/(\\+)$/, '$1$1'); + return `"${escaped}"`; +} + +function comSpec(): string { + const fromEnv = process.env.ComSpec || process.env.COMSPEC; + if (fromEnv && isFile(fromEnv)) { return fromEnv; } + const systemRoot = process.env.SystemRoot || process.env.windir; + if (systemRoot) { + const fallback = path.join(systemRoot, 'System32', 'cmd.exe'); + if (isFile(fallback)) { return fallback; } + } + return 'cmd.exe'; +} + +/** + * Build a `cmd.exe /d /s /c ""` launch spec for `args`, quoting every + * token ourselves. Throws when an argument cannot be represented safely. + */ +export function buildCmdExeLaunch(args: readonly string[]): LaunchSpec { + const commandLine = args.map(quoteForCmdExe).join(' '); + return { + file: comSpec(), + // /d skips AutoRun commands, /s makes the outer quote stripping + // deterministic, /c runs and exits. + args: ['/d', '/s', '/c', `"${commandLine}"`], + windowsVerbatimArguments: true, + }; +} + +/** + * Build everything `execFile` needs to run `cmd` with `args` and **no shell**. + * + * Throws when an argument cannot be passed safely through the `cmd.exe` shim + * path; callers should surface the message to the user. + */ +export function buildLaunchSpec(cmd: string, args: readonly string[]): LaunchSpec { + const resolved = resolveExecutable(cmd); + + if (!resolved) { + // Let `execFile` do its own lookup and fail with the usual ENOENT. + // Still no shell — a missing CLI must never become a shell invocation. + return { file: cmd, args: [...args], windowsVerbatimArguments: false }; + } + + const ext = path.extname(resolved).toLowerCase(); + if (IS_WINDOWS && SHIM_EXTENSIONS.includes(ext)) { + return buildCmdExeLaunch([resolved, ...args]); + } + + return { file: resolved, args: [...args], windowsVerbatimArguments: false }; +} diff --git a/src/extension.ts b/src/extension.ts index 6644c1b..b41826c 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -5,6 +5,7 @@ import { BinlogTreeDataProvider, BinlogTreeItem, AboutInfo } from './binlogTreeV import { McpClient, buildMcpArgs } from './mcpClient'; import { BinlogDocumentProvider, BINLOG_SCHEME, openBinlogDocument } from './binlogDocumentProvider'; import { downloadCiBinlog, setCiContext } from './ciIntegration'; +import { buildCmdExeLaunch } from './commandResolver'; import { initBuildCheckDiagnostics, runBuildCheck, pushBuildCheckToProblemsPanel, formatBuildCheckForChat, detectSdkVersion, buildWithPropertyTracking, @@ -717,8 +718,24 @@ export async function activate(context: vscode.ExtensionContext) { return; } // Launch with OS default app (Structured Log Viewer registers .binlog) + // The path can come from a CI-downloaded artifact, so its file name is + // attacker-influenced. `execFile` does not escape `&`/`|`/`;` for + // cmd.exe, so quote the whole command line ourselves. const { execFile } = require('child_process'); - execFile('cmd', ['/c', 'start', '', targetPath], { shell: false }, (err: Error | null) => { + const revealInExplorer = () => execFile('explorer', ['/select,', targetPath], { shell: false }); + let startSpec; + try { + startSpec = buildCmdExeLaunch(['start', '', targetPath!]); + } catch { + // Path contains a character cmd.exe cannot carry safely (`%`, `!`, + // `"`). explorer.exe opens the default handler without a shell. + execFile('explorer', [targetPath], { shell: false }); + return; + } + execFile(startSpec.file, startSpec.args, { + shell: false, + windowsVerbatimArguments: startSpec.windowsVerbatimArguments, + }, (err: Error | null) => { if (err) { // No app registered for .binlog — offer to reveal in Explorer or install SLV vscode.window.showWarningMessage( @@ -729,7 +746,7 @@ export async function activate(context: vscode.ExtensionContext) { if (choice === 'Download Viewer') { vscode.env.openExternal(vscode.Uri.parse('https://msbuildlog.com/')); } else if (choice === 'Reveal in Explorer') { - execFile('explorer', ['/select,', targetPath], { shell: false }); + revealInExplorer(); } }); } diff --git a/src/gitRemoteParse.ts b/src/gitRemoteParse.ts new file mode 100644 index 0000000..0571142 --- /dev/null +++ b/src/gitRemoteParse.ts @@ -0,0 +1,211 @@ +/** + * Parsing of git remote URLs into CI coordinates. + * + * These parsers are a security boundary: whatever they return is passed + * straight to `gh` / `az` as command arguments, so they must never accept a + * host they do not fully control, and they must never emit an owner/repo/org/ + * project containing shell metacharacters. + * + * The previous implementation matched `github.com` as a *substring*, so + * `https://evil-github.com/attacker/payload` and + * `https://notgithub.com/attacker/payload` both parsed as GitHub repositories. + * This module does real URL parsing and anchors on the hostname instead. + * + * Nothing here imports `vscode`, so it is unit-testable. + */ +import { URL } from 'url'; +import { isValidRepoIdentifier } from './ciSafety'; + +export interface GitHubRemote { + owner: string; + repo: string; +} + +export interface AzdoRemote { + org: string; + project: string; +} + +/** + * Outcome of parsing a remote as GitHub. + * + * `enterprise` exists purely so callers can say *why* nothing happened: + * GitHub Enterprise Server hosts have never been supported, and previously + * failed with a generic "no CI provider" experience. + */ +export type GitHubRemoteParseResult = + | { kind: 'github'; owner: string; repo: string } + | { kind: 'enterprise'; host: string } + | { kind: 'none' }; + +/** User-facing explanation for GitHub Enterprise Server remotes. */ +export const GITHUB_ENTERPRISE_UNSUPPORTED_MESSAGE = + 'GitHub Enterprise remotes are not supported.'; + +/** + * `[user@]host:path` (scp-like) remotes, e.g. `git@github.com:owner/repo.git`. + * The host must contain a dot so that `https:`/`ssh:` URL schemes and Windows + * drive letters (`C:\…`) can never be mistaken for a hostname. + */ +const SCP_LIKE_REMOTE = /^(?:[^@/\\\s]+@)?([A-Za-z0-9-]+(?:\.[A-Za-z0-9-]+)+):(?![/\\])(.*)$/; + +/** Schemes git uses that carry a hostname. */ +const SUPPORTED_PROTOCOLS = new Set(['http:', 'https:', 'ssh:', 'git:', 'git+ssh:', 'git+https:']); + +/** `host/path` without a scheme, e.g. `github.com/owner/repo`. */ +const SCHEMELESS_WITH_HOST = /^[A-Za-z0-9-]+(?:\.[A-Za-z0-9-]+)+(?::\d+)?\//; + +const HAS_SCHEME = /^[A-Za-z][A-Za-z0-9+.-]*:\/\//; + +export interface RemoteParts { + hostname: string; + /** Raw (still percent-encoded) path, without leading slash guarantees. */ + pathname: string; +} + +/** + * Split any git remote form into hostname + path. Returns `null` when the + * input is not a remote URL we understand (e.g. a bare `owner/repo`). + */ +export function splitRemoteUrl(remoteUrl: string): RemoteParts | null { + if (typeof remoteUrl !== 'string') { return null; } + const trimmed = remoteUrl.trim(); + if (!trimmed) { return null; } + + const scp = SCP_LIKE_REMOTE.exec(trimmed); + if (scp) { + return { hostname: normalizeHost(scp[1]), pathname: scp[2] }; + } + + let candidate = trimmed; + if (!HAS_SCHEME.test(candidate)) { + if (!SCHEMELESS_WITH_HOST.test(candidate)) { return null; } + candidate = 'https://' + candidate; + } + + let url: URL; + try { + url = new URL(candidate); + } catch { + return null; + } + if (!SUPPORTED_PROTOCOLS.has(url.protocol)) { return null; } + if (!url.hostname) { return null; } + return { hostname: normalizeHost(url.hostname), pathname: url.pathname }; +} + +function normalizeHost(host: string): string { + // Lower-case, drop the root-label dot, and strip IPv6 brackets so a + // bracketed literal can never look like a suffix match. + return host.toLowerCase().replace(/\.$/, '').replace(/^\[|\]$/g, ''); +} + +/** Decode and drop empty path segments. Returns `null` on malformed escapes. */ +function pathSegments(pathname: string): string[] | null { + const raw = pathname.split('/').filter(segment => segment.length > 0); + const decoded: string[] = []; + for (const segment of raw) { + try { + decoded.push(decodeURIComponent(segment)); + } catch { + return null; + } + } + return decoded; +} + +/** + * Classify a hostname as github.com, a GitHub Enterprise Server host, or + * something unrelated. + * + * `github.com.attacker.io` is deliberately *not* treated as Enterprise: only a + * `github..` shape qualifies, and a second label of `com` is the + * classic look-alike pattern. + */ +export function classifyGitHubHost(hostname: string): 'github' | 'enterprise' | 'other' { + const host = normalizeHost(hostname); + if (host === 'github.com' || host.endsWith('.github.com')) { return 'github'; } + const labels = host.split('.'); + if (labels.length >= 3 && labels[0] === 'github' && labels[1] !== 'com') { + return 'enterprise'; + } + return 'other'; +} + +/** + * Parse a GitHub remote, distinguishing "not GitHub" from "GitHub Enterprise, + * which we do not support". + */ +export function parseGitHubRemoteDetailed(remoteUrl: string): GitHubRemoteParseResult { + const parts = splitRemoteUrl(remoteUrl); + if (!parts) { return { kind: 'none' }; } + + const classification = classifyGitHubHost(parts.hostname); + if (classification === 'other') { return { kind: 'none' }; } + if (classification === 'enterprise') { return { kind: 'enterprise', host: parts.hostname }; } + + const segments = pathSegments(parts.pathname); + if (!segments || segments.length < 2) { return { kind: 'none' }; } + + const owner = segments[0]; + let repo = segments[1]; + if (repo.length > 4 && repo.toLowerCase().endsWith('.git')) { + repo = repo.slice(0, -4); + } + + if (!isValidRepoIdentifier(owner) || !isValidRepoIdentifier(repo)) { + return { kind: 'none' }; + } + return { kind: 'github', owner, repo }; +} + +/** Detect GitHub owner/repo from a git remote URL. `null` when unsupported. */ +export function parseGitHubRemote(remoteUrl: string): GitHubRemote | null { + const result = parseGitHubRemoteDetailed(remoteUrl); + return result.kind === 'github' ? { owner: result.owner, repo: result.repo } : null; +} + +/** True when the remote points at a GitHub Enterprise Server installation. */ +export function isGitHubEnterpriseRemote(remoteUrl: string): boolean { + return parseGitHubRemoteDetailed(remoteUrl).kind === 'enterprise'; +} + +/** Azure DevOps route markers that can never be an org or a project. */ +function isRouteMarker(segment: string | undefined): boolean { + return !segment || segment.startsWith('_'); +} + +/** + * Detect an Azure DevOps org/project from a remote URL. Only `dev.azure.com`, + * `ssh.dev.azure.com` and `.visualstudio.com` are accepted. + */ +export function parseAzdoRemote(remoteUrl: string): AzdoRemote | null { + const parts = splitRemoteUrl(remoteUrl); + if (!parts) { return null; } + + const host = parts.hostname; + const segments = pathSegments(parts.pathname); + if (!segments) { return null; } + + let org: string | undefined; + let project: string | undefined; + + if (host === 'dev.azure.com' || host === 'ssh.dev.azure.com') { + // ssh form is `v3/{org}/{project}/{repo}`; https form is `{org}/{project}/…` + const rest = segments[0]?.toLowerCase() === 'v3' ? segments.slice(1) : segments; + org = rest[0]; + project = rest[1]; + } else if (host.endsWith('.visualstudio.com') && host.split('.').length >= 3) { + org = host.split('.')[0]; + project = segments[0]; + if (project?.toLowerCase() === 'defaultcollection') { + project = segments[1]; + } + } else { + return null; + } + + if (isRouteMarker(org) || isRouteMarker(project)) { return null; } + if (!isValidRepoIdentifier(org!) || !isValidRepoIdentifier(project!)) { return null; } + return { org: org!, project: project! }; +} diff --git a/src/test/ciSafety.test.ts b/src/test/ciSafety.test.ts index 96ccf08..61c51b7 100644 --- a/src/test/ciSafety.test.ts +++ b/src/test/ciSafety.test.ts @@ -8,7 +8,20 @@ import { safeBasename, psSingleQuote, assertNoZipSlip, + isValidRepoIdentifier, + isValidGitRef, + isValidArtifactName, + isValidRunId, + assertValidRepoIdentifier, + assertValidGitRef, + assertValidArtifactName, + assertValidRunId, } from '../ciSafety'; +import { + buildLaunchSpec, + quoteForCmdExe, + clearCommandResolutionCache, +} from '../commandResolver'; suite('ciSafety', () => { @@ -158,3 +171,290 @@ suite('ciSafety', () => { }); }); }); + +suite('ciSafety — command argument allow-lists', () => { + + suite('isValidRepoIdentifier / assertValidRepoIdentifier', () => { + test('accepts real owners, repos, orgs and projects', () => { + for (const value of ['dotnet', 'templating', 'My.Repo', 'dnceng-public', 'a_b', 'x1', 'Repo.Name-2']) { + assert.strictEqual(isValidRepoIdentifier(value), true, `expected ${value} to be accepted`); + assert.strictEqual(assertValidRepoIdentifier(value, 'repo'), value); + } + }); + test('rejects shell metacharacters', () => { + for (const value of ['repo&calc', 'repo;whoami', 'repo|x', 'repo$(calc)', 'repo`calc`', + 'repo>out', 'repo { + for (const value of ['a/b', 'a\\b', 'a b', 'a\tb', 'a\nb', 'a\u0000b']) { + assert.strictEqual(isValidRepoIdentifier(value), false, `expected ${JSON.stringify(value)} to be rejected`); + } + }); + test('rejects option injection and traversal', () => { + assert.strictEqual(isValidRepoIdentifier('-oProxyCommand=calc'), false); + assert.strictEqual(isValidRepoIdentifier('--repo'), false); + assert.strictEqual(isValidRepoIdentifier('..'), false); + assert.strictEqual(isValidRepoIdentifier('.'), false); + }); + test('rejects empty and non-string values', () => { + assert.strictEqual(isValidRepoIdentifier(''), false); + assert.strictEqual(isValidRepoIdentifier(undefined), false); + assert.strictEqual(isValidRepoIdentifier(null), false); + assert.strictEqual(isValidRepoIdentifier(42), false); + }); + test('the error names the offending value and the field', () => { + assert.throws( + () => assertValidRepoIdentifier('repo&calc', 'GitHub repository'), + /GitHub repository.*repo&calc/ + ); + }); + }); + + suite('isValidGitRef / assertValidGitRef', () => { + test('accepts real branch names and refs', () => { + for (const value of ['main', 'feature/my-branch', 'release/10.0.3xx', + 'refs/heads/main', 'refs/pull/12345/merge', 'v1.2.3', 'user/fix-#42']) { + assert.strictEqual(isValidGitRef(value), true, `expected ${value} to be accepted`); + assert.strictEqual(assertValidGitRef(value), value); + } + }); + test('rejects the verified injection payloads', () => { + assert.strictEqual(isValidGitRef('main&calc'), false); + assert.strictEqual(isValidGitRef('main&echo X'), false); + assert.strictEqual(isValidGitRef('$(calc)'), false); + assert.strictEqual(isValidGitRef('main|calc'), false); + assert.strictEqual(isValidGitRef('main;whoami'), false); + assert.strictEqual(isValidGitRef('main`calc`'), false); + assert.strictEqual(isValidGitRef('main>out.txt'), false); + assert.strictEqual(isValidGitRef('%COMSPEC%'), false); + }); + test('rejects control characters and whitespace', () => { + assert.strictEqual(isValidGitRef('main\ncalc'), false); + assert.strictEqual(isValidGitRef('main\rcalc'), false); + assert.strictEqual(isValidGitRef('main\u0000'), false); + assert.strictEqual(isValidGitRef('my branch'), false); + }); + test('rejects option injection and traversal', () => { + assert.strictEqual(isValidGitRef('--upload-pack=calc'), false); + assert.strictEqual(isValidGitRef('a/../b'), false); + assert.strictEqual(isValidGitRef('/main'), false); + assert.strictEqual(isValidGitRef('main/'), false); + assert.strictEqual(isValidGitRef(''), false); + }); + test('the error names the offending value', () => { + assert.throws(() => assertValidGitRef('main&calc'), /branch.*main&calc/); + }); + }); + + suite('isValidArtifactName / assertValidArtifactName', () => { + test('accepts realistic artifact names', () => { + for (const value of ['BuildLogs', 'build_Debug', 'binlogs-net8.0', 'Windows x64 logs', 'logs.zip']) { + assert.strictEqual(isValidArtifactName(value), true, `expected ${value} to be accepted`); + } + }); + test('rejects path separators and traversal', () => { + assert.strictEqual(isValidArtifactName('a/b'), false); + assert.strictEqual(isValidArtifactName('a\\b'), false); + assert.strictEqual(isValidArtifactName('../../etc/passwd'), false); + assert.strictEqual(isValidArtifactName('..'), false); + }); + test('rejects shell metacharacters', () => { + for (const value of ['logs&calc', 'logs;whoami', 'logs|x', 'logs$(calc)', 'logs`calc`', + 'logs%PATH%', 'logs!x', 'logs"x', 'logs(1)']) { + assert.strictEqual(isValidArtifactName(value), false, `expected ${value} to be rejected`); + } + }); + test('rejects empty and option-like names', () => { + assert.strictEqual(isValidArtifactName(''), false); + assert.strictEqual(isValidArtifactName('-dir'), false); + assert.strictEqual(isValidArtifactName(undefined), false); + }); + test('the error names the offending value', () => { + assert.throws(() => assertValidArtifactName('logs&calc'), /artifact name.*logs&calc/); + }); + }); + + suite('isValidRunId / assertValidRunId', () => { + test('accepts numeric ids', () => { + assert.strictEqual(isValidRunId('1354651'), true); + assert.strictEqual(assertValidRunId('23634010652'), '23634010652'); + }); + test('rejects anything else', () => { + assert.strictEqual(isValidRunId('123&calc'), false); + assert.strictEqual(isValidRunId('-1'), false); + assert.strictEqual(isValidRunId(''), false); + assert.strictEqual(isValidRunId(123 as unknown as string), false); + }); + }); +}); + +suite('commandResolver — launching without a shell', () => { + let tmpDir: string; + let originalPath: string | undefined; + let originalPathExt: string | undefined; + + setup(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'binlog-cmdresolve-')); + originalPath = process.env.PATH; + originalPathExt = process.env.PATHEXT; + clearCommandResolutionCache(); + }); + + teardown(() => { + if (originalPath === undefined) { delete process.env.PATH; } else { process.env.PATH = originalPath; } + if (originalPathExt === undefined) { delete process.env.PATHEXT; } else { process.env.PATHEXT = originalPathExt; } + clearCommandResolutionCache(); + try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch { /* */ } + }); + + test('a real executable is launched directly with an untouched argv', () => { + const name = 'faketool'; + const file = path.join(tmpDir, process.platform === 'win32' ? `${name}.exe` : name); + fs.writeFileSync(file, ''); + if (process.platform !== 'win32') { fs.chmodSync(file, 0o755); } + process.env.PATH = tmpDir; + process.env.PATHEXT = '.COM;.EXE;.BAT;.CMD'; + + const spec = buildLaunchSpec(name, ['run', 'list', '--branch', 'main&calc']); + assert.strictEqual(spec.file, file); + assert.deepStrictEqual(spec.args, ['run', 'list', '--branch', 'main&calc']); + assert.strictEqual(spec.windowsVerbatimArguments, false); + }); + + test('a .cmd shim runs through cmd.exe with every argument quoted', function() { + if (process.platform !== 'win32') { this.skip(); return; } + const shim = path.join(tmpDir, 'faketool.cmd'); + fs.writeFileSync(shim, '@echo off\r\n'); + process.env.PATH = tmpDir; + process.env.PATHEXT = '.COM;.EXE;.BAT;.CMD'; + + const spec = buildLaunchSpec('faketool', ['run', 'list', '--branch', 'main&calc']); + assert.ok(/cmd\.exe$/i.test(spec.file), `expected cmd.exe, got ${spec.file}`); + assert.strictEqual(spec.windowsVerbatimArguments, true); + assert.deepStrictEqual(spec.args.slice(0, 3), ['/d', '/s', '/c']); + + // cmd.exe strips the outermost pair of quotes; what remains must consist + // exclusively of quoted tokens, so `&` can never act as a separator. + const inner = spec.args[3].slice(1, -1); + assert.ok(inner.includes('"main&calc"'), `expected the payload to stay quoted, got ${inner}`); + assert.strictEqual(inner.replace(/"[^"]*"/g, '').trim(), ''); + assert.ok(inner.includes(`"${shim}"`)); + }); + + test('an unresolvable command still never asks for a shell', () => { + process.env.PATH = path.join(tmpDir, 'nowhere'); + const spec = buildLaunchSpec('definitely-not-installed-xyz', ['--version']); + assert.strictEqual(spec.file, 'definitely-not-installed-xyz'); + assert.deepStrictEqual(spec.args, ['--version']); + assert.strictEqual(spec.windowsVerbatimArguments, false); + }); + + test('the current directory is never searched', function() { + if (process.platform !== 'win32') { this.skip(); return; } + fs.writeFileSync(path.join(tmpDir, 'planted.exe'), ''); + process.env.PATH = path.join(tmpDir, 'nowhere'); + const originalCwd = process.cwd(); + try { + process.chdir(tmpDir); + const spec = buildLaunchSpec('planted', []); + assert.strictEqual(spec.file, 'planted', 'a binary in the cwd must not be resolved'); + } finally { + process.chdir(originalCwd); + } + }); + + suite('quoteForCmdExe', () => { + test('quotes ordinary arguments', () => { + assert.strictEqual(quoteForCmdExe('main'), '"main"'); + assert.strictEqual(quoteForCmdExe('a b'), '"a b"'); + }); + test('neutralises cmd.exe separators by quoting them', () => { + assert.strictEqual(quoteForCmdExe('main&calc'), '"main&calc"'); + assert.strictEqual(quoteForCmdExe('a|b'), '"a|b"'); + assert.strictEqual(quoteForCmdExe('a>b'), '"a>b"'); + assert.strictEqual(quoteForCmdExe('a^b'), '"a^b"'); + assert.strictEqual(quoteForCmdExe('$(calc)'), '"$(calc)"'); + }); + test('doubles trailing backslashes so the closing quote survives', () => { + assert.strictEqual(quoteForCmdExe('C:\\tmp\\'), '"C:\\tmp\\\\"'); + assert.strictEqual(quoteForCmdExe('C:\\tmp'), '"C:\\tmp"'); + }); + test('refuses characters that cannot be neutralised', () => { + assert.throws(() => quoteForCmdExe('a"b'), /cannot be passed/); + assert.throws(() => quoteForCmdExe('%COMSPEC%'), /cannot be passed/); + assert.throws(() => quoteForCmdExe('a!b!'), /cannot be passed/); + assert.throws(() => quoteForCmdExe('a\nb'), /cannot be passed/); + assert.throws(() => quoteForCmdExe('a\u0000b'), /cannot be passed/); + }); + }); +}); + +suite('ciIntegration — no shell is ever requested', () => { + /** Strip comments so documentation about the old bug cannot satisfy the check. */ + function stripComments(source: string): string { + return source + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/(^|[^:])\/\/.*$/gm, '$1'); + } + + function readSource(fileName: string): string { + const candidates = [ + path.resolve(__dirname, '..', '..', 'src', fileName), + path.resolve(process.cwd(), 'src', fileName), + ]; + for (const candidate of candidates) { + if (fs.existsSync(candidate)) { return fs.readFileSync(candidate, 'utf8'); } + } + throw new Error(`Could not locate src/${fileName} (looked in ${candidates.join(', ')})`); + } + + for (const fileName of ['ciIntegration.ts', 'commandResolver.ts']) { + test(`${fileName} never passes a shell option to child_process`, () => { + const source = stripComments(readSource(fileName)); + const match = /\bshell\s*:/.exec(source); + assert.strictEqual( + match, + null, + `${fileName} must not pass a \`shell\` option — Node does not escape args when a shell is used` + ); + }); + } + + test('no source file anywhere in src/ enables a shell', () => { + const srcDir = [ + path.resolve(__dirname, '..', '..', 'src'), + path.resolve(process.cwd(), 'src'), + ].find(dir => fs.existsSync(dir)); + assert.ok(srcDir, 'could not locate the src directory'); + + const offenders: string[] = []; + const walk = (dir: string) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { walk(full); continue; } + if (!entry.name.endsWith('.ts')) { continue; } + const source = stripComments(fs.readFileSync(full, 'utf8')); + // `shell: false` is explicit and fine; anything else is not. + const bad = source.match(/\bshell\s*:\s*(?!false\b)[^,\s}]+/g); + if (bad) { offenders.push(`${entry.name}: ${bad.join(', ')}`); } + } + }; + walk(srcDir!); + + assert.deepStrictEqual( + offenders, + [], + 'child_process must never be given a truthy `shell` option (DEP0190: args are not escaped)' + ); + }); + + test('ciIntegration routes every child process through buildLaunchSpec', () => { + const source = stripComments(readSource('ciIntegration.ts')); + assert.ok(source.includes('buildLaunchSpec('), 'execCommand must resolve executables explicitly'); + const execFileCalls = source.match(/cp\.execFile\(/g) || []; + assert.strictEqual(execFileCalls.length, 1, 'expected exactly one execFile call site'); + assert.ok(/cp\.execFile\(\s*spec\.file\s*,/.test(source), 'execFile must launch the resolved executable'); + }); +}); diff --git a/src/test/gitRemoteParse.test.ts b/src/test/gitRemoteParse.test.ts new file mode 100644 index 0000000..84c72ad --- /dev/null +++ b/src/test/gitRemoteParse.test.ts @@ -0,0 +1,242 @@ +import * as assert from 'assert'; +import { + parseGitHubRemote, + parseGitHubRemoteDetailed, + parseAzdoRemote, + classifyGitHubHost, + splitRemoteUrl, +} from '../gitRemoteParse'; + +suite('gitRemoteParse', () => { + + suite('splitRemoteUrl', () => { + test('parses https remotes', () => { + assert.deepStrictEqual(splitRemoteUrl('https://github.com/owner/repo.git'), { + hostname: 'github.com', + pathname: '/owner/repo.git', + }); + }); + test('parses scp-form remotes', () => { + assert.deepStrictEqual(splitRemoteUrl('git@github.com:owner/repo.git'), { + hostname: 'github.com', + pathname: 'owner/repo.git', + }); + }); + test('parses ssh:// remotes', () => { + const parts = splitRemoteUrl('ssh://git@github.com/owner/repo.git'); + assert.strictEqual(parts?.hostname, 'github.com'); + }); + test('does not treat a scheme as a host', () => { + // `https` has no dot, so the scp-like branch must not claim it. + assert.strictEqual(splitRemoteUrl('https://github.com/o/r')?.hostname, 'github.com'); + }); + test('rejects bare owner/repo input', () => { + assert.strictEqual(splitRemoteUrl('dotnet/templating'), null); + assert.strictEqual(splitRemoteUrl(''), null); + assert.strictEqual(splitRemoteUrl(' '), null); + }); + test('rejects non-git protocols', () => { + assert.strictEqual(splitRemoteUrl('file:///etc/passwd'), null); + assert.strictEqual(splitRemoteUrl('javascript:alert(1)'), null); + }); + test('uses the real host, not userinfo', () => { + assert.strictEqual(splitRemoteUrl('https://github.com@evil.com/o/r')?.hostname, 'evil.com'); + }); + }); + + suite('classifyGitHubHost', () => { + test('recognises github.com and its subdomains', () => { + assert.strictEqual(classifyGitHubHost('github.com'), 'github'); + assert.strictEqual(classifyGitHubHost('GitHub.COM'), 'github'); + assert.strictEqual(classifyGitHubHost('www.github.com'), 'github'); + }); + test('recognises GitHub Enterprise Server hosts', () => { + assert.strictEqual(classifyGitHubHost('github.contoso.com'), 'enterprise'); + assert.strictEqual(classifyGitHubHost('github.mycorp.co.uk'), 'enterprise'); + }); + test('rejects look-alike hosts', () => { + assert.strictEqual(classifyGitHubHost('evil-github.com'), 'other'); + assert.strictEqual(classifyGitHubHost('notgithub.com'), 'other'); + assert.strictEqual(classifyGitHubHost('github.com.attacker.io'), 'other'); + assert.strictEqual(classifyGitHubHost('githubbcom'), 'other'); + }); + }); + + suite('parseGitHubRemote — spoofed hosts', () => { + const spoofed = [ + 'https://evil-github.com/attacker/payload', + 'https://notgithub.com/attacker/payload', + 'https://github.com.attacker.io/attacker/payload', + 'https://github.com.attacker.io/attacker/payload.git', + 'git@evil-github.com:attacker/payload.git', + 'https://github.com@evil.com/attacker/payload', + 'https://evil.com/github.com/attacker/payload', + ]; + for (const remote of spoofed) { + test(`rejects ${remote}`, () => { + assert.strictEqual(parseGitHubRemote(remote), null); + assert.strictEqual(parseGitHubRemoteDetailed(remote).kind, 'none'); + }); + } + }); + + suite('parseGitHubRemote — shell metacharacter payloads', () => { + const payloads = [ + 'https://github.com/owner/repo&calc', + 'https://github.com/owner/repo;whoami', + 'https://github.com/owner/repo|x', + 'https://github.com/owner/repo$(calc)', + 'https://github.com/owner/repo`calc`', + 'https://github.com/own er/repo', + 'https://github.com/owner&calc/repo', + 'git@github.com:owner/repo&calc.git', + ]; + for (const remote of payloads) { + test(`rejects ${remote}`, () => { + assert.strictEqual(parseGitHubRemote(remote), null); + }); + } + + test('rejects percent-encoded metacharacters', () => { + // %26 decodes to `&` — it must not survive as an argument. + assert.strictEqual(parseGitHubRemote('https://github.com/owner/repo%26calc'), null); + assert.strictEqual(parseGitHubRemote('https://github.com/owner/repo%2Fnested'), null); + }); + + test('rejects leading-dash owners (option injection)', () => { + assert.strictEqual(parseGitHubRemote('https://github.com/-oProxyCommand/repo'), null); + }); + }); + + suite('parseGitHubRemote — legitimate remotes', () => { + test('https with .git suffix', () => { + assert.deepStrictEqual( + parseGitHubRemote('https://github.com/dotnet/templating.git'), + { owner: 'dotnet', repo: 'templating' } + ); + }); + test('https without .git suffix', () => { + assert.deepStrictEqual( + parseGitHubRemote('https://github.com/dotnet/templating'), + { owner: 'dotnet', repo: 'templating' } + ); + }); + test('trailing slash', () => { + assert.deepStrictEqual( + parseGitHubRemote('https://github.com/dotnet/templating/'), + { owner: 'dotnet', repo: 'templating' } + ); + assert.deepStrictEqual( + parseGitHubRemote('https://github.com/dotnet/templating.git/'), + { owner: 'dotnet', repo: 'templating' } + ); + }); + test('scp form', () => { + assert.deepStrictEqual( + parseGitHubRemote('git@github.com:owner/repo.git'), + { owner: 'owner', repo: 'repo' } + ); + }); + test('ssh:// form', () => { + assert.deepStrictEqual( + parseGitHubRemote('ssh://git@github.com/owner/repo.git'), + { owner: 'owner', repo: 'repo' } + ); + }); + test('dotted repo names survive', () => { + assert.deepStrictEqual( + parseGitHubRemote('https://github.com/My.Org/My.Repo'), + { owner: 'My.Org', repo: 'My.Repo' } + ); + assert.deepStrictEqual( + parseGitHubRemote('https://github.com/dotnet/dotnet.github.io'), + { owner: 'dotnet', repo: 'dotnet.github.io' } + ); + }); + test('deep URLs (Actions run links)', () => { + assert.deepStrictEqual( + parseGitHubRemote('https://github.com/dotnet/templating/actions/runs/12345'), + { owner: 'dotnet', repo: 'templating' } + ); + }); + test('scheme-less github.com URLs', () => { + assert.deepStrictEqual( + parseGitHubRemote('github.com/dotnet/templating'), + { owner: 'dotnet', repo: 'templating' } + ); + }); + test('query strings and fragments are ignored', () => { + assert.deepStrictEqual( + parseGitHubRemote('https://github.com/dotnet/templating?tab=readme#top'), + { owner: 'dotnet', repo: 'templating' } + ); + }); + }); + + suite('parseGitHubRemoteDetailed — GitHub Enterprise', () => { + test('reports Enterprise hosts explicitly rather than "not found"', () => { + const result = parseGitHubRemoteDetailed('https://github.contoso.com/owner/repo.git'); + assert.strictEqual(result.kind, 'enterprise'); + if (result.kind === 'enterprise') { + assert.strictEqual(result.host, 'github.contoso.com'); + } + }); + test('Enterprise remotes stay unsupported', () => { + assert.strictEqual(parseGitHubRemote('https://github.contoso.com/owner/repo.git'), null); + assert.strictEqual(parseGitHubRemote('git@github.contoso.com:owner/repo.git'), null); + }); + test('look-alikes are not misreported as Enterprise', () => { + assert.strictEqual(parseGitHubRemoteDetailed('https://github.com.attacker.io/o/r').kind, 'none'); + assert.strictEqual(parseGitHubRemoteDetailed('https://evil-github.com/o/r').kind, 'none'); + }); + }); + + suite('parseAzdoRemote', () => { + test('dev.azure.com https remotes', () => { + assert.deepStrictEqual( + parseAzdoRemote('https://dev.azure.com/dnceng-public/public/_git/dotnet-runtime'), + { org: 'dnceng-public', project: 'public' } + ); + assert.deepStrictEqual( + parseAzdoRemote('https://dev.azure.com/dnceng-public/public/_build/results?buildId=1354651'), + { org: 'dnceng-public', project: 'public' } + ); + }); + test('ssh.dev.azure.com scp remotes', () => { + assert.deepStrictEqual( + parseAzdoRemote('git@ssh.dev.azure.com:v3/myorg/myproject/myrepo'), + { org: 'myorg', project: 'myproject' } + ); + }); + test('legacy visualstudio.com remotes', () => { + assert.deepStrictEqual( + parseAzdoRemote('https://contoso.visualstudio.com/MyProject/_git/MyRepo'), + { org: 'contoso', project: 'MyProject' } + ); + assert.deepStrictEqual( + parseAzdoRemote('https://contoso.visualstudio.com/DefaultCollection/MyProject/_git/MyRepo'), + { org: 'contoso', project: 'MyProject' } + ); + }); + test('rejects spoofed Azure DevOps hosts', () => { + assert.strictEqual(parseAzdoRemote('https://evil-dev.azure.com/org/project/_git/r'), null); + assert.strictEqual(parseAzdoRemote('https://dev.azure.com.attacker.io/org/project/_git/r'), null); + assert.strictEqual(parseAzdoRemote('https://notvisualstudio.com/org/project/_git/r'), null); + assert.strictEqual(parseAzdoRemote('https://contoso.visualstudio.com.attacker.io/p/_git/r'), null); + assert.strictEqual(parseAzdoRemote('https://evil.com/dev.azure.com/org/project'), null); + }); + test('rejects shell metacharacter payloads', () => { + assert.strictEqual(parseAzdoRemote('https://dev.azure.com/org&calc/project/_git/r'), null); + assert.strictEqual(parseAzdoRemote('https://dev.azure.com/org/project;whoami/_git/r'), null); + assert.strictEqual(parseAzdoRemote('https://dev.azure.com/org/project|x'), null); + assert.strictEqual(parseAzdoRemote('https://contoso.visualstudio.com/proj$(calc)/_git/r'), null); + }); + test('rejects route markers as org/project', () => { + assert.strictEqual(parseAzdoRemote('https://dev.azure.com/org/_git/repo'), null); + assert.strictEqual(parseAzdoRemote('https://dev.azure.com/org'), null); + }); + test('rejects GitHub remotes', () => { + assert.strictEqual(parseAzdoRemote('https://github.com/owner/repo.git'), null); + }); + }); +});