Reading the entire binary into memory with fs.readFileSync can lead to high memory consumption or crashes if the binary is large (e.g., a full Postgres distribution). It is safer to use a streaming approach to compute the hash, even when using synchronous I/O.

export function sha256File(filePath) {
  const hash = crypto.createHash('sha256');
  const fd = fs.openSync(filePath, 'r');
  try {
    const buffer = Buffer.alloc(65536);
    let bytesRead;
    while ((bytesRead = fs.readSync(fd, buffer, 0, buffer.length)) !== 0) {
      hash.update(buffer.subarray(0, bytesRead));
    }
  } finally {
    fs.closeSync(fd);
  }
  return hash.digest('hex');
}

Include spawnSync in the imports to support a safer way of invoking psql that avoids shell expansion issues.

Using execSync with a template string and JSON.stringify(sql) is vulnerable to shell expansion. Specifically, the DO $$ blocks in the migration SQL will have $$ expanded by the shell (e.g., bash) to the current PID before reaching psql, causing a syntax error in Postgres. Using spawnSync with an argument array bypasses the shell and is much safer.

function pgQuery({ db, sql, captureStdout = false, port = CANONICAL_PORT }) {
  const env = { ...process.env, PGPASSWORD: process.env.PGPASSWORD || 'postgres' };
  const args = ['-h', '127.0.0.1', '-p', String(port), '-U', 'postgres', '-d', db, '-At', '-c', sql];
  const result = spawnSync('psql', args, { env, stdio: ['ignore', 'pipe', 'pipe'] });
  if (result.status !== 0) {
    const err = result.stderr?.toString() || result.error?.message || 'unknown error';
    throw new Error(`psql failed: ${err}`);
  }
  return captureStdout
    ? result.stdout.toString().trim()
    : result.stdout;
}

There is a discrepancy between the documentation (line 55) which specifies using the first 32 characters of the SHA256, and the implementation which only uses 16. Increasing this to 32 provides better collision resistance for the cache fingerprint and aligns with the design documentation.

The argument parser does not validate that a value follows the --bundle or --cosign-bin flags. If these are the last arguments provided, args[++i] will be undefined, which could lead to unexpected behavior or confusing error messages.

    else if (a === '--bundle') {
      opts.bundlePath = args[++i];
      if (opts.bundlePath === undefined) {
        process.stderr.write('pgserve verify: --bundle requires a path\n');
        return { exit: EXIT_INVOCATION };
      }
    } else if (a === '--cosign-bin') {
      opts.cosignBin = args[++i];
      if (opts.cosignBin === undefined) {
        process.stderr.write('pgserve verify: --cosign-bin requires a path\n');
        return { exit: EXIT_INVOCATION };
      }
    }

Prevent skip-sigstore cache hits from bypassing cosign checks

This early return accepts any valid cache token before evaluating --skip-sigstore, so a token created in skip mode (tier: self_signed) is later treated as a successful default verification. In practice, running pgserve verify <bin> --skip-sigstore once allows subsequent plain pgserve verify <bin> calls to exit 0 from cache without requiring bundle/cosign verification until the token expires, which weakens the default trust model.

Useful? React with 👍 / 👎.

Execute psql without shell interpolation for migration SQL

The migration SQL is injected into a shell command string, but the schema step includes a DO $$ ... $$ block; when passed through the shell, $$ is expanded to the shell PID (and escaped newlines are also altered), corrupting the SQL sent to psql. This causes the cosign metadata migration statement to fail at runtime on real upgrades, so the step can silently skip DBs instead of reliably applying the intended schema delta.

Useful? React with 👍 / 👎.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Doc / code mismatch on fingerprint length.

