Code review & security audit: fix 5 critical issues, clear all dependency advisories - #69
Merged
Merged
Conversation
VetKeys bundle encryption derived its AES-256-GCM key solely from the ICP principal ID, which is a public identifier written into the bundle header itself. decryptBundle read the principal back out and re-derived the key with no secret input, so anyone holding a bundle could decrypt it. The 210k PBKDF2 iterations stretched nothing. Introduce a v2 wire format (magic VKE2) whose key is derived from a caller-supplied secret, falling back to AGENTVAULT_BUNDLE_SECRET, with the principal and a random per-bundle salt bound in as KDF context. Encryption and decryption both fail loudly when no secret is available rather than silently degrading to a reconstructable key. Legacy v1 bundles (magic VKEB) remain decryptable so existing encrypted state can be recovered and re-encrypted; a warning is emitted on every v1 read. v1 is never produced any more. The previous tests asserted the broken behaviour (decryptBundle with no secret succeeding). They now assert the security property instead: a bundle cannot be decrypted without the secret, and cannot be re-keyed by tampering the embedded principal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
The "Shamir's Secret Sharing" used for threshold key derivation was not secret
sharing. generateParticipantSecret built each share as
`indexByte || fullMnemonic`, hex-encoded, so any single share disclosed the
master seed phrase in full. Supporting defects in the same path:
- the share was encrypted under a key derived (via PBKDF2) from the very
plaintext it was encrypting, making the ciphertext a confirmation oracle
for a guessed mnemonic rather than a protection;
- the loop emitted `threshold` shares instead of `totalParties`, so n
participants could never all be provisioned;
- masterCommitment was computed over the still-empty shares array, so it was
always the SHA-256 of nothing.
Add src/security/shamir.ts: a real GF(2^8) implementation (AES polynomial,
generator 3) that splits a secret byte-wise over random degree-(t-1)
polynomials and reconstructs by Lagrange interpolation at x=0. Shares carry a
non-zero x-coordinate and one y-value per secret byte. The leading coefficient
is resampled when zero so the polynomial degree — and hence the threshold —
cannot silently drop for a given byte.
Both copies of generateSecretShares (vetkeys.ts and the duplicate in types.ts)
now use it; the dead generateParticipantSecret/encryptShare helpers are
removed. Downstream consumers treat shares as opaque strings, so this does not
change any call-site contract.
Also stop returning seedPhrase from VetKeysClient.deriveThresholdKey in
types.ts — the sibling implementation in vetkeys.ts already withheld it.
Tests cover reconstruction from every t-of-n subset, non-recovery from t-1
shares, absence of the secret in any individual share, encoding round-trips,
and parameter validation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
Two independent problems in canister/agent.mo, both of which meant the ICP
deployment path could never have run against a real build of this canister.
1. The file did not compile at all.
A bad merge replaced the `/**` opening a module doc comment with a
`// ==================== Types ====================` marker, leaving ~20
orphaned ` * ` continuation lines that the parser read as operators, and
left behind a duplicate partial import block above it. Separately, the file
had no `actor` declaration whatsoever — every `public` member sat at module
scope. (memory-repo.mo declares `actor MemoryRepo` correctly; agent.mo never
did.)
Restore the comment opener, drop the duplicated imports, and wrap the body
in `actor AgentVault { ... }`. Module-scope declarations that must resolve at
compile time (imports, the management-canister interface, `mgmt`) stay above
the actor. The body is not re-indented, to keep the diff reviewable.
2. The mirror-replication surface had no access control.
setMirrorCanister, clearMirrorCanister, syncToMirror, syncFromMirror,
receiveSync and exportSyncState were all declared `public shared func`
*without binding the caller*, and expireStaleConsensusProposals was likewise
unguarded. receiveSync unconditionally assigned memories, tasks, context and
agentConfig, so any principal — including anonymous — could replace an
agent's entire state, bypassing both the kill switch and frozen mode.
syncToMirror let anyone force repeated inter-canister calls to a target of
their choosing, a cycle-drain primitive. exportSyncState returned every
memory, task and context entry to any caller.
Add a `isSyncPeer` check (primary and mirror deploy the same WASM and
register each other, so the check is symmetric) and an `assertSyncAllowed`
guard. Config changes are now owner-only; state-mutating syncs require
assertWriteAllowed; receiveSync and exportSyncState accept authorized
principals or the registered peer. The mirror stable vars move up beside the
other stable state so the guards can reference them without a forward
reference.
expireStaleConsensusProposals is split: the public entry point now requires
authorization, and the heartbeat calls a private expireStaleProposalsInternal
which has no caller to authorize.
Verified with the Motoko compiler (JS build): agent.mo goes from 144 parse
errors to 0, matching memory-repo.mo. Full semantic typechecking against
mo:base could not be run — the base package is not reachable from this
environment's network policy — so the type-level changes still need a
`dfx build` before release.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
24 of the 50 API routes performed no authentication. Each route opted *in* by
calling validateAuthToken itself, and only the 26 polytician/* routes did. The
gap included routes that write to the filesystem and shell out to dfx:
- POST /api/deployments took sourcePath and projectRoot from the request body
and ran packageAgent + deployAgent against them;
- POST /api/backups/import took an arbitrary inputPath (arbitrary file read,
plus a restore that writes into ~/.agentvault);
- POST /api/backups/export took an arbitrary outputPath (arbitrary file write);
- GET /api/wallets and /api/agents listed wallet and agent state.
Add webapp/src/middleware.ts covering /api/:path*, so routes are protected by
default and a new route cannot be forgotten into being public. PUBLIC_API_PATHS
is empty by design — every current endpoint reads or mutates agent, wallet,
backup or deployment state.
Replace the `!==` token comparison in lib/server/auth.ts with a comparison that
has no input-dependent early exit; the previous one leaked a prefix-matching
oracle to anyone able to time responses. Implemented in plain JS rather than
crypto.timingSafeEqual because the middleware runs on the Edge runtime, where
node:crypto is unavailable.
Add lib/server/paths.ts to confine caller-supplied backup locations to a backup
root (AGENTVAULT_BACKUP_DIR, default ~/.agentvault/backups), rejecting absolute
paths and traversal, and apply it in the import/export routes. Validate the
deploy request's `network` against the networks declared in dfx.json.
BREAKING: API clients must now send `Authorization: Bearer <token>` on every
/api route, and AGENTVAULT_POLYTICIAN_API_TOKEN must be set for the API to
serve any request at all. Backup paths are now interpreted relative to the
backup root rather than the process working directory.
The webapp typecheck reports 10 pre-existing errors, unchanged by this commit
(verified by comparing against a clean tree); none are in the files touched here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
restoreFromEncryptedZip ran `unzip -o` and then copied the extracted tree over
~/.agentvault with cpSync(..., { force: true }). unzip restores symlink entries
and stored paths verbatim, so a crafted backup could place a file — or a
symlink pointing at an arbitrary target — outside the extraction directory.
Validate entry names before extracting (rejecting absolute paths and any `..`
component) and scan the extracted tree for symlinks before the copy. Entry-name
validation alone is insufficient: a symlink can have an entirely benign name and
an escaping target.
Tests build real hostile archives with the `zip` CLI — a traversal entry patched
in at equal length so the zip stays structurally valid, and a symlink stored
with `zip -y` — and assert the restore refuses both. Verified that both tests
fail without the guards: with them removed, the symlink case reached the cpSync
and planted ~/.agentvault/stolen -> <external path> before failing later on a
missing WASM.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
…UL byte
Three unrelated correctness bugs.
1. `wallet export --format encrypted` produced permanently undecryptable files.
The handler serialized the envelope and then immediately overwrote it:
data = JSON.stringify(backup, null, 2);
data = JSON.stringify({ encrypted }); // drops iv + salt
Only the ciphertext reached disk, so the PBKDF2 salt and GCM IV needed to
derive the key were gone. Emit the envelope wallet-import actually reads
(metadata + `encrypted` ciphertext + `iv` + `salt`), without the plaintext
wallets. Also write the file 0o600 — it previously landed at 0o644 while the
command itself warned that it contains private keys — and exit non-zero on
failure instead of returning 0.
The existing export-encrypted tests reimplemented the crypto inline and never
called the handler (one asserted `expect(algorithm).toBe('aes-256-gcm')`).
Added a round trip that runs handleExport and decrypts the artifact the way
wallet-import does.
2. `cycles mint`, `cycles transfer` and `tokens transfer` always passed
undefined.
Each declared `.argument()` values but destructured them off an `options`
object. Commander passes declared arguments positionally, ahead of options,
so `mintCycles(undefined)` was called and the spinner read
"Minting undefined cycles...". Take the arguments positionally. Tests drive
the real Commander programs.
3. src/hypervault/pipeline.ts contained a literal NUL byte in a template
literal (`${r.table}<NUL>${id}`), written as a raw control character instead
of the `\0` escape. `file` classified the repo's largest source file as
binary data and ripgrep/grep skipped it entirely, hiding ~1000 lines from
codebase-wide search. Replaced with `\0`; runtime behaviour is identical. A
byte-level scan confirms it was the only affected file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
tests/wallet/export-json.test.ts called handleExport with the repo as the
working directory. handleExport writes to `<cwd>/backups`, so every `npm test`
run rewrote the tracked backups/test-backup.json with fresh timestamps and left
the working tree dirty. Point the working directory at a temp dir for the
duration, as the encrypted-export tests now do.
While there, replace the file's placebo assertions with real ones. It contained
checks like `expect(fs.existsSync(backupDir) || true).toBe(true)`,
`expect(files.length).toBeGreaterThanOrEqual(0)`, and a "Security Warnings"
suite that computed `const shouldWarn = hasPrivateKey && format === 'json'` and
asserted it was true — none of which could fail, and none of which inspected the
exported artifact. They now read the file back and assert its metadata, wallet
contents, filename handling and 0o600 mode.
.gitignore fixes:
- package-lock.json was listed under "Misc" while being tracked and required
by CI's `npm ci`. One `git rm --cached` away from breaking release; removed
from the ignore list to match reality (AGENTS.md already calls it
authoritative).
- added dist-cli/ and test-build/, which were committed build output (57
files), and .agentvault/, the per-project scaffolding `agentvault init`
creates.
Untracked those build outputs, the leftover .agentvault/.gitignore, and
backups/test-backup.json — the last of which held a `"privateKey": "0xabcdef…"`
field. It was test data, but a committed file shaped exactly like a leaked key
trips secret scanners, and nothing reads it now that the test writes to a temp
directory.
Neither dist-cli/ nor test-build/ is referenced by any script; package.json's
`files` allowlist and .npmignore already excluded them from the published
package.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
The webapp did not build. CI runs `npm run build` and `npm run typecheck` for
it on every push touching webapp/** or src/**, so this has been failing on main.
Three separate blockers:
1. Ten TypeScript errors. Most were null-safety in dashboard pages: the
`const hasCanister = entry.canister` idiom does not narrow `entry.canister`
for TypeScript, so five `entry.canister.x` accesses were unchecked — bind
the value instead of a truthiness flag. Similarly
`Boolean(selectedArweaveWallet) && !selectedArweaveWallet.hasJwk` does not
narrow; compare against undefined directly. Replaced an
`Object.fromEntries(...) as ArtifactPaths` cast with an explicit object, so
a missing key is now a compile error rather than a silent undefined.
2. `SyntaxError: Octal escape sequences are not allowed in template strings`
during page-data collection. @polkadot/util-crypto contains
`` `proving${'\0'}` ``, which is valid, but SWC's minifier folds the
interpolation into the literal as `proving\00` — an octal escape, illegal in
template strings. Added the @PolkaDot packages to serverExternalPackages so
they load from node_modules instead of going through the minifier.
3. src/packaging/state-format.ts failed to compile under the webapp's DOM lib:
a Uint8Array may be backed by a SharedArrayBuffer, which crypto.subtle.digest
does not accept. Copy into an ArrayBuffer-backed view. (The root tsconfig
does not pull in DOM types, which is why `npm run typecheck` passed.)
While in that function: its last-resort fallback computed a 32-bit
non-cryptographic hash and zero-padded it to 64 hex characters, producing a
value indistinguishable from a real SHA-256 digest with ~32 bits of entropy.
Since these checksums are used for integrity verification, it now throws
instead. The branch requires both Web Crypto and node:crypto to be missing,
which no supported runtime should hit.
Dependencies: `npm audit fix` plus `npm update` takes the root package from 5
high-severity advisories (undici, postcss, brace-expansion, js-yaml, nanoid) to
0, and brings every in-range direct dependency current. undici moves to 7.29.0,
which is the fix for the five undici advisories; the rest resolve transitively.
No package.json ranges needed to change.
Deferred (all semver-major, none with a known advisory against the pinned
version): chalk 6, commander 15, eslint 10, execa 10, inquirer 14, ora 9,
typescript 7, undici 8, @noble/curves 2, @types/node 26.
The webapp's own 3 high-severity advisories (postcss and sharp, both via
next 15) require a Next 16 major and are not addressed here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
Closes the webapp's remaining high-severity advisories — postcss (XSS via
unescaped </style>, and three sourceMappingURL path-traversal / arbitrary
.map-file disclosure issues) and sharp (four inherited libvips CVEs) — both of
which were reachable only through next@15. `npm audit` now reports 0
vulnerabilities in both the root package and the webapp.
Migration steps required:
- Removed the `eslint` key from next.config.ts. Next 16 dropped `next lint`
and the corresponding config, so `NextConfig` no longer accepts it and
typecheck failed on it. Linting is no longer part of `next build`, so
there is nothing left to opt out of; the `lint` script now invokes eslint
directly against the root config.
- Next 16 enables Turbopack by default and errors out when a custom `webpack`
config is present. This project needs one: the shared ../src tree uses
NodeNext-style `.js` specifiers for TypeScript sources, which webpack only
resolves via `resolve.extensionAlias`. Pinned `dev` and `build` to
`--webpack` rather than attempt a Turbopack migration that nothing here can
validate.
- Next regenerated tsconfig.json and next-env.d.ts (reformatting, plus
`jsx: react-jsx` and the `.next/dev/types` include). These are
framework-owned files.
Verified beyond a green build, since the webapp has no test suite: started the
production server and exercised it over HTTP. The dashboard renders (redirects
/ -> /canisters, correct document title), and the auth middleware added earlier
behaves correctly end to end — /api/agents returns 401 with no token, 401 with
a wrong token, and 200 with the configured token.
Deferred, both non-blocking: the `middleware` file convention is deprecated in
favour of `proxy` (still builds and runs, registering as "ƒ Proxy
(Middleware)"; `npx @next/codemod middleware-to-proxy .` applies the rename),
and the Turbopack migration above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
CI: `npm test` is `vitest run` with no `--coverage`, so
./coverage/coverage-final.json was never produced and the Codecov upload step
had been silently uploading nothing — masked by its `if: always()`. The
coverage provider named in vitest.config.ts (`v8`) was not installed either, so
`--coverage` would have failed outright.
Added @vitest/coverage-v8, a `test:coverage` script, and pointed CI at it.
Actual coverage is 36.98% of statements — worth knowing, given the suite's
1,599 tests. Also excluded dist-cli/ and test-build/ from the coverage report;
they are compiled copies of src/ and cli/ and would double-count.
Docs:
- docs/development/testing.md claimed "508 tests across 31 test files"
against an actual 1,599 across 92 — roughly a third of the suite. Corrected,
with a pointer to run `npm test` rather than trust the number, plus an
honest note about which subsystems have no tests at all.
- CHANGELOG.md carried three `## [Unreleased]` headings; two were empty merge
artifacts and are removed. The two conflicting `## [1.0.0]` entries
(2025-02-12 "Final Release" and 2025-02-10 "Phase 5", the latter sitting
above the 1.0.0-rc.* entries) are kept verbatim with a note explaining the
duplication, rather than guessing which shipped.
- Deleted fixup_REALEASE_PRD.md, byte-identical to fixup_1_0_OSS_release.md
and referenced by nothing.
- AGENTS.md: build-outputs list now includes test-build/ and states these are
untracked, and explains why the inert pnpm-workspace.yaml is still present
despite npm being authoritative.
Not addressed: CHANGELOG has no entries for 1.0.5 or 1.0.6 although
package.json is at 1.0.6. Inventing them is worse than flagging the gap.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
Covers the five critical findings none of the previous audit documents reported, the fixes applied across ten commits, the dependency work, and the technical debt left open with recommended next steps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RsEBvNTac38kaKuiCsq9aH
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
2 participants
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.
Full code review, security audit, dependency refresh and debt reduction. Report:
SECURITY_AUDIT_2026_08_16.md.Summary
Found five critical issues that no previous audit reported, two of which meant core subsystems could never have worked as documented.
npm auditnpm auditagent.moparseRoot typecheck and lint stay clean (0 errors).
Critical findings
canister/agent.mohas never compiled. It has noactordeclaration at all — everypublicmember sits at module scope — plus a broken block comment from a bad merge (a/**replaced by a// ==== Types ====marker, leaving orphaned*lines) and a duplicated import block. Verified with the Motoko compiler: 144 parse errors → 0. This corroborates two other findings:src/canister/actor.idl.tsdeclares ~30 methods absent from the canister, andicpClient.tssilently returns synthetic success on deploy errors.Mirror-sync had no access control.
receiveSync,syncFromMirror,syncToMirror,setMirrorCanister,clearMirrorCanisterandexportSyncStatewerepublic shared funcwithout binding the caller.receiveSyncunconditionally reassignedmemories,tasks,contextandagentConfig, so any principal — including anonymous — could replace an agent's entire state, bypassing the kill switch and frozen mode.syncToMirrorwas an unauthenticated cycle-drain primitive.agent.didomits all 27 of these methods, so the interface never revealed the surface.VetKeys bundle encryption provided no confidentiality. The AES key was derived solely from the ICP principal ID and salt — both stored in the bundle header.
decryptBundleread the principal back out and re-derived the key with no secret input. The 210k PBKDF2 iterations stretched nothing. The test suite asserted this broken behaviour."Shamir's Secret Sharing" embedded the full mnemonic in every share (
indexByte || fullMnemonic, hex-encoded). Also: shares encrypted under a key derived from their own plaintext,thresholdshares emitted instead oftotalParties, and a commitment computed over an empty array. No reconstruction path existed anywhere — the shares were write-only artifacts that leaked the master secret.24 of 50 webapp API routes were unauthenticated, including
POST /api/deployments(takessourcePath/projectRootfrom the body and shells out todfx) andPOST /api/backups/import(arbitrary file read). Nomiddleware.tsexisted; token comparison used!==.Also fixed
~/.agentvault/stolensymlink pointing outside the extraction directory, in the real home directory.JSON.stringify({ encrypted }), discarding the IV and salt. Silent total data loss. Also written0o644while warning it contains private keys.cycles mint,cycles transfer,tokens transferalways passedundefined(positional Commander args read off the options object).calculateChecksum's fallback forged a SHA-256-shaped value with ~32 bits of entropy.npm testrewrote a committed fixture on every run;package-lock.jsonwas gitignored while tracked and required by CI'snpm ci.Approach
Fixes are in eleven small commits, in priority order (security → correctness → dependencies → debt), each with tests. New tests verify Shamir reconstruction from every t-of-n subset and non-recovery from every t−1 subset; that hostile archives are refused (built with the real
zipCLI); that encrypted exports round-trip through the import path; and that the CLI arguments actually arrive. Where a fix replaced a placebo test, the report says so.Verification
Root: typecheck ✅, lint ✅ 0 errors, 1,599 tests ✅,
npm audit✅ 0,npm run build✅.Webapp: typecheck ✅, build ✅,
npm audit✅ 0.Canisters: parse-clean under the Motoko compiler.
Because the webapp has no test suite, the Next 16 upgrade was validated by running it — server start, dashboard render, and the full auth matrix (
401no token /401wrong token /200correct token).Breaking changes
Authorization: Bearerheader carrying the API token on every/apiroute, andAGENTVAULT_POLYTICIAN_API_TOKENmust be set for the API to serve any request. Backup paths are interpreted relative to a backup root rather than the process CWD.AGENTVAULT_BUNDLE_SECRET). Legacy v1 bundles stay readable so existing data can be recovered, with a warning on each read; v1 is never written again.--webpackpinned (Turbopack migration deferred),eslintconfig key removed,@polkadot/*externalized.Not done — needs follow-up
mo:basecould not be run (package unreachable under this environment's network policy). Adfx buildis required before release — parse-level correctness is verified, type-level is not.actor.idl.ts/agent.did/agent.mo— until then the deploy path stays non-functional.icpClient.ts's stub-mode fallback behind an explicit--allow-stubflag.transform = null, which trips the kill switch after three rounds by design of the IC's consensus over response headers.Remaining debt (crypto reimplemented in 11 files with divergent KDF params, circular wallet encryption, duplicated subsystems, unadopted path/atomic-write helpers, source-text assertions in tests) is catalogued with file references in the report.