Skip to content

Commit c90eb29

Browse files
committed
Fix npm auth environment variables with yarn 4
1 parent 3be8fb1 commit c90eb29

6 files changed

Lines changed: 219 additions & 21 deletions

File tree

.github/workflows/pr.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,10 @@ jobs:
8080
id: test
8181

8282
# See packageManager.ts comments...
83-
- name: test publish (Windows bash)
83+
- name: run specific tests in Windows bash
8484
if: matrix.os == 'windows-latest'
8585
shell: bash
86-
run: yarn workspace beachball test packagePublish
86+
run: yarn workspace beachball test packagePublish npmAuthEnvPassthrough
8787

8888
# For a few specific test failures, it can be helpful to see npm debug logs
8989
# (usually not relevant, but there's not a good way to distinguish by specific failure)
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
{
2+
"changes": [
3+
{
4+
"type": "patch",
5+
"comment": "Fix npm auth environment variables with yarn 4",
6+
"packageName": "beachball",
7+
"email": "elcraig@microsoft.com",
8+
"dependentChangeType": "patch"
9+
}
10+
]
11+
}
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
import fs from 'fs';
2+
import path from 'path';
3+
import { afterEach, describe, expect, it } from '@jest/globals';
4+
import { checkNpmAuthEnvPassthrough, filterPathForNpm } from '../../packageManager/npmAuthEnvPassthrough';
5+
import { BeachballError } from '../../types/BeachballError';
6+
import { initMockLogs } from '../../__fixtures__/mockLogs';
7+
import { tmpdir, removeTempDir } from '../../__fixtures__/tmpdir';
8+
9+
describe('filterPathForNpm', () => {
10+
const makePathEnv = (...parts: string[]) => parts.join(path.delimiter);
11+
12+
it('keeps a plain path unchanged', () => {
13+
const p = makePathEnv('/usr/local/bin', '/usr/bin', '/bin');
14+
expect(filterPathForNpm(p)).toEqual(p);
15+
});
16+
17+
it('removes entries whose basename starts with yarn--', () => {
18+
const p = makePathEnv('/usr/local/bin', '/tmp/yarn--1234567890', '/usr/bin');
19+
expect(filterPathForNpm(p)).toEqual(makePathEnv('/usr/local/bin', '/usr/bin'));
20+
});
21+
22+
it('removes entries whose basename starts with xfs-', () => {
23+
const p = makePathEnv('/usr/local/bin', '/tmp/xfs-abc123', '/usr/bin');
24+
expect(filterPathForNpm(p)).toEqual(makePathEnv('/usr/local/bin', '/usr/bin'));
25+
});
26+
27+
it('removes multiple filtered entries', () => {
28+
const p = makePathEnv('/usr/local/bin', '/tmp/yarn--abc', '/tmp/xfs-xyz', '/usr/bin');
29+
expect(filterPathForNpm(p)).toEqual(makePathEnv('/usr/local/bin', '/usr/bin'));
30+
});
31+
32+
it('keeps entries where yarn-- or xfs- appears in a parent segment but not the basename', () => {
33+
const p = makePathEnv('/home/user/yarn--stuff/tools', '/home/user/xfs-stuff/tools');
34+
expect(filterPathForNpm(p)).toEqual(p);
35+
});
36+
37+
it('handles a single entry path', () => {
38+
expect(filterPathForNpm('/usr/local/bin')).toEqual('/usr/local/bin');
39+
});
40+
41+
it('returns empty string for empty input', () => {
42+
expect(filterPathForNpm('')).toEqual('');
43+
});
44+
});
45+
46+
describe('checkNpmAuthEnvPassthrough', () => {
47+
const logs = initMockLogs();
48+
const commonOptions = { path: process.cwd(), registry: 'https://registry.npmjs.org/' };
49+
50+
let wrapperDir: string | undefined;
51+
52+
afterEach(() => {
53+
wrapperDir && removeTempDir(wrapperDir);
54+
wrapperDir = undefined;
55+
});
56+
57+
it('passes when the real node binary is on PATH', async () => {
58+
// process.execPath's directory is on PATH in the test environment
59+
await checkNpmAuthEnvPassthrough({ ...commonOptions });
60+
expect(logs.getMockLines('error')).toEqual('');
61+
});
62+
63+
it('passes when PATH is given explicitly with node on it', async () => {
64+
const nodeBinDir = path.dirname(process.execPath);
65+
await checkNpmAuthEnvPassthrough({ ...commonOptions, pathEnv: nodeBinDir });
66+
expect(logs.getMockLines('error')).toEqual('');
67+
});
68+
69+
// A shebang-based wrapper only works on POSIX
70+
// eslint-disable-next-line no-restricted-properties
71+
const itPosixLike = path.delimiter === ':' ? it : it.skip;
72+
itPosixLike('throws when node is wrapped by a script that drops special env vars', async () => {
73+
wrapperDir = tmpdir({ prefix: 'beachball-test-wrapper-' });
74+
const wrapperScript = path.join(wrapperDir, 'node');
75+
76+
// Create a `node` wrapper script (using the real node binary in the shebang to avoid PATH lookup)
77+
// that mimics POSIX sh behavior of dropping invalid env var names.
78+
// We can't directly use `/bin/sh` because on some systems such as macOS, `sh` is actually `bash`.
79+
fs.writeFileSync(
80+
wrapperScript,
81+
[
82+
`#!${process.execPath}`,
83+
`const { spawnSync } = require('child_process');`,
84+
`const filteredEnv = Object.fromEntries(`,
85+
` Object.entries(process.env).filter(([k]) => /^[a-zA-Z_][a-zA-Z0-9_]*$/.test(k))`,
86+
`);`,
87+
`const r = spawnSync(process.execPath, process.argv.slice(2), { env: filteredEnv, stdio: 'inherit' });`,
88+
`process.exit(r.status ?? 1);`,
89+
].join('\n'),
90+
{ mode: 0o755 }
91+
);
92+
93+
const pathWithWrapper = wrapperDir + path.delimiter + process.env.PATH;
94+
95+
await expect(checkNpmAuthEnvPassthrough({ ...commonOptions, pathEnv: pathWithWrapper })).rejects.toThrow(
96+
BeachballError
97+
);
98+
const errorLogs = logs.getMockLines('error');
99+
expect(errorLogs).toContain('The environment variable used to pass the npm auth token');
100+
expect(errorLogs).toContain(`Your PATH:\n${pathWithWrapper}`);
101+
expect(errorLogs).toContain('node bin/beachball.js <args>');
102+
});
103+
});

