Repository navigation
fix(fork): harden the launch isolation gate — discovery, teardown, DMG fidelity, dry runs #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,11 @@ on: | |
| required: false | ||
| default: true | ||
| type: boolean | ||
| dry_run: | ||
| description: "Build and run the launch isolation gate without publishing" | ||
| required: false | ||
| default: false | ||
| type: boolean | ||
|
|
||
| permissions: | ||
| contents: write | ||
|
|
@@ -115,43 +120,85 @@ jobs: | |
| # fork's t3code-fork, not upstream's t3code. | ||
| # release/ holds only the distributables: the build script copies | ||
| # files out of its temp stage directory and skips directories, so the | ||
| # unpacked .app never lands here. Unpack the shipped zip instead — | ||
| # which is the stronger check anyway, since it exercises exactly the | ||
| # bundle a user downloads rather than a build-tree intermediate. | ||
| zip=$(find release -maxdepth 1 -name "*.zip" -type f | head -1) | ||
| if [ -z "$zip" ]; then | ||
| echo "No .zip artifact found under release/" >&2 | ||
| # unpacked .app never lands here. Launch from the DMG — the headline | ||
| # artifact users drag to /Applications — and hold the zip (what | ||
| # electron-updater downloads) to the same executable, so verifying | ||
| # one verifies both. Every discovery below asserts exactly-one | ||
| # rather than picking with head -1: a gate that can verify a | ||
| # different artifact than the one published is not a gate, and a | ||
| # SIGPIPE under pipefail dies with no message at all. | ||
| # (The zip's existence depends on the build script expanding | ||
| # `--target dmg` into [dmg, zip]; the fork guard pins the flag.) | ||
| mapfile -t dmgs < <(find release -maxdepth 1 -name "*.dmg" -type f 2>/dev/null) | ||
| if [ "${#dmgs[@]}" -ne 1 ]; then | ||
| echo "Expected exactly one .dmg under release/, found ${#dmgs[@]}" >&2 | ||
| ls -l release >&2 || true | ||
| exit 1 | ||
| fi | ||
| dmg="${dmgs[0]}" | ||
| mapfile -t zips < <(find release -maxdepth 1 -name "*.zip" -type f 2>/dev/null) | ||
| if [ "${#zips[@]}" -ne 1 ]; then | ||
| echo "Expected exactly one .zip under release/, found ${#zips[@]}" >&2 | ||
| ls -l release >&2 || true | ||
| exit 1 | ||
| fi | ||
| zip="${zips[0]}" | ||
| mount=$(mktemp -d) | ||
| unpacked=$(mktemp -d) | ||
| # ditto, not unzip: it preserves the bundle's symlinks, extended | ||
| # attributes, and code signature, so what gets launched is the | ||
| # bundle the zip actually ships. | ||
| ditto -x -k "$zip" "$unpacked" | ||
| app=$(find "$unpacked" -maxdepth 2 -name "*.app" -type d | head -1) | ||
| if [ -z "$app" ]; then | ||
| echo "No .app bundle inside $zip" >&2 | ||
| find "$unpacked" -maxdepth 2 >&2 || true | ||
| scratch=$(mktemp -d) | ||
| # Every exit path — including the early ones — must sweep the | ||
| # process tree (the server child can outlive the Electron main: | ||
| # that IS the v0.1.1 bug), unmount the DMG, and drop the temp | ||
| # trees. | ||
| cleanup() { | ||
| if [ -n "${app:-}" ]; then | ||
| pkill -9 -f "$app/Contents" 2>/dev/null || true | ||
| fi | ||
| hdiutil detach "$mount" -force >/dev/null 2>&1 || true | ||
| rm -rf "$unpacked" "$scratch" 2>/dev/null || true | ||
| } | ||
| trap cleanup EXIT | ||
| hdiutil attach "$dmg" -nobrowse -readonly -mountpoint "$mount" >/dev/null | ||
| mapfile -t apps < <(find "$mount" -maxdepth 1 -name "*.app" -type d 2>/dev/null) | ||
| if [ "${#apps[@]}" -ne 1 ]; then | ||
| echo "Expected exactly one .app in the DMG, found ${#apps[@]}" >&2 | ||
| ls "$mount" >&2 || true | ||
| exit 1 | ||
| fi | ||
| app="${apps[0]}" | ||
| # Derive the executable from the bundle rather than hardcoding the | ||
| # product name — nightly-shaped versions get a different name, and | ||
| # every hardcoded copy is one more string a rename must find. | ||
| # $app is absolute (mktemp -d is), which is what `defaults read` | ||
| # needs to treat it as a path instead of a domain name. | ||
| # $app is absolute (the mountpoint is), which is what `defaults | ||
| # read` needs to treat it as a path instead of a domain name. | ||
| executable=$(defaults read "$app/Contents/Info" CFBundleExecutable) | ||
| binary="$app/Contents/MacOS/$executable" | ||
| if [ ! -x "$binary" ]; then | ||
| echo "Expected executable missing: $binary" >&2 | ||
| ls "$app/Contents/MacOS" >&2 | ||
| exit 1 | ||
| fi | ||
| scratch=$(mktemp -d) | ||
| # ditto, not unzip: unzip flattens the framework symlinks | ||
| # (Versions/Current), producing a bundle that cannot launch — which | ||
| # would fail this gate for a reason unrelated to isolation. ditto | ||
| # also preserves xattrs, and the code signature once signing lands | ||
| # (codesign --verify belongs here at that point). | ||
| ditto -x -k "$zip" "$unpacked" | ||
| mapfile -t zipapps < <(find "$unpacked" -maxdepth 2 -name "*.app" -type d 2>/dev/null) | ||
| if [ "${#zipapps[@]}" -ne 1 ]; then | ||
| echo "Expected exactly one .app inside the zip, found ${#zipapps[@]}" >&2 | ||
| find "$unpacked" -maxdepth 2 >&2 || true | ||
| exit 1 | ||
| fi | ||
| zipbinary="${zipapps[0]}/Contents/MacOS/$executable" | ||
| if ! cmp -s "$binary" "$zipbinary"; then | ||
| echo "The DMG and zip ship different executables:" >&2 | ||
| shasum "$binary" "$zipbinary" >&2 || true | ||
| exit 1 | ||
| fi | ||
| violated() { | ||
| echo "ISOLATION VIOLATED: $1" >&2 | ||
| find "$2" >&2 || true | ||
| pkill -9 -f "$app/Contents" 2>/dev/null || true | ||
| exit 1 | ||
| } | ||
| env -u T3CODE_HOME HOME="$scratch" "$binary" >/dev/null 2>&1 & | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. launching users don't run from the DMG; they copy to trap then only sweeps processes + temp dirs. DMG fidelity stays (you still read the headline artifact); you just stop holding the volume open for minutes. |
||
|
|
@@ -169,7 +216,6 @@ jobs: | |
| fi | ||
| if [ "$SECONDS" -ge "$deadline" ]; then | ||
| echo "Server never created $scratch/.t3-fork/userdata/state.sqlite within 180s" >&2 | ||
| pkill -9 -f "$app/Contents" 2>/dev/null || true | ||
| exit 1 | ||
| fi | ||
| sleep 2 | ||
|
|
@@ -200,6 +246,10 @@ jobs: | |
| find "$scratch/.t3-fork" -maxdepth 2 | ||
|
|
||
| - name: Collect release assets | ||
| # A dry run exercises the build and the launch isolation gate without | ||
| # publishing anything — the gate can only be tested by running it, and | ||
| # iterating on it must not cost a version number per attempt. | ||
| if: ${{ !inputs.dry_run }} | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
|
|
@@ -220,6 +270,7 @@ jobs: | |
| ls -lh release-publish | ||
|
|
||
| - name: Publish GitHub Release | ||
| if: ${{ !inputs.dry_run }} | ||
| uses: softprops/action-gh-release@v2 | ||
| with: | ||
| tag_name: ${{ steps.meta.outputs.tag }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,8 +60,35 @@ describe("fork guard: fork-desktop-release", () => { | |
| it("builds the artifact the release advertises", () => { | ||
| // The release body promises a macOS arm64 build; if the build step is | ||
| // retargeted without updating that text, users download the wrong thing. | ||
| // `--target dmg` is additionally load-bearing for the launch isolation | ||
| // gate: the build script expands dmg into [dmg, zip], and the gate | ||
| // launches the DMG's bundle and compares the zip's executable against | ||
| // it. Retargeting silently breaks artifact discovery — the failure mode | ||
| // that cost the first v0.1.2 release run. | ||
| const workflow = readForkRelease(); | ||
| expect(workflow).toContain("--platform mac"); | ||
| expect(workflow).toContain("--arch arm64"); | ||
| expect(workflow).toContain("--target dmg"); | ||
| }); | ||
|
|
||
| it("keeps the launch isolation gate and its negative assertions", () => { | ||
| // The gate is the only executable proof of the fork's data isolation — | ||
| // v0.1.1 passed every static check while its server child opened the | ||
| // real ~/.t3/userdata database. Weakening or deleting this step must not | ||
| // pass silently. Pin its presence, both halves of the positive assertion | ||
| // (the fork base appears; the fork Electron dir appears), and the | ||
| // violation branch that makes the negative assertions real. | ||
| const workflow = readForkRelease(); | ||
| expect(workflow).toContain("Launch isolation check"); | ||
| expect(workflow).toContain(".t3-fork/userdata/state.sqlite"); | ||
| expect(workflow).toContain("ISOLATION VIOLATED"); | ||
| expect(workflow).toContain('"$support/t3code"'); | ||
| expect(workflow).toContain('"$support/t3code-fork"'); | ||
| // Publishing must be conditional on the gate: the gate step precedes the | ||
| // collect step, and a dry run skips publishing but never the gate. | ||
| expect(workflow.indexOf("Launch isolation check")).toBeLessThan( | ||
| workflow.indexOf("Collect release assets"), | ||
| ); | ||
| expect(workflow).toContain("dry_run"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this guard over-couples to incidental string shape and under-pins the invariants this PR claims to harden.
pin outcomes, not surface syntax: workflow invokes the gate script; Collect/Publish carry |
||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this gate is no longer a workflow step — it's a multi-phase program (discover ×4, mount, ditto, cmp, launch, poll, shutdown, assert) living as a YAML
run: |blob. that is a structural regression: hard to run locally, painful to review, and the place every future hardening will keep accruing.code-judo: extract
scripts/check-fork-launch-isolation.sh(or equivalent) and make this step one invocation. while extracting, collapse these four identicalmapfile/-ne 1/exit 1blocks into oneexactly_onehelper — the copy-paste is the signal the logic already outgrew inline YAML. do not "clean up" the duplication in place and leave the blob here.pin the workflow → script call from the fork guard so the gate cannot be deleted by renaming a step string.