The JSDoc says "we use the realpath + sha256 first 32 chars" but the implementation uses sha256.slice(0, 16) (16 hex chars / 64 bits). Either is fine for cache-key disambiguation, but the comment should match the code so future readers don't reach for the wrong slice when adjusting collision tolerance.

 /**
  * Compute the cache fingerprint for a binary. We use the realpath +
- * sha256 first 32 chars so two distinct binaries get distinct cache
+ * sha256 first 16 chars so two distinct binaries get distinct cache
  * entries even if they share a directory layout, while keeping the
  * filename short enough to be readable in `ls`.
  */
 export function computeFingerprint(binaryRealpath, sha256) {
   return `${path.basename(binaryRealpath).replace(/[^A-Za-z0-9._-]/g, '_')}.${sha256.slice(0, 16)}`;
 }

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/commands/verify.js` around lines 53 - 61, The JSDoc for
computeFingerprint claims it uses the sha256 first 32 chars but the function
actually uses sha256.slice(0, 16); update the comment to match the
implementation (i.e. state "sha256 first 16 chars" or "first 16 hex chars / 64
bits"), or if you intended 32 chars change the implementation to use
sha256.slice(0, 32); ensure the symbols referenced are computeFingerprint,
binaryRealpath and sha256.slice so readers and future edits remain consistent.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Cache-write failures should not be reported as verification failures.

Both the --skip-sigstore branch (Lines 262-268) and the cosign branch (Lines 309-324) translate a writeCacheToken exception into { ok: false, reason: 'cache-write-failed' } + EXIT_VERIFY_FAILED (2). At that point verification has already succeeded — cosign returned ok, or the offline trust file matched. A transient ENOSPC / EACCES / quota error on ~/.local/state/pgserve/verified/<fp>.token then flips a successful verify into a "FAILED" outcome to operators and CI, and the documented exit-code semantics ("2 = verification failed") are violated.

The cache is already designed as a soft optimization (touchCacheToken swallows rewrite failures and returns null). Persistence should follow the same model: emit a warning and exit 0, or at the very least map to EXIT_INVOCATION/a distinct code so callers don't conflate it with a tampered binary.

   if (opts.skipSigstore) {
     ...
-    let cacheFile = null;
-    if (!opts.noCache) {
-      try {
-        cacheFile = writeCacheToken(payload, {});
-      } catch (err) {
-        emit(opts, { ok: false, reason: 'cache-write-failed', detail: err.message });
-        return EXIT_VERIFY_FAILED;
-      }
-    }
+    let cacheFile = null;
+    let cacheWriteError = null;
+    if (!opts.noCache) {
+      try {
+        cacheFile = writeCacheToken(payload, {});
+      } catch (err) {
+        cacheWriteError = err.message;
+        process.stderr.write(`pgserve verify: cache token write failed (${err.message}); continuing without cache\n`);
+      }
+    }
     emit(opts, {
       ok: true,
       cached: false,
       binary: binaryPath,
       identity,
       tier: 'self_signed',
       sha256,
       cacheFile,
+      cacheWriteError,
       skipSigstore: true,
     });
     return EXIT_OK;
   }

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/commands/verify.js` around lines 261 - 324, The code currently turns
writeCacheToken exceptions into a verification failure (emit ok:false + return
EXIT_VERIFY_FAILED); instead, treat cache-write failures as non-fatal: in both
the skip-sigstore branch (where the token is built earlier) and the
cosign-success branch (after verifyBinary returns ok and you call
buildTokenPayload/writeCacheToken), catch errors from writeCacheToken, call emit
to record a warning (preserve ok:true and verification details or include a
clear warning field/message and err.message), and return EXIT_OK (do not return
EXIT_VERIFY_FAILED); update the error-handling blocks that reference
writeCacheToken, emit, EXIT_VERIFY_FAILED so they instead log a warning and exit
success to match touchCacheToken’s soft-fail behavior.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

pg_constraint probe is missing a conrelid filter — false positive skips the constraint.

The IF NOT EXISTS guard only matches on conname. Any other table in the same database that coincidentally has a constraint named pgserve_meta_verified_tier_check will cause the DO block to skip applying the CHECK to pgserve_meta, leaving the tier column unconstrained with no warning.

