From 9e15c525935add5a7c1952e1e37da7e7599d98b6 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 20:13:00 +0000 Subject: [PATCH 1/2] security(release-env): allowlist GITHUB_* and RUNNER_* by prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #794 landed a strict allowlist for env vars forwarded to preReleaseCommand / postReleaseCommand: PATH, GITHUB_TOKEN, HOME, USER, GIT_COMMITTER_NAME, GIT_AUTHOR_NAME, EMAIL, plus per-command CRAFT_* extras. That was conservative by design, but it broke consuming repos whose release scripts relied on GitHub Actions context vars that the runner injects into every step (GITHUB_REPOSITORY, GITHUB_SHA, GITHUB_RUN_ID, GITHUB_WORKFLOW, GITHUB_API_URL, ...). See getsentry/sentry-cocoa's ./scripts/update-package-sha.sh failing with 'GITHUB_RUN_ID: unbound variable': https://github.com/getsentry/sentry-cocoa/actions/runs/24797916943/job/72575447439 Extend the allowlist to forward every env var that starts with GITHUB_ or RUNNER_. This covers the entire namespace of GitHub Actions context metadata that release scripts commonly read, and does NOT cover credential env vars — those use unrelated prefixes by convention (NPM_TOKEN, CRATES_IO_TOKEN, DOCKER_PASSWORD, GPG_PRIVATE_KEY, AWS_SECRET_ACCESS_KEY, TWINE_PASSWORD, ...). Verified against the publish.yml env block: the only GITHUB_*-named key it sets explicitly is GITHUB_TOKEN, which is a credential and already in the allowlist. Tests: - Both prepare.test.ts and publish.test.ts gain a new regression test that sets GITHUB_RUN_ID, GITHUB_REPOSITORY, RUNNER_OS, NPM_TOKEN, and DOCKER_PASSWORD on process.env, runs the release command, and asserts the first three reach the child while the last two do not. - Existing 'does not forward arbitrary env vars' tests still guard the negative side. - expectedBaseEnv helper in both test files is now dynamic so it stays correct when run under GHA (where runtime process.env contains a full GITHUB_* / RUNNER_* set). --- src/commands/__tests__/prepare.test.ts | 82 ++++++++++++++++++++++---- src/commands/__tests__/publish.test.ts | 82 ++++++++++++++++++-------- src/utils/releaseCommandEnv.ts | 46 ++++++++++++--- 3 files changed, 166 insertions(+), 44 deletions(-) diff --git a/src/commands/__tests__/prepare.test.ts b/src/commands/__tests__/prepare.test.ts index a6690823..0e87b72f 100644 --- a/src/commands/__tests__/prepare.test.ts +++ b/src/commands/__tests__/prepare.test.ts @@ -15,17 +15,27 @@ 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, - }); + const expectedBaseEnv = () => { + const env: Record = { + PATH: process.env.PATH, + 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, + }; + // Prefix-match keys are forwarded as a group — enumerate whatever's + // currently on `process.env` (keeps the test stable across local + // runs, CI runs with GHA env, and CI runs with runner env). + for (const key of Object.keys(process.env)) { + if (key.startsWith('GITHUB_') || key.startsWith('RUNNER_')) { + env[key] = process.env[key]; + } + } + env.CRAFT_NEW_VERSION = newVersion; + env.CRAFT_OLD_VERSION = oldVersion; + return env; + }; beforeEach(() => { vi.clearAllMocks(); @@ -117,6 +127,56 @@ describe('runPreReleaseCommand', () => { } } }); + + test('forwards GITHUB_* and RUNNER_* by prefix, not credential-named vars', async () => { + // Regression test for the sentry-cocoa breakage where + // ./scripts/update-package-sha.sh read GITHUB_RUN_ID and exploded + // with "unbound variable" because Craft was stripping the whole + // GITHUB_* namespace. + const before = { + GITHUB_RUN_ID: process.env.GITHUB_RUN_ID, + GITHUB_REPOSITORY: process.env.GITHUB_REPOSITORY, + RUNNER_OS: process.env.RUNNER_OS, + NPM_TOKEN: process.env.NPM_TOKEN, + DOCKER_PASSWORD: process.env.DOCKER_PASSWORD, + }; + process.env.GITHUB_RUN_ID = '123456'; + process.env.GITHUB_REPOSITORY = 'getsentry/sentry-cocoa'; + process.env.RUNNER_OS = 'Linux'; + process.env.NPM_TOKEN = 'npm_xxx_must_not_leak'; + process.env.DOCKER_PASSWORD = 'dockerpw_must_not_leak'; + + try { + await runPreReleaseCommand({ + oldVersion, + newVersion, + rootDir, + preReleaseCommand: 'scripts/bump-version.sh', + }); + + const envArg = mockedSpawnProcess.mock.calls[0][2].env as Record< + string, + unknown + >; + + // GITHUB_* and RUNNER_* pass through. + expect(envArg.GITHUB_RUN_ID).toBe('123456'); + expect(envArg.GITHUB_REPOSITORY).toBe('getsentry/sentry-cocoa'); + expect(envArg.RUNNER_OS).toBe('Linux'); + + // Credential-named vars do not. + expect(envArg.NPM_TOKEN).toBeUndefined(); + expect(envArg.DOCKER_PASSWORD).toBeUndefined(); + } finally { + for (const [key, val] of Object.entries(before)) { + if (val === undefined) { + delete process.env[key]; + } else { + process.env[key] = val; + } + } + } + }); }); describe('checkVersionOrPart', () => { diff --git a/src/commands/__tests__/publish.test.ts b/src/commands/__tests__/publish.test.ts index 6c461e9d..f2bd7349 100644 --- a/src/commands/__tests__/publish.test.ts +++ b/src/commands/__tests__/publish.test.ts @@ -22,6 +22,24 @@ describe('runPostReleaseCommand', () => { const mockedSpawnProcess = spawnProcess as Mock; const mockedHasExecutable = hasExecutable as Mock; + const expectedBaseEnv = () => { + const env: Record = { + PATH: process.env.PATH, + 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, + }; + for (const key of Object.keys(process.env)) { + if (key.startsWith('GITHUB_') || key.startsWith('RUNNER_')) { + env[key] = process.env[key]; + } + } + env.CRAFT_RELEASED_VERSION = newVersion; + return env; + }; + beforeEach(() => { vi.clearAllMocks(); }); @@ -36,18 +54,7 @@ describe('runPostReleaseCommand', () => { expect(mockedSpawnProcess).toHaveBeenCalledWith( '/bin/bash', [pathJoin('scripts', 'post-release.sh'), '', newVersion], - { - env: { - CRAFT_RELEASED_VERSION: newVersion, - 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, - }, - }, + { env: expectedBaseEnv() }, ); }); @@ -72,18 +79,7 @@ describe('runPostReleaseCommand', () => { expect(mockedSpawnProcess).toHaveBeenCalledWith( 'python', ['./increase_version.py', 'argument 1', '', newVersion], - { - env: { - CRAFT_RELEASED_VERSION: newVersion, - 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, - }, - }, + { env: expectedBaseEnv() }, ); }); @@ -126,6 +122,44 @@ describe('runPostReleaseCommand', () => { } } }); + + test('forwards GITHUB_* and RUNNER_* by prefix, not credential-named vars', async () => { + const before = { + GITHUB_RUN_ID: process.env.GITHUB_RUN_ID, + GITHUB_REPOSITORY: process.env.GITHUB_REPOSITORY, + RUNNER_OS: process.env.RUNNER_OS, + NPM_TOKEN: process.env.NPM_TOKEN, + DOCKER_PASSWORD: process.env.DOCKER_PASSWORD, + }; + process.env.GITHUB_RUN_ID = '9876'; + process.env.GITHUB_REPOSITORY = 'getsentry/sentry-cocoa'; + process.env.RUNNER_OS = 'Linux'; + process.env.NPM_TOKEN = 'npm_xxx_must_not_leak'; + process.env.DOCKER_PASSWORD = 'dockerpw_must_not_leak'; + + try { + await runPostReleaseCommand(newVersion, 'bash -c true'); + + const envArg = mockedSpawnProcess.mock.calls[0][2].env as Record< + string, + unknown + >; + + expect(envArg.GITHUB_RUN_ID).toBe('9876'); + expect(envArg.GITHUB_REPOSITORY).toBe('getsentry/sentry-cocoa'); + expect(envArg.RUNNER_OS).toBe('Linux'); + expect(envArg.NPM_TOKEN).toBeUndefined(); + expect(envArg.DOCKER_PASSWORD).toBeUndefined(); + } 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/utils/releaseCommandEnv.ts b/src/utils/releaseCommandEnv.ts index b9f658d7..e6b958fc 100644 --- a/src/utils/releaseCommandEnv.ts +++ b/src/utils/releaseCommandEnv.ts @@ -15,8 +15,10 @@ */ /** - * Environment variables to pass through to user-defined release commands. + * Exact-match environment variables passed through to user-defined + * release commands. * + * - `PATH` is needed so the shell can locate the command itself. * - `HOME` is needed so Git can find `~/.gitconfig` with `safe.directory` * settings, which fixes "fatal: detected dubious ownership in repository" * errors in CI runners. @@ -24,6 +26,7 @@ * commit operations in post-release scripts. */ const ALLOWED_ENV_VARS = [ + 'PATH', 'HOME', 'USER', 'GIT_COMMITTER_NAME', @@ -31,14 +34,36 @@ const ALLOWED_ENV_VARS = [ 'EMAIL', ] as const; +/** + * Prefix-match environment variables passed through to user-defined + * release commands. + * + * - `GITHUB_*` — context metadata that GitHub Actions injects into every + * step (`GITHUB_REPOSITORY`, `GITHUB_SHA`, `GITHUB_REF`, `GITHUB_RUN_ID`, + * `GITHUB_ACTOR`, `GITHUB_WORKFLOW`, `GITHUB_API_URL`, `GITHUB_TOKEN`, + * ...). User release scripts commonly read these (e.g. sentry-cocoa's + * bump script stamps `GITHUB_RUN_ID` into a file for a follow-up step). + * `GITHUB_TOKEN` is a credential but is already the single-most-common + * thing release scripts need; pretending it's not in scope would force + * every consumer to proxy it via `CRAFT_*`. + * - `RUNNER_*` — non-secret runner metadata (`RUNNER_OS`, `RUNNER_ARCH`, + * `RUNNER_TEMP`, ...) with the same ergonomic justification. + * + * These prefixes do NOT cover credential env vars (`NPM_TOKEN`, + * `CRATES_IO_TOKEN`, `DOCKER_PASSWORD`, `GPG_PRIVATE_KEY`, + * `AWS_SECRET_ACCESS_KEY`, `TWINE_PASSWORD`, ...) which are all named + * outside the `GITHUB_` / `RUNNER_` namespaces by convention, so the + * prefix allowlist does not expand the credential-leak surface. + */ +const ALLOWED_ENV_VAR_PREFIXES = ['GITHUB_', 'RUNNER_'] 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}, + * - every key in {@link ALLOWED_ENV_VARS}, + * - every key in `process.env` that starts with one of + * {@link ALLOWED_ENV_VAR_PREFIXES}, * - any caller-supplied `extras` (e.g. `CRAFT_NEW_VERSION`). * * Undefined values are preserved (Node's `spawn` treats `undefined` as @@ -50,14 +75,17 @@ const ALLOWED_ENV_VARS = [ export function buildReleaseCommandEnv( extras: Record = {}, ): NodeJS.ProcessEnv { - const env: NodeJS.ProcessEnv = { - PATH: process.env.PATH, - GITHUB_TOKEN: process.env.GITHUB_TOKEN, - }; + const env: NodeJS.ProcessEnv = {}; for (const key of ALLOWED_ENV_VARS) { env[key] = process.env[key]; } + for (const key of Object.keys(process.env)) { + if (ALLOWED_ENV_VAR_PREFIXES.some(prefix => key.startsWith(prefix))) { + env[key] = process.env[key]; + } + } + return { ...env, ...extras }; } From b0382785b7fcdc1a618911c72da5182a5dffc3dc Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 22 Apr 2026 20:30:38 +0000 Subject: [PATCH 2/2] fixup: address self-review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - src/utils/releaseCommandEnv.ts: expand the prefix-namespace JSDoc to explicitly call out GITHUB_API_TOKEN (a credential Craft reads as a ghcr.io / cross-repo fallback — see targets/docker.ts:338, targets/commitOnGitRepository.ts:148, utils/githubApi.ts:78) so the trade-off is visible to the next maintainer. Also note GITHUB_ENV / GITHUB_PATH / GITHUB_OUTPUT / GITHUB_STEP_SUMMARY are forwarded but grant no new primitive since the attacker already has shell exec in the same docker mount as those files. - src/commands/prepare.ts: stale comment enumerating the old allowlist (PATH, GITHUB_TOKEN, HOME, ...) replaced with a pointer to releaseCommandEnv.ts so the comment can't drift. - src/utils/releaseCommandEnv.ts: buildReleaseCommandEnv JSDoc now explains the semantic difference between exact-match keys (always present in returned object, possibly undefined) and prefix-match keys (only present when set). Both shapes behave identically under child_process.spawn; the note exists for callers using Object.keys. --- AGENTS.md | 108 ++++++++++++++++++++------------- src/commands/prepare.ts | 6 +- src/utils/releaseCommandEnv.ts | 49 ++++++++++----- 3 files changed, 102 insertions(+), 61 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f9d1f88d..f48dc3b5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -112,93 +112,117 @@ Some operations need explicit `isDryRun()` checks: - Operations that need to return mock data in dry-run mode - User experience optimizations (e.g., skipping sleep timers) - + ## Long-term Knowledge ### Architecture - + -- **Craft post-publish merge is non-blocking housekeeping**: After all publish targets complete, \`handleReleaseBranch()\` in \`src/commands/publish.ts\` merges the release branch back into the default branch and deletes it. This is a housekeeping step — failures are caught, reported to Sentry via \`captureException\`, and logged as warnings without failing the command. The merge uses \`--no-ff\` with the default (ort) strategy first, then retries with \`-s resolve\` if conflicts occur (handles criss-cross ambiguities in files like CHANGELOG.md). An \`isAuthError()\` helper distinguishes authentication failures (expired tokens) from merge conflicts to provide targeted diagnostics. +- **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 static bumpVersion pattern across targets**: Craft auto version bumping is opt-in per target class via a static \`bumpVersion(rootDir, newVersion): Promise\\` method. \`src/utils/versionBump.ts\` discovers which target classes have it via \`hasBumpVersion()\`, calls each unique class once in config order (not per-target-instance), and records bumpable vs skipped. Returning \`false\` means no-op (e.g., no package.json found); throwing fails the whole release with context. Targets implementing it today: npm, pypi, crates, gem, pubDev, hex, nuget. \`github\` target intentionally has none. Tests live in \`src/\_\_tests\_\_/versionBump.test.ts\` with a hoisted \`mockSpawnProcess\` + \`mockHasExecutable\` pattern that also stubs \`runWithExecutable\` to re-invoke the mocks (since real \`runWithExecutable\` internally calls unmocked \`hasExecutable\`). +- **Craft spawnProcess strips dynamic-linker env from subprocess env**: PR #800: \`spawnProcess\` in \`src/utils/system.ts\` now sanitizes \`LD_PRELOAD\`/\`LD_LIBRARY_PATH\`/\`LD_AUDIT\`/\`DYLD\_\*\` from the child's env before every spawn — defence-in-depth layer on top of startup-time \`sanitizeDynamicLinkerEnv()\` (which only protects \`process.env\` of Craft itself, not subprocess inheritance if something re-injects mid-run). Helper \`sanitizeSpawnEnv(env)\` lives in new standalone \`src/utils/dynamicLinkerEnv.ts\` (no other imports) to avoid circular dep. Same \`CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1\` opt-out as startup path. \`env.ts\` re-exports constants for backward compat. Complements \[\[019db09e-ac9b-765d-a091-bb6bb512b987]]. - + -- **getsentry/publish poller: active-polling via self-dispatch, cron as fallback**: GitHub Actions \`\*/5\` cron drifts to 30-40 min under load, stalling releases with fast CI. \`ci-poller.yml\` uses self-dispatch: when pending issues remain, it \`gh workflow run\`s itself (cap 60 attempts ~30min). Concurrency group \`ci-status-poller\` with \`cancel-in-progress: false\` queues runs — GHA startup gives ~30-60s between checks. Chain starts via \`waiting-for-ci\` job in \`publish.yml\` when \`accepted\` label is added. Cron is safety net (also skipped when \`CI_POLLER_HAS_PENDING == 'false'\`). Zero idle cost — no chain running unless human action occurred. Self-dispatch step must guard on \`steps.token.outcome == 'success'\` and \`steps.remaining.outcome == 'success'\` to avoid dispatching with empty token. +- **Craft target \*\_BIN env vars allow redirecting binary execution**: Craft targets honor \`\*\_BIN\` env var overrides (\`DOCKER_BIN\`, \`NPM_BIN\`, \`YARN_BIN\`, \`GEM_BIN\`, \`TWINE_BIN\`, \`MIX_BIN\`, \`NUGET_DOTNET_BIN\`, \`POWERSHELL_BIN\`, \`COCOAPODS_BIN\`, ...) to locate tool binaries. These are NOT attacker-controlled from a PR, but compose with \`preReleaseCommand\`: a malicious pre-release script could \`export NPM_BIN=/tmp/evil-npm\` before exiting, and subsequent target invocations use it. Hardening: resolve each \`\*\_BIN\` once at startup via \`resolveExecutable\`, warn if any points outside standard PATH dirs, reject relative paths. - + -- **Publish repo event-driven CI gate replaces polling in craft publish**: The \`getsentry/publish\` repo uses a label-based state machine instead of having \`craft publish\` poll CI status for up to 60 minutes. Labels: \`ci-pending\` (set at issue creation), \`ci-ready\` (set by poller or dispatch when CI passes). The \`publish.yml\` job requires \`accepted && !ci-pending\`. A \`ci-poller.yml\` cron (every 5 min) checks CI via GitHub API and swaps labels when ready. A \`ci-ready.yml\` handles \`repository_dispatch\` for repos that signal directly. This eliminates wasted runner time from idle polling and preserves the GitHub App token's 1-hour lifetime for the actual publish step. +- **publish.yml → Craft state-file handoff via XDG_STATE_HOME**: getsentry/publish's \`publish.yml\` writes the publish-state file to \`$GITHUB\_WORKSPACE/.craft-state/craft/publish-state-\-\-\-\.json\` and sets \`XDG\_STATE\_HOME=/github/workspace/.craft-state\` on the docker step. Craft reads from \`$XDG_STATE_HOME/craft/publish-state-\*.json\`. The container_cwd is \`/github/workspace/\_\_repo\_\_\` (root) or \`/github/workspace/\_\_repo\_\_/\\` (monorepo) — bash case statement handles both. Outside \`\_\_repo\_\_/\` means repo contents cannot pre-populate (security). Previous dual-write compat removed in getsentry/publish#7892 after Craft 2.26.0 shipped with the new-location reader. + + + +- **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}}\`. + +### 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 - + -- **actions/create-github-app-token v3 deprecated app-id input**: \*\*actions/create-github-app-token v3 required + app-id deprecated\*\*: v3 is needed for Node.js 24 runners (v2 uses Node 20, deprecated). v3 also deprecated the \`app-id\` input in favor of \`client-id\`. When the input references a variable named \`\*\_APP_ID\`, the variable value may already be a client ID (check \`vars.SENTRY_INTERNAL_APP_ID\` vs \`vars.SENTRY_RELEASE_BOT_CLIENT_ID\`). Replace both the action version and the input name. Affects all workflow files using the action. +- **Allowlist cutting GITHUB\_\* breaks user release scripts**: PR #794's release-command env allowlist was too tight — it stripped all \`GITHUB\_\*\` except \`GITHUB_TOKEN\`, breaking user scripts that read GHA context vars like \`GITHUB_RUN_ID\`, \`GITHUB_REPOSITORY\`, \`GITHUB_SHA\` (sentry-cocoa's bump.sh failed on \`unbound variable GITHUB_RUN_ID\`). Fix in #807: extend \`buildReleaseCommandEnv\` to forward any \`process.env\` key starting with \`GITHUB\_\` or \`RUNNER\_\` by prefix. Safe because \`publish.yml\` only sets \`GITHUB_TOKEN\` itself under that namespace — all other secrets use unrelated prefixes (\`NPM_TOKEN\`, \`CRATES_IO_TOKEN\`, \`AWS_SECRET_ACCESS_KEY\`, etc.). Prefix allowlist does not expand credential-leak surface. - + -- **auto-approve race: can fire before ci-pending.yml adds its label**: \*\*Resolved in getsentry/publish PR #7881\*\*: Eliminated the race by restructuring the label flow — \`ci-pending\` is now added by \`waiting-for-ci\` in \`publish.yml\` only AFTER \`accepted\` is present, not by a separate \`ci-pending.yml\` on \`issues:opened\`. The \`ci-pending.yml\` workflow was deleted. New flow: craft creates issue → (auto-approve or human) adds \`accepted\` → \`waiting-for-ci\` job adds \`ci-pending\`, comments, and triggers the poller → poller swaps to \`ci-ready\` when CI passes → \`publish.yml\` fires. The publish gate also requires \`label == accepted || label == ci-ready\`, so publish can never fire from the initial \`accepted\` label event alone — it must wait for the poller's \`ci-ready\` transition. This closes the race regardless of label ordering. +- **Craft .craft-publish-\.json state file is unauthenticated**: Craft's publish state file was \`.craft-publish-\.json\` in cwd, writable by any earlier CI step or committed repo file → silent target-skip → pipeline manipulation. Fixed in PR #797 (shipped in Craft 2.26.0): moved to \`$XDG_STATE_HOME/craft/publish-state-\-\-\-\.json\` via \`src/utils/publishState.ts\`. Fallback when GH config unresolved: \`publish-state-\-\.json\`. Craft warns (doesn't read) if legacy file found in cwd. Companion publish.yml PR #7886 dual-wrote both locations; legacy write removed in #7892 after 2.26.0 shipped. \`XDG_STATE_HOME=/github/workspace/.craft-state\` set on the docker step — outside \`\_\_repo\_\_/\` so repo contents can't pre-populate. sha1(cwd) disambiguates monorepo subpaths. - + -- **bun pm pack reads workspace versions from bun.lock, not package.json**: \`bun pm pack\` rewrites \`workspace:\*\` specifiers to concrete versions at pack time, reading them from \`bun.lock\` — NOT from the workspace \`package.json\`. Neither \`npm version\` nor \`bun install\` (even \`--lockfile-only\`) updates the lockfile's workspace \`version\` entries once present; only \`rm bun.lock && bun install --lockfile-only\` refreshes them. If craft bumps package.json but leaves bun.lock stale, published tarballs ship with \`dependencies\` pointing at an old never-published version, causing \`ETARGET\` on install. Fix in NpmTarget.bumpVersion: after successful bumps, regex-patch bun.lock — find each workspace's path-relative key (normalize \`\\\` → \`/\` for Windows, escape regex chars) and replace the first \`"version": "..."\` line in that block. Use \`safeFs.writeFileSync\` for dry-run safety. Idempotent; no bun binary required. +- **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. - + -- **CI poller needs release-bot token for private repo CI status checks**: The \`getsentry/publish\` CI poller's cross-repo API calls (commit status, check suites, git refs) require the \`sentry-release-bot\` app token with \`owner: getsentry\` scope — not the \`sentry-internal-app\` token. The internal app isn't installed on all getsentry repos (e.g. \`sentry-xbox\`, \`sentry-playstation\`, \`sentry-switch\`, \`service-registry\`), causing 404s on all CI status API calls. Combined with the \`gh api --jq\` stdout error leak, this silently leaves issues stuck in \`ci-pending\` indefinitely. Fix (PR #7843): added a separate \`release-bot-token\` step and a \`gh_api_release\` helper that uses \`GH_TOKEN="$RELEASE_TOKEN"\` for all cross-repo calls. +- **Craft .craft.yml discovery walks up from cwd — ancestor configs auto-load**: \`src/config.ts:findConfigFile()\` walks upward from \`cwd\` up to 1024 dirs looking for \`.craft.yml\`. Any stray \`.craft.yml\` in an ancestor (including \`$HOME\`) is loaded unconditionally and its \`preReleaseCommand\` executes. No \`--config\` flag exists to pin the path. Hardening: restrict discovery to the current git worktree root (first \`.git\` found), optionally require the file to be tracked by git, and add a \`--config \\` flag that disables the walk. Complements the \`--allow-remote-config\` gate \[\[019db09e-acae-76ae-8813-a317c0e6f6f9]] and release env sanitization \[\[019db09e-ac9b-765d-a091-bb6bb512b987]]. - + -- **GitHub App tokens expire after 1 hour — breaks long-running CI publishes**: GitHub App installation tokens expire after 1 hour (non-configurable). For publish jobs exceeding this (e.g., sentry-native's ~1h 23m symbol upload), the token expires before Craft's post-publish \`git push\` for the release branch merge. Git fails with \`could not read Username for 'https://github.com': No such device or address\` — which looks like a credential config issue but is actually token expiration. No code change in Craft alone can fix this — the \`GITHUB_TOKEN\` env var, git \`http.extraheader\`, and Octokit all use the same expired token. The real fix requires the CI workflow (\`getsentry/publish\`) to generate a fresh token after the Docker container exits, before the merge step. +- **Craft commitOnGitRepository uses execSync with string-interpolated tar path**: Craft commitOnGitRepository previously ran \`childProcess.execSync(\\\`tar -zxvf ${archivePath}${stripComponentsArg}\\\`)\` — shell string concatenation (fragile even if archivePath was Craft-constructed). Fixed in PR #799: replaced with \`tar.x({ file: archivePath, cwd: directory, strip: stripComponents })\` from the already-present \`node-tar\` dep. No shell, no string interpolation. Tests mock \`tar\` via \`vi.hoisted(() => ({ tarExtractMock: vi.fn() }))\` + \`vi.mock('tar', () => ({ x: tarExtractMock }))\` — required because ESM prevents \`vi.spyOn(tar, 'x')\` (throws 'Cannot redefine property'). - + -- **npm version --workspaces fails on workspace:\* deps but still bumps files**: \`npm version \ --workspaces --include-workspace-root\` on a monorepo with \`"workspace:\*"\` deps successfully writes the new version to every package.json, then exits non-zero with \`EUNSUPPORTEDPROTOCOL\` when validating dep URLs. Craft's \`NpmTarget.bumpVersion\` (src/targets/npm.ts:312) treats the exit code as failure and falls back to per-package bumping, which hits the same validator. Fix: on spawn failure, post-check observable state — read every package.json and if all show \`version === newVersion\`, warn (citing npm/cli#8845) and proceed. Genuine failures (bad version, perm errors) still throw because files weren't bumped. Apply the same post-check per-package inside \`bumpWorkspacePackagesIndividually\`. Guard with \`isDryRun()\` since \`spawnProcess\` no-ops in dry-run and files won't be touched. +- **Craft GPG TOCTOU: private key written to fixed /tmp path**: Craft GPG key import previously wrote \`GPG_PRIVATE_KEY\` to \`path.join(tmpdir(), 'private-key.asc')\` — predictable world-readable path vulnerable to TOCTOU via symlink races. Fixed in PR #798: \`src/utils/gpg.ts\` now pipes the key via stdin to \`gpg --batch --import\` using \`spawnProcess(cmd, args, opts, { stdin: privateKey })\`. Key never touches disk. \`spawnProcess\` already supports stdin piping (sets stdio\[0]='pipe', writes + ends stdin). - + -- **prepare-dry-run e2e tests fail without EDITOR in dumb terminals**: The 7 tests in \`src/\_\_tests\_\_/prepare-dry-run.e2e.test.ts\` fail in environments where \`TERM=dumb\` and \`EDITOR\` is unset (e.g., inside agent shells or minimal CI containers). The error is \`Terminal is dumb, but EDITOR unset\` from git commit. This is a pre-existing environment issue, not a code defect. These tests pass in normal CI (Node.js 20/22 runners) where terminal capabilities are available. +- **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. - + -- **Publish issue title monorepo suffix breaks owner/repo parsing**: Craft titles like \`publish: getsentry/relay/py@0.9.26\` have a subdirectory path suffix. Naive \`sed 's/^publish: \\(.\*\\)@.\*/\1/p'\` extracts \`getsentry/relay/py\` — causes 404 on every GitHub API call (\`repos/getsentry/relay/py/...\` isn't a valid repo). Fix: match only first two path segments: \`sed -n 's|^publish: \\(\[^/]\*/\[^/@]\*\\).\*@.\*|\1|p'\`. Applies to any workflow parsing publish issue titles (ci-poller.yml, auto-approve.yml, craft-action.yml's request-publish step). +- **Craft scripts/bump-version.sh and scripts/post-release.sh auto-run from cwd**: Craft's \`prepare.ts\` (DEFAULT_BUMP_VERSION_PATH) and \`publish.ts\` (DEFAULT_POST_RELEASE_SCRIPT_PATH) silently auto-execute \`scripts/bump-version.sh\` / \`scripts/post-release.sh\` when no explicit \`preReleaseCommand\`/\`postReleaseCommand\` is set in \`.craft.yml\`. A PR that merely adds one of these files gets executed on the next \`craft prepare\`/\`publish\` with the allowlisted release env (still includes \`GITHUB_TOKEN\`) — no \`.craft.yml\` edit required. Env sanitization from PR #794 mitigates LD_PRELOAD but not the script contents themselves. Hardening: require explicit opt-in via \`preReleaseCommand\`/\`postReleaseCommand\` in \`.craft.yml\`; drop the file-exists fallback. Related: \[\[019db09e-ac9b-765d-a091-bb6bb512b987]]. - + -- **spawnProcess no-ops in dry-run — breaks post-check logic**: \`src/utils/system.ts\` \`spawnProcess\` returns \`undefined\` immediately in dry-run mode (unless \`enableInDryRunMode\` or worktree mode is set) — it never actually spawns the child. Any logic that relies on observable side effects (files written, state changed) after a spawn will see unchanged state in dry-run. Example trap: post-checking package.json \`version\` after \`npm version\` to detect successful-but-errored bumps would incorrectly trigger fallback paths in dry-run because the spawn is skipped and files stay stale. Mitigation: guard post-check / fallback logic with \`isDryRun()\` and short-circuit to the existing dry-run success behavior. File writes don't have this problem if routed through \`safeFs.writeFileSync\` which handles dry-run natively. +- **docker://getsentry/craft:latest tag lag vs release completion**: The \`docker://getsentry/craft:latest\` tag on DockerHub advances AFTER the GitHub release completes — there's a gap of minutes between \`release/X.Y.Z\` merging and \`:latest\` pointing to the new digest. Publish runs that trigger during this window pull the PREVIOUS version. Verify the digest mapping with \`gh api /repos/getsentry/craft/.../\` or DockerHub's tags API before assuming a publish run used the newly-released Craft. The \`image.yml\` workflow on master produces this tag; check its completion timestamp against the publish run's \`docker pull\` timestamp. -### Pattern + + +- **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. - + + +- **system.ts → env.ts import creates circular dep via config.ts**: Importing \`env.ts\` from \`src/utils/system.ts\` creates a circular dep: system.ts → env.ts → config.ts → artifact_providers/github.ts → (back to) system.ts (\`extractZipArchive\`). Symptom: \`BaseArtifactProvider\` evaluates as \`undefined\` at class-extension time, crashing ~8 test files. Fix: put any helper shared between system.ts and env.ts in a leaf module with no other imports (e.g. \`src/utils/dynamicLinkerEnv.ts\`). Don't import config/env-heavy modules from system.ts. + +### Pattern -- **CI poller variable gate with dedicated app token in getsentry/publish**: \*\*CI poller variable gate with dedicated app token in getsentry/publish\*\*: The \`ci-poller.yml\` cron uses repo variable \`CI_POLLER_HAS_PENDING\` as a fast gate — \`'true'\` runs, otherwise skips. \`workflow_dispatch\` bypasses the gate. \*\*Self-dispatch for fast re-checking\*\*: when pending issues remain, the poller dispatches itself (up to 60 attempts, ~30 min cap) for ~30-60s intervals instead of relying on unreliable cron. \*\*Two tokens required\*\*: (1) \`sentry-release-bot\` (\`SENTRY_RELEASE_BOT_CLIENT_ID\`/\`SENTRY_RELEASE_BOT_PRIVATE_KEY\` with \`owner: getsentry\`) for cross-repo API calls (CI status, check suites, git refs) since sentry-internal-app isn't installed on all repos. (2) \`CI_POLLER_APP\` token for writing repo variables. \`gh issue list/edit/comment\` uses sentry-internal-app token so label changes trigger \`publish.yml\`. Key: cleanup steps use \`if: always()\`; disable steps guard on \`steps.poller-token.outcome == 'success'\`. + - +- **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 action.yml opt-in ci_ready input + signal-ready composite action**: In getsentry/craft, the \`ci_ready: 'true'\` input on \`action.yml\` makes \`craft prepare\` add a \`ci-pending\` label at issue creation (atomic with \`gh issue create --label\`), opting the repo into event-driven publishing. A separate composite action \`signal-ready/action.yml\` lets target repo CI send a \`repository_dispatch\` event of type \`ci-ready\` with \`{repo, version, sha}\` payload to the publish repo when CI passes. The publish repo's \`ci-ready.yml\` handler finds the issue by exact title match (\`publish: {repo}@{version}\`), verifies \`ci-pending\` is present, swaps labels. Auth: the dispatch sender needs a token with write access to the publish repo (sentry-release-bot app token, not \`GITHUB_TOKEN\`). + - +- **Split independent security fixes into separate PRs**: User preference: when tackling multiple independent security hardening items, open one PR per item rather than a single combined PR. Each PR self-contained, independently reviewable, and revertable, with no ordering dependency. Branch naming: \`security/\\`. Examples from this session: .craft.env / GPG stdin / node-tar / subprocess env sanitization / action pinning / publish-state move → separate PRs (#794, #797-#801) on 4+ branches off master. Companion PRs that coordinate across repos (e.g. Craft #797 + publish #7886 dual-write + publish #7892 legacy-drop) are also split by repo and sequenced deliberately. -- **Craft Docker container auth: credentials come from volume-mounted git config**: When Craft runs inside Docker in CI (via \`getsentry/craft:latest\`), git authentication comes from \`actions/checkout\`'s \`http.extraheader\` in the local \`.git/config\`, which is volume-mounted into the container at \`/github/workspace\`. The container sets \`HOME=/root\`, so global git config from the host runner isn't available. The \`GITHUB_TOKEN\` env var is passed separately. Craft's \`handleReleaseBranch()\` doesn't set up its own auth — it relies on whatever git config is present. Other targets (registry, commitOnGitRepository) explicitly inject tokens into clone URLs via \`GitHubRemote.getRemoteStringWithAuth()\` or URL manipulation. + - +- **Synthetic docker-free test for released Craft binary behavior**: When Docker pull stalls (common on flaky networks), validate a released Craft binary's behavior without containers: \`gh release download \ -p 'craft' -O /tmp/craft-\\`, create a minimal git repo with valid \`.craft.yml\` (needs \`statusProvider.name: github\`, not \`none\`), commit+tag, then \`CRAFT_LOG_LEVEL=Debug GITHUB_TOKEN=... craft publish --no-status-check --no-merge \\`. Pre-seed state files at paths computed from \`sha1(cwd)\[:12]\` — local cwd hash differs from container's \`/github/workspace/\_\_repo\_\_\` hash, compute fresh. Log lines like \`Found publish state file, resuming from there...\` prove the read path works without production runs. -- **Craft workspace discovery supports npm/yarn/pnpm uniformly**: \`src/utils/workspaces.ts\` \`discoverWorkspaces(rootDir)\` returns \`{type: 'npm'|'yarn'|'pnpm'|'none', packages: WorkspacePackage\[]}\`. Tries pnpm-workspace.yaml first (more specific), then package.json \`workspaces\` field (array or \`{packages: \[]}\` object form). npm vs yarn is distinguished by presence of \`yarn.lock\`. Each \`WorkspacePackage\` carries \`name\`, absolute \`location\`, \`private\`, \`hasPublicAccess\` (publishConfig.access === 'public'), and \`workspaceDependencies\` (deps that reference other workspace packages by name — across dependencies/peer/optional, not dev). \`NpmTarget.expand\` uses this to auto-generate per-package target configs, filters private packages, validates public→private deps, and topologically sorts via \`topologicalSortPackages\` (depth-based, detects cycles). Artifact filenames generated via \`packageNameToArtifactPattern\` or a user \`artifactTemplate\` with \`{{name}}\`/\`{{simpleName}}\`/\`{{version}}\`. + - +- **Synthetic end-to-end test for Craft binary behavior without Docker**: When Docker pull fails or is slow, validate a released Craft binary's behavior by downloading the raw bundled binary from the GitHub release (\`gh release download \ -p 'craft' -O /tmp/craft-\\`), creating a minimal git repo with a valid \`.craft.yml\` (need \`statusProvider.name: github\`, not \`none\`), committing + tagging, then running with \`CRAFT_LOG_LEVEL=Debug GITHUB_TOKEN=... craft publish --no-status-check --no-merge \\`. Pre-seed state files at paths Craft computes from \`sha1(cwd)\[:12]\` — the local cwd hash differs from the container's \`/github/workspace/\_\_repo\_\_\` hash, so compute it fresh. Log lines like \`Found publish state file, resuming from there...\` prove the read path works without needing a production run. -- **getsentry/publish: label-based state machine for publish gating**: getsentry/publish label state machine (post-PR #7881 + retry-race fix): \`accepted\` (human/auto-approve, added first), \`ci-pending\` (added by \`waiting-for-ci\` in publish.yml AFTER \`accepted\`), \`ci-ready\` (added by poller when CI passes), \`ci-failed\` (added by poller on failure, also removes \`accepted\`). \*\*Publish gate fires ONLY on \`ci-ready\` label event\*\* — not \`accepted\` — to prevent race where \`publish\` and \`waiting-for-ci\` run in parallel on the same \`labeled: accepted\` event when \`ci-ready\` is stale from a previous failed publish. \`waiting-for-ci\` job (on \`accepted\` labeled, when \`ci-pending\` or \`ci-failed\` present): removes \`ci-failed\` AND \`ci-ready\` (critical: stale \`ci-ready\` must be cleared so poller's re-add generates a fresh \`labeled\` event), adds \`ci-pending\`, comments, triggers poller. Concurrency group uses \`github.event.issue.title\` to lock on repo@version. +### Preference - + -- **GitHub App token scoping for cross-repo CI status checks**: When a workflow in repo A needs to call APIs on repo B, \`secrets.GITHUB_TOKEN\` won't work (scoped to repo A only). Use \`actions/create-github-app-token@v3\` with \`owner: \\` to get a token with access to all org repos (app must be installed org-wide). In \`getsentry/publish\`, the CI poller uses TWO tokens: \`sentry-release-bot\` (installed on all getsentry repos) for cross-repo CI status calls, and \`sentry-internal-app\` for label/comment operations on the publish repo itself. Separate tokens because some repos don't have sentry-internal-app installed. +- **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. - + -- **Use release-bot token for cross-repo API calls in getsentry org workflows**: The \`sentry-internal-app\` is NOT installed on all getsentry repos (e.g. \`sentry-xbox\`, \`sentry-playstation\`, \`sentry-switch\`, \`service-registry\`). Using its token for cross-repo API calls like \`gh api repos/{other-repo}/commits/...\` returns 404 "Not Found" — looks like a missing resource but is actually a missing installation. Use \`sentry-release-bot\` instead: configure with \`client-id: ${{ vars.SENTRY\_RELEASE\_BOT\_CLIENT\_ID }}\`, \`private-key: ${{ secrets.SENTRY\_RELEASE\_BOT\_PRIVATE\_KEY }}\`, and \`owner: getsentry\` to get a token with access to all org repos. Pattern in getsentry/publish CI poller: generate both tokens, use internal-app for publish-repo label changes, release-bot for cross-repo \`repos/{repo}/commits/{sha}/status\` and \`/check-runs\` API calls. +- **Pin only third-party GitHub Actions — skip GitHub/Sentry owned**: User scope for \`uses:\` SHA-pinning in \`.github/workflows/\*.yml\`: pin ONLY non-GitHub, non-Sentry actions. Skip \`actions/\*\` (GitHub-owned), \`getsentry/\*\` (Sentry-owned), local \`./\` and local reusable workflows. As of PR #801, pinned third-parties are \`pnpm/action-setup\` and \`rossjrw/pr-preview-action\`. Include a \`# vX.Y.Z\` trailing comment next to each SHA for reviewer readability. Resolve SHAs via \`git ls-remote\` + \`^{}\` deref for annotated tags. diff --git a/src/commands/prepare.ts b/src/commands/prepare.ts index 50b4f488..15ecfd91 100644 --- a/src/commands/prepare.ts +++ b/src/commands/prepare.ts @@ -452,9 +452,9 @@ async function runCustomPreReleaseCommand( // 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. + // allowlisted env plus the CRAFT_* version vars to avoid leaking + // secrets into a potentially malicious command. See + // src/utils/releaseCommandEnv.ts for the exact allowlist and rationale. await spawnProcess(sysCommand, args, { env: buildReleaseCommandEnv({ CRAFT_NEW_VERSION: newVersion, diff --git a/src/utils/releaseCommandEnv.ts b/src/utils/releaseCommandEnv.ts index e6b958fc..d7bfb12e 100644 --- a/src/utils/releaseCommandEnv.ts +++ b/src/utils/releaseCommandEnv.ts @@ -40,20 +40,30 @@ const ALLOWED_ENV_VARS = [ * * - `GITHUB_*` — context metadata that GitHub Actions injects into every * step (`GITHUB_REPOSITORY`, `GITHUB_SHA`, `GITHUB_REF`, `GITHUB_RUN_ID`, - * `GITHUB_ACTOR`, `GITHUB_WORKFLOW`, `GITHUB_API_URL`, `GITHUB_TOKEN`, - * ...). User release scripts commonly read these (e.g. sentry-cocoa's - * bump script stamps `GITHUB_RUN_ID` into a file for a follow-up step). - * `GITHUB_TOKEN` is a credential but is already the single-most-common - * thing release scripts need; pretending it's not in scope would force - * every consumer to proxy it via `CRAFT_*`. + * `GITHUB_ACTOR`, `GITHUB_WORKFLOW`, `GITHUB_API_URL`, ...). User release + * scripts commonly read these (e.g. sentry-cocoa's bump script stamps + * `GITHUB_RUN_ID` into a file for a follow-up step). The namespace also + * contains credentials — `GITHUB_TOKEN` and `GITHUB_API_TOKEN` (used as + * a ghcr.io / cross-repo fallback in `src/targets/docker.ts`, + * `src/targets/commitOnGitRepository.ts`, and `src/utils/githubApi.ts`). + * Both are forwarded by this prefix match. This is an intentional + * trade-off: a malicious `preReleaseCommand` that already has shell + * execution could exfiltrate whichever of the two is set, but the + * alternative (splitting context and credential by explicit lists) + * would force every consumer to proxy routine Actions metadata via + * `CRAFT_*`, which is the ergonomic regression that #807 exists to + * fix. Also note: `GITHUB_ENV`, `GITHUB_PATH`, `GITHUB_OUTPUT`, and + * `GITHUB_STEP_SUMMARY` point at runner files that subsequent workflow + * steps read — forwarding them lets a compromised release command + * influence later steps. The attacker already has shell exec in the + * same docker mount as those files, so no new primitive is granted. * - `RUNNER_*` — non-secret runner metadata (`RUNNER_OS`, `RUNNER_ARCH`, * `RUNNER_TEMP`, ...) with the same ergonomic justification. * - * These prefixes do NOT cover credential env vars (`NPM_TOKEN`, - * `CRATES_IO_TOKEN`, `DOCKER_PASSWORD`, `GPG_PRIVATE_KEY`, - * `AWS_SECRET_ACCESS_KEY`, `TWINE_PASSWORD`, ...) which are all named - * outside the `GITHUB_` / `RUNNER_` namespaces by convention, so the - * prefix allowlist does not expand the credential-leak surface. + * These prefixes do NOT cover credential env vars named with unrelated + * prefixes (`NPM_TOKEN`, `CRATES_IO_TOKEN`, `DOCKER_PASSWORD`, + * `GPG_PRIVATE_KEY`, `AWS_SECRET_ACCESS_KEY`, `TWINE_PASSWORD`, ...) — + * those remain blocked by the allowlist. */ const ALLOWED_ENV_VAR_PREFIXES = ['GITHUB_', 'RUNNER_'] as const; @@ -61,13 +71,20 @@ const ALLOWED_ENV_VAR_PREFIXES = ['GITHUB_', 'RUNNER_'] as const; * Builds the environment for a user-defined release command subprocess. * * The returned env contains only: - * - every key in {@link ALLOWED_ENV_VARS}, + * - every key in {@link ALLOWED_ENV_VARS} (values from `process.env`, + * which may be `undefined`), * - every key in `process.env` that starts with one of - * {@link ALLOWED_ENV_VAR_PREFIXES}, - * - any caller-supplied `extras` (e.g. `CRAFT_NEW_VERSION`). + * {@link ALLOWED_ENV_VAR_PREFIXES}; prefix-match keys are only + * included when actually present on `process.env`, + * - any caller-supplied `extras` (e.g. `CRAFT_NEW_VERSION`), which + * override values from the above buckets. * - * Undefined values are preserved (Node's `spawn` treats `undefined` as - * "unset" rather than the string "undefined"). + * Node's `child_process.spawn` treats an `undefined` value and an + * absent key identically — both result in the variable being unset in + * the child. Callers inspecting the returned object via `Object.keys` + * should be aware that exact-match keys are always present (even with + * `undefined` values) while prefix-match keys are only present when + * set. * * @param extras Additional key/value pairs to include in the child env. * @returns A fresh env object safe to pass to `spawn` / `spawnProcess`.