-
Notifications
You must be signed in to change notification settings - Fork 56
fix(security): hook-log redaction + postinstall SHA pinning (wish G3+G4) #1664
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 all commits
1baaf6e
bc1cc41
5658299
8836140
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 |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # Wish Status — dep-hygiene-and-resilience | ||
|
|
||
| **Branch:** `wish/dep-hygiene-and-resilience` (cut from `origin/dev` 2026-05-05) | ||
| **Worktree:** `/home/genie/workspace/repos/genie-dep-hygiene` | ||
|
|
||
| ## Done | ||
|
|
||
| | Group | Commit | Scope | | ||
| |-------|--------|-------| | ||
| | **G3** Hook-fallback log security | `1baaf6e5` | Mode 0600 + token-shape redaction at write. New `src/hooks/redaction.ts` (gh / sk / glpat / 40+ hex). 12 new tests, all 31 hook tests pass, typecheck clean. | | ||
| | **G4** Postinstall SHA-256 pinning | `bc1cc414` | `package.json#binarySha256` pins 4 tmux tarballs (real upstream SHAs computed). `postinstall-tmux.js` pin-or-fail. `postinstall-hook-binary.js` logs source path (local-compile exempt). `.github/workflows/binary-sha-drift.yml` re-checks pins on PR. Bootstrap doc inside wish dir. Tamper-test verified locally. | | ||
|
|
||
| ## Not started | ||
|
|
||
| | Group | Reason | | ||
| |-------|--------| | ||
| | **G1** pgserve-wrapper PATH fallback | Lives in sibling `automagik-dev/pgserve` repo. Out-of-branch; needs PR there + version bump here once merged. | | ||
| | **G2** TUI pgserve-unreachable degrade panel | Needs new `PgserveUnreachable.tsx` + mocked-pgserve test. Recommend pulling `src/lib/pgserve-recovery.ts` first as the shared recovery-text constant (also used by G6). | | ||
| | **G5** pm2 lifecycle | Largest group. Recommend splitting per /review MEDIUM-1: 5a (pm2 declared dep + self-redeploy via `ensurePm2Installed`) and 5b (canonical-supervisor lock + orphan-prevention pidfile semantics). 5a is independently shippable and unblocks G6. | | ||
| | **G6** Doctor real probes | Depends on G2 (recovery-text constant) and G5a (`ensurePm2Installed` for `--fix`). | | ||
| | **G7** Dep audit + manifest cleanup | Depends on G5 having added `pm2` to manifest first (so audit sees the final shape). | | ||
| | **G8** QA / outage replay | Final group; depends on everything. | | ||
|
|
||
| ## Outstanding /review MEDIUMs | ||
|
|
||
| From the Plan-review verdict (SHIP, two MEDIUMs flagged for pre-execution wish edits): | ||
|
|
||
| 1. **Split G5 → 5a + 5b.** 11 deliverables / 11 tests in one group is the largest in the wish; G6 only needs 5a's `ensurePm2Installed`. | ||
| 2. **G5 deliverable 7 missing failure path** for `pm2 restart` itself failing. Add a 5th return state `pm2_unavailable` to `routeServeStartThroughPm2()` so recovery doesn't silently fall through to detached-spawn. | ||
|
|
||
| Both are wish edits, not implementation — folded into the G5 brief whenever it's picked up. | ||
|
|
||
| ## /review LOWs (defer, inline during execution) | ||
|
|
||
| - darwin variant for `repro-empty-oven-bun.sh` and pidfile parent check | ||
| - Document initial SHA bootstrap process (✅ done in `binary-sha-bootstrap.md`) | ||
| - Make post-cutover `trustedDependencies` action conditional in G7 | ||
| - Specify per-probe timeout budgets in G6 (UDS+SELECT=2s, bun/pgserve/tmux=1s each, `Promise.all`) | ||
|
|
||
| ## Validation gates passed in this session | ||
|
|
||
| - `bun test src/hooks/__tests__/redaction.test.ts` — 12/12 pass | ||
| - `bun test src/hooks/__tests__/dispatch.test.ts` (regression) — 19/19 pass | ||
| - `bun run typecheck` — clean | ||
| - `bunx biome check` — clean across all G3+G4 files | ||
| - Manual SHA tamper-test — match passes, mutation detected |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| # Binary SHA-256 Pinning — Bootstrap & Maintenance | ||
|
|
||
| The `binarySha256` block in `package.json` pins SHA-256 values for every | ||
| binary downloaded by `scripts/postinstall-*.js`. The download path is | ||
| **pin-or-fail**: a missing or mismatching pin aborts the install. | ||
|
|
||
| This doc is the single source of truth for: | ||
|
|
||
| 1. How to compute the initial set of pins (one-time, when adding a new | ||
| downloaded binary). | ||
| 2. How to bump pins when an upstream binary version changes. | ||
| 3. How CI surfaces drift on every release. | ||
|
|
||
| Wish: [`dep-hygiene-and-resilience`](./WISH.md) Group 4. | ||
|
|
||
| > **NOTE:** This doc lives inside the wish directory because the worktree | ||
| > ships without the `.docs-vendor/genie` submodule initialized. Promote to | ||
| > `docs/_internal/binary-sha-bootstrap.md` (i.e. inside the docs vendor) | ||
| > as a follow-up commit on the docs submodule. | ||
|
|
||
| ## What gets pinned | ||
|
|
||
| | Script | Source | Pinning policy | | ||
| |--------|--------|----------------| | ||
| | `scripts/postinstall-tmux.js` | Downloads `tmux-<ver>-<platform>.tar.gz` from `tmux/tmux-builds` GH releases | **Mandatory.** Missing key or mismatch aborts install. | | ||
| | `scripts/postinstall-hook-binary.js` | Compiles `genie-hook` locally via `bun build --compile` | **Exempt.** Logs source path so operators can verify by other means. | | ||
| | `scripts/postinstall-migrations.js` | No external fetches — invokes local `genie migrate` | **Not applicable.** | | ||
|
|
||
| ## Computing initial pins (bootstrap) | ||
|
|
||
| Run on a machine with `curl` and `sha256sum` available: | ||
|
|
||
| ```bash | ||
| TMUX_VERSION=3.6a | ||
| WORK=$(mktemp -d) && cd "$WORK" | ||
| for asset in \ | ||
| tmux-${TMUX_VERSION}-linux-x86_64.tar.gz \ | ||
| tmux-${TMUX_VERSION}-linux-arm64.tar.gz \ | ||
| tmux-${TMUX_VERSION}-macos-arm64.tar.gz \ | ||
| tmux-${TMUX_VERSION}-macos-x86_64.tar.gz; do | ||
| curl -sL "https://github.com/tmux/tmux-builds/releases/download/v${TMUX_VERSION}/$asset" -o "$asset" | ||
| sha256sum "$asset" | ||
| done | ||
| ``` | ||
|
|
||
| Paste the resulting `<sha256> <asset>` lines into `package.json` under | ||
| `binarySha256` (key = asset filename, value = hex SHA-256). | ||
|
|
||
| ## Bumping a binary version | ||
|
|
||
| When `TMUX_VERSION` (or any other downloaded-binary version) changes: | ||
|
|
||
| 1. Update the version constant in the relevant `scripts/postinstall-*.js`. | ||
| 2. Re-run the bootstrap snippet above against the new version. | ||
| 3. Update `package.json#binarySha256` keys (rename old keys to new | ||
| asset filenames, replace SHAs). | ||
| 4. Commit. CI's drift check (next section) will validate that the pinned | ||
| SHAs match what the upstream actually serves. | ||
|
|
||
| ## CI drift detection | ||
|
|
||
| `.github/workflows/binary-sha-drift.yml` runs on every PR that touches | ||
| `package.json`, `scripts/postinstall-*.js`, or this doc. It: | ||
|
|
||
| 1. Re-downloads each pinned asset from upstream. | ||
| 2. Computes its SHA-256. | ||
| 3. Compares against the pin in `package.json`. | ||
| 4. Surfaces any mismatch as a deliberate diff comment so reviewers see | ||
| the version bump as an explicit change, not a silent drift. | ||
|
|
||
| ## Failure modes | ||
|
|
||
| | Scenario | Postinstall behavior | | ||
| |----------|---------------------| | ||
| | Pin matches actual SHA | Install proceeds; one-line `[genie] SHA-256 verified: …` notice. | | ||
| | Pin mismatch (tarball corrupted or attacker-modified) | `Error: <asset> SHA-256 mismatch — expected <pinned>, got <actual>. Aborting install.` Exit 1. | | ||
| | Asset key absent from `binarySha256` block | `Error: <asset> has no SHA-256 pin in package.json#binarySha256. Aborting install.` Exit 1. | | ||
| | `binarySha256` block entirely absent (running script outside published package) | Soft warning, install continues. **This path is local-dev only — published installs MUST pin.** | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| name: Binary SHA Drift | ||
|
|
||
| # Surfaces drift between pinned binarySha256 entries in package.json and the | ||
| # actual SHA-256 of the upstream tarball. Runs on every PR that touches the | ||
| # pin manifest or the postinstall scripts so version bumps are deliberate. | ||
| # | ||
| # Wish: dep-hygiene-and-resilience G4. | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: | ||
| - 'package.json' | ||
| - 'scripts/postinstall-tmux.js' | ||
| - 'scripts/postinstall-hook-binary.js' | ||
| - '.genie/wishes/dep-hygiene-and-resilience/binary-sha-bootstrap.md' | ||
| - '.github/workflows/binary-sha-drift.yml' | ||
| workflow_dispatch: {} | ||
|
|
||
| jobs: | ||
| drift-check: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Verify pinned SHA-256 against upstream | ||
| shell: bash | ||
| run: | | ||
| set -euo pipefail | ||
| # Extract every "<asset>": "<sha>" pair from package.json#binarySha256. | ||
| # We use jq to parse so the comment field "_comment" is naturally skipped | ||
| # (its value isn't a 64-hex string and our filter only emits matching pairs). | ||
| mapfile -t pairs < <(jq -r ' | ||
| (.binarySha256 // {}) as $b | ||
| | $b | to_entries[] | ||
| | select(.value | test("^[a-f0-9]{64}$"; "i")) | ||
| | "\(.key)\t\(.value | ascii_downcase)" | ||
| ' package.json) | ||
|
|
||
| if [[ ${#pairs[@]} -eq 0 ]]; then | ||
| echo "::warning::No pinned binarySha256 entries found in package.json — nothing to verify." | ||
| exit 0 | ||
| fi | ||
|
|
||
| fail=0 | ||
| tmpdir="$(mktemp -d)" | ||
| trap 'rm -rf "$tmpdir"' EXIT | ||
|
|
||
| for pair in "${pairs[@]}"; do | ||
| asset="${pair%%$'\t'*}" | ||
| pinned="${pair##*$'\t'}" | ||
|
|
||
| # Determine upstream URL based on asset name prefix. Currently only | ||
| # tmux-builds is wired up; extend this when other downloaders appear. | ||
| if [[ "$asset" == tmux-*.tar.gz ]]; then | ||
| # tmux-3.6a-linux-x86_64.tar.gz -> version 3.6a | ||
| ver="$(echo "$asset" | sed -E 's/^tmux-([^-]+)-.*/\1/')" | ||
| url="https://github.com/tmux/tmux-builds/releases/download/v${ver}/${asset}" | ||
| else | ||
| echo "::warning::Unknown asset prefix '$asset' — skipping (extend drift check to support it)." | ||
| continue | ||
| fi | ||
|
|
||
| echo "Checking $asset ..." | ||
| curl -sSL "$url" -o "$tmpdir/$asset" | ||
| actual="$(sha256sum "$tmpdir/$asset" | awk '{print $1}')" | ||
|
|
||
| if [[ "$actual" == "$pinned" ]]; then | ||
| echo " SHA matches: ${actual:0:12}..." | ||
| else | ||
| echo "::error::SHA drift for $asset" | ||
| echo " expected: $pinned" | ||
| echo " upstream: $actual" | ||
| fail=1 | ||
| fi | ||
| done | ||
|
|
||
| exit "$fail" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,15 +10,61 @@ | |
| */ | ||
|
|
||
| import { spawnSync } from 'node:child_process'; | ||
| import { chmodSync, copyFileSync, existsSync, mkdirSync, rmSync, unlinkSync, writeFileSync } from 'node:fs'; | ||
| import { createHash } from 'node:crypto'; | ||
| import { | ||
| chmodSync, | ||
| copyFileSync, | ||
| existsSync, | ||
| mkdirSync, | ||
| readFileSync, | ||
| rmSync, | ||
| unlinkSync, | ||
| writeFileSync, | ||
| } from 'node:fs'; | ||
| import { arch, homedir, platform, tmpdir } from 'node:os'; | ||
| import { join } from 'node:path'; | ||
| import { dirname, join, resolve as resolvePath } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| const TMUX_VERSION = '3.6a'; | ||
| const GENIE_HOME = process.env.GENIE_HOME || join(homedir(), '.genie'); | ||
| const BIN_DIR = join(GENIE_HOME, 'bin'); | ||
| const TMUX_PATH = join(BIN_DIR, 'tmux'); | ||
|
|
||
| // Resolve our own package.json so we can read the binarySha256 pin block. | ||
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
| const PKG_JSON_PATH = resolvePath(__dirname, '..', 'package.json'); | ||
|
|
||
| /** | ||
| * Look up the pinned SHA-256 for a downloaded asset. Returns: | ||
| * - { kind: 'pinned', sha256 } — pin found, MUST verify | ||
| * - { kind: 'unpinned' } — no pin block present (older install or local dev) | ||
| * - { kind: 'missing-key' } — pin block present but this asset key is absent (treat as fail) | ||
| * | ||
| * Pinning is mandatory in the wished design: a missing key for a downloaded | ||
| * asset is a hard failure, not a soft warning. The only soft case is when | ||
| * binarySha256 is entirely absent (e.g. running this script standalone outside | ||
| * the published package). | ||
| */ | ||
| function getPinnedSha(assetName) { | ||
| let pkg; | ||
| try { | ||
| pkg = JSON.parse(readFileSync(PKG_JSON_PATH, 'utf-8')); | ||
| } catch (err) { | ||
| return { kind: 'unpinned', reason: `cannot read ${PKG_JSON_PATH}: ${err.message}` }; | ||
| } | ||
| const block = pkg?.binarySha256; | ||
| if (!block || typeof block !== 'object') return { kind: 'unpinned' }; | ||
| const sha = block[assetName]; | ||
| if (typeof sha === 'string' && /^[a-f0-9]{64}$/i.test(sha)) { | ||
| return { kind: 'pinned', sha256: sha.toLowerCase() }; | ||
| } | ||
| return { kind: 'missing-key' }; | ||
| } | ||
|
|
||
| function sha256OfBuffer(buf) { | ||
| return createHash('sha256').update(buf).digest('hex'); | ||
| } | ||
|
|
||
| function getPlatformAsset() { | ||
| const os = platform(); | ||
| const cpu = arch(); | ||
|
|
@@ -73,6 +119,35 @@ async function downloadTmux() { | |
| const sizeMB = (buffer.byteLength / 1024 / 1024).toFixed(1); | ||
| console.error(`[genie] Downloaded ${sizeMB} MB`); | ||
|
|
||
| // SHA-256 verification BEFORE writing to disk and BEFORE chmod +x. | ||
| // Pin-or-fail: a tampered tarball or a download corruption aborts the | ||
| // install with a clear message. Same code path that produced empty | ||
| // bin/bun on 2026-05-05 is the path a tarball-swap attack would exploit. | ||
| const pin = getPinnedSha(asset); | ||
| const actual = sha256OfBuffer(buffer); | ||
| if (pin.kind === 'pinned') { | ||
| if (actual.toLowerCase() !== pin.sha256) { | ||
| console.error( | ||
| `[genie] Error: ${asset} SHA-256 mismatch — expected ${pin.sha256}, got ${actual}. Aborting install.`, | ||
| ); | ||
| return false; | ||
|
Comment on lines
+129
to
+133
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.
The new SHA check paths return Useful? React with 👍 / 👎. |
||
| } | ||
| console.error(`[genie] SHA-256 verified: ${actual.slice(0, 12)}…`); | ||
| } else if (pin.kind === 'missing-key') { | ||
| console.error(`[genie] Error: ${asset} has no SHA-256 pin in package.json#binarySha256. Aborting install.`); | ||
| console.error( | ||
| '[genie] Add the expected SHA-256 to the binarySha256 block. See .genie/wishes/dep-hygiene-and-resilience/binary-sha-bootstrap.md.', | ||
| ); | ||
| return false; | ||
|
Comment on lines
+137
to
+141
Contributor
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. Similar to the mismatch case, a missing mandatory pin should trigger a non-zero exit code to abort the installation. Setting console.error("[genie] Error: " + asset + " has no SHA-256 pin in package.json#binarySha256. Aborting install.");
console.error(
"[genie] Add the expected SHA-256 to the binarySha256 block. See .genie/wishes/dep-hygiene-and-resilience/binary-sha-bootstrap.md.",
);
process.exitCode = 1;
return false; |
||
| } else { | ||
| // Unpinned (e.g. running this script outside a published package). Soft warning; | ||
| // operators running from a clean clone may not have computed pins yet. | ||
| console.error( | ||
| `[genie] Warning: no binarySha256 block found in package.json — skipping SHA verification for ${asset}.`, | ||
| ); | ||
| console.error('[genie] This path is for local-dev only; published installs must pin.'); | ||
|
Comment on lines
+146
to
+148
Contributor
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. The warning message assumes the console.error(
"[genie] Warning: skipping SHA verification for " + asset + (pin.reason ? ": " + pin.reason : " (no binarySha256 block found in package.json)") + ".",
); |
||
| } | ||
|
|
||
| mkdirSync(tempDir, { recursive: true }); | ||
| const tarball = join(tempDir, asset); | ||
| writeFileSync(tarball, buffer); | ||
|
|
||
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.
To ensure the installation actually aborts on a SHA-256 mismatch as intended by the 'pin-or-fail' design, you should set a non-zero exit code. Returning
falsefromdownloadTmuxis insufficient because the script's entry point (line 198) does not check the return value. Usingprocess.exitCode = 1allows thefinallyblock to perform necessary cleanup before the process terminates.