fix(security): symlink rejection + accumulate-all-tampered-files in integrity verifier (#160) — 0.22.2#172
Conversation
…low-up) 0.1.1 brings three false-positive suppressions surfaced by `secretless-ai status` dogfooding inside this repo on 2026-04-29: - Block-comment marker recognition in isKnownExample: /*, <!--, -->, ''', """, JSDoc-continuation lines (^\s*\*) join // and #. - Localhost+demo-password DB connection allowlist with anchored localhost / 127.0.0.1 / [::1] host check (Phase 4.5 case-insensitive password match — Password123 no longer slips, IPv6 loopback recognized). - Bare 'fake' in PLACEHOLDER_INDICATORS (replaces 'fake_' / 'fake-' — catches sk-proj-fake1234567890abc... shape values). hackmyagent's CredVaultPlugin catalog at src/plugins/credvault.ts is unchanged. The 10-entry local subset stays the synchronous CJS detection source; the 0.1.1 isKnownExample additions live in the package and apply only when consumers call the package helpers directly (secretless-ai 0.16.4 does this). Lockstep test __tests__/plugins/credvault/lockstep.test.ts re-runs against 0.1.1 and stays green: every local pattern's regex.source + regex.flags continues to match exactly one entry in the package (the 0.1.1 diff was additive; it did not narrow any of the 56 regexes). Zero behavior change in runtime detection path. Self-scan: 89 → 89. Tests: 2054/2080 pass. Bumps: hackmyagent 0.22.0 → 0.22.1; @opena2a/credential-patterns 0.1.0 → 0.1.1 (exact pin per OpenA2A convention).
…ntegrity verifier (#160) (b) loadManifest now uses lstatSync at every probed manifest location and rejects any path that resolves to a symbolic link. Defends against post- install file-replacement attacks where an attacker swaps the manifest for a symlink pointing at attacker-controlled data. On rejection, writes INTEGRITY MANIFEST REJECTED: symlink at <path> to stderr and falls through to dev-mode CLEAN with explicit reason "Manifest rejected (symlink) -- dev mode" on the manifest_load check. (c) checkPackageIntegrity now accumulates all hash mismatches into a sorted list and emits "<count> files tampered: <first 5>, ...and N more" instead of short-circuiting on the first failure. Output is capped at 200 chars to defeat attacker log-flooding. Missing files still short- circuit (a missing file can hide further tamper, so operators need to know about it first). Drops the previously-discussed (a) baked-in signing key design. SLSA v1 provenance via npm Trusted Publishing is the canonical trust root for rebuild attacks; adding a long-lived MANIFEST_PRIVATE_KEY GHA secret next to OIDC weakens the supply chain rather than strengthens it. README and CHANGELOG updated with the explicit `npm view hackmyagent dist.attestations --json` verification path. Sigstore/cosign keyless filed as deferred future-Wave (#171). 18 new deterministic CI tests: - integrity-verifier-symlink-rejection.test.ts (7 cases) - integrity-verifier-multifile-tamper.test.ts (9 cases) - integrity-verifier-end-to-end-tamper.test.ts (2 cases, the load- bearing gate that copies dist/ to a tmpdir, tampers cli.js, runs node dist/cli.js --version, and asserts EXIT=3 + INTEGRITY CHECK FAILED) Tests: 2072/2098 pass. Self-scan: 98/100 unchanged. Manual smoke: cp dist/cli.js /tmp/x; echo>>dist/cli.js; node dist/cli.js --version; echo "EXIT=$?" -> EXIT=3 with stderr "Failed: package_integrity -- 1 files tampered: cli.js"
ML-DSA-44 benchmark (ubuntu-latest, c6i.xlarge-class)Budget (AIComply D17): sign p99 < 2.5ms, verify p99 < 1.5ms. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Security Review: PR #172
VERDICT: APPROVE
SUMMARY
This PR implements symlink rejection in loadManifest and accumulates all tampered files in checkPackageIntegrity as documented security hardening measures. I verified each change against the full source files for potential vulnerabilities. The symlink rejection uses lstatSync correctly (does not follow links), error handling includes explicit stderr output (avoiding silent failures), and the multi-file tamper accumulation has a 200-char hard cap to prevent log flooding. All mitigations are present in the implementation. The changes are test-covered with 18 new deterministic tests including an end-to-end tamper gate. No CRITICAL or HIGH security issues remain after verification.
FINDINGS
None — all potential issues have adequate mitigations in the same code:
Verified and cleared:
-
Symlink traversal risk (integrity-verifier.ts:131-158) — Uses
lstatSyncNOTstat, so symlinks are never followed. Broken symlinks correctly rejected viastat.isSymbolicLink()check before any target resolution. -
Path traversal in manifest loading (integrity-verifier.ts:125-127) — Candidate paths use
join(packageRoot, 'dist', MANIFEST_FILENAME)with constants, not user input. No dynamic path construction from external data. -
Unhandled exceptions in lstat (integrity-verifier.ts:135-149) — Wrapped in try/catch with explicit ENOENT handling and stderr output for non-ENOENT errors. No silent swallowing (checked against feedback_iife_after_parseasync_timing_trap.md requirement).
-
Array sort stability (integrity-verifier.ts:428) — Uses default
Array.prototype.sort()on strings which is deterministic (16-bit code-unit comparison per ECMAScript spec). Comment explicitly documents this (line 425-426). -
String slice out-of-bounds (integrity-verifier.ts:443-448) — All slice operations are guarded by budget calculations and capped at
HARD_CAP. No negative indices possible (budget checked<= 0before use). -
Race condition in module-scoped flag (integrity-verifier.ts:182, 592) — Single-threaded synchronous flow:
loadManifestsets flag →verifyAllreads + clears flag. Comment at line 589 explicitly documents "synchronous call ordering ensures no race."
Reviewed 9 files changed (48432 bytes)
1 participant
Closes #160 follow-ups (b) and (c). Drops (a) baked-HMAC per [CHIEF-CSR]+[CHIEF-CA] decision: SLSA v1 provenance via Trusted Publishing remains the canonical trust root.
Summary
loadManifest— defends against post-install file-replacement attacks where an attacker swaps the manifest for a symlink pointing at attacker-controlled data. UseslstatSync(NOTexistsSync+readFileSync) at every probed manifest location.checkPackageIntegrity— operators now see total count + first 5 paths in lexicographic order instead of first-failure short-circuit. Output capped at 200 chars.npm view hackmyagent dist.attestations --jsonas the canonical SLSA v1 trust-root verification.wontfix).This PR also folds in the previously-published 0.22.1 commit (
@opena2a/credential-patterns@0.1.1re-pin) which was tagged but not yet merged to main.Validation (load-bearing end-to-end gate)
Tests
Test plan
npm run buildpassesnpm test2072/2098 pass--version,--help,secure .,scan-soul test/,check express,explain AST-PROMPT-001)