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.
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).