diff --git a/AGENTS.md b/AGENTS.md index d7816dad1..b98541a9f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,6 +122,10 @@ Some operations need explicit `isDryRun()` checks: - **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 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. + - **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. @@ -138,14 +142,34 @@ Some operations need explicit `isDryRun()` checks: ### Gotcha + + +- **Craft .craft-publish-\.json state file is unauthenticated**: \`publish.ts\` reads/writes \`.craft-publish-\.json\` in \`cwd\` to skip already-published targets. Any earlier CI step or committed file can pre-mark targets as published → silent skip → pipeline manipulation. The official \`getsentry/publish\` workflow (publish.yml \`Set targets\` step) \*pre-seeds\* this file before invoking craft to exclude unchecked targets from the publish issue, so any fix must preserve that pre-seed path. Planned fix: move state to \`$XDG_STATE_HOME/craft/publish-state-\-\-\-\.json\` (HOME=/root in the publish Docker image, writable by workflow, not by repo contents). Rollout: dual read/write in publish workflow first → release craft using new-only location → drop dual R/W. sha1(cwd) disambiguates monorepo subpaths. + - **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.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]]. + + + +- **Craft commitOnGitRepository uses execSync with string-interpolated tar path**: \`src/targets/commitOnGitRepository.ts\` (~line 171) runs \`\` childProcess.execSync(\`tar -zxvf ${archivePath}${stripComponentsArg}\`) \`\` — shell string concatenation. \`archivePath\` is currently Craft-constructed so injection isn't exploitable today, but the pattern is fragile against future refactors. Fix: switch to \`spawnSync('tar', \['-zxvf', archivePath, ...stripComponentsArgs])\` to avoid shell parsing entirely. + + + +- **Craft GPG TOCTOU: private key written to fixed /tmp path**: \`src/utils/gpg.ts\` writes \`GPG_PRIVATE_KEY\` to \`path.join(tmpdir(), 'private-key.asc')\` — a fixed, predictable path in world-readable \`/tmp\`. Coresident processes can race with a symlink to redirect the write, or read the file between \`writeFile\` and \`unlink\`. Fix: use \`mkdtemp('craft-gpg-')\` for a per-invocation directory (mode 0700), or pipe the key via stdin to \`gpg --import --batch\` and never touch disk. + - **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 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]]. + - **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. diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 44ffc82f4..a2e5f9fc0 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -1,9 +1,9 @@ import { Arguments, Argv, CommandBuilder } from 'yargs'; import chalk from 'chalk'; -import { existsSync, readFileSync } from 'fs'; +import { existsSync, mkdirSync, readFileSync } from 'fs'; import { safeFs } from '../utils/dryRun'; -import { join } from 'path'; +import { dirname, join } from 'path'; import shellQuote from 'shell-quote'; import stringLength from 'string-length'; @@ -47,6 +47,7 @@ import { } from '../utils/git'; import { withTracing } from '../utils/tracing'; import { buildReleaseCommandEnv } from '../utils/releaseCommandEnv'; +import { getPublishStatePath } from '../utils/publishState'; /** Default path to post-release script, relative to project root */ const DEFAULT_POST_RELEASE_SCRIPT_PATH = join('scripts', 'post-release.sh'); @@ -670,8 +671,40 @@ export async function publishMain(argv: PublishOptions): Promise { // Expand any npm workspace targets into individual package targets let targetConfigList = await expandWorkspaceTargets(config.targets || []); + // Resolve the GitHub config up front so we can key the publish-state + // file by owner/repo. `getGlobalGitHubConfig()` returns cached data on + // subsequent calls, so this is effectively free. + let publishStateGithubConfig = null; + try { + publishStateGithubConfig = await getGlobalGitHubConfig(); + } catch { + // Fall through with null — getPublishStatePath() handles this by + // falling back to a cwd-hash-only filename, keeping the file in + // $XDG_STATE_HOME/craft/ rather than the repo. + } + const publishStateFile = getPublishStatePath( + newVersion, + publishStateGithubConfig, + ); + logger.info(`Looking for publish state file for ${newVersion}...`); - const publishStateFile = `.craft-publish-${newVersion}.json`; + logger.debug(`Publish state file path: ${publishStateFile}`); + + // Warn when a file at the legacy cwd location is detected. We never + // read it (see security/move-publish-state-to-xdg): repo-contents are + // attacker-influenceable via PRs and could pre-populate the "already + // published" set. Users / workflows that were writing to the legacy + // path need to migrate to $XDG_STATE_HOME/craft/. + const legacyStateFile = `.craft-publish-${newVersion}.json`; + if (existsSync(legacyStateFile)) { + logger.warn( + `Found legacy publish state file at "${legacyStateFile}" in the project directory. ` + + `This file is no longer read for security reasons. ` + + `Craft now stores publish state at "${publishStateFile}". ` + + `If you were pre-seeding published targets, update your workflow to write to the new location.`, + ); + } + const earlierStateExists = existsSync(publishStateFile); let publishState: PublishState; if (earlierStateExists) { @@ -715,6 +748,11 @@ export async function publishMain(argv: PublishOptions): Promise { await withTempDir(async (downloadDirectory: string) => { artifactProvider.setDownloadDirectory(downloadDirectory); + // Ensure the state directory exists. `mkdirSync` with + // `recursive: true` is idempotent, so this is safe on resumed + // runs where the directory was already created. + mkdirSync(dirname(publishStateFile), { recursive: true }); + // Publish to all targets for (const target of targetList) { await publishToTarget(target, newVersion, revision); diff --git a/src/utils/__tests__/publishState.test.ts b/src/utils/__tests__/publishState.test.ts new file mode 100644 index 000000000..c2ff6f7cd --- /dev/null +++ b/src/utils/__tests__/publishState.test.ts @@ -0,0 +1,137 @@ +import { describe, test, expect, beforeEach, afterEach } from 'vitest'; +import { homedir } from 'os'; +import { join } from 'path'; + +import { + getCraftStateDir, + getPublishStateFilename, + getPublishStatePath, +} from '../publishState'; + +describe('publishState', () => { + const savedEnv = { ...process.env }; + + beforeEach(() => { + delete process.env.XDG_STATE_HOME; + }); + + afterEach(() => { + process.env = { ...savedEnv }; + }); + + describe('getCraftStateDir', () => { + test('defaults to $HOME/.local/state/craft when XDG_STATE_HOME is unset', () => { + expect(getCraftStateDir()).toBe( + join(homedir(), '.local', 'state', 'craft'), + ); + }); + + test('honours XDG_STATE_HOME when set', () => { + process.env.XDG_STATE_HOME = '/var/lib/ci-state'; + expect(getCraftStateDir()).toBe('/var/lib/ci-state/craft'); + }); + + test('falls back to HOME when XDG_STATE_HOME is empty string', () => { + process.env.XDG_STATE_HOME = ''; + expect(getCraftStateDir()).toBe( + join(homedir(), '.local', 'state', 'craft'), + ); + }); + }); + + describe('getPublishStateFilename', () => { + const cwd = '/workspace/repo'; + + test('includes owner, repo, short cwd hash, and version', () => { + const name = getPublishStateFilename( + '1.2.3', + { owner: 'getsentry', repo: 'craft' }, + cwd, + ); + expect(name).toMatch( + /^publish-state-getsentry-craft-[0-9a-f]{12}-1\.2\.3\.json$/, + ); + }); + + test('disambiguates monorepo subpaths via cwd hash', () => { + const a = getPublishStateFilename( + '1.2.3', + { owner: 'o', repo: 'r' }, + '/workspace/repo/packages/foo', + ); + const b = getPublishStateFilename( + '1.2.3', + { owner: 'o', repo: 'r' }, + '/workspace/repo/packages/bar', + ); + expect(a).not.toBe(b); + }); + + test('sanitises owner/repo/version characters', () => { + const name = getPublishStateFilename( + '1.2.3+build/hack$', + { owner: 'Weird Owner', repo: 'Re po!' }, + cwd, + ); + // No slashes, no plus, no dollar — all collapsed to underscores. + expect(name).not.toMatch(/[/$+!]/); + expect(name).toMatch(/^publish-state-weird_owner-re_po-[0-9a-f]{12}-/); + }); + + test('falls back to sha256(cwd)-only filename when github config is null', () => { + const name = getPublishStateFilename('1.2.3', null, cwd); + expect(name).toMatch(/^publish-state-[0-9a-f]{16}-1\.2\.3\.json$/); + }); + + test('fallback filename also disambiguates by cwd', () => { + const a = getPublishStateFilename('1.0.0', null, '/a'); + const b = getPublishStateFilename('1.0.0', null, '/b'); + expect(a).not.toBe(b); + }); + + test('same inputs produce stable filenames', () => { + const a = getPublishStateFilename( + '1.2.3', + { owner: 'o', repo: 'r' }, + cwd, + ); + const b = getPublishStateFilename( + '1.2.3', + { owner: 'o', repo: 'r' }, + cwd, + ); + expect(a).toBe(b); + }); + }); + + describe('getPublishStatePath', () => { + test('joins state dir + filename', () => { + process.env.XDG_STATE_HOME = '/var/state'; + const path = getPublishStatePath( + '1.2.3', + { owner: 'o', repo: 'r' }, + '/workspace/repo', + ); + expect(path.startsWith('/var/state/craft/')).toBe(true); + expect(path.endsWith('-1.2.3.json')).toBe(true); + }); + + test('does not place the state file inside cwd', () => { + // This is the crucial security property: the file must NEVER + // live inside the repository being published, because repo + // contents are attacker-influenceable via PRs. + const cwd = '/workspace/untrusted-repo'; + process.env.XDG_STATE_HOME = '/var/state'; + const path = getPublishStatePath('1.2.3', { owner: 'o', repo: 'r' }, cwd); + expect(path.startsWith(cwd)).toBe(false); + }); + + test('does not place fallback state file inside cwd', () => { + // Same property must hold when github config is unavailable. + const cwd = '/workspace/untrusted-repo'; + process.env.XDG_STATE_HOME = '/var/state'; + const path = getPublishStatePath('1.2.3', null, cwd); + expect(path.startsWith(cwd)).toBe(false); + }); + }); +}); diff --git a/src/utils/publishState.ts b/src/utils/publishState.ts new file mode 100644 index 000000000..a3e59dc7b --- /dev/null +++ b/src/utils/publishState.ts @@ -0,0 +1,110 @@ +/** + * Helpers for locating Craft's publish-state file in a path that is NOT + * writable by the repository being published. + * + * Background: `craft publish` writes a small JSON file listing targets + * that have completed so a resumed run can skip them. Before this module + * existed, the file lived at `.craft-publish-.json` in the + * project's cwd. That path is inside the repository checkout, so any + * committed file at the same path (or any earlier CI step) could + * pre-populate the "published" set and trick Craft into silently + * skipping targets. + * + * The file now lives under `$XDG_STATE_HOME/craft/` (falling back to + * `$HOME/.local/state/craft/`). The filename is keyed on + * owner, repo, a hash of cwd (to disambiguate monorepo subpaths), and + * the version being published. `getsentry/publish` runs inside a Docker + * image with `HOME=/root`, so the XDG state dir is a clean, + * workflow-writable location that committed repo contents cannot reach. + */ + +import { createHash } from 'crypto'; +import { homedir } from 'os'; +import { join } from 'path'; + +import type { GitHubGlobalConfig } from '../schemas/project_config'; + +const STATE_DIR_NAME = 'craft'; + +/** + * Resolves `$XDG_STATE_HOME/craft/` with the standard fallback to + * `$HOME/.local/state/craft/` when `XDG_STATE_HOME` is unset. + * + * Exported for tests and for the publish workflow helper (see + * `scripts/print-publish-state-path.ts` if present) that needs to + * compute the same path. + */ +export function getCraftStateDir(): string { + const xdgStateHome = process.env.XDG_STATE_HOME; + if (xdgStateHome && xdgStateHome.length > 0) { + return join(xdgStateHome, STATE_DIR_NAME); + } + return join(homedir(), '.local', 'state', STATE_DIR_NAME); +} + +/** + * Sanitises a string for inclusion in a filename: lowercases, replaces + * any character outside `[a-z0-9._-]` with `_`, and collapses runs. + * Owner/repo names are restricted by GitHub to `[A-Za-z0-9._-]` so this + * is mostly belt-and-braces. + */ +function sanitiseForFilename(raw: string): string { + return raw + .toLowerCase() + .replace(/[^a-z0-9._-]+/g, '_') + .replace(/^_+|_+$/g, ''); +} + +/** + * Short (12-char) hex digest of the absolute cwd path. Used to + * disambiguate monorepo subpaths so `packages/foo` and `packages/bar` + * get separate state files even at the same version. + */ +function shortCwdHash(cwd: string): string { + return createHash('sha1').update(cwd).digest('hex').slice(0, 12); +} + +/** + * Builds the filename for the publish-state file. + * + * With a resolvable GitHub config: + * `publish-state----.json` + * + * Without GitHub config (offline / non-GitHub test harnesses) the + * filename falls back to a cwd-hash-only form so Craft still refuses + * to write into the repo itself: + * `publish-state--.json` + */ +export function getPublishStateFilename( + version: string, + githubConfig: GitHubGlobalConfig | null, + cwd: string = process.cwd(), +): string { + const safeVersion = sanitiseForFilename(version); + if (githubConfig) { + const owner = sanitiseForFilename(githubConfig.owner); + const repo = sanitiseForFilename(githubConfig.repo); + return `publish-state-${owner}-${repo}-${shortCwdHash(cwd)}-${safeVersion}.json`; + } + const cwdDigest = createHash('sha256').update(cwd).digest('hex').slice(0, 16); + return `publish-state-${cwdDigest}-${safeVersion}.json`; +} + +/** + * Full absolute path to the publish-state file for the given version. + * + * @param version The version being published. + * @param githubConfig Resolved GitHub owner/repo (may be null when + * Craft is running outside a GitHub context). + * @param cwd Override cwd; defaults to `process.cwd()`. Used by tests. + */ +export function getPublishStatePath( + version: string, + githubConfig: GitHubGlobalConfig | null, + cwd: string = process.cwd(), +): string { + return join( + getCraftStateDir(), + getPublishStateFilename(version, githubConfig, cwd), + ); +}