[rush] Prevent shell injection in publish commit details (#5862)

TheLarkInn · selarkin · Copilot · web-flow · commit ac0947092e1d · 2026-07-13T18:42:46.000Z
* [rush] Prevent shell injection in publish commit details

Co-authored-by: Copilot App &lt;223556219+Copilot@users.noreply.github.com&gt;
Copilot-Session: 054aa0b3-8963-4ef3-a1f3-b7bf9d8b3479

* [rush] Preserve Git wrapper support

Co-authored-by: Copilot App &lt;223556219+Copilot@users.noreply.github.com&gt;
Copilot-Session: 054aa0b3-8963-4ef3-a1f3-b7bf9d8b3479

* [rush] Address publish review feedback

Co-authored-by: Copilot App &lt;223556219+Copilot@users.noreply.github.com&gt;
Copilot-Session: 054aa0b3-8963-4ef3-a1f3-b7bf9d8b3479

---------

Co-authored-by: selarkin &lt;selarkin+odspmdb@microsoft.com&gt;
Co-authored-by: Copilot App &lt;223556219+Copilot@users.noreply.github.com&gt;
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
@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@microsoft/rush",
+      "comment": "Pass change file paths as discrete Git arguments when adding 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
@@ -7,7 +7,7 @@
  */
 
 import * as path from 'node:path';
-import { execSync } 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';
@@ -83,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,
@@ -374,16 +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<void> {
     try {
       const gitPath: string = git.getGitPathOrThrow();
-      const fileLog: string = execSync(`${gitPath} log -n 1 ${filename}`, {
-        cwd: path.dirname(filename)
-      }).toString();
+      const gitProcess: child_process.ChildProcess = Executable.spawn(
+        gitPath,
+        ['log', '-n', '1', '--', filename],
+        {
+          currentWorkingDirectory: path.dirname(filename)
+        }
+      );
+      const { stdout: fileLog, exitCode, signal } = await Executable.waitForExitAsync(gitProcess, {
+        encoding: 'utf8'
+      });
+      if (exitCode !== 0 || signal) {
+        return;
+      }
+
       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
@@ -1,16 +1,34 @@
 // Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
 // See LICENSE in the project root for license information.
 
+import * as path from 'node:path';
+import type { ChildProcess } from 'node:child_process';
+
+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';
 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);
 }
 
+function createGitResult(
+  stdout: string,
+  exitCode: IWaitForExitResult<string>['exitCode'] = 0,
+  signal: IWaitForExitResult<string>['signal'] = null
+): IWaitForExitResult<string> {
+  return {
+    stdout,
+    stderr: '',
+    exitCode,
+    signal
+  };
+}
+
 function generateChangeSnapshot(
   allPackages: ReadonlyMap<string, RushConfigurationProject>,
   allChanges: IChangeRequests
@@ -83,6 +101,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<string, RushConfigurationProject> =
       packagesRushConfiguration.projectsByName;
@@ -96,6 +118,94 @@ describe(PublishUtilities.findChangeRequestsAsync.name, () => {
     expect(allChanges.versionPolicyChanges.size).toEqual(0);
   });
 
+  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',
+      'common',
+      'changes',
+      'change & echo injected.json'
+    );
+    const changes: IChangeInfo[] = [{ packageName: 'd' }];
+    const git: Git = new Git(packagesRushConfiguration);
+    const gitProcess: ChildProcess = {} as ChildProcess;
+
+    jest.spyOn(git, 'getGitPathOrThrow').mockReturnValue(gitPath);
+    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 <test@example.com>\n')
+      );
+
+    await PublishUtilities['_updateCommitDetailsAsync'](git, changeFilePath, changes);
+
+    expect(spawnSpy).toHaveBeenCalledWith(
+      gitPath,
+      ['log', '-n', '1', '--', changeFilePath],
+      { currentWorkingDirectory: path.dirname(changeFilePath) }
+    );
+    expect(waitForExitSpy).toHaveBeenCalledWith(gitProcess, { encoding: 'utf8' });
+    expect(changes).toEqual([
+      {
+        packageName: 'd',
+        author: 'Test Author <test@example.com>',
+        commit: '0123456789abcdef'
+      }
+    ]);
+  });
+
+  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 spawnSpy: jest.SpyInstance = jest.spyOn(Executable, 'spawn').mockReturnValue(gitProcess);
+    jest
+      .spyOn(Executable, 'waitForExitAsync')
+      .mockResolvedValue(
+        createGitResult('commit 0123456789abcdef\nAuthor: Test Author <test@example.com>\n')
+      );
+
+    await PublishUtilities['_updateCommitDetailsAsync'](git, changeFilePath, changes);
+
+    expect(spawnSpy).toHaveBeenCalledWith(
+      gitPath,
+      ['log', '-n', '1', '--', changeFilePath],
+      { currentWorkingDirectory: path.dirname(changeFilePath) }
+    );
+    expect(changes[0].commit).toEqual('0123456789abcdef');
+  });
+
+  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 <test@example.com>\n',
+          exitCode,
+          signal
+        )
+      );
+
+      await PublishUtilities['_updateCommitDetailsAsync'](git, path.resolve('change.json'), changes);
+
+      expect(changes).toEqual([{ packageName: 'd' }]);
+    }
+  );
+
   it('returns 1 change when changing a leaf package', async () => {
     const allPackages: ReadonlyMap<string, RushConfigurationProject> =
       packagesRushConfiguration.projectsByName;
