From cd12614cc9da54bd3bdb022ba5b83fa06968972a Mon Sep 17 00:00:00 2001 From: Sirich Rima Date: Sun, 19 Jul 2026 18:22:40 +0300 Subject: [PATCH 1/2] fix: replace shell-based path expansion with native Node.js resolution --- pnpm-lock.yaml | 19 +- .../yeoman-ui/packages/backend/package.json | 5 +- .../backend/src/utils/customLocation.ts | 33 ++- .../packages/backend/src/utils/npm.ts | 58 ++++-- .../backend/test/utils/customLocation.spec.ts | 189 ++++++++++++++++++ .../test/utils/workspaceFolders.spec.ts | 27 ++- 6 files changed, 297 insertions(+), 34 deletions(-) create mode 100644 projects/yeoman-ui/packages/backend/test/utils/customLocation.spec.ts diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 05348523..3a048ada 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -699,6 +699,9 @@ importers: copy-webpack-plugin: specifier: ^12.0.2 version: 12.0.2(webpack@5.106.2) + cross-env: + specifier: ^10.1.0 + version: 10.1.0 lcov-result-merger: specifier: 5.0.0 version: 5.0.0 @@ -1702,6 +1705,9 @@ packages: resolution: {integrity: sha512-4B4OijXeVNOPZlYA2oEwWOTkzyltLao+xbotHQeqN++Rv27Y6s818+n2Qkp8q+Fxhn0t/5lA5X1Mxktud8eayQ==} engines: {node: '>=14.17.0'} + '@epic-web/invariant@1.0.0': + resolution: {integrity: sha512-lrTPqgvfFQtR/eY/qkIzp98OGdNJu0m5ji3q/nJI8v3SXkRKEnWiOxMmbvcSoAIzv/cGiuvRy57k4suKQSAdwA==} + '@esbuild/aix-ppc64@0.25.5': resolution: {integrity: sha512-9o3TMmpmftaCMepOdA5k/yDw8SfInyzWWTjYTFCX3kPSDJMROQTb8jg+h9Cnwnmm1vOzvxN7gIfB5V2ewpjtGA==} engines: {node: '>=18'} @@ -4369,6 +4375,11 @@ packages: create-require@1.1.1: resolution: {integrity: sha512-dcKFX3jn0MpIaXjisoRvexIJVEKzaq7z2rZKxf+MSr9TkdmHmsU4m2lcLojrj/FHl8mk5VxMmYA+ftRkP/3oKQ==} + cross-env@10.1.0: + resolution: {integrity: sha512-GsYosgnACZTADcmEyJctkJIoqAhHjttw7RsFrVoJNXbsWWqaq6Ym+7kZjq6mS45O0jij6vtiReppKQEtqWy6Dw==} + engines: {node: '>=20'} + hasBin: true + cross-spawn@6.0.5: resolution: {integrity: sha512-eTVLrBSt7fjbDygz805pMnstIs2VTBNkRm0qxZd+M7A5XDdxVRWO5MxGBXZhjY4cqLYLdtrGqRf8mBPmzwSpWQ==} engines: {node: '>=4.8'} @@ -11612,6 +11623,8 @@ snapshots: '@discoveryjs/json-ext@0.6.3': {} + '@epic-web/invariant@1.0.0': {} + '@esbuild/aix-ppc64@0.25.5': optional: true @@ -12717,7 +12730,6 @@ snapshots: chokidar: 3.6.0 js-yaml: 4.2.0 jsdom: 25.0.1(bufferutil@4.1.0)(utf-8-validate@5.0.10) - lodash: 4.18.1 micromatch: 4.0.8 simple-git: 3.36.0 uuid: 14.0.0 @@ -15054,6 +15066,11 @@ snapshots: create-require@1.1.1: {} + cross-env@10.1.0: + dependencies: + '@epic-web/invariant': 1.0.0 + cross-spawn: 7.0.6 + cross-spawn@6.0.5: dependencies: nice-try: 1.0.5 diff --git a/projects/yeoman-ui/packages/backend/package.json b/projects/yeoman-ui/packages/backend/package.json index aac628a0..e575a881 100644 --- a/projects/yeoman-ui/packages/backend/package.json +++ b/projects/yeoman-ui/packages/backend/package.json @@ -36,11 +36,11 @@ "clean:frontend": "cd ./dist && shx rm -rf ./media", "compile": "tsc", "compile:watch": "tsc -watch", - "coverage": "NODE_OPTIONS='--max-old-space-size=8192 --no-experimental-strip-types' c8 mocha", + "coverage": "cross-env NODE_OPTIONS=\"--max-old-space-size=8192 --no-experimental-strip-types\" c8 mocha", "coverage:copy": "shx mkdir -p ../../../../coverage && shx cp -u ./coverage/lcov.info ../../../../coverage/lcov_backend.info", "frontend:copy": "npm-run-all clean:frontend && shx cp -r ../frontend/dist/. ./dist/media/", "package": "vsce package --no-dependencies", - "test": "NODE_OPTIONS='--no-experimental-strip-types' mocha", + "test": "cross-env NODE_OPTIONS=\"--no-experimental-strip-types\" mocha", "webpack": "webpack --config webpack.config.cjs --mode development", "webpack-dev:watch": "webpack --config webpack.config.cjs --mode development --watch", "ws:egRun": "node ./dist/src/webSocketServer/exploregens.js", @@ -214,6 +214,7 @@ "@vscode/vsce": "2.24.0", "c8": "11.0.0", "copy-webpack-plugin": "^12.0.2", + "cross-env": "^10.1.0", "lcov-result-merger": "5.0.0", "sinon": "^18.0.1", "string-replace-loader": "3.0.3", diff --git a/projects/yeoman-ui/packages/backend/src/utils/customLocation.ts b/projects/yeoman-ui/packages/backend/src/utils/customLocation.ts index 05336439..50520963 100644 --- a/projects/yeoman-ui/packages/backend/src/utils/customLocation.ts +++ b/projects/yeoman-ui/packages/backend/src/utils/customLocation.ts @@ -3,27 +3,39 @@ import { existsSync, mkdirSync } from "fs"; import { homedir } from "os"; import lodash from "lodash"; import { vscode } from "./vscodeProxy.js"; -import { execSync } from "child_process"; const { isEmpty, trim } = lodash; export const GLOBAL_CONFIG_KEY = "ApplicationWizard.installationLocation"; +// Matches leading home-dir tokens: ~, $HOME, %USERPROFILE% (case-insensitive). +// The token must be followed by / \ or end-of-string — not a longer word. +const HOME_TOKEN_RE = /^(~|\$HOME|%USERPROFILE%)(\/|\\|$)/i; + +// Expand leading ~ and common home-dir env vars to their absolute paths. +// Handles ~ / ~/path / ~\path, $HOME/path, %USERPROFILE%/path (case-insensitive). +// This keeps backward compatibility with configs that used shell-expanded values. +const normaliseToAbsolute = (filePath: string): string => { + const home = homedir(); + const match = HOME_TOKEN_RE.exec(filePath); + if (match) { + filePath = home + filePath.slice(match[1].length); + } + if (!path.isAbsolute(filePath)) { + filePath = path.resolve(home, filePath); + } + return filePath; +}; + const getAbsoluteCustomPath = (): string | undefined => { - let customPath = trim( + const customPath = trim( vscode.workspace.getConfiguration().get(GLOBAL_CONFIG_KEY) ); if (isEmpty(customPath)) { return; } - customPath = trim(execSync(`echo ${customPath}`).toString()); - - if (!path.isAbsolute(customPath)) { - customPath = path.resolve(homedir(), customPath); - } - - return customPath; + return normaliseToAbsolute(customPath); }; const isCustomPathExist = (customPath: string) => { @@ -33,6 +45,9 @@ const isCustomPathExist = (customPath: string) => { export const getPath = (): string => { const customPath = getAbsoluteCustomPath(); + if (!customPath) { + return undefined; + } return isCustomPathExist(customPath) ? trim(customPath) : undefined; }; diff --git a/projects/yeoman-ui/packages/backend/src/utils/npm.ts b/projects/yeoman-ui/packages/backend/src/utils/npm.ts index 69950070..fb4d4a4f 100644 --- a/projects/yeoman-ui/packages/backend/src/utils/npm.ts +++ b/projects/yeoman-ui/packages/backend/src/utils/npm.ts @@ -1,4 +1,4 @@ -import { exec } from "child_process"; +import { exec, execSync } from "child_process"; import { promisify } from "util"; import { platform } from "os"; import _ from "lodash"; @@ -41,6 +41,19 @@ const SEARCH_QUERY_SUFFIX = const CANCELED = "Action cancelled"; const HAS_ACCESS = "Has Access"; +// Wrap a filesystem path for safe interpolation into a shell command string. +// Use only for commands that must go through a shell (e.g. sudo-prompt). +// Prefer spawn() with an argument array over this whenever possible. +const shellQuotePath = (p: string): string => { + if (isWin32) { + // cmd.exe: wrap in double-quotes, escape any embedded double-quotes as "" + return `"${p.replace(/"/g, '""')}"`; + } + // POSIX sh: single-quote the entire path; a single-quote inside is + // closed, escaped with \', then reopened: 'it'\''s safe' + return `'${p.replace(/'/g, "'\\''")}'`; +}; + class Command { private globalNodeModulesPathPromise: Promise; private readonly SET_DEFAULT_LOCATION; @@ -62,11 +75,11 @@ class Command { ); } - private getGenLocationParams(): string { + private getGenLocationArgs(): string[] { const customInstallationPath = customLocation.getPath(); return _.isEmpty(customInstallationPath) - ? "-g" - : `--prefix ${customInstallationPath}`; + ? ["-g"] + : ["--prefix", customInstallationPath]; } private async execCommand(arg: string): Promise { @@ -139,9 +152,28 @@ class Command { private async grantAccessForGlobalNodeModulesPath() { const globalNodeModulesPath = await this.getGlobalNodeModulesPath(); + // Resolve the real username in Node.js so $USER is not evaluated inside + // the elevated shell where env_reset typically sets it to "root". + const realUser = isWin32 + ? undefined + : (() => { + try { + return execSync("logname", { encoding: "utf8" }).trim(); + } catch { + /* fall through */ + } + try { + return execSync("id -un", { encoding: "utf8" }).trim(); + } catch { + /* fall through */ + } + return os.userInfo().username; + })(); const changeOwnerCommand = isWin32 - ? `icacls ${globalNodeModulesPath} /grant Users:(OI)(CI)F` - : `chown -R $USER ${globalNodeModulesPath}`; + ? `icacls ${shellQuotePath(globalNodeModulesPath)} /grant Users:(OI)(CI)F` + : `chown -R ${shellQuotePath(realUser)} ${shellQuotePath( + globalNodeModulesPath + )}`; const globalPath = await this.getGlobalPath(); const statusBarMessage = vscode.window.setStatusBarMessage( messages.changing_owner_permissions(globalPath) @@ -224,15 +256,17 @@ class Command { } public async install(packageName: string): Promise { - const locationParams = this.getGenLocationParams(); - const command = `${NPM} install ${locationParams} ${packageName}@latest`; - return this.execCommand(command); + const locationArgs = this.getGenLocationArgs(); + return this.spawnCommand(NPM, [ + "install", + ...locationArgs, + `${packageName}@latest`, + ]); } public async uninstall(packageName: string): Promise { - const locationParams = this.getGenLocationParams(); - const command = `${NPM} uninstall ${locationParams} ${packageName}`; - return this.execCommand(command); + const locationArgs = this.getGenLocationArgs(); + return this.spawnCommand(NPM, ["uninstall", ...locationArgs, packageName]); } public async getNodeProcessVersions(): Promise { diff --git a/projects/yeoman-ui/packages/backend/test/utils/customLocation.spec.ts b/projects/yeoman-ui/packages/backend/test/utils/customLocation.spec.ts new file mode 100644 index 00000000..497d24f5 --- /dev/null +++ b/projects/yeoman-ui/packages/backend/test/utils/customLocation.spec.ts @@ -0,0 +1,189 @@ +import { expect } from "chai"; +import { createSandbox, SinonSandbox } from "sinon"; +import { homedir, tmpdir } from "os"; +import * as path from "path"; +import * as fs from "fs"; +import { vscode } from "../mockUtil.js"; +import * as customLocation from "../../src/utils/customLocation.js"; +import { GLOBAL_CONFIG_KEY } from "../../src/utils/customLocation.js"; + +const NONEXISTENT = path.join(tmpdir(), "customLocation-nonexistent-xyzabc999"); + +describe("customLocation unit test", () => { + let sandbox: SinonSandbox; + let configGetStub: sinon.SinonStub; + + beforeEach(() => { + sandbox = createSandbox(); + configGetStub = sandbox.stub(vscode.workspace.getConfiguration(), "get"); + }); + + afterEach(() => { + sandbox.restore(); + }); + + describe("getPath — empty / missing config", () => { + it("returns undefined (not throw) when config value is empty string", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns(""); + expect(() => customLocation.getPath()).to.not.throw(); + expect(customLocation.getPath()).to.be.undefined; + }); + + it("returns undefined (not throw) when config value is whitespace only", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns(" "); + expect(() => customLocation.getPath()).to.not.throw(); + expect(customLocation.getPath()).to.be.undefined; + }); + + it("returns undefined when path does not exist on disk", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns(NONEXISTENT); + expect(customLocation.getPath()).to.be.undefined; + }); + }); + + describe("getPath — tilde expansion", () => { + it("expands bare '~' to homedir()", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns("~"); + expect(customLocation.getPath()).to.equal(homedir()); + }); + + it("expands '~/..' to an existing path one level above homedir()", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns("~/.."); + // After tilde expansion the path is homedir() + "/.." which is absolute, + // so path.resolve is skipped. existsSync accepts it (OS resolves the ..). + const result = customLocation.getPath(); + expect(result).to.not.be.undefined; + expect(result).to.include(".."); + }); + + it("returns undefined for a '~/nonexistent' path that does not exist", () => { + configGetStub + .withArgs(GLOBAL_CONFIG_KEY) + .returns("~/nonexistent-xyzabc999"); + expect(customLocation.getPath()).to.be.undefined; + }); + }); + + describe("getPath — relative path resolution", () => { + it("resolves bare '.' against homedir()", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns("."); + expect(customLocation.getPath()).to.equal(homedir()); + }); + + it("returns an existing absolute path (tmpdir) unchanged", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns(tmpdir()); + expect(customLocation.getPath()).to.equal(tmpdir()); + }); + }); + + describe("getPath — $HOME / %USERPROFILE% backward compatibility", () => { + it("expands bare '$HOME' to homedir()", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns("$HOME"); + expect(customLocation.getPath()).to.equal(homedir()); + }); + + it("$HOME/nonexistent does not throw and returns undefined", () => { + configGetStub + .withArgs(GLOBAL_CONFIG_KEY) + .returns("$HOME/nonexistent-xyzabc999"); + expect(() => customLocation.getPath()).to.not.throw(); + expect(customLocation.getPath()).to.be.undefined; + }); + + it("does NOT expand '$HOMEFOO' — token must be followed by / \\ or end-of-string", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns("$HOMEFOO"); + // $HOMEFOO is not a home-dir token; treated as a relative/nonexistent path + expect(customLocation.getPath()).to.be.undefined; + }); + + it("expands '%USERPROFILE%' to homedir()", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns("%USERPROFILE%"); + expect(customLocation.getPath()).to.equal(homedir()); + }); + + it("expands '%userprofile%' (lowercase) to homedir()", () => { + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns("%userprofile%"); + expect(customLocation.getPath()).to.equal(homedir()); + }); + }); + + describe("getPath — injection prevention (no shell invoked)", () => { + // Proof-of-exploit test: the OLD code ran execSync(`echo ${customPath}`). + // These payloads verify that a malicious .vscode/settings.json cannot execute + // arbitrary OS commands when a developer opens a crafted repository. + // + // Strategy: each payload writes a marker file to disk if a shell executes it. + // After calling getPath(), we assert the marker was never created. + + let markerFile: string; + + beforeEach(() => { + markerFile = path.join(tmpdir(), `injection-proof-${Date.now()}`); + }); + + afterEach(() => { + // Clean up in case a test bug caused the marker to be created + try { + fs.unlinkSync(markerFile); + } catch { + /* already absent */ + } + }); + + it("semicolon command separator does not execute injected command", () => { + // Old vulnerable code: execSync(`echo /safe; touch ${markerFile}`) → marker created + // Fixed code: no shell → marker never created + const payload = `/safe; touch ${markerFile}`; + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns(payload); + expect(() => customLocation.getPath()).to.not.throw(); + expect( + fs.existsSync(markerFile), + "shell command was executed — marker file exists" + ).to.be.false; + }); + + it("$() subshell substitution does not execute injected command", () => { + // Old vulnerable code: execSync(`echo $(touch ${markerFile})`) → marker created + const payload = `$(touch ${markerFile})`; + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns(payload); + expect(() => customLocation.getPath()).to.not.throw(); + expect( + fs.existsSync(markerFile), + "shell command was executed — marker file exists" + ).to.be.false; + }); + + it("backtick subshell does not execute injected command", () => { + // Old vulnerable code: execSync(`echo \`touch ${markerFile}\``) → marker created + const payload = `\`touch ${markerFile}\``; + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns(payload); + expect(() => customLocation.getPath()).to.not.throw(); + expect( + fs.existsSync(markerFile), + "shell command was executed — marker file exists" + ).to.be.false; + }); + + it("pipe operator does not execute injected command", () => { + const payload = `/nonexistent | touch ${markerFile}`; + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns(payload); + expect(() => customLocation.getPath()).to.not.throw(); + expect( + fs.existsSync(markerFile), + "shell command was executed — marker file exists" + ).to.be.false; + }); + + it("tilde-prefixed payload does not execute injected command", () => { + // Tilde is expanded by normaliseToAbsolute — no shell. The injected command after + // the separator must not run. + const payload = `~/safe; touch ${markerFile}`; + configGetStub.withArgs(GLOBAL_CONFIG_KEY).returns(payload); + expect(() => customLocation.getPath()).to.not.throw(); + expect( + fs.existsSync(markerFile), + "shell command was executed — marker file exists" + ).to.be.false; + }); + }); +}); diff --git a/projects/yeoman-ui/packages/backend/test/utils/workspaceFolders.spec.ts b/projects/yeoman-ui/packages/backend/test/utils/workspaceFolders.spec.ts index 4cbea4de..5807c8b2 100644 --- a/projects/yeoman-ui/packages/backend/test/utils/workspaceFolders.spec.ts +++ b/projects/yeoman-ui/packages/backend/test/utils/workspaceFolders.spec.ts @@ -9,6 +9,10 @@ import { import type { WorkspaceFolder } from "vscode"; const { Uri } = vscode; +// Derive expected fsPath values from the same Uri.file() the production code +// uses — avoids hardcoded POSIX strings that break on Windows. +const p = (...parts: string[]) => Uri.file("/" + parts.join("/")).fsPath; + describe("workspaceFolders utility tests", () => { let sandbox: SinonSandbox; let workspaceMock: SinonMock; @@ -65,8 +69,8 @@ describe("workspaceFolders utility tests", () => { const result = getWorkspaceFolders(); expect(result).to.deep.equal([ - "/path/to/workspace1", - "/path/to/workspace2", + p("path", "to", "workspace1"), + p("path", "to", "workspace2"), ]); }); @@ -86,7 +90,7 @@ describe("workspaceFolders utility tests", () => { sandbox.stub(vscode.workspace, "workspaceFolders").value(mockFolders); const result = getWorkspaceFolders(); - expect(result).to.deep.equal(["/local/path"]); + expect(result).to.deep.equal([p("local", "path")]); }); it("filters out multiple virtual schemes (ssh, wsl, vscode-vfs)", () => { @@ -120,7 +124,7 @@ describe("workspaceFolders utility tests", () => { sandbox.stub(vscode.workspace, "workspaceFolders").value(mockFolders); const result = getWorkspaceFolders(); - expect(result).to.deep.equal(["/local/path1", "/local/path2"]); + expect(result).to.deep.equal([p("local", "path1"), p("local", "path2")]); }); it("returns empty array when only virtual workspaces exist", () => { @@ -164,7 +168,10 @@ describe("workspaceFolders utility tests", () => { sandbox.stub(vscode.workspace, "workspaceFolders").value(mockFolders); const result = getWorkspaceFolders(); - expect(result).to.deep.equal(["/valid/path", "/another/valid/path"]); + expect(result).to.deep.equal([ + p("valid", "path"), + p("another", "valid", "path"), + ]); }); }); @@ -246,8 +253,8 @@ describe("workspaceFolders utility tests", () => { const result = getFileSchemeWorkspaceFolders(); expect(result).to.have.lengthOf(2); - expect(result[0].uri.fsPath).to.equal("/valid/path"); - expect(result[1].uri.fsPath).to.equal("/another/valid/path"); + expect(result[0].uri.fsPath).to.equal(p("valid", "path")); + expect(result[1].uri.fsPath).to.equal(p("another", "valid", "path")); }); }); @@ -267,8 +274,8 @@ describe("workspaceFolders utility tests", () => { ]; sandbox.stub(vscode.workspace, "workspaceFolders").value(mockFolders); - const result = getFirstWorkspacePath("/fallback/path"); - expect(result).to.equal("/first/path"); + const result = getFirstWorkspacePath(p("fallback", "path")); + expect(result).to.equal(p("first", "path")); }); it("returns fallback when no workspaces exist", () => { @@ -320,7 +327,7 @@ describe("workspaceFolders utility tests", () => { sandbox.stub(vscode.workspace, "workspaceFolders").value(mockFolders); const result = getFirstWorkspacePath("/fallback/path"); - expect(result).to.equal("/local/path"); + expect(result).to.equal(p("local", "path")); }); it("works with empty string fallback", () => { From 7ba15d32453b1d423ec222c46a797ad44b56cbc8 Mon Sep 17 00:00:00 2001 From: Sirich Rima Date: Sun, 19 Jul 2026 19:08:57 +0300 Subject: [PATCH 2/2] ci: add changeset file --- .changeset/clean-spawn-patch.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 .changeset/clean-spawn-patch.md diff --git a/.changeset/clean-spawn-patch.md b/.changeset/clean-spawn-patch.md new file mode 100644 index 00000000..a4286fb0 --- /dev/null +++ b/.changeset/clean-spawn-patch.md @@ -0,0 +1,23 @@ +--- +"yeoman-ui": patch +--- + +Replace shell-based path expansion with native Node.js resolution + +The `ApplicationWizard.installationLocation` setting is now resolved +entirely in Node.js. Tilde (`~`), `$HOME`, and `%USERPROFILE%` prefixes +are expanded without spawning a shell process, preserving backward +compatibility with existing configurations. + +Generator install and uninstall operations now use `spawn()` with explicit +argument arrays instead of building shell command strings, eliminating +the shell as an intermediary for those operations. A shared `shellQuotePath` +helper is used for the remaining elevated commands that require a shell +(`icacls`, `chown`). The real username is resolved in Node.js before being +passed to `sudo.exec` to ensure correct ownership on Linux and macOS. + +Additional fixes: + +- Prevent a crash when `installationLocation` is unset (default state) +- Fix cross-platform test failures caused by hardcoded POSIX path separators +- Fix `NODE_OPTIONS` assignment in npm scripts using `cross-env` for Windows compatibility