From f028c3c71118918a4dc06bf146ed7e71f6323676 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 21 Apr 2026 15:21:58 +0000 Subject: [PATCH 1/2] security: disable .craft.env reading and harden release subprocess env Addresses the LD_PRELOAD RCE vector demonstrated in getsentry/action-release#315, where a PR to a Craft-using repo could plant a .craft.env file with LD_PRELOAD=./preload.so plus a malicious shared library and gain arbitrary code execution in the release pipeline with access to all CI secrets. Primary fix: - Remove .craft.env file reading entirely. process.env is no longer hydrated from $HOME/.craft.env or /.craft.env. Warn at startup if a legacy file is detected, pointing users at the shell / CI environment. The nvar dependency and src/types/nvar.ts shim are dropped. Defence-in-depth changes, in case a future regression (or an earlier CI step) plants dangerous env vars: - Strip dynamic-linker env vars (LD_PRELOAD, LD_LIBRARY_PATH, LD_AUDIT, DYLD_INSERT_LIBRARIES, DYLD_LIBRARY_PATH, DYLD_FRAMEWORK_PATH, DYLD_FALLBACK_LIBRARY_PATH, DYLD_FALLBACK_FRAMEWORK_PATH) from process.env at startup. Emergency opt-out via CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1. - preReleaseCommand subprocesses now receive an allowlisted env (PATH, GITHUB_TOKEN, HOME, USER, GIT_COMMITTER_NAME, GIT_AUTHOR_NAME, EMAIL, plus CRAFT_NEW_VERSION/CRAFT_OLD_VERSION) instead of the full process.env. Matches the existing postReleaseCommand behaviour; the allowlist is now shared via src/utils/releaseCommandEnv.ts. - --config-from now requires --allow-remote-config (or CRAFT_ALLOW_REMOTE_CONFIG=1) to opt in. Remote .craft.yml is untrusted input and can execute arbitrary commands via preReleaseCommand. --- AGENTS.md | 31 ++-- docs/src/content/docs/getting-started.md | 16 +- package.json | 1 - pnpm-lock.yaml | 9 - src/commands/__tests__/prepare.test.ts | 117 +++++++++++-- src/commands/__tests__/publish.test.ts | 40 +++++ src/commands/prepare.ts | 63 ++++++- src/commands/publish.ts | 24 +-- src/index.ts | 11 +- src/types/nvar.ts | 1 - src/utils/__tests__/env.test.ts | 199 ++++++++++++++++------- src/utils/env.ts | 158 ++++++++++-------- src/utils/releaseCommandEnv.ts | 63 +++++++ 13 files changed, 516 insertions(+), 217 deletions(-) delete mode 100644 src/types/nvar.ts create mode 100644 src/utils/releaseCommandEnv.ts diff --git a/AGENTS.md b/AGENTS.md index cdda1af2c..6a593cec3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,36 +113,45 @@ Some operations need explicit `isDryRun()` checks: - User experience optimizations (e.g., skipping sleep timers) - ## Long-term Knowledge ### Architecture - + +* **Craft release commands forward sanitized env, not full process.env**: Craft's pre/postReleaseCommand invocations (\`prepare.ts\` runCustomPreReleaseCommand, \`publish.ts\` runPostReleaseCommand) must NOT forward \`...process.env\` to subprocesses — that pattern lets attacker-controlled \`.craft.env\` or config inject \`LD\_PRELOAD\`/\`LD\_LIBRARY\_PATH\` for RCE via the CI release pipeline. Use the shared allowlist helper in \`src/utils/releaseCommandEnv.ts\` which returns only {PATH, HOME, USER, GIT\_COMMITTER\_NAME, GIT\_AUTHOR\_NAME, EMAIL, GITHUB\_TOKEN, CRAFT\_\*} plus per-command additions (CRAFT\_NEW\_VERSION/OLD\_VERSION for prepare, CRAFT\_RELEASED\_VERSION for publish). Tests in \`prepare.test.ts\`/\`publish.test.ts\` assert LD\_PRELOAD and secret env vars are stripped. -- **Registry target: repo_url auto-derived from git remote, not user-configurable**: \`repo_url\` in registry manifests is always set by Craft as \`https://github.com/${owner}/${repo}\`. Resolution: (1) explicit \`github: { owner, repo }\` in \`.craft.yml\` (rare), (2) fallback: auto-detect from git \`origin\` remote URL via \`git-url-parse\` library (\`git.ts:194-217\`, \`config.ts:286-316\`). Works with HTTPS and SSH remote URLs. Always overwritten on every publish — existing manifest values are replaced (\`registry.ts:417-418\`). Result is cached globally with \`Object.freeze\`. If remote isn't \`github.com\` and no explicit config exists, throws \`ConfigurationError\`. Most repos need no configuration — the git origin remote is sufficient. + +* **Registry target: repo\_url auto-derived from git remote, not user-configurable**: \`repo\_url\` in registry manifests is always set by Craft as \`https://github.com/${owner}/${repo}\`. Resolution: (1) explicit \`github: { owner, repo }\` in \`.craft.yml\` (rare), (2) fallback: auto-detect from git \`origin\` remote URL via \`git-url-parse\` library (\`git.ts:194-217\`, \`config.ts:286-316\`). Works with HTTPS and SSH remote URLs. Always overwritten on every publish — existing manifest values are replaced (\`registry.ts:417-418\`). Result is cached globally with \`Object.freeze\`. If remote isn't \`github.com\` and no explicit config exists, throws \`ConfigurationError\`. Most repos need no configuration — the git origin remote is sufficient. +* **Registry target: urlTemplate generates artifact download URLs in manifest**: \`urlTemplate\` in the registry target config generates download URLs for release artifacts in the registry manifest's \`files\` field. Uses Mustache rendering with variables \`{{version}}\`, \`{{file}}\`, \`{{revision}}\`. Primarily useful for apps (standalone binaries) and CDN-hosted assets — SDK packages published to public registries (npm, PyPI, gem) typically don't need it. If neither \`urlTemplate\` nor \`checksums\` is configured, Craft skips adding file data entirely (warns at \`registry.ts:341-349\`). Real-world pattern: \`https://downloads.sentry-cdn.com/\/{{version}}/{{file}}\`. -- **Registry target: urlTemplate generates artifact download URLs in manifest**: \`urlTemplate\` in the registry target config generates download URLs for release artifacts in the registry manifest's \`files\` field. Uses Mustache rendering with variables \`{{version}}\`, \`{{file}}\`, \`{{revision}}\`. Primarily useful for apps (standalone binaries) and CDN-hosted assets — SDK packages published to public registries (npm, PyPI, gem) typically don't need it. If neither \`urlTemplate\` nor \`checksums\` is configured, Craft skips adding file data entirely (warns at \`registry.ts:341-349\`). Real-world pattern: \`https://downloads.sentry-cdn.com/\/{{version}}/{{file}}\`. +### Decision + + +* **--config-from gated behind --allow-remote-config**: Craft's \`prepare --config-from \\` fetches \`.craft.yml\` from a remote git ref and feeds it to \`loadConfigurationFromString\`, which can execute arbitrary commands via preReleaseCommand. Now gated: \`assertRemoteConfigAllowed()\` in \`src/commands/prepare.ts\` throws ConfigurationError unless \`--allow-remote-config\` (or \`CRAFT\_ALLOW\_REMOTE\_CONFIG=1\`) is set. When opted in, a warning is logged naming the branch. Exported helper is unit-tested; full \`prepareMain\` is not (heavy mock surface). ### Gotcha - + +* **Craft .craft.env file reading removed — security hazard via LD\_PRELOAD**: Craft used to hydrate \`process.env\` from \`$HOME/.craft.env\` and \`\/.craft.env\` via \`nvar\`. Removed because an attacker PR could add \`.craft.env\` with \`LD\_PRELOAD=./preload.so\` + a malicious shared library, giving RCE in the release pipeline with access to all secrets (demo: getsentry/action-release#315). \`src/utils/env.ts\` now only exports \`warnIfCraftEnvFileExists()\` (startup warning, no file read, no env mutation) and \`checkEnvForPrerequisite\` (unchanged). \`nvar\` dep and \`src/types/nvar.ts\` were removed. Consumers must set env vars via shell/CI. -- **Craft postReleaseCommand env vars pollute shared bump-version scripts**: Craft's \`runPostReleaseCommand\` sets \`CRAFT_NEW_VERSION=\\` in the subprocess env. If a post-release script calls a shared \`bump-version.sh\` that reads \`NEW_VERSION="${CRAFT\_NEW\_VERSION:-${2:-}}"\`, the env var takes precedence over the positional arg (e.g. \`nightly\`), causing the script to set the version to the already-current release version → no diff → no commit → master stays on the release version. Fixed by replacing \`CRAFT_NEW_VERSION\`/\`CRAFT_OLD_VERSION\` with \`CRAFT_RELEASED_VERSION\` in the post-release env (\`publish.ts:563-564\`). The pre-release command (\`prepare.ts\`) still correctly uses \`CRAFT_NEW_VERSION\`. Consuming repos don't need changes unless they explicitly read \`CRAFT_NEW_VERSION\` in their post-release scripts. + +* **Craft postReleaseCommand env vars pollute shared bump-version scripts**: Craft's \`runPostReleaseCommand\` sets \`CRAFT\_NEW\_VERSION=\\` in the subprocess env. If a post-release script calls a shared \`bump-version.sh\` that reads \`NEW\_VERSION="${CRAFT\_NEW\_VERSION:-${2:-}}"\`, the env var takes precedence over the positional arg (e.g. \`nightly\`), causing the script to set the version to the already-current release version → no diff → no commit → master stays on the release version. Fixed by replacing \`CRAFT\_NEW\_VERSION\`/\`CRAFT\_OLD\_VERSION\` with \`CRAFT\_RELEASED\_VERSION\` in the post-release env (\`publish.ts:563-564\`). The pre-release command (\`prepare.ts\`) still correctly uses \`CRAFT\_NEW\_VERSION\`. Consuming repos don't need changes unless they explicitly read \`CRAFT\_NEW\_VERSION\` in their post-release scripts. - -- **ESM modules prevent vi.spyOn of child_process.spawnSync — use test subclass pattern**: In ESM (Vitest or Bun), you cannot \`vi.spyOn\` exports from Node built-in modules — throws 'Module namespace is not configurable'. Workaround: create a test subclass that overrides the method calling the built-in and injects controllable values. \`vi.mock\` at module level works but affects all tests in the file. +* **ESM modules prevent vi.spyOn of child\_process.spawnSync — use test subclass pattern**: In ESM (Vitest or Bun), you cannot \`vi.spyOn\` exports from Node built-in modules — throws 'Module namespace is not configurable'. Workaround: create a test subclass that overrides the method calling the built-in and injects controllable values. \`vi.mock\` at module level works but affects all tests in the file. - -- **pnpm overrides with >= can cross major versions — use ^ to constrain**: pnpm overrides gotchas: (1) \`>=\` crosses major versions — use \`^\` to constrain within same major. (2) Version-range selectors don't reliably force re-resolution of compatible transitive deps; use blanket overrides when safe. (3) Overrides become stale — audit with \`pnpm why \\` after dependency changes. (4) Never manually resolve pnpm-lock.yaml conflicts — \`git checkout --theirs\` then \`pnpm install\` to regenerate deterministically. +* **pnpm overrides with >= can cross major versions — use ^ to constrain**: pnpm overrides gotchas: (1) \`>=\` crosses major versions — use \`^\` to constrain within same major. (2) Version-range selectors don't reliably force re-resolution of compatible transitive deps; use blanket overrides when safe. (3) Overrides become stale — audit with \`pnpm why \\` after dependency changes. (4) Never manually resolve pnpm-lock.yaml conflicts — \`git checkout --theirs\` then \`pnpm install\` to regenerate deterministically. ### Pattern +* **Craft/Publish release flow: prepare then accept publish issue**: Craft's release flow is two-phase: (1) Run \`release.yml\` GitHub Action with version "auto" — this runs \`craft prepare\`, auto-determines version from commits, creates the \`release/X.Y.Z\` branch, and opens a publish issue on \`getsentry/publish\` repo (e.g. \`publish: getsentry/craft@2.25.3\`). (2) Add the \`accepted\` label to that publish issue to trigger the actual publish pipeline. Do NOT manually create release branches — always use the workflow. The publish issue URL is emitted in the release job logs as a \`::notice::Created publish request:\` line. The publish repo is configured via \`PUBLISH\_REPO\` (defaults to \`getsentry/publish\`). + +### Preference -- **Craft/Publish release flow: prepare then accept publish issue**: Craft's release flow is two-phase: (1) Run \`release.yml\` GitHub Action with version "auto" — this runs \`craft prepare\`, auto-determines version from commits, creates the \`release/X.Y.Z\` branch, and opens a publish issue on \`getsentry/publish\` repo (e.g. \`publish: getsentry/craft@2.25.3\`). (2) Add the \`accepted\` label to that publish issue to trigger the actual publish pipeline. Do NOT manually create release branches — always use the workflow. The publish issue URL is emitted in the release job logs as a \`::notice::Created publish request:\` line. The publish repo is configured via \`PUBLISH_REPO\` (defaults to \`getsentry/publish\`). + +* **CHANGELOG.md is auto-managed — do not edit manually**: Craft's CHANGELOG.md is auto-generated from PR descriptions by the release pipeline. Do NOT add entries manually, even for breaking changes. The user will reject such edits. Describe breaking changes in the PR body instead; the auto-managed process surfaces them in the changelog. diff --git a/docs/src/content/docs/getting-started.md b/docs/src/content/docs/getting-started.md index a5d5ed74f..463d90c30 100644 --- a/docs/src/content/docs/getting-started.md +++ b/docs/src/content/docs/getting-started.md @@ -242,21 +242,9 @@ Dry-run still requires `GITHUB_TOKEN` for commands that fetch PR information fro Since Craft relies heavily on GitHub, set the `GITHUB_TOKEN` environment variable to a [GitHub Personal Access Token](https://github.com/settings/tokens) with `repo` scope. -### Environment Files +### Environment Variables -Craft reads configuration from these locations (in order of precedence): - -1. `$HOME/.craft.env` -2. `$PROJECT_DIR/.craft.env` -3. Shell environment - -Example `.craft.env`: - -```shell -# ~/.craft.env -GITHUB_TOKEN=token123 -export NUGET_API_TOKEN=abcdefgh -``` +Credentials and other configuration (`GITHUB_TOKEN`, `NPM_TOKEN`, etc.) must be provided directly via your shell or CI environment. Craft no longer reads `.craft.env` files — if one is found in your home directory or project directory, Craft will emit a warning at startup. Please migrate any values stored there to your shell or CI configuration. ## Caveats diff --git a/package.json b/package.json index 6a624cd76..5594990a8 100644 --- a/package.json +++ b/package.json @@ -55,7 +55,6 @@ "mustache": "3.0.1", "nock": "^13.2.4", "node-fetch": "^2.6.1", - "nvar": "1.3.1", "ora": "5.4.0", "prettier": "^3.4.2", "prompts": "2.4.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9befa895a..72bbd92d0 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -164,9 +164,6 @@ importers: node-fetch: specifier: ^2.6.1 version: 2.7.0 - nvar: - specifier: 1.3.1 - version: 1.3.1 ora: specifier: 5.4.0 version: 5.4.0 @@ -2676,10 +2673,6 @@ packages: resolution: {integrity: sha512-6eZs5Ls3WtCisHWp9S2GUy8dqkpGi4BVSz3GaqiE6ezub0512ESztXUwUB6C6IKbQkY2Pnb/mD4WYojCRwcwLA==} engines: {node: '>=0.10.0'} - nvar@1.3.1: - resolution: {integrity: sha512-2U58nVI2o0SXvjiTiQ6UdXpfNu0/A6rqvjCPk7bEsqQujXWDkmLdKppMkh3UXXAvfX9b2LSs1YYYkX2uNE/oAg==} - engines: {node: '>=0.10'} - once@1.4.0: resolution: {integrity: sha512-lNaJgI+2Q5URQBkccEKHTQOPaXdUxnZZElQTZY0MFUAuaEqe1E+Nyvgdz/aIyNi6Z9MzO5dv1H8n58/GELp3+w==} @@ -6045,8 +6038,6 @@ snapshots: normalize-path@3.0.0: {} - nvar@1.3.1: {} - once@1.4.0: dependencies: wrappy: 1.0.2 diff --git a/src/commands/__tests__/prepare.test.ts b/src/commands/__tests__/prepare.test.ts index cc7b24c2f..a6690823f 100644 --- a/src/commands/__tests__/prepare.test.ts +++ b/src/commands/__tests__/prepare.test.ts @@ -1,6 +1,11 @@ import { vi, describe, test, expect, beforeEach, type Mock } from 'vitest'; import { spawnProcess } from '../../utils/system'; -import { runPreReleaseCommand, checkVersionOrPart } from '../prepare'; +import { + runPreReleaseCommand, + checkVersionOrPart, + assertRemoteConfigAllowed, +} from '../prepare'; +import { ConfigurationError } from '../../utils/errors'; vi.mock('../../utils/system'); @@ -10,6 +15,18 @@ describe('runPreReleaseCommand', () => { const rootDir = process.cwd(); const mockedSpawnProcess = spawnProcess as Mock; + const expectedBaseEnv = () => ({ + PATH: process.env.PATH, + GITHUB_TOKEN: process.env.GITHUB_TOKEN, + HOME: process.env.HOME, + USER: process.env.USER, + GIT_COMMITTER_NAME: process.env.GIT_COMMITTER_NAME, + GIT_AUTHOR_NAME: process.env.GIT_AUTHOR_NAME, + EMAIL: process.env.EMAIL, + CRAFT_NEW_VERSION: newVersion, + CRAFT_OLD_VERSION: oldVersion, + }); + beforeEach(() => { vi.clearAllMocks(); }); @@ -27,13 +44,7 @@ describe('runPreReleaseCommand', () => { expect(mockedSpawnProcess).toBeCalledWith( 'scripts/bump-version.sh', [oldVersion, newVersion], - { - env: { - ...process.env, - CRAFT_NEW_VERSION: newVersion, - CRAFT_OLD_VERSION: oldVersion, - }, - }, + { env: expectedBaseEnv() }, ); }); @@ -50,15 +61,62 @@ describe('runPreReleaseCommand', () => { expect(mockedSpawnProcess).toBeCalledWith( 'python', ['./increase_version.py', 'argument 1', oldVersion, newVersion], - { - env: { - ...process.env, - CRAFT_NEW_VERSION: newVersion, - CRAFT_OLD_VERSION: oldVersion, - }, - }, + { env: expectedBaseEnv() }, ); }); + + test('does not forward arbitrary env vars from process.env', async () => { + // Simulate an attacker having planted secrets / linker hijacks in + // process.env (e.g. via a compromised parent process or a malicious + // earlier CI step). The allowlist must keep these out of the child. + const sentinel = '__SHOULD_NOT_LEAK__'; + const before = { + LD_PRELOAD: process.env.LD_PRELOAD, + AWS_SECRET_ACCESS_KEY: process.env.AWS_SECRET_ACCESS_KEY, + SECRET_TOKEN: process.env.SECRET_TOKEN, + }; + process.env.LD_PRELOAD = `/tmp/evil.so-${sentinel}`; + process.env.AWS_SECRET_ACCESS_KEY = `aws-${sentinel}`; + process.env.SECRET_TOKEN = `token-${sentinel}`; + + try { + await runPreReleaseCommand({ + oldVersion, + newVersion, + rootDir, + preReleaseCommand: 'scripts/bump-version.sh', + }); + + const spawnCall = mockedSpawnProcess.mock.calls[0]; + const envArg = spawnCall[2].env as Record; + + // Allowlist keys are present (values taken from process.env at call + // time, which is fine — they're allowlisted by name). + expect(envArg.CRAFT_NEW_VERSION).toBe(newVersion); + expect(envArg.CRAFT_OLD_VERSION).toBe(oldVersion); + + // Dangerous / attacker-planted vars must NOT appear. + expect(envArg.LD_PRELOAD).toBeUndefined(); + expect(envArg.AWS_SECRET_ACCESS_KEY).toBeUndefined(); + expect(envArg.SECRET_TOKEN).toBeUndefined(); + + // Defensive: the sentinel must not appear in ANY value. + for (const v of Object.values(envArg)) { + if (typeof v === 'string') { + expect(v).not.toContain(sentinel); + } + } + } finally { + // Restore prior env. + for (const [key, val] of Object.entries(before)) { + if (val === undefined) { + delete process.env[key]; + } else { + process.env[key] = val; + } + } + } + }); }); describe('checkVersionOrPart', () => { @@ -125,3 +183,32 @@ describe('checkVersionOrPart', () => { } }); }); + +describe('assertRemoteConfigAllowed', () => { + test('throws ConfigurationError when --allow-remote-config is not set', () => { + expect(() => + assertRemoteConfigAllowed('untrusted-branch', false), + ).toThrowError(ConfigurationError); + expect(() => + assertRemoteConfigAllowed('untrusted-branch', undefined), + ).toThrowError(ConfigurationError); + }); + + test('error message names the branch and the opt-in flag', () => { + try { + assertRemoteConfigAllowed('evil-branch', false); + throw new Error('expected throw'); + } catch (err) { + const message = (err as Error).message; + expect(message).toContain('evil-branch'); + expect(message).toContain('--allow-remote-config'); + expect(message).toContain('CRAFT_ALLOW_REMOTE_CONFIG'); + } + }); + + test('returns silently when opt-in is true', () => { + expect(() => + assertRemoteConfigAllowed('trusted-branch', true), + ).not.toThrow(); + }); +}); diff --git a/src/commands/__tests__/publish.test.ts b/src/commands/__tests__/publish.test.ts index 9bf6a16c5..6c461e9d0 100644 --- a/src/commands/__tests__/publish.test.ts +++ b/src/commands/__tests__/publish.test.ts @@ -86,6 +86,46 @@ describe('runPostReleaseCommand', () => { }, ); }); + + test('does not forward arbitrary env vars from process.env', async () => { + // Same threat model as runPreReleaseCommand: attacker-planted env vars + // must not leak to the post-release subprocess. + const sentinel = '__POST_RELEASE_SHOULD_NOT_LEAK__'; + const before = { + LD_PRELOAD: process.env.LD_PRELOAD, + AWS_SECRET_ACCESS_KEY: process.env.AWS_SECRET_ACCESS_KEY, + SECRET_TOKEN: process.env.SECRET_TOKEN, + }; + process.env.LD_PRELOAD = `/tmp/evil.so-${sentinel}`; + process.env.AWS_SECRET_ACCESS_KEY = `aws-${sentinel}`; + process.env.SECRET_TOKEN = `token-${sentinel}`; + + try { + await runPostReleaseCommand(newVersion, 'bash -c true'); + + const spawnCall = mockedSpawnProcess.mock.calls[0]; + const envArg = spawnCall[2].env as Record; + + expect(envArg.CRAFT_RELEASED_VERSION).toBe(newVersion); + expect(envArg.LD_PRELOAD).toBeUndefined(); + expect(envArg.AWS_SECRET_ACCESS_KEY).toBeUndefined(); + expect(envArg.SECRET_TOKEN).toBeUndefined(); + + for (const v of Object.values(envArg)) { + if (typeof v === 'string') { + expect(v).not.toContain(sentinel); + } + } + } finally { + for (const [key, val] of Object.entries(before)) { + if (val === undefined) { + delete process.env[key]; + } else { + process.env[key] = val; + } + } + } + }); }); describe('handleReleaseBranch', () => { diff --git a/src/commands/prepare.ts b/src/commands/prepare.ts index 8a8585841..50b4f488f 100644 --- a/src/commands/prepare.ts +++ b/src/commands/prepare.ts @@ -60,6 +60,7 @@ import { } from '../utils/helpers'; import { formatJson } from '../utils/strings'; import { spawnProcess } from '../utils/system'; +import { buildReleaseCommandEnv } from '../utils/releaseCommandEnv'; import { withTracing } from '../utils/tracing'; import { getVersion, isValidVersion } from '../utils/version'; import { runAutomaticVersionBumps } from '../utils/versionBump'; @@ -122,9 +123,19 @@ export const builder: CommandBuilder = (yargs: Argv) => }) .option('config-from', { description: - 'Load .craft.yml from the specified remote branch instead of local file', + 'Load .craft.yml from the specified remote branch instead of local file. ' + + 'Requires --allow-remote-config since the remote config can run arbitrary ' + + 'commands via preReleaseCommand.', type: 'string', }) + .option('allow-remote-config', { + description: + 'Opt in to loading .craft.yml from a remote branch via --config-from. ' + + 'Without this flag, --config-from is rejected because remotely-fetched ' + + 'config is untrusted input that can execute arbitrary commands.', + type: 'boolean', + default: false, + }) .option('calver-offset', { description: 'Days to go back for CalVer date calculation (overrides config)', @@ -150,6 +161,8 @@ interface PrepareOptions { publish: boolean; /** Load config from specified remote branch */ configFrom?: string; + /** Explicit opt-in required to use --config-from */ + allowRemoteConfig?: boolean; /** Override CalVer offset (days to go back) */ calverOffset?: number; } @@ -160,6 +173,33 @@ interface PrepareOptions { */ const SLEEP_BEFORE_PUBLISH_SECONDS = 30; +/** + * Throws a {@link ConfigurationError} if `--config-from` was provided + * without the explicit `--allow-remote-config` opt-in. + * + * Remote-fetched `.craft.yml` is untrusted input: a malicious or + * compromised branch can set `preReleaseCommand` to an arbitrary shell + * command that Craft will execute with the allowlisted release env + * (including `GITHUB_TOKEN`). Requiring an explicit opt-in makes this + * tradeoff visible in CI logs and prevents accidental use. + * + * @param configFrom The branch name passed to `--config-from`. + * @param allowRemoteConfig Whether the user explicitly opted in. + */ +export function assertRemoteConfigAllowed( + configFrom: string, + allowRemoteConfig: boolean | undefined, +): void { + if (!allowRemoteConfig) { + throw new ConfigurationError( + `--config-from loads ${CONFIG_FILE_NAME} from a remote git ref, which ` + + `can execute arbitrary commands via preReleaseCommand. Pass ` + + `--allow-remote-config (or set CRAFT_ALLOW_REMOTE_CONFIG=1) to ` + + `confirm you trust the remote branch "${configFrom}".`, + ); + } +} + /** * Checks the provided version argument for validity * @@ -410,13 +450,16 @@ async function runCustomPreReleaseCommand( args = [...args, nonEmptyOldVersion, newVersion]; logger.info('Running the pre-release command...'); - const additionalEnv = { - CRAFT_NEW_VERSION: newVersion, - CRAFT_OLD_VERSION: nonEmptyOldVersion, - }; - + // The pre-release command comes from .craft.yml, which is + // attacker-influenceable via untrusted PRs. We forward only an + // allowlisted env (PATH, GITHUB_TOKEN, HOME, etc.) plus the CRAFT_* + // version vars to avoid leaking secrets into a potentially malicious + // command. See src/utils/releaseCommandEnv.ts for rationale. await spawnProcess(sysCommand, args, { - env: { ...process.env, ...additionalEnv }, + env: buildReleaseCommandEnv({ + CRAFT_NEW_VERSION: newVersion, + CRAFT_OLD_VERSION: nonEmptyOldVersion, + }), }); return true; @@ -790,7 +833,11 @@ export async function prepareMain(argv: PrepareOptions): Promise { // Handle --config-from: load config from remote branch if (argv.configFrom) { - logger.info(`Loading configuration from remote branch: ${argv.configFrom}`); + assertRemoteConfigAllowed(argv.configFrom, argv.allowRemoteConfig); + logger.warn( + `Loading configuration from remote branch "${argv.configFrom}" (opted in via --allow-remote-config). ` + + `Ensure this branch is trusted — its .craft.yml will configure commands Craft will execute.`, + ); try { await git.fetch([argv.remote, argv.configFrom]); const configContent = await git.show([ diff --git a/src/commands/publish.ts b/src/commands/publish.ts index f3f880fd6..44ffc82f4 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -46,24 +46,11 @@ import { findReleaseBranches, } from '../utils/git'; import { withTracing } from '../utils/tracing'; +import { buildReleaseCommandEnv } from '../utils/releaseCommandEnv'; /** Default path to post-release script, relative to project root */ const DEFAULT_POST_RELEASE_SCRIPT_PATH = join('scripts', 'post-release.sh'); -/** - * Environment variables to pass through to the post-release command. - * HOME is needed so Git can find ~/.gitconfig with safe.directory settings, - * which fixes "fatal: detected dubious ownership in repository" errors. - * The git identity vars help with commit operations in post-release scripts. - */ -const ALLOWED_ENV_VARS = [ - 'HOME', - 'USER', - 'GIT_COMMITTER_NAME', - 'GIT_AUTHOR_NAME', - 'EMAIL', -] as const; - export const command = ['publish NEW-VERSION']; export const aliases = ['pp', 'publish']; export const description = '🛫 Publish artifacts'; @@ -560,14 +547,7 @@ export async function runPostReleaseCommand( args = [...args, '', newVersion]; logger.info(`Running the post-release command...`); await spawnProcess(sysCommand as string, args as string[], { - env: { - CRAFT_RELEASED_VERSION: newVersion, - PATH: process.env.PATH, - GITHUB_TOKEN: process.env.GITHUB_TOKEN, - ...Object.fromEntries( - ALLOWED_ENV_VARS.map(key => [key, process.env[key]]), - ), - }, + env: buildReleaseCommandEnv({ CRAFT_RELEASED_VERSION: newVersion }), }); return true; } diff --git a/src/index.ts b/src/index.ts index d3cfeefe1..9dfec11ca 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,7 +6,10 @@ import isCI from 'is-ci'; import yargs from 'yargs'; import { logger, LogLevel } from './logger'; -import { readEnvironmentConfig } from './utils/env'; +import { + sanitizeDynamicLinkerEnv, + warnIfCraftEnvFileExists, +} from './utils/env'; import { envToBool, setGlobals } from './utils/helpers'; import { getPackageVersion } from './utils/version'; import { withTracing } from './utils/tracing'; @@ -70,9 +73,13 @@ function fixGlobalBooleanFlags(argv: string[]): string[] { * Main entrypoint */ async function main(): Promise { + // Strip dynamic-linker env vars (LD_PRELOAD, DYLD_*, ...) before anything + // else so they cannot leak into subprocesses Craft spawns. + sanitizeDynamicLinkerEnv(); + printVersion(); - readEnvironmentConfig(); + warnIfCraftEnvFileExists(); const argv = fixGlobalBooleanFlags(process.argv.slice(2)); diff --git a/src/types/nvar.ts b/src/types/nvar.ts deleted file mode 100644 index f969eb0fe..000000000 --- a/src/types/nvar.ts +++ /dev/null @@ -1 +0,0 @@ -declare module 'nvar'; diff --git a/src/utils/__tests__/env.test.ts b/src/utils/__tests__/env.test.ts index e675e042a..2a3530b64 100644 --- a/src/utils/__tests__/env.test.ts +++ b/src/utils/__tests__/env.test.ts @@ -8,8 +8,8 @@ import os = require('os'); import * as config from '../../config'; import { checkEnvForPrerequisite, - readEnvironmentConfig, - ENV_FILE_NAME, + sanitizeDynamicLinkerEnv, + warnIfCraftEnvFileExists, } from '../env'; import { ConfigurationError } from '../errors'; import { logger } from '../../logger'; @@ -156,111 +156,184 @@ describe('env utils functions', () => { }); // end describe('multiple variables') }); // end describe('checkEnvForPrerequisites') - describe('readEnvironmentConfig', () => { + describe('warnIfCraftEnvFileExists', () => { const invalidDir = '/invalid/invalid'; + const LEGACY_FILE = '.craft.env'; - function writeConfigFileSync(directory: string): void { - const outConfigFile = join(directory, config.CONFIG_FILE_NAME); - writeFileSync(outConfigFile, ''); - } - - test('calls homedir/findConfigFile', () => { - process.env.TEST_BLA = '123'; - + test('does not warn when no files exist', () => { homedirMock.mockReturnValue(invalidDir); getConfigFileDirMock.mockReturnValue(invalidDir); - readEnvironmentConfig(); + warnIfCraftEnvFileExists(); - expect(getConfigFileDirMock).toHaveBeenCalledTimes(1); - expect(homedirMock).toHaveBeenCalledTimes(1); - expect(process.env.TEST_BLA).toBe('123'); - expect(ENV_FILE_NAME.length).toBeGreaterThanOrEqual(1); + expect(logger.warn).not.toHaveBeenCalled(); }); - test('checks the config directory', async () => { + test('does not warn when getConfigFileDir returns undefined', () => { homedirMock.mockReturnValue(invalidDir); + getConfigFileDirMock.mockReturnValue(undefined); - await withTempDir(directory => { - getConfigFileDirMock.mockReturnValue(directory); - - writeConfigFileSync(directory); + warnIfCraftEnvFileExists(); - const outFile = join(directory, ENV_FILE_NAME); - writeFileSync(outFile, 'export TEST_BLA=234\nexport TEST_ANOTHER=345'); - - readEnvironmentConfig(); - - expect(process.env.TEST_BLA).toBe('234'); - }); + expect(logger.warn).not.toHaveBeenCalled(); }); - test('checks home directory', async () => { + + test('warns when a legacy file exists in the home directory', async () => { getConfigFileDirMock.mockReturnValue(invalidDir); await withTempDir(directory => { homedirMock.mockReturnValue(directory); - const outFile = join(directory, ENV_FILE_NAME); + const outFile = join(directory, LEGACY_FILE); writeFileSync(outFile, 'export TEST_BLA=234\n'); - readEnvironmentConfig(); + warnIfCraftEnvFileExists(); - expect(process.env.TEST_BLA).toBe('234'); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining(outFile), + ); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('no longer reads this file'), + ); }); }); - test('checks home directory first, and then the config directory', async () => { - await withTempDir(async dir1 => { - await withTempDir(dir2 => { - homedirMock.mockReturnValue(dir1); + test('warns when a legacy file exists in the config directory', async () => { + homedirMock.mockReturnValue(invalidDir); - const outHome = join(dir1, ENV_FILE_NAME); - writeFileSync(outHome, 'export TEST_BLA=from_home'); + await withTempDir(directory => { + getConfigFileDirMock.mockReturnValue(directory); + const outFile = join(directory, LEGACY_FILE); + writeFileSync(outFile, 'export TEST_BLA=234\n'); - getConfigFileDirMock.mockReturnValue(dir2); - writeConfigFileSync(dir2); - const configDirFile = join(dir2, ENV_FILE_NAME); - writeFileSync(configDirFile, 'export TEST_BLA=from_config_dir'); + warnIfCraftEnvFileExists(); - readEnvironmentConfig(); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining(outFile), + ); + }); + }); - expect(process.env.TEST_BLA).toBe('from_config_dir'); + test('warns once per location when both files exist', async () => { + await withTempDir(async homeDir => { + await withTempDir(configDir => { + homedirMock.mockReturnValue(homeDir); + getConfigFileDirMock.mockReturnValue(configDir); + + const homeFile = join(homeDir, LEGACY_FILE); + const configFile = join(configDir, LEGACY_FILE); + writeFileSync(homeFile, 'export TEST_BLA=from_home'); + writeFileSync(configFile, 'export TEST_BLA=from_config_dir'); + + warnIfCraftEnvFileExists(); + + expect(logger.warn).toHaveBeenCalledTimes(2); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining(homeFile), + ); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining(configFile), + ); }); }); }); - test('does not overwrite existing variables by default', async () => { + test('does not mutate process.env', async () => { homedirMock.mockReturnValue(invalidDir); - process.env.TEST_BLA = 'existing'; - await withTempDir(directory => { getConfigFileDirMock.mockReturnValue(directory); - const outFile = join(directory, ENV_FILE_NAME); + const outFile = join(directory, LEGACY_FILE); writeFileSync(outFile, 'export TEST_BLA=new_value'); - readEnvironmentConfig(); - - expect(process.env.TEST_BLA).toBe('existing'); + const before = process.env.TEST_BLA; + warnIfCraftEnvFileExists(); + expect(process.env.TEST_BLA).toBe(before); }); }); + }); // end describe('warnIfCraftEnvFileExists') + + describe('sanitizeDynamicLinkerEnv', () => { + const DYNAMIC_LINKER_KEYS = [ + 'LD_PRELOAD', + 'LD_LIBRARY_PATH', + 'LD_AUDIT', + 'DYLD_INSERT_LIBRARIES', + 'DYLD_LIBRARY_PATH', + 'DYLD_FRAMEWORK_PATH', + 'DYLD_FALLBACK_LIBRARY_PATH', + 'DYLD_FALLBACK_FRAMEWORK_PATH', + ]; + + beforeEach(() => { + for (const key of DYNAMIC_LINKER_KEYS) { + delete process.env[key]; + } + delete process.env.CRAFT_ALLOW_DYNAMIC_LINKER_ENV; + }); - test('overwrites existing variables if explicitly stated', async () => { - homedirMock.mockReturnValue(invalidDir); + test('does nothing when no dynamic-linker vars are set', () => { + sanitizeDynamicLinkerEnv(); - process.env.TEST_BLA = 'existing'; + expect(logger.warn).not.toHaveBeenCalled(); + expect(logger.info).not.toHaveBeenCalled(); + }); - await withTempDir(directory => { - getConfigFileDirMock.mockReturnValue(directory); + test('strips LD_PRELOAD and warns without logging the value', () => { + const secret = '/tmp/attacker-preload.so'; + process.env.LD_PRELOAD = secret; - writeConfigFileSync(directory); + sanitizeDynamicLinkerEnv(); - const outFile = join(directory, ENV_FILE_NAME); - writeFileSync(outFile, 'export TEST_BLA=new_value'); + expect(process.env.LD_PRELOAD).toBeUndefined(); + expect(logger.warn).toHaveBeenCalledTimes(1); + const warnArgs = (logger.warn as any).mock.calls[0]; + const combined = warnArgs.join(' '); + expect(combined).toContain('LD_PRELOAD'); + // Value must NOT appear anywhere in the log call. + expect(combined).not.toContain(secret); + }); - readEnvironmentConfig(true); + test('strips every known dynamic-linker var when set', () => { + for (const key of DYNAMIC_LINKER_KEYS) { + process.env[key] = `value-for-${key}`; + } - expect(process.env.TEST_BLA).toBe('new_value'); - }); + sanitizeDynamicLinkerEnv(); + + for (const key of DYNAMIC_LINKER_KEYS) { + expect(process.env[key]).toBeUndefined(); + } + expect(logger.warn).toHaveBeenCalledTimes(DYNAMIC_LINKER_KEYS.length); + }); + + test('preserves vars when CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1', () => { + process.env.CRAFT_ALLOW_DYNAMIC_LINKER_ENV = '1'; + process.env.LD_PRELOAD = '/tmp/legit.so'; + process.env.LD_LIBRARY_PATH = '/opt/lib'; + + sanitizeDynamicLinkerEnv(); + + expect(process.env.LD_PRELOAD).toBe('/tmp/legit.so'); + expect(process.env.LD_LIBRARY_PATH).toBe('/opt/lib'); + expect(logger.warn).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledTimes(1); + const infoArgs = (logger.info as any).mock.calls[0]; + const combined = infoArgs.join(' '); + expect(combined).toContain('LD_PRELOAD'); + expect(combined).toContain('LD_LIBRARY_PATH'); + }); + + test('opt-out only applies when value is exactly "1"', () => { + process.env.CRAFT_ALLOW_DYNAMIC_LINKER_ENV = 'true'; + process.env.LD_PRELOAD = '/tmp/x.so'; + + sanitizeDynamicLinkerEnv(); + + // "true" is not the magic opt-out value; var should still be stripped. + expect(process.env.LD_PRELOAD).toBeUndefined(); + expect(logger.warn).toHaveBeenCalledTimes(1); }); - }); // end describe('readEnvironmentConfig') + }); // end describe('sanitizeDynamicLinkerEnv') }); // end describe('env utils functions') diff --git a/src/utils/env.ts b/src/utils/env.ts index 4bc5261ed..2c87740e6 100644 --- a/src/utils/env.ts +++ b/src/utils/env.ts @@ -1,17 +1,18 @@ -import { existsSync, statSync } from 'fs'; +import { existsSync } from 'fs'; import { join } from 'path'; // XXX(BYK): This is to be able to spy on `homedir()` in tests // TODO(BYK): Convert this to ES6 imports import os = require('os'); -import nvar from 'nvar'; - -import { CONFIG_FILE_NAME, getConfigFileDir } from '../config'; +import { getConfigFileDir } from '../config'; import { ConfigurationError } from './errors'; import { logger } from '../logger'; -/** File name where additional environment variables are stored */ -export const ENV_FILE_NAME = '.craft.env'; +/** + * Legacy filename no longer read by Craft. Retained as a constant for the + * startup warning helper below. + */ +const LEGACY_ENV_FILE_NAME = '.craft.env'; /** * A token, key, or other value which can be stored either in an env file or @@ -72,86 +73,101 @@ function envHasVar(envVar: RequiredConfigVar): boolean { } /** - * Checks that the file is only readable for the owner + * Dynamic-linker environment variables that Craft refuses to propagate. * - * It is assumed that the file already exists - * @param path File path - * @returns true if file is private, false otherwise + * Setting these allows arbitrary code to be loaded into every subprocess + * spawned by Craft (`LD_PRELOAD` on Linux, `DYLD_*` on macOS). They are a + * well-known supply-chain attack vector: an attacker who can influence + * Craft's environment (e.g. via a dotfile, a previous build step, or a + * misconfigured CI secret) can silently execute code with access to every + * release credential Craft touches. We strip these at startup as + * defence-in-depth — legitimate uses are extremely rare and can be + * re-enabled per-invocation via `CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1`. */ -function checkFileIsPrivate(path: string): boolean { - const FULL_MODE_MASK = 0o777; - const GROUP_MODE_MASK = 0o070; - const OTHER_MODE_MASK = 0o007; - const mode = statSync(path).mode; - if (mode & GROUP_MODE_MASK || mode & OTHER_MODE_MASK) { - const perms = (mode & FULL_MODE_MASK).toString(8); +const DYNAMIC_LINKER_ENV_VARS = [ + // Linux / glibc / musl + 'LD_PRELOAD', + 'LD_LIBRARY_PATH', + 'LD_AUDIT', + // macOS dyld + 'DYLD_INSERT_LIBRARIES', + 'DYLD_LIBRARY_PATH', + 'DYLD_FRAMEWORK_PATH', + 'DYLD_FALLBACK_LIBRARY_PATH', + 'DYLD_FALLBACK_FRAMEWORK_PATH', +] as const; + +/** Opt-out env var for {@link sanitizeDynamicLinkerEnv}. */ +const ALLOW_DYNAMIC_LINKER_ENV_VAR = 'CRAFT_ALLOW_DYNAMIC_LINKER_ENV'; + +/** + * Strips dynamic-linker environment variables (`LD_PRELOAD`, `LD_LIBRARY_PATH`, + * `DYLD_*`, etc.) from `process.env` at startup, logging a warning for each + * stripped key. Values are never logged. + * + * Users who legitimately require these variables (e.g. for an instrumented + * build toolchain) can set `CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1` to opt out; + * this is noisy by design to make the escape hatch visible in CI logs. + */ +export function sanitizeDynamicLinkerEnv(): void { + const allowOverride = process.env[ALLOW_DYNAMIC_LINKER_ENV_VAR] === '1'; + const presentKeys = DYNAMIC_LINKER_ENV_VARS.filter( + key => process.env[key] !== undefined, + ); + + if (presentKeys.length === 0) { + return; + } + + if (allowOverride) { + logger.info( + `${ALLOW_DYNAMIC_LINKER_ENV_VAR}=1 set; preserving dynamic-linker environment variables: ${presentKeys.join( + ', ', + )}. This is not recommended.`, + ); + return; + } + + for (const key of presentKeys) { logger.warn( - `Permissions 0${perms} for file "${path}" are too open. Consider making it readable only for the user.`, + `Stripping dynamic-linker environment variable "${key}" for security reasons. ` + + `Set ${ALLOW_DYNAMIC_LINKER_ENV_VAR}=1 to override (not recommended).`, ); - return false; + delete process.env[key]; } - return true; } /** - * Loads environment variables from ".craft.env" files in certain locations + * Warns the user if a legacy `.craft.env` file is present in the home + * directory or the configuration file directory. * - * The following two places are checked: - * - The user's home directory - * - The configuration file directory - * - * @param overwriteExisting If set to true, overwrite the existing environment - * variables + * Craft used to load environment variables from these files, but the behavior + * was removed for security reasons: arbitrary values (including credentials) + * could be silently injected into `process.env` based on the current working + * directory. This helper emits a one-time warning per location pointing users + * at their shell / CI environment for credential management. */ -export function readEnvironmentConfig(overwriteExisting = false): void { - let newEnv = {} as any; - - // Read from home dir - const homedirEnvFile = join(os.homedir(), ENV_FILE_NAME); - if (existsSync(homedirEnvFile)) { - logger.debug( - 'Found environment file in the home directory:', - homedirEnvFile, - ); - checkFileIsPrivate(homedirEnvFile); - const homedirEnv = {}; - nvar({ path: homedirEnvFile, target: homedirEnv }); - newEnv = { ...newEnv, ...homedirEnv }; - logger.trace('Read the following variables from env file:', homedirEnv); - } else { - logger.debug( - 'No environment file found in the home directory:', - homedirEnvFile, - ); - } +export function warnIfCraftEnvFileExists(): void { + const candidatePaths: string[] = []; - // Read from the directory where the configuration file is located + try { + candidatePaths.push(join(os.homedir(), LEGACY_ENV_FILE_NAME)); + } catch { + // os.homedir() can throw in edge cases; skip silently. + } const configFileDir = getConfigFileDir(); - const configDirEnvFile = configFileDir && join(configFileDir, ENV_FILE_NAME); - if (!configDirEnvFile) { - logger.debug('No configuration file found:', CONFIG_FILE_NAME); - } else if (configDirEnvFile && existsSync(configDirEnvFile)) { - logger.debug( - 'Found environment file in the configuration directory:', - configDirEnvFile, - ); - checkFileIsPrivate(configDirEnvFile); - const configDirEnv = {}; - nvar({ path: configDirEnvFile, target: configDirEnv }); - newEnv = { ...newEnv, ...configDirEnv }; - logger.trace('Read the following variables from env file:', configDirEnv); - } else { - logger.debug( - 'No environment file found in the configuration directory:', - configDirEnvFile, - ); + if (configFileDir) { + candidatePaths.push(join(configFileDir, LEGACY_ENV_FILE_NAME)); } - // Add non-existing values to env - for (const key of Object.keys(newEnv)) { - if (overwriteExisting || process.env[key] === undefined) { - process.env[key] = newEnv[key]; + for (const path of candidatePaths) { + if (existsSync(path)) { + logger.warn( + `Found legacy "${LEGACY_ENV_FILE_NAME}" file at "${path}". ` + + `Craft no longer reads this file for security reasons. ` + + `Please set the required variables in your shell or CI environment instead.`, + ); } } } diff --git a/src/utils/releaseCommandEnv.ts b/src/utils/releaseCommandEnv.ts new file mode 100644 index 000000000..18347441b --- /dev/null +++ b/src/utils/releaseCommandEnv.ts @@ -0,0 +1,63 @@ +/** + * Allowlist-based environment construction for user-defined release + * commands (`preReleaseCommand` / `postReleaseCommand` from `.craft.yml`). + * + * `.craft.yml` is attacker-influenceable via untrusted PRs: a malicious + * contributor can set `preReleaseCommand` to any shell command, and if we + * forward the full `process.env` to the subprocess, they gain access to + * every secret in the CI environment (GitHub tokens, npm tokens, GPG + * keys, etc.) and can exfiltrate them. + * + * The mitigation is to forward only a small, explicit allowlist of + * environment variables to the subprocess. Scripts that need additional + * variables should be updated to read them from a secrets file, or to + * use the `CRAFT_` prefix (which yargs surfaces as explicit CLI options). + */ + +/** + * Environment variables to pass through to user-defined release commands. + * + * - `HOME` is needed so Git can find `~/.gitconfig` with `safe.directory` + * settings, which fixes "fatal: detected dubious ownership in repository" + * errors in CI runners. + * - `USER`, `GIT_COMMITTER_NAME`, `GIT_AUTHOR_NAME`, and `EMAIL` help with + * commit operations in post-release scripts. + */ +export const ALLOWED_ENV_VARS = [ + 'HOME', + 'USER', + 'GIT_COMMITTER_NAME', + 'GIT_AUTHOR_NAME', + 'EMAIL', +] as const; + +/** + * Builds the environment for a user-defined release command subprocess. + * + * The returned env contains only: + * - `PATH` (so the shell can locate the command), + * - `GITHUB_TOKEN` (Craft's primary authentication credential, commonly + * required by release scripts to push tags, create releases, etc.), + * - the keys listed in {@link ALLOWED_ENV_VARS}, + * - any caller-supplied `extras` (e.g. `CRAFT_NEW_VERSION`). + * + * Undefined values are preserved (Node's `spawn` treats `undefined` as + * "unset" rather than the string "undefined"). + * + * @param extras Additional key/value pairs to include in the child env. + * @returns A fresh env object safe to pass to `spawn` / `spawnProcess`. + */ +export function buildReleaseCommandEnv( + extras: Record = {}, +): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { + PATH: process.env.PATH, + GITHUB_TOKEN: process.env.GITHUB_TOKEN, + }; + + for (const key of ALLOWED_ENV_VARS) { + env[key] = process.env[key]; + } + + return { ...env, ...extras }; +} From e76d00ac38eb4e57ccb11e88029c4b9e2f2f9fa4 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 21 Apr 2026 15:35:50 +0000 Subject: [PATCH 2/2] fixup: format AGENTS.md + drop unused ALLOWED_ENV_VARS export - Run prettier over AGENTS.md to fix format-check CI. - Drop export on ALLOWED_ENV_VARS (Cursor Bugbot): it's only used internally by buildReleaseCommandEnv in the same file. --- AGENTS.md | 31 +++++++++++++++++++++---------- src/utils/releaseCommandEnv.ts | 2 +- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6a593cec3..d7816dad1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,45 +113,56 @@ Some operations need explicit `isDryRun()` checks: - User experience optimizations (e.g., skipping sleep timers) + ## Long-term Knowledge ### Architecture -* **Craft release commands forward sanitized env, not full process.env**: Craft's pre/postReleaseCommand invocations (\`prepare.ts\` runCustomPreReleaseCommand, \`publish.ts\` runPostReleaseCommand) must NOT forward \`...process.env\` to subprocesses — that pattern lets attacker-controlled \`.craft.env\` or config inject \`LD\_PRELOAD\`/\`LD\_LIBRARY\_PATH\` for RCE via the CI release pipeline. Use the shared allowlist helper in \`src/utils/releaseCommandEnv.ts\` which returns only {PATH, HOME, USER, GIT\_COMMITTER\_NAME, GIT\_AUTHOR\_NAME, EMAIL, GITHUB\_TOKEN, CRAFT\_\*} plus per-command additions (CRAFT\_NEW\_VERSION/OLD\_VERSION for prepare, CRAFT\_RELEASED\_VERSION for publish). Tests in \`prepare.test.ts\`/\`publish.test.ts\` assert LD\_PRELOAD and secret env vars are stripped. + +- **Craft release commands forward sanitized env, not full process.env**: Craft's pre/postReleaseCommand invocations (\`prepare.ts\` runCustomPreReleaseCommand, \`publish.ts\` runPostReleaseCommand) must NOT forward \`...process.env\` to subprocesses — that pattern lets attacker-controlled \`.craft.env\` or config inject \`LD_PRELOAD\`/\`LD_LIBRARY_PATH\` for RCE via the CI release pipeline. Use the shared allowlist helper in \`src/utils/releaseCommandEnv.ts\` which returns only {PATH, HOME, USER, GIT_COMMITTER_NAME, GIT_AUTHOR_NAME, EMAIL, GITHUB_TOKEN, CRAFT\_\*} plus per-command additions (CRAFT_NEW_VERSION/OLD_VERSION for prepare, CRAFT_RELEASED_VERSION for publish). Tests in \`prepare.test.ts\`/\`publish.test.ts\` assert LD_PRELOAD and secret env vars are stripped. -* **Registry target: repo\_url auto-derived from git remote, not user-configurable**: \`repo\_url\` in registry manifests is always set by Craft as \`https://github.com/${owner}/${repo}\`. Resolution: (1) explicit \`github: { owner, repo }\` in \`.craft.yml\` (rare), (2) fallback: auto-detect from git \`origin\` remote URL via \`git-url-parse\` library (\`git.ts:194-217\`, \`config.ts:286-316\`). Works with HTTPS and SSH remote URLs. Always overwritten on every publish — existing manifest values are replaced (\`registry.ts:417-418\`). Result is cached globally with \`Object.freeze\`. If remote isn't \`github.com\` and no explicit config exists, throws \`ConfigurationError\`. Most repos need no configuration — the git origin remote is sufficient. + +- **Registry target: repo_url auto-derived from git remote, not user-configurable**: \`repo_url\` in registry manifests is always set by Craft as \`https://github.com/${owner}/${repo}\`. Resolution: (1) explicit \`github: { owner, repo }\` in \`.craft.yml\` (rare), (2) fallback: auto-detect from git \`origin\` remote URL via \`git-url-parse\` library (\`git.ts:194-217\`, \`config.ts:286-316\`). Works with HTTPS and SSH remote URLs. Always overwritten on every publish — existing manifest values are replaced (\`registry.ts:417-418\`). Result is cached globally with \`Object.freeze\`. If remote isn't \`github.com\` and no explicit config exists, throws \`ConfigurationError\`. Most repos need no configuration — the git origin remote is sufficient. -* **Registry target: urlTemplate generates artifact download URLs in manifest**: \`urlTemplate\` in the registry target config generates download URLs for release artifacts in the registry manifest's \`files\` field. Uses Mustache rendering with variables \`{{version}}\`, \`{{file}}\`, \`{{revision}}\`. Primarily useful for apps (standalone binaries) and CDN-hosted assets — SDK packages published to public registries (npm, PyPI, gem) typically don't need it. If neither \`urlTemplate\` nor \`checksums\` is configured, Craft skips adding file data entirely (warns at \`registry.ts:341-349\`). Real-world pattern: \`https://downloads.sentry-cdn.com/\/{{version}}/{{file}}\`. + +- **Registry target: urlTemplate generates artifact download URLs in manifest**: \`urlTemplate\` in the registry target config generates download URLs for release artifacts in the registry manifest's \`files\` field. Uses Mustache rendering with variables \`{{version}}\`, \`{{file}}\`, \`{{revision}}\`. Primarily useful for apps (standalone binaries) and CDN-hosted assets — SDK packages published to public registries (npm, PyPI, gem) typically don't need it. If neither \`urlTemplate\` nor \`checksums\` is configured, Craft skips adding file data entirely (warns at \`registry.ts:341-349\`). Real-world pattern: \`https://downloads.sentry-cdn.com/\/{{version}}/{{file}}\`. ### Decision -* **--config-from gated behind --allow-remote-config**: Craft's \`prepare --config-from \\` fetches \`.craft.yml\` from a remote git ref and feeds it to \`loadConfigurationFromString\`, which can execute arbitrary commands via preReleaseCommand. Now gated: \`assertRemoteConfigAllowed()\` in \`src/commands/prepare.ts\` throws ConfigurationError unless \`--allow-remote-config\` (or \`CRAFT\_ALLOW\_REMOTE\_CONFIG=1\`) is set. When opted in, a warning is logged naming the branch. Exported helper is unit-tested; full \`prepareMain\` is not (heavy mock surface). + +- **--config-from gated behind --allow-remote-config**: Craft's \`prepare --config-from \\` fetches \`.craft.yml\` from a remote git ref and feeds it to \`loadConfigurationFromString\`, which can execute arbitrary commands via preReleaseCommand. Now gated: \`assertRemoteConfigAllowed()\` in \`src/commands/prepare.ts\` throws ConfigurationError unless \`--allow-remote-config\` (or \`CRAFT_ALLOW_REMOTE_CONFIG=1\`) is set. When opted in, a warning is logged naming the branch. Exported helper is unit-tested; full \`prepareMain\` is not (heavy mock surface). ### Gotcha -* **Craft .craft.env file reading removed — security hazard via LD\_PRELOAD**: Craft used to hydrate \`process.env\` from \`$HOME/.craft.env\` and \`\/.craft.env\` via \`nvar\`. Removed because an attacker PR could add \`.craft.env\` with \`LD\_PRELOAD=./preload.so\` + a malicious shared library, giving RCE in the release pipeline with access to all secrets (demo: getsentry/action-release#315). \`src/utils/env.ts\` now only exports \`warnIfCraftEnvFileExists()\` (startup warning, no file read, no env mutation) and \`checkEnvForPrerequisite\` (unchanged). \`nvar\` dep and \`src/types/nvar.ts\` were removed. Consumers must set env vars via shell/CI. + +- **Craft .craft.env file reading removed — security hazard via LD_PRELOAD**: Craft used to hydrate \`process.env\` from \`$HOME/.craft.env\` and \`\/.craft.env\` via \`nvar\`. Removed because an attacker PR could add \`.craft.env\` with \`LD_PRELOAD=./preload.so\` + a malicious shared library, giving RCE in the release pipeline with access to all secrets (demo: getsentry/action-release#315). \`src/utils/env.ts\` now only exports \`warnIfCraftEnvFileExists()\` (startup warning, no file read, no env mutation) and \`checkEnvForPrerequisite\` (unchanged). \`nvar\` dep and \`src/types/nvar.ts\` were removed. Consumers must set env vars via shell/CI. -* **Craft postReleaseCommand env vars pollute shared bump-version scripts**: Craft's \`runPostReleaseCommand\` sets \`CRAFT\_NEW\_VERSION=\\` in the subprocess env. If a post-release script calls a shared \`bump-version.sh\` that reads \`NEW\_VERSION="${CRAFT\_NEW\_VERSION:-${2:-}}"\`, the env var takes precedence over the positional arg (e.g. \`nightly\`), causing the script to set the version to the already-current release version → no diff → no commit → master stays on the release version. Fixed by replacing \`CRAFT\_NEW\_VERSION\`/\`CRAFT\_OLD\_VERSION\` with \`CRAFT\_RELEASED\_VERSION\` in the post-release env (\`publish.ts:563-564\`). The pre-release command (\`prepare.ts\`) still correctly uses \`CRAFT\_NEW\_VERSION\`. Consuming repos don't need changes unless they explicitly read \`CRAFT\_NEW\_VERSION\` in their post-release scripts. + +- **Craft postReleaseCommand env vars pollute shared bump-version scripts**: Craft's \`runPostReleaseCommand\` sets \`CRAFT_NEW_VERSION=\\` in the subprocess env. If a post-release script calls a shared \`bump-version.sh\` that reads \`NEW_VERSION="${CRAFT\_NEW\_VERSION:-${2:-}}"\`, the env var takes precedence over the positional arg (e.g. \`nightly\`), causing the script to set the version to the already-current release version → no diff → no commit → master stays on the release version. Fixed by replacing \`CRAFT_NEW_VERSION\`/\`CRAFT_OLD_VERSION\` with \`CRAFT_RELEASED_VERSION\` in the post-release env (\`publish.ts:563-564\`). The pre-release command (\`prepare.ts\`) still correctly uses \`CRAFT_NEW_VERSION\`. Consuming repos don't need changes unless they explicitly read \`CRAFT_NEW_VERSION\` in their post-release scripts. -* **ESM modules prevent vi.spyOn of child\_process.spawnSync — use test subclass pattern**: In ESM (Vitest or Bun), you cannot \`vi.spyOn\` exports from Node built-in modules — throws 'Module namespace is not configurable'. Workaround: create a test subclass that overrides the method calling the built-in and injects controllable values. \`vi.mock\` at module level works but affects all tests in the file. + +- **ESM modules prevent vi.spyOn of child_process.spawnSync — use test subclass pattern**: In ESM (Vitest or Bun), you cannot \`vi.spyOn\` exports from Node built-in modules — throws 'Module namespace is not configurable'. Workaround: create a test subclass that overrides the method calling the built-in and injects controllable values. \`vi.mock\` at module level works but affects all tests in the file. -* **pnpm overrides with >= can cross major versions — use ^ to constrain**: pnpm overrides gotchas: (1) \`>=\` crosses major versions — use \`^\` to constrain within same major. (2) Version-range selectors don't reliably force re-resolution of compatible transitive deps; use blanket overrides when safe. (3) Overrides become stale — audit with \`pnpm why \\` after dependency changes. (4) Never manually resolve pnpm-lock.yaml conflicts — \`git checkout --theirs\` then \`pnpm install\` to regenerate deterministically. + +- **pnpm overrides with >= can cross major versions — use ^ to constrain**: pnpm overrides gotchas: (1) \`>=\` crosses major versions — use \`^\` to constrain within same major. (2) Version-range selectors don't reliably force re-resolution of compatible transitive deps; use blanket overrides when safe. (3) Overrides become stale — audit with \`pnpm why \\` after dependency changes. (4) Never manually resolve pnpm-lock.yaml conflicts — \`git checkout --theirs\` then \`pnpm install\` to regenerate deterministically. ### Pattern -* **Craft/Publish release flow: prepare then accept publish issue**: Craft's release flow is two-phase: (1) Run \`release.yml\` GitHub Action with version "auto" — this runs \`craft prepare\`, auto-determines version from commits, creates the \`release/X.Y.Z\` branch, and opens a publish issue on \`getsentry/publish\` repo (e.g. \`publish: getsentry/craft@2.25.3\`). (2) Add the \`accepted\` label to that publish issue to trigger the actual publish pipeline. Do NOT manually create release branches — always use the workflow. The publish issue URL is emitted in the release job logs as a \`::notice::Created publish request:\` line. The publish repo is configured via \`PUBLISH\_REPO\` (defaults to \`getsentry/publish\`). + +- **Craft/Publish release flow: prepare then accept publish issue**: Craft's release flow is two-phase: (1) Run \`release.yml\` GitHub Action with version "auto" — this runs \`craft prepare\`, auto-determines version from commits, creates the \`release/X.Y.Z\` branch, and opens a publish issue on \`getsentry/publish\` repo (e.g. \`publish: getsentry/craft@2.25.3\`). (2) Add the \`accepted\` label to that publish issue to trigger the actual publish pipeline. Do NOT manually create release branches — always use the workflow. The publish issue URL is emitted in the release job logs as a \`::notice::Created publish request:\` line. The publish repo is configured via \`PUBLISH_REPO\` (defaults to \`getsentry/publish\`). ### Preference -* **CHANGELOG.md is auto-managed — do not edit manually**: Craft's CHANGELOG.md is auto-generated from PR descriptions by the release pipeline. Do NOT add entries manually, even for breaking changes. The user will reject such edits. Describe breaking changes in the PR body instead; the auto-managed process surfaces them in the changelog. + +- **CHANGELOG.md is auto-managed — do not edit manually**: Craft's CHANGELOG.md is auto-generated from PR descriptions by the release pipeline. Do NOT add entries manually, even for breaking changes. The user will reject such edits. Describe breaking changes in the PR body instead; the auto-managed process surfaces them in the changelog. diff --git a/src/utils/releaseCommandEnv.ts b/src/utils/releaseCommandEnv.ts index 18347441b..b9f658d72 100644 --- a/src/utils/releaseCommandEnv.ts +++ b/src/utils/releaseCommandEnv.ts @@ -23,7 +23,7 @@ * - `USER`, `GIT_COMMITTER_NAME`, `GIT_AUTHOR_NAME`, and `EMAIL` help with * commit operations in post-release scripts. */ -export const ALLOWED_ENV_VARS = [ +const ALLOWED_ENV_VARS = [ 'HOME', 'USER', 'GIT_COMMITTER_NAME',