Skip to content

Commit e248180

Browse files
authored
fix(build): run hole-punch before signing to preserve code signature (#1037)
## Summary Fixes #1033 — nightly macOS binaries had an invalid code signature because `script/build.ts` ran `binpunch.processBinary()` **after** `fossilize` had already signed + notarized the binary. The mutated bytes invalidated the signature, causing macOS AMFI to SIGKILL the downloaded binary, which surfaced as the opaque `Setup failed with exit code 1` during `cli upgrade` — leaving the install bricked. ## Root cause `postProcessTarget()` in `build.ts` called `processBinary(outfile)` at line 388, **after** fossilize returned (which includes signing + notarization on `main`/`release` builds). The hole-punched bytes broke the signature. ### Why CI didn't catch it The issue claimed "the darwin binary is never executed on macOS in CI" — that's incorrect. CI runs the darwin-arm64 smoke test on `macos-latest` with `can-test: true`, and it **passes** even on signed+hole-punched `main` builds ([run 26542990891](https://github.com/getsentry/cli/actions/runs/26542990891)). The real reason: AMFI only SIGKILLs an invalid-signature binary when it carries the `com.apple.quarantine` xattr (i.e. after being **downloaded**). A freshly-built binary on the build machine is not quarantined. ## Changes ### 1. Move hole-punch into fossilize (`--hole-punch` flag) - **Depends on [BYK/fossilize#18](BYK/fossilize#18 (fossilize 0.8.0) - Remove `binpunch` devDep and the post-sign `processBinary()` call from `build.ts` - Pass `--hole-punch` to the fossilize CLI invocation — fossilize now runs binpunch internally between `chmod` and `sign+notarize`, so the signature covers the final bytes ### 2. CI signature verification gate - Add a `Verify code signature (darwin)` step using `rcodesign verify` after the smoke tests, gated on `FOSSILIZE_SIGN=y` (main/release pushes) + darwin targets - This catches broken signatures that execution-based smoke tests miss (no quarantine xattr on CI) ### 3. Upgrade resilience (SIGKILL detection) - In `spawnWithRetry()`: capture the `signal` parameter from the child's `close` event - When `signal === 'SIGKILL'`: throw a clear `UpgradeError` explaining the likely cause (invalid code signature) instead of the opaque `Setup failed with exit code 1` ### 4. Cleanup - Remove duplicate top-level `patchedDependencies` (already present in `pnpm` block) - Update `.lore.md` pipeline description to reflect hole-punch running inside fossilize ## Pipeline order (fixed) ``` download → unsign → strip → inject SEA → chmod → HOLE-PUNCH → sign + notarize → gzip ``` ## Blocked on - [ ] [BYK/fossilize#18](BYK/fossilize#18) merged + released as 0.8.0 on npm (CI will fail to install `fossilize@^0.8.0` until then)
1 parent 5e68ac2 commit e248180

6 files changed

Lines changed: 46 additions & 31 deletions

File tree

.github/workflows/ci.yml

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ jobs:
308308
# resolve correctly; workflow-level env evaluates before the job's
309309
# environment: is applied.
310310
SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }}
311-
# Set on main/release branches so build.ts runs binpunch + creates .gz
311+
# Set on main/release branches so build.ts creates .gz archives
312312
RELEASE_BUILD: ${{ github.event_name != 'pull_request' && '1' || '' }}
313313
# Codesigning: only on main/release pushes (fork PRs lack secrets)
314314
FOSSILIZE_SIGN: ${{ github.event_name == 'push' && (github.ref_name == 'main' || startsWith(github.ref_name, 'release/')) && 'y' || 'n' }}
@@ -350,6 +350,19 @@ jobs:
350350
echo "$OUTPUT"
351351
exit 1
352352
fi
353+
- name: Verify code signature (darwin)
354+
# Only runs when signing was active (main/release pushes) and target is darwin.
355+
# The smoke test alone does NOT catch invalid signatures — AMFI only SIGKILLs
356+
# quarantined (downloaded) binaries, not freshly-built ones on the build machine.
357+
if: >-
358+
startsWith(matrix.target, 'darwin') &&
359+
github.event_name == 'push' &&
360+
(github.ref_name == 'main' || startsWith(github.ref_name, 'release/'))
361+
shell: bash
362+
run: |
363+
BIN=./dist-bin/sentry-${{ matrix.target }}
364+
echo "Verifying code signature on $BIN..."
365+
rcodesign verify "$BIN"
353366
- name: Upload binary artifact
354367
uses: actions/upload-artifact@v7
355368
with:

