Fix npm auth environment variables with yarn 4 (v3)

.github/workflows/pr.yml

@@ -71,10 +71,10 @@ jobs:
         id: test
 
       # See packageManager.ts comments...
-      - name: test publish (Windows bash)
+      - name: run specific tests in Windows bash
         if: matrix.os == 'windows-latest'
         shell: bash
-        run: yarn workspace beachball test packagePublish
+        run: yarn workspace beachball test packagePublish npmAuthEnvPassthrough
 
       # For a few specific test failures, it can be helpful to see npm debug logs
       # (usually not relevant, but there's not a good way to distinguish by specific failure)

change/change-4095d8fa-faa9-49db-8687-4a11e49f08fd.json

@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "type": "patch",
+      "comment": "Fix npm auth environment variables with yarn 4",
+      "packageName": "beachball",
+      "email": "elcraig@microsoft.com",
+      "dependentChangeType": "patch"
+    }
+  ]
+}

packages/beachball/src/__functional__/packageManager/npmAuthEnvPassthrough.test.ts

@@ -0,0 +1,103 @@
+import fs from 'fs';
+import path from 'path';
+import { afterEach, describe, expect, it } from '@jest/globals';
+import { checkNpmAuthEnvPassthrough, filterPathForNpm } from '../../packageManager/npmAuthEnvPassthrough';
+import { BeachballError } from '../../types/BeachballError';
+import { initMockLogs } from '../../__fixtures__/mockLogs';
+import { tmpdir, removeTempDir } from '../../__fixtures__/tmpdir';
+
+describe('filterPathForNpm', () => {
+  const makePathEnv = (...parts: string[]) => parts.join(path.delimiter);
+
+  it('keeps a plain path unchanged', () => {
+    const p = makePathEnv('/usr/local/bin', '/usr/bin', '/bin');
+    expect(filterPathForNpm(p)).toEqual(p);
+  });
+
+  it('removes entries whose basename starts with yarn--', () => {
+    const p = makePathEnv('/usr/local/bin', '/tmp/yarn--1234567890', '/usr/bin');
+    expect(filterPathForNpm(p)).toEqual(makePathEnv('/usr/local/bin', '/usr/bin'));
+  });
+
+  it('removes entries whose basename starts with xfs-', () => {
+    const p = makePathEnv('/usr/local/bin', '/tmp/xfs-abc123', '/usr/bin');
+    expect(filterPathForNpm(p)).toEqual(makePathEnv('/usr/local/bin', '/usr/bin'));
+  });
+
+  it('removes multiple filtered entries', () => {
+    const p = makePathEnv('/usr/local/bin', '/tmp/yarn--abc', '/tmp/xfs-xyz', '/usr/bin');
+    expect(filterPathForNpm(p)).toEqual(makePathEnv('/usr/local/bin', '/usr/bin'));
+  });
+
+  it('keeps entries where yarn-- or xfs- appears in a parent segment but not the basename', () => {
+    const p = makePathEnv('/home/user/yarn--stuff/tools', '/home/user/xfs-stuff/tools');
+    expect(filterPathForNpm(p)).toEqual(p);
+  });
+
+  it('handles a single entry path', () => {
+    expect(filterPathForNpm('/usr/local/bin')).toEqual('/usr/local/bin');
+  });
+
+  it('returns empty string for empty input', () => {
+    expect(filterPathForNpm('')).toEqual('');
+  });
+});
+
+describe('checkNpmAuthEnvPassthrough', () => {
+  const logs = initMockLogs();
+  const commonOptions = { path: process.cwd(), registry: 'https://registry.npmjs.org/' };
+
+  let wrapperDir: string | undefined;
+
+  afterEach(() => {
+    wrapperDir && removeTempDir(wrapperDir);
+    wrapperDir = undefined;
+  });
+
+  it('passes when the real node binary is on PATH', async () => {
+    // process.execPath's directory is on PATH in the test environment
+    await checkNpmAuthEnvPassthrough({ ...commonOptions });
+    expect(logs.getMockLines('error')).toEqual('');
+  });
+
+  it('passes when PATH is given explicitly with node on it', async () => {
+    const nodeBinDir = path.dirname(process.execPath);
+    await checkNpmAuthEnvPassthrough({ ...commonOptions, pathEnv: nodeBinDir });
+    expect(logs.getMockLines('error')).toEqual('');
+  });
+
+  // A shebang-based wrapper only works on POSIX
+  // eslint-disable-next-line no-restricted-properties
+  const itPosixLike = path.delimiter === ':' ? it : it.skip;
+  itPosixLike('throws when node is wrapped by a script that drops special env vars', async () => {
+    wrapperDir = tmpdir({ prefix: 'beachball-test-wrapper-' });
+    const wrapperScript = path.join(wrapperDir, 'node');
+
+    // Create a `node` wrapper script (using the real node binary in the shebang to avoid PATH lookup)
+    // that mimics POSIX sh behavior of dropping invalid env var names.
+    // We can't directly use `/bin/sh` because on some systems such as macOS, `sh` is actually `bash`.
+    fs.writeFileSync(
+      wrapperScript,
+      [
+        `#!${process.execPath}`,
+        `const { spawnSync } = require('child_process');`,
+        `const filteredEnv = Object.fromEntries(`,
+        `  Object.entries(process.env).filter(([k]) => /^[a-zA-Z_][a-zA-Z0-9_]*$/.test(k))`,
+        `);`,
+        `const r = spawnSync(process.execPath, process.argv.slice(2), { env: filteredEnv, stdio: 'inherit' });`,
+        `process.exit(r.status ?? 1);`,
+      ].join('\n'),
+      { mode: 0o755 }
+    );
+
+    const pathWithWrapper = wrapperDir + path.delimiter + process.env.PATH!;
+
+    await expect(checkNpmAuthEnvPassthrough({ ...commonOptions, pathEnv: pathWithWrapper })).rejects.toThrow(
+      BeachballError
+    );
+    const errorLogs = logs.getMockLines('error');
+    expect(errorLogs).toContain('The environment variable used to pass the npm auth token');
+    expect(errorLogs).toContain(`Your PATH:\n${pathWithWrapper}`);
+    expect(errorLogs).toContain('node bin/beachball.js <args>');
+  });
+});

