Skip to content

Commit 42d342a

Browse files
committed
fix: address Copilot PR review feedback
- Switch RESERVED_ENV_VARS from a fixed denylist to prefix-based blocking (RUNNER_*, GITHUB_*) with GITHUB_TOKEN on an explicit allowlist. Previously a `.npmrc` referencing e.g. ${GITHUB_REF} or ${RUNNER_NAME} would re-export those via GITHUB_ENV and clobber runner-provided values for later steps - Skip registry values containing ${VAR} when computing registriesNeedingAuth; npm/pnpm do not expand env vars inside `.npmrc` keys, so synthesizing an `_authToken` line against a non-literal URL would silently produce an unreachable auth entry. The referenced var is still collected for env propagation - Reword the README so `registry-url` is described as bypassing the action's repo-.npmrc detection, not the package manager's config resolution
1 parent f61ca9e commit 42d342a

4 files changed

Lines changed: 62 additions & 19 deletions

File tree

README.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,9 @@ steps:
117117
If you already have the `_authToken` line in your repo `.npmrc` (e.g. for local
118118
dev symmetry), that's respected as-is and the action won't overwrite it.
119119

120-
Alternatively, pass `registry-url` explicitly to skip repo-level `.npmrc` entirely:
120+
Alternatively, pass `registry-url` explicitly to bypass the action's repo-level
121+
`.npmrc` detection and auth propagation logic (the package manager may still
122+
read the repo `.npmrc` per its own config resolution):
121123

122124
```yaml
123125
steps:

dist/index.mjs

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

src/auth.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,34 @@ describe("propagateProjectNpmrcAuth", () => {
251251
expect(exportVariable).not.toHaveBeenCalledWith("HOME", expect.anything());
252252
});
253253

254+
it("blocks runner-managed GITHUB_* and RUNNER_* vars by default", () => {
255+
mockNpmrc(
256+
[
257+
"tag=${GITHUB_REF}",
258+
"agent=${RUNNER_NAME}",
259+
"//npm.pkg.github.com/:_authToken=${NODE_AUTH_TOKEN}",
260+
].join("\n"),
261+
);
262+
vi.stubEnv("GITHUB_REF", "refs/heads/main");
263+
vi.stubEnv("RUNNER_NAME", "runner-1");
264+
vi.stubEnv("NODE_AUTH_TOKEN", "tok");
265+
266+
propagateProjectNpmrcAuth(projectDir);
267+
268+
expect(exportVariable).not.toHaveBeenCalledWith("GITHUB_REF", expect.anything());
269+
expect(exportVariable).not.toHaveBeenCalledWith("RUNNER_NAME", expect.anything());
270+
expect(exportVariable).toHaveBeenCalledWith("NODE_AUTH_TOKEN", "tok");
271+
});
272+
273+
it("allows GITHUB_TOKEN through as an auth token", () => {
274+
mockNpmrc("//npm.pkg.github.com/:_authToken=${GITHUB_TOKEN}");
275+
vi.stubEnv("GITHUB_TOKEN", "gh-token");
276+
277+
propagateProjectNpmrcAuth(projectDir);
278+
279+
expect(exportVariable).toHaveBeenCalledWith("GITHUB_TOKEN", "gh-token");
280+
});
281+
254282
it("exports all referenced auth-like env vars, deduping repeats", () => {
255283
mockNpmrc(
256284
[
@@ -384,6 +412,17 @@ describe("propagateProjectNpmrcAuth", () => {
384412
expect(exportVariable).toHaveBeenCalledWith("NPM_CONFIG_USERCONFIG", supplementalPath);
385413
});
386414

415+
it("skips registries whose value contains ${VAR} (cannot synthesize a valid auth key)", () => {
416+
mockNpmrc("@myorg:registry=${CUSTOM_REGISTRY}");
417+
vi.stubEnv("NODE_AUTH_TOKEN", "tok");
418+
vi.stubEnv("CUSTOM_REGISTRY", "https://npm.example.com");
419+
420+
propagateProjectNpmrcAuth(projectDir);
421+
422+
expect(writeFileSync).not.toHaveBeenCalled();
423+
expect(exportVariable).toHaveBeenCalledWith("CUSTOM_REGISTRY", "https://npm.example.com");
424+
});
425+
387426
it("treats _authToken key case-insensitively when checking project .npmrc", () => {
388427
mockNpmrc(
389428
[

src/auth.ts

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -85,21 +85,18 @@ function writeRegistryToFile(registryUrl: string, fileLocation: string, scope?:
8585
exportVariable("NODE_AUTH_TOKEN", process.env.NODE_AUTH_TOKEN || "XXXXX-XXXXX-XXXXX-XXXXX");
8686
}
8787

88-
// Env vars the runner/system manages — never re-export via GITHUB_ENV
89-
const RESERVED_ENV_VARS = new Set([
90-
"PATH",
91-
"HOME",
92-
"USERPROFILE",
93-
"TMPDIR",
94-
"RUNNER_TEMP",
95-
"RUNNER_OS",
96-
"RUNNER_ARCH",
97-
"GITHUB_ACTIONS",
98-
"GITHUB_WORKSPACE",
99-
"GITHUB_REPOSITORY",
100-
"GITHUB_REPOSITORY_OWNER",
101-
"CI",
102-
]);
88+
// GitHub-Actions-managed namespaces: re-exporting any of these via GITHUB_ENV
89+
// could clobber runner-provided values for subsequent steps. Block the whole
90+
// prefix by default; allow only vars that are legitimately passed as auth tokens.
91+
const RUNTIME_ENV_ALLOWLIST = new Set(["GITHUB_TOKEN"]);
92+
const ALWAYS_RESERVED = new Set(["PATH", "HOME", "USERPROFILE", "TMPDIR", "CI"]);
93+
94+
function isReservedEnvVar(name: string): boolean {
95+
if (ALWAYS_RESERVED.has(name)) return true;
96+
if (name.startsWith("RUNNER_")) return true;
97+
if (name.startsWith("GITHUB_")) return !RUNTIME_ENV_ALLOWLIST.has(name);
98+
return false;
99+
}
103100

104101
function analyzeProjectNpmrc(content: string): {
105102
registriesNeedingAuth: string[];
@@ -119,7 +116,12 @@ function analyzeProjectNpmrc(content: string): {
119116
const value = line.slice(eq + 1).trim();
120117

121118
if (lowerKey === "registry" || lowerKey.endsWith(":registry")) {
122-
registries.add(value.endsWith("/") ? value : value + "/");
119+
// Skip values that rely on env-var expansion — the key for the matching
120+
// `_authToken` line must be a literal URL, and `${VAR}` isn't expanded
121+
// inside `.npmrc` keys by npm/pnpm.
122+
if (!value.includes("${")) {
123+
registries.add(value.endsWith("/") ? value : value + "/");
124+
}
123125
}
124126
if (lowerKey.startsWith("//") && lowerKey.endsWith(":_authtoken")) {
125127
authKeys.add(lowerKey);
@@ -185,7 +187,7 @@ export function propagateProjectNpmrcAuth(projectDir: string): void {
185187
}
186188

187189
const propagatable = [...envVarRefs].filter(
188-
(name) => !RESERVED_ENV_VARS.has(name) && !!process.env[name],
190+
(name) => !isReservedEnvVar(name) && !!process.env[name],
189191
);
190192

191193
if (propagatable.length === 0) {

0 commit comments

Comments
 (0)