From 6cad05079fcb8f73b84971613f7a0d48ec7c8431 Mon Sep 17 00:00:00 2001 From: selarkin Date: Mon, 13 Jul 2026 08:51:20 -0700 Subject: [PATCH 1/3] [rush] Prevent shell injection in publish commit details Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 054aa0b3-8963-4ef3-a1f3-b7bf9d8b3479 --- ...sh-command-injection_2026-07-13-15-40.json | 10 +++++ .../rush-lib/src/logic/PublishUtilities.ts | 4 +- .../src/logic/test/PublishUtilities.test.ts | 41 +++++++++++++++++++ 3 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 common/changes/@microsoft/rush/fix-publish-command-injection_2026-07-13-15-40.json diff --git a/common/changes/@microsoft/rush/fix-publish-command-injection_2026-07-13-15-40.json b/common/changes/@microsoft/rush/fix-publish-command-injection_2026-07-13-15-40.json new file mode 100644 index 00000000000..068218b0d0f --- /dev/null +++ b/common/changes/@microsoft/rush/fix-publish-command-injection_2026-07-13-15-40.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@microsoft/rush", + "comment": "Run Git without a shell when adding change file commit details during publishing.", + "type": "patch" + } + ], + "packageName": "@microsoft/rush" +} diff --git a/libraries/rush-lib/src/logic/PublishUtilities.ts b/libraries/rush-lib/src/logic/PublishUtilities.ts index a76f946f92e..be15fbc1d95 100644 --- a/libraries/rush-lib/src/logic/PublishUtilities.ts +++ b/libraries/rush-lib/src/logic/PublishUtilities.ts @@ -7,7 +7,7 @@ */ import * as path from 'node:path'; -import { execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; import * as semver from 'semver'; @@ -377,7 +377,7 @@ export class PublishUtilities { private static _updateCommitDetails(git: Git, filename: string, changes: IChangeInfo[] | undefined): void { try { const gitPath: string = git.getGitPathOrThrow(); - const fileLog: string = execSync(`${gitPath} log -n 1 ${filename}`, { + const fileLog: string = execFileSync(gitPath, ['log', '-n', '1', '--', filename], { cwd: path.dirname(filename) }).toString(); const author: string = fileLog.match(/Author: (.*)/)![1]; diff --git a/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts b/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts index 3ac792fe0f6..b9e08d2ba36 100644 --- a/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts +++ b/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts @@ -1,11 +1,20 @@ // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. // See LICENSE in the project root for license information. +jest.mock('node:child_process', () => ({ + ...jest.requireActual('node:child_process'), + execFileSync: jest.fn() +})); + +import * as path from 'node:path'; +import { execFileSync } from 'node:child_process'; + import { type IChangeInfo, ChangeType } from '../../api/ChangeManagement'; import { RushConfiguration } from '../../api/RushConfiguration'; import type { RushConfigurationProject } from '../../api/RushConfigurationProject'; import { PublishUtilities, type IChangeRequests } from '../PublishUtilities'; import { ChangeFiles } from '../ChangeFiles'; +import { Git } from '../Git'; function createChangeFiles(changesFolder: string): ChangeFiles { return new ChangeFiles({ changesFolder } as unknown as RushConfiguration); @@ -96,6 +105,38 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => { expect(allChanges.versionPolicyChanges.size).toEqual(0); }); + it('passes change file paths as discrete Git arguments', () => { + const gitPath: string = path.resolve('git with spaces', 'git'); + const changeFilePath: string = path.resolve( + 'repo with spaces', + 'common', + 'changes', + 'change & echo injected.json' + ); + const changes: IChangeInfo[] = [{ packageName: 'd' }]; + const git: Git = new Git(packagesRushConfiguration); + + jest.spyOn(git, 'getGitPathOrThrow').mockReturnValue(gitPath); + jest + .mocked(execFileSync) + .mockReturnValue(Buffer.from('commit 0123456789abcdef\nAuthor: Test Author \n')); + + PublishUtilities['_updateCommitDetails'](git, changeFilePath, changes); + + expect(execFileSync).toHaveBeenCalledWith( + gitPath, + ['log', '-n', '1', '--', changeFilePath], + { cwd: path.dirname(changeFilePath) } + ); + expect(changes).toEqual([ + { + packageName: 'd', + author: 'Test Author ', + commit: '0123456789abcdef' + } + ]); + }); + it('returns 1 change when changing a leaf package', async () => { const allPackages: ReadonlyMap = packagesRushConfiguration.projectsByName; From f855625f555bf929dad2025a9f536cc3d335f3a8 Mon Sep 17 00:00:00 2001 From: selarkin Date: Mon, 13 Jul 2026 10:36:44 -0700 Subject: [PATCH 2/3] [rush] Preserve Git wrapper support Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 054aa0b3-8963-4ef3-a1f3-b7bf9d8b3479 --- ...sh-command-injection_2026-07-13-15-40.json | 2 +- .../rush-lib/src/logic/PublishUtilities.ts | 23 ++++-- .../src/logic/test/PublishUtilities.test.ts | 72 +++++++++++++++---- 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/common/changes/@microsoft/rush/fix-publish-command-injection_2026-07-13-15-40.json b/common/changes/@microsoft/rush/fix-publish-command-injection_2026-07-13-15-40.json index 068218b0d0f..394179ed144 100644 --- a/common/changes/@microsoft/rush/fix-publish-command-injection_2026-07-13-15-40.json +++ b/common/changes/@microsoft/rush/fix-publish-command-injection_2026-07-13-15-40.json @@ -2,7 +2,7 @@ "changes": [ { "packageName": "@microsoft/rush", - "comment": "Run Git without a shell when adding change file commit details during publishing.", + "comment": "Pass change file paths as discrete Git arguments when adding commit details during publishing.", "type": "patch" } ], diff --git a/libraries/rush-lib/src/logic/PublishUtilities.ts b/libraries/rush-lib/src/logic/PublishUtilities.ts index be15fbc1d95..961cdec9610 100644 --- a/libraries/rush-lib/src/logic/PublishUtilities.ts +++ b/libraries/rush-lib/src/logic/PublishUtilities.ts @@ -7,7 +7,7 @@ */ import * as path from 'node:path'; -import { execFileSync } from 'node:child_process'; +import type child_process from 'node:child_process'; import * as semver from 'semver'; @@ -17,7 +17,8 @@ import { FileConstants, Text, Enum, - InternalError + InternalError, + Executable } from '@rushstack/node-core-library'; import { type IChangeInfo, ChangeType, type IVersionPolicyChangeInfo } from '../api/ChangeManagement'; @@ -377,9 +378,21 @@ export class PublishUtilities { private static _updateCommitDetails(git: Git, filename: string, changes: IChangeInfo[] | undefined): void { try { const gitPath: string = git.getGitPathOrThrow(); - const fileLog: string = execFileSync(gitPath, ['log', '-n', '1', '--', filename], { - cwd: path.dirname(filename) - }).toString(); + const gitResult: child_process.SpawnSyncReturns = Executable.spawnSync( + gitPath, + ['log', '-n', '1', '--', filename], + { + currentWorkingDirectory: path.dirname(filename) + } + ); + if (gitResult.error) { + throw gitResult.error; + } + if (gitResult.status !== 0) { + throw new Error(`Git exited with code ${gitResult.status}: ${gitResult.stderr}`); + } + + const fileLog: string = gitResult.stdout; const author: string = fileLog.match(/Author: (.*)/)![1]; const commit: string = fileLog.match(/commit (.*)/)![1]; diff --git a/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts b/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts index b9e08d2ba36..5445457ad00 100644 --- a/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts +++ b/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts @@ -1,14 +1,10 @@ // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license. // See LICENSE in the project root for license information. -jest.mock('node:child_process', () => ({ - ...jest.requireActual('node:child_process'), - execFileSync: jest.fn() -})); - import * as path from 'node:path'; -import { execFileSync } from 'node:child_process'; +import type { SpawnSyncReturns } from 'node:child_process'; +import { Executable } from '@rushstack/node-core-library'; import { type IChangeInfo, ChangeType } from '../../api/ChangeManagement'; import { RushConfiguration } from '../../api/RushConfiguration'; import type { RushConfigurationProject } from '../../api/RushConfigurationProject'; @@ -20,6 +16,17 @@ function createChangeFiles(changesFolder: string): ChangeFiles { return new ChangeFiles({ changesFolder } as unknown as RushConfiguration); } +function createGitResult(stdout: string, status: number = 0): SpawnSyncReturns { + return { + pid: 0, + output: [], + stdout, + stderr: '', + status, + signal: null + }; +} + function generateChangeSnapshot( allPackages: ReadonlyMap, allChanges: IChangeRequests @@ -92,6 +99,10 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => { repoRushConfiguration = RushConfiguration.loadFromConfigurationFile(`${__dirname}/repo/rush.json`); }); + afterEach(() => { + jest.restoreAllMocks(); + }); + it('returns no changes in an empty change folder', async () => { const allPackages: ReadonlyMap = packagesRushConfiguration.projectsByName; @@ -106,7 +117,7 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => { }); it('passes change file paths as discrete Git arguments', () => { - const gitPath: string = path.resolve('git with spaces', 'git'); + const gitPath: string = path.resolve('git with spaces', 'git.exe'); const changeFilePath: string = path.resolve( 'repo with spaces', 'common', @@ -117,16 +128,16 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => { const git: Git = new Git(packagesRushConfiguration); jest.spyOn(git, 'getGitPathOrThrow').mockReturnValue(gitPath); - jest - .mocked(execFileSync) - .mockReturnValue(Buffer.from('commit 0123456789abcdef\nAuthor: Test Author \n')); + const spawnSyncSpy: jest.SpyInstance = jest.spyOn(Executable, 'spawnSync').mockReturnValue( + createGitResult('commit 0123456789abcdef\nAuthor: Test Author \n') + ); PublishUtilities['_updateCommitDetails'](git, changeFilePath, changes); - expect(execFileSync).toHaveBeenCalledWith( + expect(spawnSyncSpy).toHaveBeenCalledWith( gitPath, ['log', '-n', '1', '--', changeFilePath], - { cwd: path.dirname(changeFilePath) } + { currentWorkingDirectory: path.dirname(changeFilePath) } ); expect(changes).toEqual([ { @@ -137,6 +148,43 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => { ]); }); + it('delegates Git wrapper paths to Executable', () => { + const gitPath: string = path.resolve('git-wrapper', 'git.cmd'); + const changeFilePath: string = path.resolve('repo', 'common', 'changes', 'change.json'); + const changes: IChangeInfo[] = [{ packageName: 'd' }]; + const git: Git = new Git(packagesRushConfiguration); + + jest.spyOn(git, 'getGitPathOrThrow').mockReturnValue(gitPath); + const spawnSyncSpy: jest.SpyInstance = jest.spyOn(Executable, 'spawnSync').mockReturnValue( + createGitResult('commit 0123456789abcdef\nAuthor: Test Author \n') + ); + + PublishUtilities['_updateCommitDetails'](git, changeFilePath, changes); + + expect(spawnSyncSpy).toHaveBeenCalledWith( + gitPath, + ['log', '-n', '1', '--', changeFilePath], + { currentWorkingDirectory: path.dirname(changeFilePath) } + ); + expect(changes[0].commit).toEqual('0123456789abcdef'); + }); + + it('does not use Git output from a failed process', () => { + const changes: IChangeInfo[] = [{ packageName: 'd' }]; + const git: Git = new Git(packagesRushConfiguration); + + jest.spyOn(git, 'getGitPathOrThrow').mockReturnValue(path.resolve('git.exe')); + jest + .spyOn(Executable, 'spawnSync') + .mockReturnValue( + createGitResult('commit 0123456789abcdef\nAuthor: Test Author \n', 1) + ); + + PublishUtilities['_updateCommitDetails'](git, path.resolve('change.json'), changes); + + expect(changes).toEqual([{ packageName: 'd' }]); + }); + it('returns 1 change when changing a leaf package', async () => { const allPackages: ReadonlyMap = packagesRushConfiguration.projectsByName; From 25b8608f31d1a478d5c2c92fc64034296e391e0a Mon Sep 17 00:00:00 2001 From: selarkin Date: Mon, 13 Jul 2026 11:17:11 -0700 Subject: [PATCH 3/3] [rush] Address publish review feedback Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 054aa0b3-8963-4ef3-a1f3-b7bf9d8b3479 --- .../rush-lib/src/logic/PublishUtilities.ts | 26 +++--- .../src/logic/test/PublishUtilities.test.ts | 83 ++++++++++++------- 2 files changed, 67 insertions(+), 42 deletions(-) diff --git a/libraries/rush-lib/src/logic/PublishUtilities.ts b/libraries/rush-lib/src/logic/PublishUtilities.ts index 961cdec9610..bf62ebe05ad 100644 --- a/libraries/rush-lib/src/logic/PublishUtilities.ts +++ b/libraries/rush-lib/src/logic/PublishUtilities.ts @@ -84,13 +84,14 @@ export class PublishUtilities { // Add the minimum changes defined by the change descriptions. for (const changeFilePath of files) { const changeRequest: IChangeInfo = JsonFile.load(changeFilePath); + const changes: IChangeInfo[] = changeRequest.changes!; if (includeCommitDetails) { const git: Git = new Git(rushConfiguration); - PublishUtilities._updateCommitDetails(git, changeFilePath, changeRequest.changes); + await PublishUtilities._updateCommitDetailsAsync(git, changeFilePath, changes); } - for (const change of changeRequest.changes!) { + for (const change of changes) { PublishUtilities._addChange({ change, changeFilePath, @@ -375,28 +376,31 @@ export class PublishUtilities { ); } - private static _updateCommitDetails(git: Git, filename: string, changes: IChangeInfo[] | undefined): void { + private static async _updateCommitDetailsAsync( + git: Git, + filename: string, + changes: IChangeInfo[] + ): Promise { try { const gitPath: string = git.getGitPathOrThrow(); - const gitResult: child_process.SpawnSyncReturns = Executable.spawnSync( + const gitProcess: child_process.ChildProcess = Executable.spawn( gitPath, ['log', '-n', '1', '--', filename], { currentWorkingDirectory: path.dirname(filename) } ); - if (gitResult.error) { - throw gitResult.error; - } - if (gitResult.status !== 0) { - throw new Error(`Git exited with code ${gitResult.status}: ${gitResult.stderr}`); + const { stdout: fileLog, exitCode, signal } = await Executable.waitForExitAsync(gitProcess, { + encoding: 'utf8' + }); + if (exitCode !== 0 || signal) { + return; } - const fileLog: string = gitResult.stdout; const author: string = fileLog.match(/Author: (.*)/)![1]; const commit: string = fileLog.match(/commit (.*)/)![1]; - changes!.forEach((change) => { + changes.forEach((change) => { change.author = author; change.commit = commit; }); diff --git a/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts b/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts index 5445457ad00..a9b73038c9f 100644 --- a/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts +++ b/libraries/rush-lib/src/logic/test/PublishUtilities.test.ts @@ -2,9 +2,9 @@ // See LICENSE in the project root for license information. import * as path from 'node:path'; -import type { SpawnSyncReturns } from 'node:child_process'; +import type { ChildProcess } from 'node:child_process'; -import { Executable } from '@rushstack/node-core-library'; +import { Executable, type IWaitForExitResult } from '@rushstack/node-core-library'; import { type IChangeInfo, ChangeType } from '../../api/ChangeManagement'; import { RushConfiguration } from '../../api/RushConfiguration'; import type { RushConfigurationProject } from '../../api/RushConfigurationProject'; @@ -16,14 +16,16 @@ function createChangeFiles(changesFolder: string): ChangeFiles { return new ChangeFiles({ changesFolder } as unknown as RushConfiguration); } -function createGitResult(stdout: string, status: number = 0): SpawnSyncReturns { +function createGitResult( + stdout: string, + exitCode: IWaitForExitResult['exitCode'] = 0, + signal: IWaitForExitResult['signal'] = null +): IWaitForExitResult { return { - pid: 0, - output: [], stdout, stderr: '', - status, - signal: null + exitCode, + signal }; } @@ -116,7 +118,7 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => { expect(allChanges.versionPolicyChanges.size).toEqual(0); }); - it('passes change file paths as discrete Git arguments', () => { + it('passes change file paths as discrete Git arguments', async () => { const gitPath: string = path.resolve('git with spaces', 'git.exe'); const changeFilePath: string = path.resolve( 'repo with spaces', @@ -126,19 +128,24 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => { ); const changes: IChangeInfo[] = [{ packageName: 'd' }]; const git: Git = new Git(packagesRushConfiguration); + const gitProcess: ChildProcess = {} as ChildProcess; jest.spyOn(git, 'getGitPathOrThrow').mockReturnValue(gitPath); - const spawnSyncSpy: jest.SpyInstance = jest.spyOn(Executable, 'spawnSync').mockReturnValue( - createGitResult('commit 0123456789abcdef\nAuthor: Test Author \n') - ); + const spawnSpy: jest.SpyInstance = jest.spyOn(Executable, 'spawn').mockReturnValue(gitProcess); + const waitForExitSpy: jest.SpyInstance = jest + .spyOn(Executable, 'waitForExitAsync') + .mockResolvedValue( + createGitResult('commit 0123456789abcdef\nAuthor: Test Author \n') + ); - PublishUtilities['_updateCommitDetails'](git, changeFilePath, changes); + await PublishUtilities['_updateCommitDetailsAsync'](git, changeFilePath, changes); - expect(spawnSyncSpy).toHaveBeenCalledWith( + expect(spawnSpy).toHaveBeenCalledWith( gitPath, ['log', '-n', '1', '--', changeFilePath], { currentWorkingDirectory: path.dirname(changeFilePath) } ); + expect(waitForExitSpy).toHaveBeenCalledWith(gitProcess, { encoding: 'utf8' }); expect(changes).toEqual([ { packageName: 'd', @@ -148,20 +155,24 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => { ]); }); - it('delegates Git wrapper paths to Executable', () => { + it('delegates Git wrapper paths to Executable', async () => { const gitPath: string = path.resolve('git-wrapper', 'git.cmd'); const changeFilePath: string = path.resolve('repo', 'common', 'changes', 'change.json'); const changes: IChangeInfo[] = [{ packageName: 'd' }]; const git: Git = new Git(packagesRushConfiguration); + const gitProcess: ChildProcess = {} as ChildProcess; jest.spyOn(git, 'getGitPathOrThrow').mockReturnValue(gitPath); - const spawnSyncSpy: jest.SpyInstance = jest.spyOn(Executable, 'spawnSync').mockReturnValue( - createGitResult('commit 0123456789abcdef\nAuthor: Test Author \n') - ); + const spawnSpy: jest.SpyInstance = jest.spyOn(Executable, 'spawn').mockReturnValue(gitProcess); + jest + .spyOn(Executable, 'waitForExitAsync') + .mockResolvedValue( + createGitResult('commit 0123456789abcdef\nAuthor: Test Author \n') + ); - PublishUtilities['_updateCommitDetails'](git, changeFilePath, changes); + await PublishUtilities['_updateCommitDetailsAsync'](git, changeFilePath, changes); - expect(spawnSyncSpy).toHaveBeenCalledWith( + expect(spawnSpy).toHaveBeenCalledWith( gitPath, ['log', '-n', '1', '--', changeFilePath], { currentWorkingDirectory: path.dirname(changeFilePath) } @@ -169,21 +180,31 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => { expect(changes[0].commit).toEqual('0123456789abcdef'); }); - it('does not use Git output from a failed process', () => { - const changes: IChangeInfo[] = [{ packageName: 'd' }]; - const git: Git = new Git(packagesRushConfiguration); - - jest.spyOn(git, 'getGitPathOrThrow').mockReturnValue(path.resolve('git.exe')); - jest - .spyOn(Executable, 'spawnSync') - .mockReturnValue( - createGitResult('commit 0123456789abcdef\nAuthor: Test Author \n', 1) + it.each([ + { exitCode: 1, signal: null }, + { exitCode: null, signal: 'SIGTERM' } + ])( + 'does not use Git output from an unsuccessful process ($exitCode, $signal)', + async ({ exitCode, signal }) => { + const changes: IChangeInfo[] = [{ packageName: 'd' }]; + const git: Git = new Git(packagesRushConfiguration); + const gitProcess: ChildProcess = {} as ChildProcess; + + jest.spyOn(git, 'getGitPathOrThrow').mockReturnValue(path.resolve('git.exe')); + jest.spyOn(Executable, 'spawn').mockReturnValue(gitProcess); + jest.spyOn(Executable, 'waitForExitAsync').mockResolvedValue( + createGitResult( + 'commit 0123456789abcdef\nAuthor: Test Author \n', + exitCode, + signal + ) ); - PublishUtilities['_updateCommitDetails'](git, path.resolve('change.json'), changes); + await PublishUtilities['_updateCommitDetailsAsync'](git, path.resolve('change.json'), changes); - expect(changes).toEqual([{ packageName: 'd' }]); - }); + expect(changes).toEqual([{ packageName: 'd' }]); + } + ); it('returns 1 change when changing a leaf package', async () => { const allPackages: ReadonlyMap =