packages/beachball/src/commands/publish.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { publishToRegistry } from '../publish/publishToRegistry';
88
import type { BeachballOptions } from '../types/BeachballOptions';
99
import type { PublishBumpInfo } from '../types/BumpInfo';
1010
import type { CommandContext } from '../types/CommandContext';
11+
import { checkNpmAuthEnvPassthrough } from '../packageManager/npmAuthEnvPassthrough';
1112

1213
/**
1314
* Potentially bump, publish, and push package changes depending on options.
@@ -68,6 +69,11 @@ export async function publish(options: BeachballOptions, context?: CommandContex
6869
console.log();
6970
}
7071

72+
if (options.token) {
73+
// Verify that passing the npm auth token via env vars works (see function comment...)
74+
await checkNpmAuthEnvPassthrough(options);
75+
}
76+
7177
// checkout publish branch
7278
const publishBranch = `publish_${Date.now()}`;
7379

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
import execa from 'execa';
2+
import path from 'path';
3+
import type { BeachballOptions } from '../types/BeachballOptions';
4+
import { getNpmAuthEnv } from './npmArgs';
5+
import { BeachballError } from '../types/BeachballError';
6+
import { findPackageRoot, getPackageInfo } from 'workspace-tools';
7+
8+
/**
9+
* Filter PATH for running npm commands, removing entries that contain shell-script node wrappers.
10+
*
11+
* Package managers inject temp directories into PATH with shims for `node` — yarn classic uses
12+
* `yarn--*` dirs, yarn berry uses `xfs-*` dirs. (Context: https://github.com/yarnpkg/yarn/issues/6685 -
13+
* yarn berry does similar but cleans up afterwards.)
14+
*
15+
* These shims are POSIX shell scripts, e.g. this `node` shim:
16+
* ```sh
17+
* #!/bin/sh
18+
* exec "/path/to/node" "$@"
19+
* ```
20+
*
21+
* Problem is POSIX `sh` drops env var names with invalid characters like `/` and `:`, which are
22+
* needed for npm auth token env vars (e.g. `npm_config_//someRegistry/:_authToken`).
23+
*
24+
* Removing those path entries fixes the issue and is unlikely to cause other problems in this context.
25+
* Use `checkNpmAuthEnvPassthrough` after this filter to detect unknown variants of this issue.
26+
*/
27+
export function filterPathForNpm(pathEnv: string): string {
28+
return pathEnv
29+
.split(path.delimiter)
30+
.filter(p => {
31+
const base = path.basename(p);
32+
return !base.startsWith('yarn--') && !base.startsWith('xfs-');
33+
})
34+
.join(path.delimiter);
35+
}
36+
37+
/**
38+
* Check whether env vars with special characters in their names (like npm auth token env vars,
39+
* which contain `//` and `:`) will be passed through to npm subprocesses. Runs a test using the same
40+
* PATH filtering applied to actual npm commands, so a failure here means the fix in
41+
* `filterPathForNpm` doesn't cover this platform/environment variant.
42+
*/
43+
export async function checkNpmAuthEnvPassthrough(
44+
options: Pick<BeachballOptions, 'registry' | 'path'> & {
45+
/** PATH override only for testing */
46+
pathEnv?: string;
47+
}
48+
): Promise<void> {
49+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
50+
const pathEnv = options.pathEnv ?? process.env.PATH!;
51+
const fakeToken = 'fake-token';
52+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- always defined if token is passed
53+
const tokenEnv = getNpmAuthEnv({ registry: options.registry, token: fakeToken })!;
54+
const envName = Object.keys(tokenEnv)[0];
55+
56+
const filteredPath = filterPathForNpm(pathEnv);
57+
try {
58+
const result = await execa('node', ['-e', `process.stdout.write(process.env[${JSON.stringify(envName)}] || '')`], {
59+
env: { ...tokenEnv, PATH: filteredPath },
60+
extendEnv: true,
61+
});
62+
if (result.stdout === fakeToken) {
63+
return;
64+
}
65+
} catch {
66+
// ignore
67+
}
68+
69+
let relBeachballBin: string | undefined;
70+
const beachballRoot = findPackageRoot(__dirname);
71+
const beachballPkg = beachballRoot ? getPackageInfo(beachballRoot) : undefined;
72+
// This should usually be found unless the code was bundled or something
73+
if (beachballRoot && beachballPkg?.name === 'beachball' && beachballPkg.bin) {
74+
const binValue =
75+
typeof beachballPkg.bin === 'string' ? beachballPkg.bin : (beachballPkg.bin as { beachball: string }).beachball;
76+
const beachballBinPath = path.join(beachballRoot, binValue);
77+
relBeachballBin = path.relative(options.path, beachballBinPath);
78+
}
79+
80+
console.error(
81+
[
82+
`The environment variable used to pass the npm auth token to "npm publish" is not being ` +
83+
`passed to npm subprocesses. This is typically caused by a shell script node wrapper in ` +
84+
`your PATH (e.g. injected by a package manager).`,
85+
'',
86+
`Please file an issue with beachball so this can be investigated. Your PATH:`,
87+
pathEnv,
88+
...(relBeachballBin
89+
? ['', `In the meantime, you can try running the CLI directly via Node:`, ` node ${relBeachballBin} <args>`]
90+
: []),
91+
].join('\n')
92+
);
93+
94+
throw new BeachballError('Error passing npm auth token', { alreadyLogged: true });
95+
}

packages/beachball/src/packageManager/packageManager.ts

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import execa from 'execa';
2-
import path from 'path';
2+
import { filterPathForNpm } from './npmAuthEnvPassthrough';
33

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

4124
try {

0 commit comments

Comments
 (0)