fix(lib): pin and verify Bun's installer - #51
Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
get-resq-software | 932aeb0 | Aug 11 2026, 10:41 AM |
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
WomB0ComB0
added a commit
that referenced
this pull request
Aug 11, 2026
The review was right and I tested the wrong thing first. I checked whether a RETURN trap set inside a function fires; the finding was that it does not stay inside that function. Verified, all three claims: without set -T scoped to the function with set -T PERSISTS - fired on an unrelated later return nested functions silently replaced the caller's own RETURN trap under set -Tu "unbound variable", status 1, once the local is gone These are library functions meant to be sourced, so a caller enabling functrace is a realistic way to break them, and the failure would surface far from here. Download, verify and run now happen in a subshell with an EXIT trap, which cannot escape and takes the temp directory with it. Exit status still propagates, so a failed install is still reported. Removed the vestigial `if true` the restructure left behind, along with its unreachable else. scripts/lib/bun.sh has the same pattern and is fixed on the #51 branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WomB0ComB0
added a commit
that referenced
this pull request
Aug 11, 2026
…po (#50) * fix(lib): verify the Nix installer; install Docker from its signed repo Items 1 and 2 from the vendor-installer discussion. #47 pinned the transport for these; this verifies the content, where the vendors make that possible. Nix. install.determinate.systems/nix is a redirect to a versioned URL — /nix/tag/v3.21.9 — which returns identical bytes across fetches. So it can be pinned and digest-checked, and now is: downloaded to a temp dir, SHA-256 compared, executed only on match, in all four places that installed Nix (install.sh, install.ps1, and both libs). scripts/lib/nix.sh additionally switches from nixos.org's rolling installer to the same Determinate one; two different installers for one tool in a single repo was its own problem. Docker. scripts/lib/docker.ps1 still piped get.docker.com to sh while docker.sh installed from Docker's apt repository, which apt verifies by GPG signature on every package. Only one of them was verified. get.docker.com is rolling, unsigned and unversioned — there is nothing to pin it against, so the fix is to stop using it rather than to constrain how it is fetched. Non-apt Linux under PowerShell now reports what to run instead of silently taking the weaker path. required.yml re-checks the Nix pin against the live URL and asserts all four copies agree, for the same reason it checks the hook digests: a stale pin would otherwise fail every user's install rather than one CI run. What this does not do: Determinate publish no digest of their own, so this pins what we execute, not what they wrote. It closes "silently different tomorrow", not "already compromised when we pinned it". Bun is unchanged — its install script is not covered by the signed SHASUMS256.txt, and fixing that properly means replacing the script with a download-and-verify against the signed manifest, which is a separate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: handle installer failures; correct Debian vs Ubuntu docker repo Review findings on #50, all valid. Silent failures. Verifying a download says the script is authentic, not that it worked. install.sh set NIX_JUST_INSTALLED after the installer ran regardless of its exit status, so a failed install printed "Nix was just installed" and a PATH hint for a Nix that was not there. The two PowerShell paths were worse: a native command's failure does not throw, so Install-Nix and Install-Docker fell through to their success returns and setup.ps1 carried on. All three now check. Debian. Both docker paths hardcoded linux/ubuntu with `lsb_release -cs`. On Debian that yields e.g. "bookworm", which the ubuntu suite does not publish, so apt errors or silently finds no candidate. Now reads ID/ID_LIKE from /etc/os-release and picks the matching suite, per Docker's own documentation. Derivatives resolve through ID_LIKE with UBUNTU_CODENAME/DEBIAN_CODENAME, which matters for Kali: its VERSION_CODENAME is "kali-rolling", not a Docker suite. Exercised across ubuntu/debian/mint/pop/kali/raspbian, with fedora correctly rejected to a manual install. The Nix pin check covered install.sh and both libs but not install.ps1, which declares its own $NixInstallUrl and $NixInstallerSha256. It could have drifted while the job passed — meaning Windows users would get a different installer than everyone else, which is the failure the check exists to prevent. Now all four are compared. Two findings not taken, with reasons in the review thread: the RETURN traps do fire (verified on normal return, early failure return, and under set -e), and the missing Apache headers are a pre-existing repo-wide gap — all 20 files in scripts/lib lack one, so fixing two of them makes the directory less consistent, not more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(lib): confine the installer cleanup trap to a subshell The review was right and I tested the wrong thing first. I checked whether a RETURN trap set inside a function fires; the finding was that it does not stay inside that function. Verified, all three claims: without set -T scoped to the function with set -T PERSISTS - fired on an unrelated later return nested functions silently replaced the caller's own RETURN trap under set -Tu "unbound variable", status 1, once the local is gone These are library functions meant to be sourced, so a caller enabling functrace is a realistic way to break them, and the failure would surface far from here. Download, verify and run now happen in a subshell with an EXIT trap, which cannot escape and takes the temp directory with it. Exit status still propagates, so a failed install is still reported. Removed the vestigial `if true` the restructure left behind, along with its unreachable else. scripts/lib/bun.sh has the same pattern and is fixed on the #51 branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ci): use an explicit if instead of A && B || C SC2015, which actionlint fails the build on. The behaviour was correct here — C should run when either test fails — but the pattern also runs C when A succeeds and B fails, which is the trap shellcheck is pointing at. An explicit if says what is meant without relying on the reader knowing that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Item 3, and it landed differently than planned. The intent was to replace bun.sh/install with our own download-and-verify against the signed SHASUMS256.txt, matching install-resq.sh. Two findings changed that: Bun's installer does CPU baseline detection — bun-linux-x64 versus -baseline for machines without AVX2. Reimplementing that wrongly installs a binary that dies with SIGILL on older hardware, a worse and far more confusing failure than the one being fixed. And SHASUMS256.txt.asc is clearsigned by a key that is not published anywhere pinnable: bun.sh/pgp.asc and bun.com/pgp.asc are 404, and SECURITY.md gives only an email address. Without a trust anchor the signature verifies nothing, and fetching a key at install time would be TOFU dressed up as verification. But bun.sh/install turns out to be byte-identical to the tagged oven-sh/bun@bun-v1.3.14 src/cli/install.sh. So the script itself can be pinned and digest-checked, exactly like the Nix installer — keeping Bun's platform logic while removing "whatever they serve today" from the trust model. The Windows installer is pinned the same way, from the same tag. Note the two are NOT equivalent: the tagged install.ps1 differs from what bun.sh/install.ps1 serves, unlike the shell pair which match. The tagged copy belongs to the release, so it is the one worth pinning. required.yml checks all three pins independently and asserts the two files agree on the shell installer. Residual gap, unchanged by this: Bun's own script does not verify what it downloads. Pinning the script closes "the installer changed under us", not "the binary it fetches is authentic". Only Bun can close the second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same defect as scripts/lib/nix.sh, carried by the same pattern. A RETURN trap set inside a function is not confined to it: under set -T it persists and fires on unrelated later returns, it silently replaces any RETURN trap the caller installed, and under set -u the out-of-scope temp variable aborts with "unbound variable". These are sourced library functions, so that breaks callers. Download, verify and run now happen in a subshell with an EXIT trap, which cannot escape and takes the temp directory with it. Exit status propagates, so a failed install is still reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WomB0ComB0
force-pushed
the
fix/pin-bun-installer
branch
from
August 11, 2026 10:41
ac17830 to
932aeb0
Compare
Merged
WomB0ComB0
added a commit
that referenced
this pull request
Aug 11, 2026
Ships the security audit: #46, #47, #50 and #51. All four are on main and none are reaching anyone — the endpoint serves what PINS says, and PINS still points at v0.4.1, which predates every one of them. install.sh on main f38612c071bcd6d4... served (v0.4.1) 639b4167471082c6... What this releases: #46 install-hooks.{sh,ps1} fetched six executables from a mutable branch with no verification, chmod +x'd them, and pointed core.hooksPath at them — on every fresh onboard, since resq is installed after hooks run. Now pinned and digest-checked, failing closed. #47 TLS pinning across scripts/lib; nix.sh piped 4xx bodies into sh; bun.sh fetched schemelessly; docker.sh used apt-key, which grants the Docker key authority over every repository on the system. #50 Nix installer pinned to a versioned URL and verified; Docker installed from its GPG-signed apt repo rather than get.docker.com; installer failures no longer report success; Debian and Ubuntu suites resolved correctly. #51 Bun's installer pinned to a tagged copy and verified. Merging this is the release. Merging the pin-bump PR that follows is what changes the served bytes. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Item 3 — and it landed differently than I planned, in a way worth recording.
What I intended, and why I didn't do it
The plan was to replace
bun.sh/installwith our own download-and-verify against the signedSHASUMS256.txt, matchinginstall-resq.sh. Two findings killed that:Bun's installer does CPU baseline detection —
bun-linux-x64vs-baselinefor machines without AVX2. Reimplementing that wrongly installs a binary that dies withSIGILLon older hardware: a worse and far more confusing failure than the one being fixed.The signature has no trust anchor.
SHASUMS256.txt.ascis clearsigned, but the key is not published anywhere pinnable —bun.sh/pgp.ascandbun.com/pgp.ascboth 404, andSECURITY.mdgives only an email address. Verifying a signature against a key fetched at install time is TOFU dressed up as verification. I'd have shipped ceremony, not security.What actually works
bun.sh/installis byte-identical to the taggedoven-sh/bun@bun-v1.3.14src/cli/install.sh(bab8acfb…). So the script can be pinned and digest-checked exactly like the Nix installer — keeping Bun's platform logic while removing "whatever they serve today" from the trust model.Windows is pinned the same way, from the same tag. The two are not equivalent: the tagged
install.ps1differs from whatbun.sh/install.ps1serves, unlike the shell pair which match. The tagged copy belongs to the release, so it is the one worth pinning — but they cannot be assumed to move together, andrequired.ymlchecks them independently.Verified
Plus the existing hook (6/6) and Nix pins still pass. shellcheck clean,
stamp --checkin sync, 48/48 worker tests.Residual gap, unchanged
Bun's own script does not verify what it downloads. Pinning the script closes "the installer changed under us"; it does not close "the binary it fetches is authentic". Only Bun can close the second, by verifying against their own
SHASUMS256.txt— which they publish and do not use.That is now the honest state of all three vendors:
🤖 Generated with Claude Code