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
2 changes: 1 addition & 1 deletion packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,4 @@ export {
type VersionPackageChangelog,
type VersionPackageUpdate,
} from './types.js';
export { sanitizePackageName } from './utils.js';
export { assertNotOption, sanitizePackageName } from './utils.js';
14 changes: 14 additions & 0 deletions packages/core/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 '-')`);
}
}
21 changes: 20 additions & 1 deletion packages/core/test/unit/utils.spec.ts
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand All @@ -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();
});
});
});
21 changes: 20 additions & 1 deletion packages/publish/src/stages/github-release.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -208,6 +208,25 @@ export async function runGithubReleaseStage(ctx: PipelineContext): Promise<void>
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.
Expand Down
4 changes: 3 additions & 1 deletion packages/publish/src/utils/npm-env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: () => {
Expand Down
24 changes: 24 additions & 0 deletions packages/publish/test/unit/stages/github-release.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
12 changes: 12 additions & 0 deletions packages/publish/test/unit/utils/npm-env.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
5 changes: 4 additions & 1 deletion packages/release/src/backfill/github-release.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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' });
}

Expand All @@ -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 };
Expand Down
22 changes: 22 additions & 0 deletions packages/release/test/unit/backfill/github-release.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand Down
Loading