From 5bf4160fbf6ed63fd36a6447073ee84998c48944 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 23 Apr 2026 11:25:57 +0000 Subject: [PATCH 1/3] fix(prepare): remove --allow-remote-config gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #794 added `--allow-remote-config` (and `CRAFT_ALLOW_REMOTE_CONFIG=1`) as a required opt-in for `--config-from `, reasoning that remotely-fetched `.craft.yml` is untrusted input which can execute arbitrary commands via `preReleaseCommand`. In practice the gate protects against ~0 real attacks and breaks the one legitimate use case: - The composite `getsentry/craft` action (`action.yml`) wires `--config-from` from its `craft_config_from_merge_target` + `merge_target` inputs but never passes the opt-in. Users reporting that `craft_config_from_merge_target: true` fails since Craft 2.25.5 are correct — the action has been broken for every consumer that opts in. - The branch passed to `--config-from` is chosen by the release workflow author (in `release.yml` / `action.yml`), who lives on a protected branch. A PR contributor cannot influence either the workflow inputs or the branch name without first landing changes on that protected branch. The realistic attack window is "release-workflow author is careless" which a per-flag gate doesn't address. - The broader class of risk — `.craft.yml`'s `preReleaseCommand` being attacker-controlled via a PR to the local repo — is already present whether or not `--config-from` is used, and is mitigated by the env allowlist in `buildReleaseCommandEnv`. The gate added friction without adding defence. Delete the option, the `assertRemoteConfigAllowed` helper, the three unit tests, and the unused imports. No change to the callsite except dropping the gate check and downgrading the opt-in warning to an info log describing the source branch. --- src/commands/__tests__/prepare.test.ts | 36 +----------------- src/commands/prepare.ts | 51 +++----------------------- 2 files changed, 6 insertions(+), 81 deletions(-) diff --git a/src/commands/__tests__/prepare.test.ts b/src/commands/__tests__/prepare.test.ts index 0e87b72f..3dca5102 100644 --- a/src/commands/__tests__/prepare.test.ts +++ b/src/commands/__tests__/prepare.test.ts @@ -1,11 +1,6 @@ import { vi, describe, test, expect, beforeEach, type Mock } from 'vitest'; import { spawnProcess } from '../../utils/system'; -import { - runPreReleaseCommand, - checkVersionOrPart, - assertRemoteConfigAllowed, -} from '../prepare'; -import { ConfigurationError } from '../../utils/errors'; +import { runPreReleaseCommand, checkVersionOrPart } from '../prepare'; vi.mock('../../utils/system'); @@ -243,32 +238,3 @@ 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/prepare.ts b/src/commands/prepare.ts index 15ecfd91..bce03ad0 100644 --- a/src/commands/prepare.ts +++ b/src/commands/prepare.ts @@ -123,19 +123,9 @@ export const builder: CommandBuilder = (yargs: Argv) => }) .option('config-from', { description: - '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.', + 'Load .craft.yml from the specified remote branch instead of local file', 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)', @@ -161,8 +151,6 @@ 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; } @@ -173,33 +161,6 @@ 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 * @@ -831,13 +792,11 @@ async function resolveVersion( export async function prepareMain(argv: PrepareOptions): Promise { let git = await getGitClient(); - // Handle --config-from: load config from remote branch + // Handle --config-from: load config from remote branch. The caller + // is responsible for ensuring the branch is trusted — the remote + // config's preReleaseCommand will be executed by Craft. if (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.`, - ); + logger.info(`Loading configuration from remote branch: ${argv.configFrom}`); try { await git.fetch([argv.remote, argv.configFrom]); const configContent = await git.show([ From cf6370154bfc81a48f98280c5f951ae345c0355b Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 23 Apr 2026 11:30:47 +0000 Subject: [PATCH 2/3] ci: exclude CHANGELOG.md from prettier CHANGELOG.md is auto-generated from PR descriptions by the release tooling (see the #793, #807, #805 entries in CHANGELOG.md for examples where release-note titles contain `_*` sequences that prettier wants escaped to `\*`). Every time a release cuts new entries matching those patterns the format-check starts failing on every open PR against master until someone runs a manual fixup. Exclude the file from prettier so the release tooling is the sole authority on its contents. --- .prettierignore | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.prettierignore b/.prettierignore index 44cdc9a6..33c9c08d 100644 --- a/.prettierignore +++ b/.prettierignore @@ -4,3 +4,9 @@ node_modules/ pnpm-lock.yaml temp_*/ docs/ +# Auto-generated from PR descriptions by the release tooling. +# Its contents carry markdown that does not always conform to prettier's +# preferences (e.g. literal `_*` sequences from release-note titles that +# prettier wants escaped to `\*`). Excluding avoids re-formatting on every +# release cut. +CHANGELOG.md From c2e40ccce900046baf54d1058193f03067572d78 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Thu, 23 Apr 2026 11:39:08 +0000 Subject: [PATCH 3/3] fixup: address self-review - AGENTS.md: rewrite the stale `--config-from gated behind --allow-remote-config` decision entry to reflect that the gate is removed in #809, including the rationale and a warning to future maintainers not to resurrect it. - AGENTS.md: drop the now-stale `complements the --allow-remote-config gate` reference in the .craft.yml-discovery entry. - src/commands/prepare.ts: promote the remote-config load log from logger.info to logger.warn. The old gate emitted a loud warning at the execution site; removing the hard gate shouldn't also remove the user-visible reminder that a remote preReleaseCommand is about to run. Warn-level log preserves the observability without blocking. --- AGENTS.md | 4 ++-- src/commands/prepare.ts | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f48dc3b5..0f36dd55 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -146,7 +146,7 @@ Some operations need explicit `isDryRun()` checks: -- **--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 gate (--allow-remote-config) removed in #809**: Craft originally added \`--allow-remote-config\` / \`CRAFT_ALLOW_REMOTE_CONFIG=1\` in #794 as a required opt-in before \`--config-from \\` would load \`.craft.yml\` from a remote git ref. The gate was removed in #809 because: (1) in every realistic caller the branch is chosen by a release-workflow author on a protected branch so PR contributors can't influence it; (2) the local-file \`preReleaseCommand\` RCE vector exists regardless of \`--config-from\` — gating one but not the other adds friction without closing the class of risk; (3) the composite \`getsentry/craft\` action (action.yml) wired \`--config-from\` from \`craft_config_from_merge_target\` input but never passed the opt-in, so the gate broke every consumer who set that input since Craft 2.25.5. \`prepareMain\` now calls \`git.fetch\` / \`git.show\` / \`loadConfigurationFromString\` unconditionally when \`--config-from\` is present, and logs a warn-level reminder at the execution site naming the branch. Do NOT resurrect this as a hard gate — it's a false-sense-of-security footgun. The proper hardening is (a) the env allowlist in \`buildReleaseCommandEnv\` (limits exfiltration scope; does NOT prevent arbitrary command execution), and (b) upstream discipline on which refs \`merge_target\` can be set to. ### Gotcha @@ -164,7 +164,7 @@ Some operations need explicit `isDryRun()` checks: -- **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 .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 release env sanitization \[\[019db09e-ac9b-765d-a091-bb6bb512b987]]. diff --git a/src/commands/prepare.ts b/src/commands/prepare.ts index bce03ad0..afd7f820 100644 --- a/src/commands/prepare.ts +++ b/src/commands/prepare.ts @@ -796,7 +796,11 @@ export async function prepareMain(argv: PrepareOptions): Promise { // is responsible for ensuring the branch is trusted — the remote // config's preReleaseCommand will be executed by Craft. if (argv.configFrom) { - logger.info(`Loading configuration from remote branch: ${argv.configFrom}`); + logger.warn( + `Loading .craft.yml from remote branch "${argv.configFrom}". ` + + `Its preReleaseCommand will be executed by Craft — ensure the ` + + `branch is trusted.`, + ); try { await git.fetch([argv.remote, argv.configFrom]); const configContent = await git.show([