packages/beachball/src/commands/publish.ts

@@ -8,6 +8,7 @@ import { publishToRegistry } from '../publish/publishToRegistry';
 import type { BeachballOptions } from '../types/BeachballOptions';
 import type { PublishBumpInfo } from '../types/BumpInfo';
 import type { CommandContext } from '../types/CommandContext';
+import { checkNpmAuthEnvPassthrough } from '../packageManager/npmAuthEnvPassthrough';
 
 /**
  * Potentially bump, publish, and push package changes depending on options.
@@ -68,6 +69,11 @@ export async function publish(options: BeachballOptions, context?: CommandContex
     console.log();
   }
 
+  if (options.token) {
+    // Verify that passing the npm auth token via env vars works (see function comment...)
+    await checkNpmAuthEnvPassthrough(options);
+  }
+
   // checkout publish branch
   const publishBranch = `publish_${Date.now()}`;
 

packages/beachball/src/packageManager/npmAuthEnvPassthrough.ts

@@ -0,0 +1,95 @@
+import execa from 'execa';
+import path from 'path';
+import type { BeachballOptions } from '../types/BeachballOptions';
+import { getNpmAuthEnv } from './npmArgs';
+import { BeachballError } from '../types/BeachballError';
+import { findPackageRoot, getPackageInfo } from 'workspace-tools';
+
+/**
+ * Filter PATH for running npm commands, removing entries that contain shell-script node wrappers.
+ *
+ * Package managers inject temp directories into PATH with shims for `node` — yarn classic uses
+ * `yarn--*` dirs, yarn berry uses `xfs-*` dirs. (Context: https://github.com/yarnpkg/yarn/issues/6685 -
+ * yarn berry does similar but cleans up afterwards.)
+ *
+ * These shims are POSIX shell scripts, e.g. this `node` shim:
+ *    ```sh
+ *    #!/bin/sh
+ *    exec "/path/to/node" "$@"
+ *    ```
+ *
+ * Problem is POSIX `sh` drops env var names with invalid characters like `/` and `:`, which are
+ * needed for npm auth token env vars (e.g. `npm_config_//someRegistry/:_authToken`).
+ *
+ * Removing those path entries fixes the issue and is unlikely to cause other problems in this context.
+ * Use `checkNpmAuthEnvPassthrough` after this filter to detect unknown variants of this issue.
+ */
+export function filterPathForNpm(pathEnv: string): string {
+  return pathEnv
+    .split(path.delimiter)
+    .filter(p => {
+      const base = path.basename(p);
+      return !base.startsWith('yarn--') && !base.startsWith('xfs-');
+    })
+    .join(path.delimiter);
+}
+
+/**
+ * Check whether env vars with special characters in their names (like npm auth token env vars,
+ * which contain `//` and `:`) will be passed through to npm subprocesses. Runs a test using the same
+ * PATH filtering applied to actual npm commands, so a failure here means the fix in
+ * `filterPathForNpm` doesn't cover this platform/environment variant.
+ */
+export async function checkNpmAuthEnvPassthrough(
+  options: Pick<BeachballOptions, 'registry' | 'path'> & {
+    /** PATH override only for testing */
+    pathEnv?: string;
+  }
+): Promise<void> {
+  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+  const pathEnv = options.pathEnv ?? process.env.PATH!;
+  const fakeToken = 'fake-token';
+  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- always defined if token is passed
+  const tokenEnv = getNpmAuthEnv({ registry: options.registry, token: fakeToken })!;
+  const envName = Object.keys(tokenEnv)[0];
+
+  const filteredPath = filterPathForNpm(pathEnv);
+  try {
+    const result = await execa('node', ['-e', `process.stdout.write(process.env[${JSON.stringify(envName)}] || '')`], {
+      env: { ...tokenEnv, PATH: filteredPath },
+      extendEnv: true,
+    });
+    if (result.stdout === fakeToken) {
+      return;
+    }
+  } catch {
+    // ignore
+  }
+
+  let relBeachballBin: string | undefined;
+  const beachballRoot = findPackageRoot(__dirname);
+  const beachballPkg = beachballRoot ? getPackageInfo(beachballRoot) : undefined;
+  // This should usually be found unless the code was bundled or something
+  if (beachballRoot && beachballPkg?.name === 'beachball' && beachballPkg.bin) {
+    const binValue =
+      typeof beachballPkg.bin === 'string' ? beachballPkg.bin : (beachballPkg.bin as { beachball: string }).beachball;
+    const beachballBinPath = path.join(beachballRoot, binValue);
+    relBeachballBin = path.relative(options.path, beachballBinPath);
+  }
+
+  console.error(
+    [
+      `The environment variable used to pass the npm auth token to "npm publish" is not being ` +
+        `passed to npm subprocesses. This is typically caused by a shell script node wrapper in ` +
+        `your PATH (e.g. injected by a package manager).`,
+      '',
+      `Please file an issue with beachball so this can be investigated. Your PATH:`,
+      pathEnv,
+      ...(relBeachballBin
+        ? ['', `In the meantime, you can try running the CLI directly via Node:`, `  node ${relBeachballBin} <args>`]
+        : []),
+    ].join('\n')
+  );
+
+  throw new BeachballError('Error passing npm auth token', { alreadyLogged: true });
+}

