diff --git a/package.json b/package.json index cdd83886..230ac0e7 100644 --- a/package.json +++ b/package.json @@ -15,6 +15,7 @@ "@salesforce/sf-plugins-core": "^12.0.7", "@salesforce/ts-types": "^2.0.11", "change-case": "^5.4.2", + "cross-spawn": "^7.0.6", "ejs": "^3.1.10", "fast-glob": "^3.3.2", "got": "^13", @@ -32,6 +33,7 @@ "@salesforce/cli-plugins-testkit": "^5.3.20", "@salesforce/dev-scripts": "^10.2.11", "@salesforce/plugin-command-reference": "^3.1.5", + "@types/cross-spawn": "^6.0.6", "@types/ejs": "^3.1.5", "@types/js-yaml": "^4.0.5", "@types/lodash.defaultsdeep": "^4.6.9", diff --git a/src/generator.ts b/src/generator.ts index 93e51a78..30c8b78c 100644 --- a/src/generator.ts +++ b/src/generator.ts @@ -13,6 +13,7 @@ import { Ux } from '@salesforce/sf-plugins-core'; import { Logger } from '@salesforce/core'; import { colorize } from '@oclif/core/ux'; import shelljs from 'shelljs'; +import spawn from 'cross-spawn'; import replace from 'replace-in-file'; import { fileExists } from './util.js'; import { PackageJson } from './types.js'; @@ -88,17 +89,23 @@ export class Generator { return; } - const args = cmd.split(' '); - const bin = args[0]; + // Tokenize into discrete argv entries so arguments are passed to the child + // process individually rather than as one shell-interpreted string. Callers + // double-quote arguments that may contain spaces (e.g. paths), so keep a + // quoted span as a single token and strip its surrounding quotes. + const [bin, ...args] = (cmd.match(/"[^"]*"|\S+/g) ?? []).map((token) => token.replace(/^"|"$/g, '')); const isBinPath = bin.includes('/') || bin.includes('\\'); - const resolved = isBinPath ? bin : shelljs.which(bin); + const resolved = isBinPath ? bin : shelljs.which(bin)?.toString(); if (!resolved) { throw new Error(`Could not find "${bin}" on PATH`); } - const resolvedCmd = [resolved.toString(), ...args.slice(1)].join(' '); - this.logger.debug(`Executing command: ${resolvedCmd}`); - shelljs.exec(resolvedCmd, { cwd: this.cwd }); + this.logger.debug(`Executing command: ${[resolved, ...args].join(' ')}`); + + // Pass argv as an array with no shell so a value like `C:\path\repo&calc.exe&\` + // is treated as literal data and cannot break out to hijack the shell. + // cross-spawn handles Windows .cmd/.bat shims (e.g. yarn.cmd) safely. + spawn.sync(resolved, args, { cwd: this.cwd, stdio: 'inherit' }); } public async loadPjson(): Promise { diff --git a/test/commands/dev/generate/command3PP.nut.ts b/test/commands/dev/generate/command3PP.nut.ts index f6731741..1a609724 100644 --- a/test/commands/dev/generate/command3PP.nut.ts +++ b/test/commands/dev/generate/command3PP.nut.ts @@ -62,6 +62,11 @@ describe('3PP', () => { env: { ...process.env, TESTKIT_EXECUTABLE_PATH: pluginExecutable, + // Disable wireit's GitHub Actions cache for the generated sub-project. In CI the + // outer job sets WIREIT_CACHE=github (plus cache credentials), which this child + // process would otherwise inherit and use to hit GitHub's cache service — a source + // of transient HTTP failures. Locally WIREIT_CACHE is unset (local disk cache). + WIREIT_CACHE: 'none', }, }); expect(result.code).to.equal(0); @@ -73,7 +78,15 @@ describe('3PP', () => { const cmd = parts.pop(); const unitTestFile = path.join(session.project.dir, 'test', 'commands', ...parts, `${cmd}.test.ts`); expect(await fileExists(unitTestFile)).to.be.true; - const result = shelljs.exec('yarn test:only', { cwd: session.project.dir }); + const result = shelljs.exec('yarn test:only', { + cwd: session.project.dir, + env: { + ...process.env, + // See note above: avoid inheriting WIREIT_CACHE=github in CI, which makes the + // generated sub-project hit GitHub's flaky cache service. + WIREIT_CACHE: 'none', + }, + }); expect(result.code).to.equal(0); expect(result.stdout).include(name.replace(/:/g, ' ')); }); diff --git a/test/commands/dev/generate/plugin.nut.ts b/test/commands/dev/generate/plugin.nut.ts index 06a1c144..cbfeed9d 100644 --- a/test/commands/dev/generate/plugin.nut.ts +++ b/test/commands/dev/generate/plugin.nut.ts @@ -64,7 +64,11 @@ describe('dev generate plugin NUTs', () => { }); expect(readFileSync(path.join(pluginDir, 'src', 'commands', 'hello', 'world.ts'), 'utf8')).to.include('Copyright'); - expect(readFileSync(path.join(pluginDir, '.eslintrc.cjs'), 'utf8')).to.include('eslint-config-salesforce-license'); + // The template's flat config re-exports eslint-config-salesforce-typescript, which now + // bundles the Apache license-header rule that used to come from eslint-config-salesforce-license. + expect(readFileSync(path.join(pluginDir, 'eslint.config.mjs'), 'utf8')).to.include( + 'eslint-config-salesforce-typescript' + ); }); it('should generate a 3PP plugin', async () => { diff --git a/test/generator.test.ts b/test/generator.test.ts index 706cc938..6d13da8c 100644 --- a/test/generator.test.ts +++ b/test/generator.test.ts @@ -5,21 +5,85 @@ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ +import { normalize } from 'node:path'; import { expect } from 'chai'; +import spawn from 'cross-spawn'; +import shelljs from 'shelljs'; import { Generator } from '../src/generator.js'; +type SpawnCall = { bin: string; args: readonly string[]; options: { cwd?: string } }; + +// `spawn.sync` is typed read-only, so mutate through a mutable view of the module. +const spawnModule = spawn as { sync: typeof spawn.sync }; + describe('Generator.execute', () => { + let calls: SpawnCall[]; + const originalSpawnSync = spawn.sync; + const originalWhich = shelljs.which; + + beforeEach(() => { + calls = []; + // Intercept cross-spawn so no real process runs and we can assert on argv. + spawnModule.sync = ((bin: string, args: readonly string[], options: { cwd?: string }) => { + calls.push({ bin, args, options }); + return {} as ReturnType; + }) as typeof spawn.sync; + }); + + afterEach(() => { + spawnModule.sync = originalSpawnSync; + shelljs.which = originalWhich; + }); + it('should throw when binary is not found on PATH', () => { const generator = new Generator(); expect(() => generator.execute('nonexistent-binary-xyz --help')).to.throw( 'Could not find "nonexistent-binary-xyz" on PATH' ); + expect(calls).to.have.length(0); }); it('should not execute commands in dry-run mode', () => { const generator = new Generator({ dryRun: true }); generator.execute('nonexistent-binary-xyz --help'); + + expect(calls).to.have.length(0); + }); + + it('should pass argv as an array without a shell so metacharacters cannot break out', () => { + const generator = new Generator(); + generator.cwd = '/tmp/project'; + + // This is the hijack path from the original vulnerability. `&` must land in + // a single argv entry, never as a shell command separator. The bin is an + // absolute path, so PATH resolution is bypassed. + generator.execute('/usr/bin/git clone https://x.git "/repo/dir&calc.exe&"'); + + expect(calls).to.have.length(1); + expect(calls[0].bin).to.equal('/usr/bin/git'); + expect(calls[0].args).to.deep.equal(['clone', 'https://x.git', '/repo/dir&calc.exe&']); + // The cwd setter normalizes paths, so compare against the platform-normalized form. + expect(calls[0].options).to.include({ cwd: normalize('/tmp/project') }); + }); + + it('should keep a quoted argument containing spaces as a single token', () => { + const generator = new Generator(); + + generator.execute('/bin/yarn prettier --write "/My Docs/a.ts"'); + + expect(calls[0].bin).to.equal('/bin/yarn'); + expect(calls[0].args).to.deep.equal(['prettier', '--write', '/My Docs/a.ts']); + }); + + it('should resolve a bare binary name via PATH', () => { + shelljs.which = (() => '/resolved/path/to/yarn') as unknown as typeof shelljs.which; + const generator = new Generator(); + + generator.execute('yarn install'); + + expect(calls[0].bin).to.equal('/resolved/path/to/yarn'); + expect(calls[0].args).to.deep.equal(['install']); }); }); diff --git a/yarn.lock b/yarn.lock index 987ba508..d8ac48dd 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2633,6 +2633,13 @@ dependencies: "@types/node" "*" +"@types/cross-spawn@^6.0.6": + version "6.0.6" + resolved "https://registry.yarnpkg.com/@types/cross-spawn/-/cross-spawn-6.0.6.tgz#0163d0b79a6f85409e0decb8dcca17147f81fd22" + integrity sha512-fXRhhUkG4H3TQk5dBhQ7m/JDdSNHKwR2BBia62lhwEIq9xGiQKLxd6LymNhn47SjXhsUEPmxi+PKw2OkW4LLjA== + dependencies: + "@types/node" "*" + "@types/ejs@^3.1.5": version "3.1.5" resolved "https://registry.yarnpkg.com/@types/ejs/-/ejs-3.1.5.tgz#49d738257cc73bafe45c13cb8ff240683b4d5117" @@ -4060,7 +4067,7 @@ create-require@^1.1.0: resolved "https://registry.yarnpkg.com/create-require/-/create-require-1.1.1.tgz#c1d7e8f1e5f6cfc9ff65f9cd352d37348756c333" integrity sha512-dcKFX3jn0MpIaXjisoRvexIJVEKzaq7z2rZKxf+MSr9TkdmHmsU4m2lcLojrj/FHl8mk5VxMmYA+ftRkP/3oKQ== -cross-spawn@^7.0.0, cross-spawn@^7.0.2, cross-spawn@^7.0.3: +cross-spawn@^7.0.0, cross-spawn@^7.0.2, cross-spawn@^7.0.3, cross-spawn@^7.0.6: version "7.0.6" resolved "https://registry.yarnpkg.com/cross-spawn/-/cross-spawn-7.0.6.tgz#8a58fe78f00dcd70c370451759dfbfaf03e8ee9f" integrity sha512-uV2QOWP2nWzsy2aMp8aRibhi9dlzF5Hgh5SHaB9OiTGEyDTiJJyx0uy51QXdyWbtAHNua4XJzUKca3OzKUd3vA==