Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@microsoft/rush",
"comment": "Fix pnpm registry credentials being dropped by POSIX shells when `provideNpmrcCredentialsViaEnvironment` is enabled.",
"type": "patch"
}
],
"packageName": "@microsoft/rush"
}
2 changes: 1 addition & 1 deletion common/config/rush/build-cache.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
* - [os] Example: "win32"
* - [arch] Example: "x64"
*/
"cacheEntryNamePattern": "[projectName:normalize]-[phaseName:normalize]-[hash]",
"cacheEntryNamePattern": "[projectName:normalize]-[phaseName:normalize]-[os]-[hash]",

/**
* (Optional) Salt to inject during calculation of the cache key. This can be used to invalidate the cache for all projects when the salt changes.
Expand Down
1 change: 1 addition & 0 deletions libraries/rush-lib/src/cli/RushPnpmCommandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ export class RushPnpmCommandLineParser {
workingDirectory: process.cwd(),
environment: pnpmEnvironmentMap.toObject(),
keepEnvironment: true,
useShell: !InstallHelpers.shouldProvideNpmrcCredentialsViaEnvironment(rushConfiguration),
onStdoutStreamChunk,
captureExitCodeAndSignal: true
});
Expand Down
6 changes: 4 additions & 2 deletions libraries/rush-lib/src/logic/Autoinstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,8 @@ export class Autoinstaller {
args: ['install', '--frozen-lockfile'],
workingDirectory: autoinstallerFullPath,
environment: this.#getPackageManagerEnvironment(autoinstallerFullPath),
keepEnvironment: true
keepEnvironment: true,
useShell: !InstallHelpers.shouldProvideNpmrcCredentialsViaEnvironment(this.#rushConfiguration)
});

// Create file: ../common/autoinstallers/my-task/.rush/temp/last-install.flag
Expand Down Expand Up @@ -245,7 +246,8 @@ export class Autoinstaller {
args: ['install'],
workingDirectory: this.folderFullPath,
environment: this.#getPackageManagerEnvironment(this.folderFullPath),
keepEnvironment: true
keepEnvironment: true,
useShell: !InstallHelpers.shouldProvideNpmrcCredentialsViaEnvironment(this.#rushConfiguration)
});

this.#logIfConsoleOutputIsNotRestricted();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -504,8 +504,9 @@ export class RushInstallManager extends BaseInstallManager {
this.rushConfiguration,
{ ...this.options, npmrcFolder: subspace.getSubspaceTempFolderPath() }
);
const keepEnvironment: boolean =
InstallHelpers.shouldProvideNpmrcCredentialsViaEnvironment(this.rushConfiguration);
const keepEnvironment: boolean = InstallHelpers.shouldProvideNpmrcCredentialsViaEnvironment(
this.rushConfiguration
);

const commonNodeModulesFolder: string = path.join(
this.rushConfiguration.commonTempFolder,
Expand Down Expand Up @@ -560,8 +561,7 @@ export class RushInstallManager extends BaseInstallManager {
// eslint-disable-next-line no-console
console.log(`Deleting ${pathToDeleteWithoutStar}\\*`);
// Glob can't handle Windows paths
const normalizedPathToDeleteWithoutStar: string =
Path.convertToSlashes(pathToDeleteWithoutStar);
const normalizedPathToDeleteWithoutStar: string = Path.convertToSlashes(pathToDeleteWithoutStar);

const { default: glob } = await import('fast-glob');
const tempModulePaths: string[] = await glob(
Expand Down Expand Up @@ -625,6 +625,7 @@ export class RushInstallManager extends BaseInstallManager {
workingDirectory: this.rushConfiguration.commonTempFolder,
environment: packageManagerEnv,
keepEnvironment,
useShell: !keepEnvironment,
suppressOutput: false
},
this.options.maxInstallAttempts,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -496,8 +496,9 @@ export class WorkspaceInstallManager extends BaseInstallManager {
this.rushConfiguration,
{ ...this.options, npmrcFolder: subspace.getSubspaceTempFolderPath() }
);
const keepEnvironment: boolean =
InstallHelpers.shouldProvideNpmrcCredentialsViaEnvironment(this.rushConfiguration);
const keepEnvironment: boolean = InstallHelpers.shouldProvideNpmrcCredentialsViaEnvironment(
this.rushConfiguration
);
if (ConsoleTerminalProvider.supportsColor) {
packageManagerEnv.FORCE_COLOR = '1';
}
Expand Down Expand Up @@ -599,6 +600,7 @@ export class WorkspaceInstallManager extends BaseInstallManager {
workingDirectory: subspace.getSubspaceTempFolderPath(),
environment: packageManagerEnv,
keepEnvironment,
useShell: !keepEnvironment,
suppressOutput: false,
onStdoutStreamChunk: onPnpmStdoutChunk
},
Expand Down
48 changes: 31 additions & 17 deletions libraries/rush-lib/src/utilities/Utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ export interface IExecuteCommandOptions {
environment?: IEnvironment;
suppressOutput?: boolean;
keepEnvironment?: boolean;
/**
* Whether to use a shell on POSIX. Defaults to true.
* Windows always uses a shell to support package manager .cmd shims.
*/
useShell?: boolean;
/**
* Note that this takes precedence over {@link IExecuteCommandOptions.suppressOutput}
*/
Expand Down Expand Up @@ -369,6 +374,7 @@ export class Utilities {
onStdoutStreamChunk,
environment,
keepEnvironment,
useShell,
captureExitCodeAndSignal
} = options;
const { exitCode, signal } = await _executeCommandInternalAsync({
Expand All @@ -389,6 +395,7 @@ export class Utilities {
['inherit', 'inherit', 'inherit'],
environment,
keepEnvironment,
useShell,
onStdoutStreamChunk,
captureOutput: false,
captureExitCodeAndSignal
Expand Down Expand Up @@ -851,39 +858,46 @@ async function _executeCommandInternalAsync({
stdio,
environment,
keepEnvironment,
useShell = true,
onStdoutStreamChunk,
captureOutput,
captureExitCodeAndSignal
}: IExecuteCommandInternalOptions): Promise<IWaitForExitResult<string> | IWaitForExitResultWithoutOutput> {
const spawnOptions: child_process.SpawnSyncOptions = {
cwd: workingDirectory,
shell: true,
shell: IS_WINDOWS || useShell,
stdio: stdio,
env: keepEnvironment
? environment
: _createEnvironmentForRushCommand({ initialEnvironment: environment }),
maxBuffer: 10 * 1024 * 1024 // Set default max buffer size to 10MB
};

// This is needed since we specify shell=true below.
// NOTE: On Windows if we escape "NPM", the spawnSync() function runs something like this:
// [ 'C:\\Windows\\system32\\cmd.exe', '/s', '/c', '""NPM" "install""' ]
//
// Due to a bug with Windows cmd.exe, the npm.cmd batch file's "%~dp0" variable will
// return the current working directory instead of the batch file's directory.
// The workaround is to not escape, npm, i.e. do this instead:
// [ 'C:\\Windows\\system32\\cmd.exe', '/s', '/c', '"npm "install""' ]
//
// We will come up with a better solution for this when we promote executeCommand()
// into node-core-library, but for now this hack will unblock people:
let childProcess: child_process.ChildProcess;
if (!spawnOptions.shell) {
// POSIX shells can discard URL-scoped npm_config_* credential variables.
childProcess = child_process.spawn(command, args, spawnOptions);
} else {
// This is needed since we specify shell=true below.
// NOTE: On Windows if we escape "NPM", the spawnSync() function runs something like this:
// [ 'C:\\Windows\\system32\\cmd.exe', '/s', '/c', '""NPM" "install""' ]
//
// Due to a bug with Windows cmd.exe, the npm.cmd batch file's "%~dp0" variable will
// return the current working directory instead of the batch file's directory.
// The workaround is to not escape, npm, i.e. do this instead:
// [ 'C:\\Windows\\system32\\cmd.exe', '/s', '/c', '"npm "install""' ]
//
// We will come up with a better solution for this when we promote executeCommand()
// into node-core-library, but for now this hack will unblock people:

// Only escape the command if it actually contains spaces:
const escapedCommand: string = escapeArgumentIfNeeded(command);
// Only escape the command if it actually contains spaces:
const escapedCommand: string = escapeArgumentIfNeeded(command);

const escapedArgs: string[] = args.map((x) => escapeArgumentIfNeeded(x));
const shellCommand: string = [escapedCommand, ...escapedArgs].join(' ');
const escapedArgs: string[] = args.map((x) => escapeArgumentIfNeeded(x));
const shellCommand: string = [escapedCommand, ...escapedArgs].join(' ');

const childProcess: child_process.ChildProcess = child_process.spawn(shellCommand, spawnOptions);
childProcess = child_process.spawn(shellCommand, spawnOptions);
}

if (onStdoutStreamChunk) {
const inspectStream: Transform = new Transform({
Expand Down
151 changes: 151 additions & 0 deletions libraries/rush-lib/src/utilities/test/Utilities.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
// 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 { FileSystem } from '@rushstack/node-core-library';

import { type IDisposable, Utilities } from '../Utilities';
import { getNpmrcEnvironmentVariables, syncNpmrc } from '../npmrcUtilities';
import { IS_WINDOWS } from '../executionUtilities';

const PACKAGE_ROOT: string = path.resolve(__dirname, '../../..');
const TEST_TEMP_FOLDER: string = `${PACKAGE_ROOT}/temp/utilities-credential-environment-test`;

function withComSpec<T>(value: string | undefined, callback: () => T): T {
const originalValue: string | undefined = process.env.comspec;
Expand All @@ -23,6 +32,148 @@ function withComSpec<T>(value: string | undefined, callback: () => T): T {
}

describe(Utilities.name, () => {
describe('package manager credential environment', () => {
const credentialKey: string = 'npm_config_//registry.example.test/npm/:_authToken';
const credentialValue: string = 'non-secret-test-token';
const scriptPath: string = `${TEST_TEMP_FOLDER}/check credentials.cjs`;
let environment: NodeJS.ProcessEnv;

beforeAll(async () => {
await FileSystem.deleteFolderAsync(TEST_TEMP_FOLDER);
const sourceFolder: string = `${TEST_TEMP_FOLDER}/source`;
const targetFolder: string = `${TEST_TEMP_FOLDER}/target`;
await FileSystem.writeFileAsync(
`${sourceFolder}/.npmrc`,
'//registry.example.test/npm/:_authToken=${RUSH_TEST_TOKEN}\n',
{ ensureFolderExists: true }
);
const sourceEnvironment: NodeJS.ProcessEnv = { RUSH_TEST_TOKEN: credentialValue };
syncNpmrc({
sourceNpmrcFolder: sourceFolder,
targetNpmrcFolder: targetFolder,
supportEnvVarFallbackSyntax: true,
moveSensitiveSettingsToEnvironment: true,
env: sourceEnvironment
});
environment = {
...process.env,
...getNpmrcEnvironmentVariables({
npmrcFolder: targetFolder,
supportEnvVarFallbackSyntax: true,
env: sourceEnvironment
})
};
await FileSystem.writeFileAsync(
scriptPath,
[
`if (process.env[${JSON.stringify(credentialKey)}] !== ${JSON.stringify(credentialValue)}) {`,
' process.exit(42);',
'}',
'process.stdout.write(JSON.stringify(process.argv.slice(2)));'
].join('\n'),
{ ensureFolderExists: true }
);
expect(await FileSystem.readFileAsync(`${targetFolder}/.npmrc`)).not.toContain(credentialValue);
});

afterAll(async () => {
await FileSystem.deleteFolderAsync(TEST_TEMP_FOLDER);
});

it('preserves generated credentials through the captured subprocess path', async () => {
const output: string = await Utilities.executeCommandAndCaptureOutputAsync({
command: process.execPath,
args: [scriptPath, 'space argument'],
workingDirectory: TEST_TEMP_FOLDER,
environment,
keepEnvironment: true,
useShell: false
});
expect(JSON.parse(output)).toEqual(['space argument']);
});

it('preserves generated credentials through the install retry path', async () => {
await Utilities.executeCommandWithRetryAsync(
{
command: process.execPath,
args: [scriptPath],
workingDirectory: TEST_TEMP_FOLDER,
environment,
keepEnvironment: true,
useShell: false,
suppressOutput: true
},
1
);
});

(IS_WINDOWS ? it.skip : it)(
Comment thread
iclanton marked this conversation as resolved.
'passes POSIX arguments without shell expansion or pre-escaping',
async () => {
const args: string[] = [
'',
'two words',
'"quoted"',
"single'quote",
'$HOME',
'$(echo expanded)',
'*'
];
const output: string = await Utilities.executeCommandAndCaptureOutputAsync({
command: process.execPath,
args: [scriptPath, ...args],
workingDirectory: TEST_TEMP_FOLDER,
environment,
keepEnvironment: true,
useShell: false
});
expect(JSON.parse(output)).toEqual(args);
}
);

it('retains exit-code capture for failed direct subprocesses', async () => {
const { exitCode } = await Utilities.executeCommandAsync({
command: process.execPath,
args: [scriptPath],
workingDirectory: TEST_TEMP_FOLDER,
environment: { ...environment, [credentialKey]: 'wrong-test-token' },
keepEnvironment: true,
useShell: false,
captureExitCodeAndSignal: true,
suppressOutput: true
});
expect(exitCode).toBe(42);
});

it('still rejects failed direct subprocesses by default', async () => {
await expect(
Utilities.executeCommandAsync({
command: process.execPath,
args: [scriptPath],
workingDirectory: TEST_TEMP_FOLDER,
environment: { ...environment, [credentialKey]: 'wrong-test-token' },
keepEnvironment: true,
useShell: false,
suppressOutput: true
})
).rejects.toThrow();
});

it('retains shell execution by default', async () => {
const output: string = await Utilities.executeCommandAndCaptureOutputAsync({
command: 'echo',
args: ['first', '&&', 'echo', 'second'],
workingDirectory: TEST_TEMP_FOLDER
});
expect(
output
.trim()
.split(/\r?\n/)
.map((line) => line.trim())
).toEqual(['first', 'second']);
});
});

describe(Utilities.usingAsync.name, () => {
let disposed: boolean;

Expand Down
Loading