From fb2811cdfdae2c7c224cbf1b3f88475a6e7f4394 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sun, 19 Jul 2026 22:32:10 +0100 Subject: [PATCH] fix(publish): harden gh calls against arg injection and lock down temp .npmrc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a leading-dash guard (assertNotOption in @releasekit/core, mirroring git's barrier) on tags passed to gh release view/create/edit, so a tag starting with '-' can never be parsed as a flag (gh exposes -R/--repo, -F/--notes-file). The publish pipeline creates the tag via git first (already guarded), but the notes-backfill path feeds gh tags enumerated from existing repo refs, so an out-of-band refs/tags/-… reached gh unguarded. Also write the token-bearing temp .npmrc with mode 0600, for parity with config's saveAuth. Closes #561 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01BT6fVBZrmExXZj7vQ29881 --- packages/core/src/index.ts | 2 +- packages/core/src/utils.ts | 14 +++++++++++ packages/core/test/unit/utils.spec.ts | 21 +++++++++++++++- packages/publish/src/stages/github-release.ts | 21 +++++++++++++++- packages/publish/src/utils/npm-env.ts | 4 +++- .../test/unit/stages/github-release.spec.ts | 24 +++++++++++++++++++ .../publish/test/unit/utils/npm-env.spec.ts | 12 ++++++++++ .../release/src/backfill/github-release.ts | 5 +++- .../test/unit/backfill/github-release.spec.ts | 22 +++++++++++++++++ 9 files changed, 120 insertions(+), 5 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e41e7970..25b19bf2 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -61,4 +61,4 @@ export { type VersionPackageChangelog, type VersionPackageUpdate, } from './types.js'; -export { sanitizePackageName } from './utils.js'; +export { assertNotOption, sanitizePackageName } from './utils.js'; diff --git a/packages/core/src/utils.ts b/packages/core/src/utils.ts index 1784ee26..d6a58ac8 100644 --- a/packages/core/src/utils.ts +++ b/packages/core/src/utils.ts @@ -5,3 +5,17 @@ export function sanitizePackageName(name: string): string { return name.startsWith('@') ? name.slice(1).replace(/\//g, '-') : name; } + +/** + * Reject a value a getopt/cobra-style CLI (git, gh) would parse as an option flag instead of a + * positional. `execFile`/`execFileSync` spawn with no shell, so there's no shell injection, but a + * leading-`-` positional still becomes argument injection: `gh release` exposes `-R/--repo` and + * `-F/--notes-file`, so a tag of `--repo=evil` or `-F/etc/passwd` would be read as a flag. Tag, ref, + * and package names can never legitimately start with `-`, so we refuse them before the value reaches + * the argv. Mirrors git's own assertNotOption barrier for the gh call sites (publish + notes-backfill). + */ +export function assertNotOption(value: string, kind: string): void { + if (value.startsWith('-')) { + throw new Error(`Refusing to run: ${kind} '${value}' looks like an option (starts with '-')`); + } +} diff --git a/packages/core/test/unit/utils.spec.ts b/packages/core/test/unit/utils.spec.ts index fd8ccdb0..08737e2d 100644 --- a/packages/core/test/unit/utils.spec.ts +++ b/packages/core/test/unit/utils.spec.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { sanitizePackageName } from '../../src/utils.js'; +import { assertNotOption, sanitizePackageName } from '../../src/utils.js'; describe('utils', () => { describe('sanitizePackageName', () => { @@ -22,4 +22,23 @@ describe('utils', () => { expect(sanitizePackageName('@scope/pkg_name')).toBe('scope-pkg_name'); }); }); + + describe('assertNotOption', () => { + it('should throw for a value starting with a dash', () => { + expect(() => assertNotOption('-rf', 'tag')).toThrow(/looks like an option/); + expect(() => assertNotOption('--repo=evil', 'tag')).toThrow(/tag '--repo=evil'/); + }); + + it('should include the kind in the error message', () => { + expect(() => assertNotOption('-x', 'ref')).toThrow(/ref '-x'/); + }); + + it('should not throw for values that do not start with a dash', () => { + expect(() => assertNotOption('v1.0.0', 'tag')).not.toThrow(); + expect(() => assertNotOption('@scope/pkg@v1.0.0', 'tag')).not.toThrow(); + expect(() => assertNotOption('', 'tag')).not.toThrow(); + // An interior dash is fine — only a leading dash is parsed as a flag. + expect(() => assertNotOption('release-v1.0.0', 'tag')).not.toThrow(); + }); + }); }); diff --git a/packages/publish/src/stages/github-release.ts b/packages/publish/src/stages/github-release.ts index 77a04e2d..04e13f89 100644 --- a/packages/publish/src/stages/github-release.ts +++ b/packages/publish/src/stages/github-release.ts @@ -1,5 +1,5 @@ import type { VersionPackageChangelog } from '@releasekit/core'; -import { debug, info, sanitizePackageName, success, warn } from '@releasekit/core'; +import { assertNotOption, debug, info, sanitizePackageName, success, warn } from '@releasekit/core'; import type { GitHubReleaseResult, PipelineContext } from '../types.js'; import { execCommand, execCommandSafe } from '../utils/exec.js'; import { isPrerelease } from '../utils/semver.js'; @@ -208,6 +208,25 @@ export async function runGithubReleaseStage(ctx: PipelineContext): Promise const skipPackages = config.githubRelease.skipPackages ?? []; for (const tag of tagsToRelease) { + // Argument-injection guard (defense-in-depth): the git stage creates the tag first and already + // rejects a leading `-`, but reject here too so a `-`-prefixed tag can never reach the `gh` argv + // (cobra would parse it as a flag, e.g. -R/--repo). Non-critical per the per-tag error strategy — + // record the failure and move on rather than aborting the whole stage. + try { + assertNotOption(tag, 'tag'); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + warn(`Failed to create GitHub release for ${tag}: ${reason}`); + ctx.output.githubReleases.push({ + tag, + draft: config.githubRelease.draft, + prerelease: false, + success: false, + reason, + }); + continue; + } + // Skip GitHub release creation when the package is listed in githubRelease.skipPackages. // Tags are still created (needed for changelog range detection on the next release) and // npm publish still runs — only the `gh release create` call is skipped. diff --git a/packages/publish/src/utils/npm-env.ts b/packages/publish/src/utils/npm-env.ts index 3009d4ae..d63189a1 100644 --- a/packages/publish/src/utils/npm-env.ts +++ b/packages/publish/src/utils/npm-env.ts @@ -13,7 +13,9 @@ export interface NpmEnvIsolation { function writeTempNpmrc(contents: string): { npmrcPath: string; cleanup: () => void } { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'releasekit-npmrc-')); const npmrcPath = path.join(dir, '.npmrc'); - fs.writeFileSync(npmrcPath, contents, 'utf-8'); + // The temp `.npmrc` carries an auth token; write it owner-only (parity with config's saveAuth) so + // it isn't left readable even if the enclosing 0700 mkdtemp dir's protection is ever weakened. + fs.writeFileSync(npmrcPath, contents, { encoding: 'utf-8', mode: 0o600 }); return { npmrcPath, cleanup: () => { diff --git a/packages/publish/test/unit/stages/github-release.spec.ts b/packages/publish/test/unit/stages/github-release.spec.ts index aa089ba0..6ed60035 100644 --- a/packages/publish/test/unit/stages/github-release.spec.ts +++ b/packages/publish/test/unit/stages/github-release.spec.ts @@ -567,6 +567,30 @@ describe('github-release stage', () => { expect(args[titleIndex + 1]).toBe('my-package: v1.0.0'); }); + it('should reject a tag beginning with a dash instead of passing it to gh', async () => { + const { execCommand, execCommandSafe } = await import('../../../src/utils/exec.js'); + const ctx = createContext({ + input: { dryRun: false, updates: [], changelogs: [], tags: ['--repo=evil'] }, + output: { + dryRun: false, + git: { committed: true, tags: ['--repo=evil'], pushed: true }, + npm: [], + cargo: [], + verification: [], + githubReleases: [], + }, + }); + + await runGithubReleaseStage(ctx); + + // The `-`-prefixed tag never reaches either gh invocation. + expect(execCommand).not.toHaveBeenCalled(); + expect(execCommandSafe).not.toHaveBeenCalled(); + expect(ctx.output.githubReleases).toHaveLength(1); + expect(ctx.output.githubReleases[0]?.success).toBe(false); + expect(ctx.output.githubReleases[0]?.reason).toMatch(/looks like an option/); + }); + describe('githubRelease.skipPackages config', () => { it('should skip GitHub release creation for packages listed in skipPackages', async () => { const { execCommand } = await import('../../../src/utils/exec.js'); diff --git a/packages/publish/test/unit/utils/npm-env.spec.ts b/packages/publish/test/unit/utils/npm-env.spec.ts index 53b0d538..6057069e 100644 --- a/packages/publish/test/unit/utils/npm-env.spec.ts +++ b/packages/publish/test/unit/utils/npm-env.spec.ts @@ -62,4 +62,16 @@ describe('npm env isolation', () => { iso.cleanup(); }); + + it('should write the token-bearing temp .npmrc with 0600 mode', () => { + process.env.NPM_TOKEN = 'npm_test_token'; + + const iso = createNpmSubprocessIsolation({ authMethod: 'token', registryUrl: 'https://registry.npmjs.org' }); + + const npmrcPath = iso.env.NPM_CONFIG_USERCONFIG as string; + createdPaths.push(npmrcPath); + expect(fs.statSync(npmrcPath).mode & 0o777).toBe(0o600); + + iso.cleanup(); + }); }); diff --git a/packages/release/src/backfill/github-release.ts b/packages/release/src/backfill/github-release.ts index 5ab40e9b..a7612086 100644 --- a/packages/release/src/backfill/github-release.ts +++ b/packages/release/src/backfill/github-release.ts @@ -1,5 +1,5 @@ import { execFileSync } from 'node:child_process'; -import { NOTES_MARKER } from '@releasekit/core'; +import { assertNotOption, NOTES_MARKER } from '@releasekit/core'; /** * Marker that identifies a GitHub release body releasekit authored. Embedded at the top of every @@ -48,6 +48,7 @@ export function withMarker(body: string): string { * or any other failure) throws so the caller surfaces it instead of silently skipping the work. */ export function getReleaseBody(tag: string): string | null { + assertNotOption(tag, 'tag'); try { return execFileSync('gh', ['release', 'view', tag, '--json', 'body', '--jq', '.body'], { encoding: 'utf8' }); } catch (err) { @@ -63,6 +64,7 @@ export function getReleaseBody(tag: string): string | null { /** Overwrite a GitHub release body via `gh release edit`. */ export function editReleaseBody(tag: string, body: string): void { + assertNotOption(tag, 'tag'); execFileSync('gh', ['release', 'edit', tag, '--notes', body], { encoding: 'utf8' }); } @@ -71,6 +73,7 @@ export function editReleaseBody(tag: string, body: string): void { * Throws on auth/CLI/network failures. */ export function getReleaseInfo(tag: string): { isDraft: boolean; body: string | null } | null { + assertNotOption(tag, 'tag'); try { const json = execFileSync('gh', ['release', 'view', tag, '--json', 'isDraft,body'], { encoding: 'utf8' }); const parsed = JSON.parse(json) as { isDraft: boolean; body: string | null }; diff --git a/packages/release/test/unit/backfill/github-release.spec.ts b/packages/release/test/unit/backfill/github-release.spec.ts index 16cfa1e2..528e5d24 100644 --- a/packages/release/test/unit/backfill/github-release.spec.ts +++ b/packages/release/test/unit/backfill/github-release.spec.ts @@ -2,6 +2,7 @@ import { execFileSync } from 'node:child_process'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { decideReleaseUpdate, + editReleaseBody, getReleaseBody, getReleaseInfo, NOTES_MARKER, @@ -99,6 +100,27 @@ describe('getReleaseBody', () => { }); }); +describe('gh argument-injection guard', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should reject a tag beginning with a dash before reaching gh', () => { + // Backfill enumerates tags from existing repo refs, so an out-of-band `refs/tags/-…` can reach + // gh unguarded — cobra would parse the leading-`-` positional as a flag. Reject before spawn. + expect(() => getReleaseBody('--repo=evil')).toThrow(/looks like an option/); + expect(() => getReleaseInfo('-F/etc/passwd')).toThrow(/looks like an option/); + expect(() => editReleaseBody('-x', 'body')).toThrow(/looks like an option/); + expect(execFileSync).not.toHaveBeenCalled(); + }); + + it('should still call gh for a normal tag', () => { + vi.mocked(execFileSync).mockReturnValue('## Notes\n'); + getReleaseBody('v1.0.0'); + expect(execFileSync).toHaveBeenCalledTimes(1); + }); +}); + describe('getReleaseInfo', () => { beforeEach(() => { vi.clearAllMocks();