fix(security): pin Sparkle and the DuckDB extension, and publish only what is verified - #2348
Merged
Conversation
… what is verified Claude-Session: https://claude.ai/code/session_013MEaba8K1HQcyDNeq5wEFk
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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.
Five supply-chain items from the CI audit. Every pin here was verified by fetching the artifact and hashing it, not copied from a report.
The signing path installed an unpinned Sparkle
sign-and-appcast.shranbrew install --cask sparkleand then picked a directory withls | head -1. That step holdsSPARKLE_PRIVATE_KEY, the EdDSA key that signs every update every user receives, so it should not be running a binary whose contents can change between releases.It now downloads a pinned release tarball and verifies it. Checked by downloading it here:
The archive has
./bin/at its root and carriesgenerate_appcast, which is the only binary this script invokes; extracted and run it to be sure. Version 2.9.5 matches the Sparkle framework pinned inPackage.resolved, so the tool and the framework move together.The iOS DuckDB build compiled a moving ref into a shipped binary
This ends up in a binary that ships on the App Store, and with a branch ref nobody can say what was in a given release. Pinned to
7e80f7ff, the head of quack'sv1.5-variegataline.One thing this does not fix, recorded in the file rather than hidden. The file's own rule is that each pin should carry a duckdb submodule matching
DUCKDB_VERSION, which isv1.5.2. That commit carriesv1.5.4, and no commit in the recent history ofv1.5-variegatacarriesv1.5.2, so an exact match is not available on that line. Closing the gap means either movingDUCKDB_VERSIONtov1.5.4, which also has to move the bundled macOSlibduckdb.a, or backporting quack. Pinned anyway, because a fixed commit on the right release line is strictly better than trackingmain, which today is further fromv1.5.2thanv1.5.4is.The comment also referenced a
QUACK_GIT_TAGvariable that has never existed. Rewritten to describe what the file actually does.The published archive carried 80 MB nothing verified
publish-libs.shbuilt the macOS archive withtar czf ... -C Libs .. The dot swept inLibs/ios,Libs/dylibsand the.downloadedmarkers, so every download of the macOS libraries also carried the iOS xcframeworks, which that script's checksum guard does not cover at all.It now publishes an allowlist: exactly the
.afiles the guard walks, pluschecksums.sha256. Measured against the current tree:61 is the 60 entries in
Libs/checksums.sha256plus the checksum file itself. Verified that the baseline is fully covered and there are no extras.A personal Apple ID in a tracked file
build-release.shcarriedAPPLE_ID="${APPLE_ID:-datngoquoc@icloud.com}"in a public repository. It is also dead: notarization goes through--keychain-profile "TablePro"at all three call sites and never reads it.A secret on the process argument list
The same script passed
ANALYTICS_HMAC_SECRET=...toxcodebuildon the command line, where any local process can read it. It is redundant:build.ymlwrites the value intoConfigs/Secrets.xcconfigbefore the build step, and that is how the project reads it. The argv pass is removed.Predictable temp paths
publish-libs.shstaged a public upload at a fixed/tmpname andcheck-redis-command-routing.shused one too, unlike every sibling check script./tmpis world-writable, so a fixed name is something another local user can create first. Both usemktemp -dwith a cleanup trap now.Verification
shellcheck -x --severity=warningis clean on all four scripts. The Sparkle pin and the archive contents were verified by actually fetching and building them. The iOS DuckDB change cannot be verified here without an iOS DuckDB build, which is why the remaining submodule mismatch is written into the file rather than left implicit.https://claude.ai/code/session_013MEaba8K1HQcyDNeq5wEFk