Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ Some operations need explicit `isDryRun()` checks:

### Architecture

<!-- lore:019db09e-ac9b-765d-a091-bb6bb512b987 -->

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

<!-- lore:019cb31a-14ce-7892-b22a-0327cfcebc13 -->

- **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.
Expand All @@ -126,8 +130,18 @@ Some operations need explicit `isDryRun()` checks:

- **Registry target: urlTemplate generates artifact download URLs in manifest**: \`urlTemplate\` in the registry target config generates download URLs for release artifacts in the registry manifest's \`files\` field. Uses Mustache rendering with variables \`{{version}}\`, \`{{file}}\`, \`{{revision}}\`. Primarily useful for apps (standalone binaries) and CDN-hosted assets — SDK packages published to public registries (npm, PyPI, gem) typically don't need it. If neither \`urlTemplate\` nor \`checksums\` is configured, Craft skips adding file data entirely (warns at \`registry.ts:341-349\`). Real-world pattern: \`https://downloads.sentry-cdn.com/\<product>/{{version}}/{{file}}\`.

### Decision

<!-- lore:019db09e-acae-76ae-8813-a317c0e6f6f9 -->

- **--config-from gated behind --allow-remote-config**: Craft's \`prepare --config-from \<branch>\` 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).

### Gotcha

<!-- lore:019db09e-aca8-7a81-b2f7-e117be50e02a -->

- **Craft .craft.env file reading removed — security hazard via LD_PRELOAD**: Craft used to hydrate \`process.env\` from \`$HOME/.craft.env\` and \`\<config-dir>/.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.

<!-- lore:019d9a8f-c76e-7716-b1ca-7546635fecc0 -->

- **Craft postReleaseCommand env vars pollute shared bump-version scripts**: Craft's \`runPostReleaseCommand\` sets \`CRAFT_NEW_VERSION=\<released-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.
Expand All @@ -145,4 +159,10 @@ Some operations need explicit `isDryRun()` checks:
<!-- lore:019d8c2f-ddaf-72f0-96db-44dd54bd56b8 -->

- **Craft/Publish release flow: prepare then accept publish issue**: Craft's release flow is two-phase: (1) Run \`release.yml\` GitHub Action with version "auto" — this runs \`craft prepare\`, auto-determines version from commits, creates the \`release/X.Y.Z\` branch, and opens a publish issue on \`getsentry/publish\` repo (e.g. \`publish: getsentry/craft@2.25.3\`). (2) Add the \`accepted\` label to that publish issue to trigger the actual publish pipeline. Do NOT manually create release branches — always use the workflow. The publish issue URL is emitted in the release job logs as a \`::notice::Created publish request:\` line. The publish repo is configured via \`PUBLISH_REPO\` (defaults to \`getsentry/publish\`).

### Preference

<!-- lore:019db09e-acb5-733a-9527-b80fe9f32b0d -->

- **CHANGELOG.md is auto-managed — do not edit manually**: Craft's CHANGELOG.md is auto-generated from PR descriptions by the release pipeline. Do NOT add entries manually, even for breaking changes. The user will reject such edits. Describe breaking changes in the PR body instead; the auto-managed process surfaces them in the changelog.
<!-- End lore-managed section -->
16 changes: 2 additions & 14 deletions docs/src/content/docs/getting-started.md
Original file line number Diff line number Diff line change
Expand Up @@ -242,21 +242,9 @@ Dry-run still requires `GITHUB_TOKEN` for commands that fetch PR information fro

Since Craft relies heavily on GitHub, set the `GITHUB_TOKEN` environment variable to a [GitHub Personal Access Token](https://github.com/settings/tokens) with `repo` scope.

### Environment Files
### Environment Variables

Craft reads configuration from these locations (in order of precedence):

1. `$HOME/.craft.env`
2. `$PROJECT_DIR/.craft.env`
3. Shell environment

Example `.craft.env`:

```shell
# ~/.craft.env
GITHUB_TOKEN=token123
export NUGET_API_TOKEN=abcdefgh
```
Credentials and other configuration (`GITHUB_TOKEN`, `NPM_TOKEN`, etc.) must be provided directly via your shell or CI environment. Craft no longer reads `.craft.env` files — if one is found in your home directory or project directory, Craft will emit a warning at startup. Please migrate any values stored there to your shell or CI configuration.

## Caveats

Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
"mustache": "3.0.1",
"nock": "^13.2.4",
"node-fetch": "^2.6.1",
"nvar": "1.3.1",
"ora": "5.4.0",
"prettier": "^3.4.2",
"prompts": "2.4.1",
Expand Down
9 changes: 0 additions & 9 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

117 changes: 102 additions & 15 deletions src/commands/__tests__/prepare.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { vi, describe, test, expect, beforeEach, type Mock } from 'vitest';
import { spawnProcess } from '../../utils/system';
import { runPreReleaseCommand, checkVersionOrPart } from '../prepare';
import {
runPreReleaseCommand,
checkVersionOrPart,
assertRemoteConfigAllowed,
} from '../prepare';
import { ConfigurationError } from '../../utils/errors';

vi.mock('../../utils/system');

Expand All @@ -10,6 +15,18 @@ describe('runPreReleaseCommand', () => {
const rootDir = process.cwd();
const mockedSpawnProcess = spawnProcess as Mock;

const expectedBaseEnv = () => ({
PATH: process.env.PATH,
GITHUB_TOKEN: process.env.GITHUB_TOKEN,
HOME: process.env.HOME,
USER: process.env.USER,
GIT_COMMITTER_NAME: process.env.GIT_COMMITTER_NAME,
GIT_AUTHOR_NAME: process.env.GIT_AUTHOR_NAME,
EMAIL: process.env.EMAIL,
CRAFT_NEW_VERSION: newVersion,
CRAFT_OLD_VERSION: oldVersion,
});

beforeEach(() => {
vi.clearAllMocks();
});
Expand All @@ -27,13 +44,7 @@ describe('runPreReleaseCommand', () => {
expect(mockedSpawnProcess).toBeCalledWith(
'scripts/bump-version.sh',
[oldVersion, newVersion],
{
env: {
...process.env,
CRAFT_NEW_VERSION: newVersion,
CRAFT_OLD_VERSION: oldVersion,
},
},
{ env: expectedBaseEnv() },
);
});

Expand All @@ -50,15 +61,62 @@ describe('runPreReleaseCommand', () => {
expect(mockedSpawnProcess).toBeCalledWith(
'python',
['./increase_version.py', 'argument 1', oldVersion, newVersion],
{
env: {
...process.env,
CRAFT_NEW_VERSION: newVersion,
CRAFT_OLD_VERSION: oldVersion,
},
},
{ env: expectedBaseEnv() },
);
});

test('does not forward arbitrary env vars from process.env', async () => {
// Simulate an attacker having planted secrets / linker hijacks in
// process.env (e.g. via a compromised parent process or a malicious
// earlier CI step). The allowlist must keep these out of the child.
const sentinel = '__SHOULD_NOT_LEAK__';
const before = {
LD_PRELOAD: process.env.LD_PRELOAD,
AWS_SECRET_ACCESS_KEY: process.env.AWS_SECRET_ACCESS_KEY,
SECRET_TOKEN: process.env.SECRET_TOKEN,
};
process.env.LD_PRELOAD = `/tmp/evil.so-${sentinel}`;
process.env.AWS_SECRET_ACCESS_KEY = `aws-${sentinel}`;
process.env.SECRET_TOKEN = `token-${sentinel}`;

try {
await runPreReleaseCommand({
oldVersion,
newVersion,
rootDir,
preReleaseCommand: 'scripts/bump-version.sh',
});

const spawnCall = mockedSpawnProcess.mock.calls[0];
const envArg = spawnCall[2].env as Record<string, unknown>;

// Allowlist keys are present (values taken from process.env at call
// time, which is fine — they're allowlisted by name).
expect(envArg.CRAFT_NEW_VERSION).toBe(newVersion);
expect(envArg.CRAFT_OLD_VERSION).toBe(oldVersion);

// Dangerous / attacker-planted vars must NOT appear.
expect(envArg.LD_PRELOAD).toBeUndefined();
expect(envArg.AWS_SECRET_ACCESS_KEY).toBeUndefined();
expect(envArg.SECRET_TOKEN).toBeUndefined();

// Defensive: the sentinel must not appear in ANY value.
for (const v of Object.values(envArg)) {
if (typeof v === 'string') {
expect(v).not.toContain(sentinel);
}
}
} finally {
// Restore prior env.
for (const [key, val] of Object.entries(before)) {
if (val === undefined) {
delete process.env[key];
} else {
process.env[key] = val;
}
}
}
});
});

describe('checkVersionOrPart', () => {
Expand Down Expand Up @@ -125,3 +183,32 @@ 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();
});
});
40 changes: 40 additions & 0 deletions src/commands/__tests__/publish.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,46 @@ describe('runPostReleaseCommand', () => {
},
);
});

test('does not forward arbitrary env vars from process.env', async () => {
// Same threat model as runPreReleaseCommand: attacker-planted env vars
// must not leak to the post-release subprocess.
const sentinel = '__POST_RELEASE_SHOULD_NOT_LEAK__';
const before = {
LD_PRELOAD: process.env.LD_PRELOAD,
AWS_SECRET_ACCESS_KEY: process.env.AWS_SECRET_ACCESS_KEY,
SECRET_TOKEN: process.env.SECRET_TOKEN,
};
process.env.LD_PRELOAD = `/tmp/evil.so-${sentinel}`;
process.env.AWS_SECRET_ACCESS_KEY = `aws-${sentinel}`;
process.env.SECRET_TOKEN = `token-${sentinel}`;

try {
await runPostReleaseCommand(newVersion, 'bash -c true');

const spawnCall = mockedSpawnProcess.mock.calls[0];
const envArg = spawnCall[2].env as Record<string, unknown>;

expect(envArg.CRAFT_RELEASED_VERSION).toBe(newVersion);
expect(envArg.LD_PRELOAD).toBeUndefined();
expect(envArg.AWS_SECRET_ACCESS_KEY).toBeUndefined();
expect(envArg.SECRET_TOKEN).toBeUndefined();

for (const v of Object.values(envArg)) {
if (typeof v === 'string') {
expect(v).not.toContain(sentinel);
}
}
} finally {
for (const [key, val] of Object.entries(before)) {
if (val === undefined) {
delete process.env[key];
} else {
process.env[key] = val;
}
}
}
});
});

describe('handleReleaseBranch', () => {
Expand Down
Loading
Loading