packages/beachball/src/packageManager/packageManager.ts

@@ -1,5 +1,5 @@
 import execa from 'execa';
-import path from 'path';
+import { filterPathForNpm } from './npmAuthEnvPassthrough';
 
 export type PackageManagerResult = execa.ExecaReturnValue & { success: boolean };
 export type PackageManagerOptions = execa.Options & { cwd: string };
@@ -18,24 +18,7 @@ export async function packageManager(
 ): Promise<PackageManagerResult> {
   let pathEnv = options.env?.PATH || process.env.PATH;
   if (manager === 'npm' && pathEnv) {
-    // Workaround for an issue on certain platforms/shells(?) if the parent command was run VIA yarn:
-    // The auth environment variable (e.g. `npm_config_//someRegistry/:_authToken`) was not being
-    // passed through to the child process. This might be because:
-    // - Special characters such as / and : aren't valid in env var names for certain shells/platforms
-    // - On every `yarn run ...` command, yarn makes temp directories like /<temp>/yarn--1776822418161-0.7992675923334178
-    //   with aliases for `node` and `yarn`. On Linux (and Mac), the `node` alias looks something like:
-    //     #!/bin/sh
-    //     exec "/path/to/node" "$@"
-    //   (see https://github.com/yarnpkg/yarn/issues/6685 for context)
-    // - Best guess: invalid environment variable names are dropped by this extra `exec` step??
-    //   (This consistently reproed on Ubuntu+bash, but not Mac+zsh or bash. The clue was that the
-    //   tests passed even on Linux when run via debugTests.js, but failed when run via yarn test.)
-    //
-    // Removing the yarn-- segment from the PATH seems to consistently fix this issue.
-    pathEnv = pathEnv
-      .split(path.delimiter)
-      .filter(p => !path.basename(p).startsWith('yarn--'))
-      .join(path.delimiter);
+    pathEnv = filterPathForNpm(pathEnv);
   }
 
   try {