-  '    SELECT 1 FROM pg_constraint',
-  `    WHERE conname = '${VERIFIED_TIER_CHECK_NAME}'`,
+  '    SELECT 1 FROM pg_constraint',
+  `    WHERE conname = '${VERIFIED_TIER_CHECK_NAME}'`,
+  `    AND conrelid = 'public.pgserve_meta'::regclass`,

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cosign/schema.js` around lines 46 - 58, The current IF NOT EXISTS check
only filters pg_constraint by conname (VERIFIED_TIER_CHECK_NAME) which can match
constraints on other tables and cause skipping; update the WHERE clause to also
restrict conrelid to the pgserve_meta table (e.g. add "AND conrelid =
'pgserve_meta'::regclass") so the check only detects an existing constraint on
pgserve_meta before skipping the ALTER TABLE that adds VERIFIED_TIER_CHECK_NAME
to pgserve_meta.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

fetchCosignBin installs an unverified binary — undermines the entire verification chain.

The downloaded cosign binary is written to ~/.pgserve/bin/cosign with no checksum or signature check. A MITM attack or a compromised GitHub CDN response could install a malicious cosign that always returns exit 0, silently making every verifyBinary call return ok: true. This is a trust-bootstrapping hole: the tool used to verify binaries is itself unverified.

Sigstore publishes per-release SHA-256 checksums alongside each release artifact. At minimum, the expected checksum for each platform/arch combination should be hardcoded next to COSIGN_RELEASE_VERSION and validated before fs.renameSync commits the binary.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cosign/verify-binary.js` around lines 123 - 136, fetchCosignBin currently
writes a downloaded cosign binary to COSIGN_BIN_FILE without integrity checking;
add a hardcoded map of expected SHA-256 checksums keyed by release version +
asset name (use COSIGN_RELEASE_VERSION and the asset name from
pickCosignAssetName) and after downloadToFileSync(tmp) compute the file's
SHA-256, compare it to the expected value, and if it does not match, remove the
tmp file and throw/log an error; only call fs.chmodSync(tmp, 0o755) and
fs.renameSync(tmp, targetFile) when the checksum matches. Ensure errors include
the expected vs actual checksum and reference fetchCosignBin,
pickCosignAssetName, COSIGN_RELEASE_VERSION, downloadToFileSync, and
COSIGN_BIN_FILE so reviewers can locate the change.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

spawnSync for cosign has no timeout — can block the process indefinitely.

Even with --bundle, cosign verifies the proof of transparency log inclusion artifact and its certificate, which can involve network calls to TUF or Rekor endpoints. Without a timeout, any network stall hangs the entire Node/Bun process with no recovery path.

   const proc = spawnSync(cosignBin, args, {
     encoding: 'utf8',
     stdio: ['ignore', 'pipe', 'pipe'],
+    timeout: 30_000, // ms — cosign may reach TUF/Rekor; surface ETIMEDOUT rather than hanging
   });

Add a corresponding result.error?.code === 'ETIMEDOUT' check in invokeCosign and surface a distinct reason: 'cosign-timeout' from verifyBinary so callers can distinguish network hangs from signature failures.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cosign/verify-binary.js` around lines 265 - 268, spawnSync(cosignBin,
args, ...) is called without a timeout which can hang the process during network
stalls; update this by passing a timeout option to spawnSync and handle the
timeout in invokeCosign: detect result.error?.code === 'ETIMEDOUT' (or
equivalent) and return a distinct error shape, then have verifyBinary map that
case to an error with reason: 'cosign-timeout' so callers can distinguish
network timeouts from signature failures; touch the spawnSync call site
(cosignBin, args), the invokeCosign error handling, and verifyBinary's returned
error mapping to implement this.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Missing await — both .rejects assertions are never evaluated.

Without await, the test function resolves before the rejection Promises settle. Both assertions always pass regardless of what applyVerifiedColumns actually does.

-    expect(applyVerifiedColumns(null)).rejects.toThrow(/query/);
-    expect(applyVerifiedColumns({})).rejects.toThrow(/query/);
+    await expect(applyVerifiedColumns(null)).rejects.toThrow(/query/);
+    await expect(applyVerifiedColumns({})).rejects.toThrow(/query/);

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/cosign/schema.test.js` around lines 91 - 94, The two assertions in the
test for applyVerifiedColumns use .rejects but are missing await, so the test
exits before the rejection is evaluated; update the test (the case referencing
applyVerifiedColumns) to await the expect(...).rejects.toThrow(...) calls (or
return the promises) for both null and {} inputs so the rejection matchers are
actually executed and the test fails/fails correctly when applyVerifiedColumns
does not reject.