.lore.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
* **Auth token env var override pattern: SENTRY\_AUTH\_TOKEN > SENTRY\_TOKEN > SQLite**: Auth token precedence in \`src/lib/db/auth.ts\`: \`SENTRY\_AUTH\_TOKEN\` > \`SENTRY\_TOKEN\` > SQLite OAuth token. \`getEnvToken()\` trims env vars (empty/whitespace = unset). \`AuthSource\` tracks provenance. \`ENV\_SOURCE\_PREFIX = "env:"\` — use \`.length\` not hardcoded 4. Env tokens bypass refresh/expiry. \`isEnvTokenActive()\` guards auth commands. Logout must NOT clear stored auth when env token active. \`runInteractiveLogin\` catches OAuth flow errors internally and returns falsy on failure; login command sets \`process.exitCode = 1\` and returns normally (does NOT reject). Tests expecting \`rejects.toThrow()\` will fail — assert via fetch-call inspection instead. \`requestDeviceCode\` requires \`SENTRY\_CLIENT\_ID\` env var.
99

1010
<!-- lore:019e4f9d-b0e0-72b0-ab0f-1740206c9d80 -->
11-
* **Binary build pipeline: esbuild → fossilize → Node SEA (replacing Bun.build compile)**: Binary build pipeline: \`src/bin.ts → \[esbuild CJS, node24 target] → dist-build/bin.js → \[fossilize --no-bundle] → Node SEA binary → \[binpunch ICU hole-punch] → gzip\`. Strip debug symbols handled INSIDE fossilize (as of fossilize 0.7.0) — fossilize strips the copied binary BEFORE postject injection. Strip MUST happen before injection — after SEA injection, \`strip\` fails ('section .text can't be allocated in segment 2'). macOS: \`strip -x\` on unsigned copy; cross-strip from Linux silently fails (caught). Windows: skipped (no debug symbols). NODE\_VERSION='lts'. ALL\_TARGETS: darwin-arm64, darwin-x64, linux-arm64, linux-x64, win32-x64 + musl variants. Post-process: rename \`sentry-win-x64.exe\`→\`sentry-windows-x64.exe\`. UPX RULED OUT — destroys ELF notes. \`FOSSILIZE\_SIGN=y\` on push to main/release. Gzip only when \`RELEASE\_BUILD=1\`. \`stripCachedNodeBinaries()\` removed from \`script/build.ts\` — superseded by fossilize 0.7.0.
11+
* **Binary build pipeline: esbuild → fossilize → Node SEA (replacing Bun.build compile)**: Binary build pipeline: \`src/bin.ts → \[esbuild CJS, node24 target] → dist-build/bin.js → \[fossilize --no-bundle --hole-punch] → Node SEA binary → gzip\`. CRITICAL ORDER: hole-punch MUST happen BEFORE signing (issue #1033 — hole-punching after signing invalidates macOS code signature, causing AMFI SIGKILL). As of fossilize 0.8.0, hole-punch runs inside fossilize via \`--hole-punch\` flag (uses binpunch internally), between chmod and sign+notarize. Strip debug symbols also handled INSIDE fossilize (as of fossilize 0.7.0). macOS: \`strip -x\` on unsigned copy; cross-strip from Linux silently fails (caught). Windows: skipped. NODE\_VERSION='lts'. ALL\_TARGETS: darwin-arm64, darwin-x64, linux-arm64, linux-x64, win32-x64 + musl variants. Post-process: rename \`sentry-win-x64.exe\`→\`sentry-windows-x64.exe\`. UPX RULED OUT — destroys ELF notes. \`FOSSILIZE\_SIGN=y\` on push to main/release. Gzip only when \`RELEASE\_BUILD=1\`. binpunch is opt-in via \`--hole-punch\` flag / \`FOSSILIZE\_HOLE\_PUNCH\` env var — NOT default-on (removes non-English ICU locale data, breaking i18n for consumers). Cache stays pristine; all mutations (strip, inject, hole-punch, sign) happen on per-build output copy.
1212

1313
<!-- lore:019e56e2-f89c-7846-8953-1e47fb470cbf -->
1414
* **Binary size breakdown: 94.5% is Node.js runtime — bundled code is ~6.3 MiB**: Binary composition (linux-x64, Node 24 LTS): Node.js runtime=121 MiB (ships with debug symbols). \`strip --strip-unneeded\` → 99 MiB (-17 MiB raw, -4 MiB compressed). Strip built into fossilize 0.7.0 — happens on the copied binary BEFORE postject injection. After strip+SEA+binpunch: ~108 MiB raw, ~30 MiB gzip (vs 125 MiB / 34 MiB unstripped). .rodata=52.5 MB: V8 snapshot ~12 MB, ICU full-icu data ~28 MB. UPX compresses to 25 MiB but DESTROYS ELF notes — ruled out. \`--with-intl=small-icu\` saves ~26-28 MiB (biggest win from custom build); \`--without-lief\` BREAKS SEA; \`--without-sqlite\` BREAKS CLI; \`--disable-single-executable-application\` BREAKS EVERYTHING. Custom build deferred — poor cost/benefit (~3.5h build vs 5min fossilize). Final vs Bun: download 30 MiB (Bun: 32 MiB), \`--version\` ~1.0s (Bun: ~1.9s), completions ~150ms (Bun: ~180ms).

package.json

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,12 @@
2626
"@types/react": "^19.2.14",
2727
"@types/semver": "^7.7.1",
2828
"@vitest/coverage-v8": "^4.1.7",
29-
"binpunch": "^1.0.0",
3029
"chalk": "^5.6.2",
3130
"cli-highlight": "^2.1.11",
3231
"consola": "^3.4.2",
3332
"esbuild": "^0.25.0",
3433
"fast-check": "^4.5.3",
35-
"fossilize": "^0.7.0",
34+
"fossilize": "^0.8.0",
3635
"hono": "^4.12.15",
3736
"http-cache-semantics": "^4.2.0",
3837
"ignore": "^7.0.5",
@@ -124,10 +123,5 @@
124123
"check:stale-refs": "pnpm tsx script/check-stale-references.ts"
125124
},
126125
"type": "module",
127-
"types": "./dist/index.d.cts",
128-
"patchedDependencies": {
129-
"@stricli/core@1.2.5": "patches/@stricli%2Fcore@1.2.5.patch",
130-
"@sentry/node-core@10.50.0": "patches/@sentry%2Fnode-core@10.50.0.patch",
131-
"@sentry/core@10.50.0": "patches/@sentry%2Fcore@10.50.0.patch"
132-
}
126+
"types": "./dist/index.d.cts"
133127
}

pnpm-lock.yaml

Lines changed: 6 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

script/build.ts

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ import { readFile, rm, stat, writeFile } from "node:fs/promises";
3333
import { join } from "node:path";
3434
import { promisify } from "node:util";
3535
import { gzip } from "node:zlib";
36-
import { processBinary } from "binpunch";
3736
import { build as esbuild } from "esbuild";
3837
import { uploadSourcemaps } from "../src/lib/api/sourcemaps.js";
3938
import { injectDebugId, PLACEHOLDER_DEBUG_ID } from "./debug-id.js";
@@ -321,6 +320,7 @@ async function compileAllTargets(
321320
[
322321
fossilizeBin,
323322
"--no-bundle",
323+
"--hole-punch",
324324
"--output-name",
325325
"sentry",
326326
"--platforms",
@@ -342,7 +342,7 @@ async function compileAllTargets(
342342
return { successes: 0, failures: targets.length };
343343
}
344344

345-
// Post-process each target: rename Windows binary, hole-punch ICU, gzip
345+
// Post-process each target: rename Windows binary, gzip
346346
let successes = 0;
347347
let failures = 0;
348348
for (const target of targets) {
@@ -361,10 +361,13 @@ async function compileAllTargets(
361361

362362
/**
363363
* Post-process a single compiled binary: rename from fossilize's output
364-
* naming to our expected naming, hole-punch ICU data, and optionally gzip.
364+
* naming to our expected naming, and optionally gzip.
365365
*
366366
* Fossilize outputs `sentry-{os}-{arch}[.exe]` where os is "win" for Windows.
367367
* We rename "win" → "windows" to match our release naming convention.
368+
*
369+
* Note: ICU hole-punch now runs inside fossilize (--hole-punch flag) before
370+
* code signing, so it's no longer done here.
368371
*/
369372
async function postProcessTarget(target: BuildTarget): Promise<void> {
370373
const packageName = getPackageName(target);
@@ -383,15 +386,6 @@ async function postProcessTarget(target: BuildTarget): Promise<void> {
383386

384387
console.log(` -> ${outfile}`);
385388

386-
// Hole-punch: zero unused ICU data entries so they compress to nearly nothing.
387-
// Always runs so the smoke test exercises the same binary as the release.
388-
const hpStats = processBinary(outfile);
389-
if (hpStats && hpStats.removedEntries > 0) {
390-
console.log(
391-
` -> hole-punched ${hpStats.removedEntries}/${hpStats.totalEntries} ICU entries`
392-
);
393-
}
394-
395389
// On main and release branches (RELEASE_BUILD=1), create gzip-compressed
396390
// copies for release downloads / GHCR nightly (~70% smaller with hole-punch).
397391
if (process.env.RELEASE_BUILD) {

src/commands/cli/upgrade.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,23 @@ async function spawnWithRetry(
432432
env,
433433
});
434434
return await new Promise<number>((resolve, reject) => {
435-
proc.on("close", (code) => resolve(code ?? 1));
435+
proc.on("close", (code, signal) => {
436+
// SIGKILL on macOS typically means AMFI killed the binary due to
437+
// an invalid code signature (the downloaded binary has quarantine
438+
// xattr). Surface this clearly instead of an opaque exit code.
439+
if (signal === "SIGKILL") {
440+
reject(
441+
new UpgradeError(
442+
"execution_failed",
443+
"Downloaded binary was killed by the operating system (SIGKILL). " +
444+
"This usually means the binary has an invalid code signature. " +
445+
"Try reinstalling: curl -sL https://sentry.io/get-cli/ | bash"
446+
)
447+
);
448+
return;
449+
}
450+
resolve(code ?? 1);
451+
});
436452
proc.on("error", (err) => reject(err));
437453
});
438454
} catch (error) {

0 commit comments

Comments
 (0)