feat(cactus): SHA-256 integrity verification for fetched model assets#530
Merged
Conversation
Introduces verifySha256, CactusIntegrityError, and a CACTUS_HASH_PLACEHOLDER sentinel that skips verification with a one-time warning during pre-release dev. Anchors the trust boundary for locally-executed model weights.
Per-asset CactusAssetSpec replaces the bare filename string. A module-load loop validates every non-placeholder hash via assertHexSha256 so malformed catalog entries fail at import time rather than at first fetch.
Verification runs BEFORE atomic rename so unverified bytes are never promoted to the cache location. Disk-cache hits are verified too; on mismatch the file is unlinked and the network path is retried. Adds inline path.resolve + startsWith containment checks at every fs call site (CodeQL js/path-injection does not trace through helpers), plus assertSafeModelId / assertSafeFilename allowlists at function boundaries as defense-in-depth.
Verification runs BEFORE cache.put so the Cache Storage entry never contains unverified bytes. Cache hits are verified on read; on mismatch the entry is deleted and the network path is retried. The replayed Response preserves content-type / content-length headers. Mirrors the Node path's assertSafeModelId / assertSafeFilename allowlists at function boundaries for parity, even though the browser variant does not touch the filesystem.
Surface the CactusIntegrityError message verbatim — the integrity layer already phrases SHA-256 vs byte-length mismatches correctly. Emits progress: undefined on the error-path phase event because there is no meaningful percentage to report (StreamPhase.progress is required).
Surface the CactusIntegrityError message verbatim — the integrity layer already phrases SHA-256 vs byte-length mismatches correctly. Emits progress: undefined on the error-path phase event because there is no meaningful percentage to report (StreamPhase.progress is required).
Covers sha256Hex against the RFC 6234 "abc" vector, verifySha256 happy path / mismatch / malformed-expected / placeholder-skip, and the CactusIntegrityError instance shape. Includes the ArrayBuffer-via- new ArrayBuffer(n) construction so the test exercises the concrete BufferSource branch of sha256Hex.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| `Path escape detected: ${JSON.stringify(mkdirTarget)} is not within ${JSON.stringify(safeRoot)}` | ||
| ); | ||
| } | ||
| await fs.mkdir(mkdirTarget, { recursive: true }); |
| `Path escape detected: ${JSON.stringify(writeTarget)} is not within ${JSON.stringify(resolvedDir)}` | ||
| ); | ||
| } | ||
| await fs.writeFile(writeTarget, bytes); |
| ); | ||
| } | ||
| await fs.writeFile(writeTarget, bytes); | ||
| const renameFrom = path.resolve(resolvedDir, `${spec.filename}.tmp`); |
| `Path escape detected: ${JSON.stringify(renameTo)} is not within ${JSON.stringify(resolvedDir)}` | ||
| ); | ||
| } | ||
| await fs.rename(renameFrom, renameTo); |
| `Path escape detected: ${JSON.stringify(renameTo)} is not within ${JSON.stringify(resolvedDir)}` | ||
| ); | ||
| } | ||
| await fs.rename(renameFrom, renameTo); |
| if (cleanupTarget !== resolvedDir && !cleanupTarget.startsWith(resolvedDir + path.sep)) { | ||
| throw err; | ||
| } | ||
| await fs.unlink(cleanupTarget).catch(() => {}); |
@workglow/cli
@workglow/ai
@workglow/browser-control
@workglow/indexeddb
@workglow/javascript
@workglow/job-queue
@workglow/knowledge-base
@workglow/mcp
@workglow/storage
@workglow/task-graph
@workglow/tasks
@workglow/util
workglow
@workglow/anthropic
@workglow/bun-webview
@workglow/cactus
@workglow/chrome-ai
@workglow/electron
@workglow/google-gemini
@workglow/huggingface-inference
@workglow/huggingface-transformers
@workglow/node-llama-cpp
@workglow/ollama
@workglow/openai
@workglow/playwright
@workglow/postgres
@workglow/sqlite
@workglow/supabase
@workglow/tf-mediapipe
commit: |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds asset integrity verification and path-safety hardening to the Cactus (needle-rs) provider so locally executed model assets are validated against catalog-pinned SHA-256 digests (with size pre-check support) across Node filesystem and browser Cache Storage.
Changes:
- Introduces
Cactus_Integrityutilities (sha256Hex,verifySha256,CactusIntegrityError) and integrates verification into browser/node asset fetch paths. - Extends the model catalog to describe assets as
{ filename, sha256, size }specs and validates non-placeholder hashes at module load. - Adds defense-in-depth path allowlists/containment checks and updates download flows to use the new asset spec shape.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| providers/cactus/src/ai/common/Cactus_Runtime.ts | Node+shared runtime: path containment sanitizers; verify size/hash before cache writes and atomic rename; supports asset specs. |
| providers/cactus/src/ai/common/Cactus_Runtime.browser.ts | Browser runtime: verifies cached/network assets before persisting to Cache Storage; supports asset specs. |
| providers/cactus/src/ai/common/Cactus_ModelCatalog.ts | Catalog schema changed to per-asset specs with SHA-256 + size; adds module-load hash validation and assetSpecsOf. |
| providers/cactus/src/ai/common/Cactus_Integrity.ts | New integrity module implementing SHA-256 hashing, verification, placeholder bypass warning, and error type. |
| providers/cactus/src/ai/common/Cactus_Download.ts | Updates download run-fn to iterate asset specs and surface integrity errors via phase events. |
| providers/cactus/src/ai/common/Cactus_Download.browser.ts | Same as node variant for browser bundle. |
| providers/cactus/src/ai/common/tests/Cactus_Integrity.test.ts | Adds unit tests for hashing/verification behavior and error shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| : path.resolve(models_dir); | ||
| const resolvedDir = path.resolve(safeRoot, model_id); | ||
| if (resolvedDir !== safeRoot && !resolvedDir.startsWith(safeRoot + path.sep)) { |
| if (hit) { | ||
| return new Uint8Array(await hit.arrayBuffer()); | ||
| const bytes = new Uint8Array(await hit.arrayBuffer()); | ||
| try { |
Comment on lines
+307
to
+321
| try { | ||
| await verifySha256(bytes, spec.sha256, { url: `file:${filePath}`, filename: spec.filename }); | ||
| return bytes; | ||
| } catch (err) { | ||
| if (err instanceof CactusIntegrityError) { | ||
| // On-disk asset is corrupt; evict and fall through to network. | ||
| const unlinkPath = path.resolve(resolvedDir, spec.filename); | ||
| if (unlinkPath !== resolvedDir && !unlinkPath.startsWith(resolvedDir + path.sep)) { | ||
| throw new Error( | ||
| `Path escape detected: ${JSON.stringify(unlinkPath)} is not within ${JSON.stringify(resolvedDir)}` | ||
| ); | ||
| } | ||
| await fs.unlink(unlinkPath).catch(() => {}); | ||
| } else { | ||
| throw err; |
Comment on lines
+354
to
+361
| try { | ||
| const writeTarget = path.resolve(resolvedDir, `${spec.filename}.tmp`); | ||
| if (writeTarget !== resolvedDir && !writeTarget.startsWith(resolvedDir + path.sep)) { | ||
| throw new Error( | ||
| `Path escape detected: ${JSON.stringify(writeTarget)} is not within ${JSON.stringify(resolvedDir)}` | ||
| ); | ||
| } | ||
| await fs.writeFile(writeTarget, bytes); |
Comment on lines
+137
to
+148
| try { | ||
| await verifySha256(bytes, spec.sha256, { url, filename: spec.filename }); | ||
| return bytes; | ||
| } catch (err) { | ||
| if (err instanceof CactusIntegrityError) { | ||
| try { | ||
| await cache.delete(url); | ||
| } catch { | ||
| /* best effort */ | ||
| } | ||
| } else { | ||
| throw err; |
Comment on lines
+1
to
+14
| /** | ||
| * @license | ||
| * Copyright 2026 Steven Roussey <sroussey@gmail.com> | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { describe, expect, it } from "vitest"; | ||
| import { | ||
| CACTUS_HASH_PLACEHOLDER, | ||
| CactusIntegrityError, | ||
| isHashPlaceholder, | ||
| sha256Hex, | ||
| verifySha256, | ||
| } from "../Cactus_Integrity"; |
Comment on lines
37
to
47
| @@ -18,13 +41,30 @@ export interface CactusCatalogEntry { | |||
| readonly hf_repo: string; | |||
| readonly revision: string; | |||
| readonly assets: { | |||
| readonly weights: string; | |||
| readonly vocab: string; | |||
| readonly config: string; | |||
| readonly weights: CactusAssetSpec; | |||
| readonly vocab: CactusAssetSpec; | |||
| readonly config: CactusAssetSpec; | |||
| }; | |||
…eQL-canonical)
The previous `startsWith(root + path.sep)` shape breaks when `root` is a
filesystem root ("/" on POSIX or a drive root like "C:\" on Windows):
the concatenated separator produces "//" / "C:\\" that no legitimate
child can match. Switch every inline check to the canonical
`path.relative` pattern, which is root-safe and the shape CodeQL's
`js/path-injection` query recognizes natively.
Both network paths already byte-length-check `spec.size` before calling `verifySha256`. The Node disk-read cache-hit path skipped it. Adding the pre-check inside the existing try-block lets the existing catch branch (which unlinks the on-disk asset and falls through to refetch) handle size and hash mismatches uniformly.
The browser network path already byte-length-checks `spec.size` before calling `verifySha256`. The Cache Storage hit path skipped it. Adding the pre-check inside the existing try-block lets the existing catch branch (which deletes the cache entry and falls through to refetch) handle size and hash mismatches uniformly.
| ); | ||
| } | ||
| } | ||
| const buf = await fs.readFile(readPath); |
`assertSafeFilename` previously allowed up to 255 chars, but the Node
atomic-write path writes to `${filename}.tmp` before renaming. A 252+
char filename would exceed the 255-byte per-component limit on common
filesystems (ext4, APFS, NTFS) when the `.tmp` suffix is appended.
Tighten the cap to 251 in both Cactus_Runtime.ts and
Cactus_Runtime.browser.ts so the browser-validated filename always
round-trips through the Node atomic-write path.
Apply the same `MAX_FILENAME_LEN = 251` cap as Cactus_Runtime.ts so a filename that validates in the browser also validates on Node — the Node atomic-write path appends `.tmp` (4 chars) and would otherwise exceed the 255-byte per-component limit on common filesystems.
Co-locates the integrity unit test with the other ai-provider-cactus tests under `packages/test/src/test/ai-provider-cactus/` so it runs in the same vitest project that the CI pipeline already invokes. The provider package itself does not run vitest in CI, so the test was otherwise unreachable.
Routes through `_testOnly` because `Cactus_Integrity` symbols are internal helpers — exposing them publicly would suggest they are part of the stable API surface, which they are not. Consumers should rely on the catalog's `sha256` field and the runtime's automatic verification.
The integrity helpers (sha256Hex, verifySha256, CactusIntegrityError, isHashPlaceholder, CACTUS_HASH_PLACEHOLDER) are pure/stateless and the unit test in packages/test/ needs to import them. Routing through `_testOnly` (rather than a public re-export) signals that they are internal — consumers should rely on the catalog's `sha256` field and the runtime's automatic verification.
Mirror the Node variant: re-export the integrity helpers via `_testOnly` on the browser entry point so the test can import them from either target. Pure/stateless helpers, internal only.
Coverage Report
File CoverageNo changed files found. |
sroussey
added a commit
that referenced
this pull request
May 23, 2026
* fix(cactus): catch only ENOENT in fetchAssetBytesNode
Previously the outer try/catch around the cache-read path swallowed every
filesystem error (EACCES, EIO, EISDIR, EMFILE, ...) and silently fell through
to the network refetch. That masked real misconfigurations as innocuous
cache misses, hiding the underlying cause from operators and forcing
unnecessary HuggingFace fetches.
The catch block now distinguishes:
- CactusIntegrityError re-throws (unreachable; handled inside),
- ENOENT falls through (the legitimate cache-miss case),
- any other code wraps the original error with a descriptive message
and `cause` so callers can see what actually went wrong.
Regression test in Cactus_Runtime.node.test.ts covers both branches by
driving the production code through real fs state — ENOENT via an empty
temp dir, and a non-ENOENT (EISDIR) by making the asset path a directory.
* fix(cactus): validate model_id inside fetchAssetBytesBrowser
`fetchAssetBytes` (the public entry) already called `assertSafeModelId` once
before delegating, but the helper `fetchAssetBytesBrowser` did not — so a
future refactor that hits the helper from a different code path could
slip a hostile model_id past the allowlist. The Node variant
`fetchAssetBytesNode` already re-validates at the call site; this brings
the browser helper to parity.
The helper now takes `model_id` as an explicit parameter and asserts it
as the first statement, matching the Node helper's pattern. The single
call site in `fetchAssetBytes` threads `model_id` through.
* fix(chrome-ai): touch chat session on each delta to defer idle eviction
The unified session store evicts idle entries after 30 minutes
(WEB_BROWSER_SESSION_IDLE_MS). Before this fix, a single long-running
chat turn whose stream exceeded the idle window would have its cached
session destroyed mid-stream by the idle timer, even though the model
was actively producing output. The next turn would then look up a
now-missing session.
The fix wires `touchWebBrowserSession(sessionId)` into the chat run-fn's
`trackingEmit` helper alongside the existing `deltaEmitted` bookkeeping,
so every `text-delta` event resets the idle clock. Idle eviction still
fires after 30 minutes of true silence (no further deltas), so the
mechanism is preserved — only active streams defer it.
Regression test in WebBrowser_Chat.idleTouch.test.ts: pre-populates the
cache, drives a controllable ReadableStream through two chunks separated
by 25 simulated minutes (50 min total elapsed), and asserts the session
is still cached. A second test confirms post-stream silence still evicts
at the 30-min mark.
* fix(cactus): add hash-catalog script + production placeholder guard
Three changes that close the loop on the SHA-256 integrity machinery
that landed in #530:
1. New `providers/cactus/scripts/hash-catalog.ts` (Bun) fetches every
asset URL referenced by `CACTUS_CATALOG`, computes `sha256Hex` and
byte length, prints a JSON report, and rewrites the catalog file in
place when `--write` is passed. Wired as `bun run hash-catalog` in
the cactus package. Re-running after a `revision` bump regenerates
everything; the matcher is conservative and only replaces
placeholder blocks so partially-populated catalogs are safe.
2. `Cactus_ModelCatalog.ts` gains a production guard: when
`NODE_ENV === "production"` or `CACTUS_REQUIRE_REAL_HASHES === "1"`,
module load throws if any asset still has the placeholder sha256
or a non-positive size. Dev/test stays permissive (no env var) so
contributors can iterate against `TODO_FILL_AT_RELEASE`. Exposes
`CATALOG_HAS_PLACEHOLDERS` so release tooling can opt-in to the
same check.
3. Catalog values populated with real hashes + sizes obtained by
running the script against the live HuggingFace asset URLs:
- needle.safetensors (22,259,039 bytes)
- vocab.txt (122,132 bytes)
- config.json (320 bytes)
With real sizes now in place, the network-fallthrough assertion in
`Cactus_Runtime.node.test.ts` is tightened to assert via the
downstream integrity rejection (which can only fire if fetch ran),
keeping the ENOENT-branch coverage intact.
Tests in `Cactus_ModelCatalog.test.ts` verify both env-var-gated
states by spawning a child Bun process with and without
CACTUS_REQUIRE_REAL_HASHES set.
* fix(test): cast WebBrowser_Chat input to AiChatProviderInput in idleTouch test
The run-fn only reads input.messages (and optionally input.temperature) at
runtime, but AiChatProviderInput requires model+prompt at the schema layer,
which the production dispatcher provides but is irrelevant for this unit test.
Match the dispatcher convention (any-typed input) via an explicit cast so the
test compiles under bun run build:types.
Unblocks CI build/CodeQL/publish-preview/vitest jobs that were skipping after
the build job failed on this TS2345.
---------
Co-authored-by: Claude <noreply@anthropic.com>
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.
Replaces closed PR #529 with HIGH-1 only. The runtime-state singleton (HIGH-2 in the original review) was overscoped — under the actual usage pattern (workers import
./ai-runtime, main imports./ai, never both in one realm), the cross-bundle desync doesn't manifest. The module-level Maps +_testOnlyaggregator the maintainer added in #524 are sufficient.Summary
Anchors the trust boundary for locally-executed model weights at the catalog: every byte loaded from disk, Cache Storage, or the network must hash to the catalog-pinned SHA-256. Anything else is refused.
Cactus_Integrity.tswithverifySha256,CactusIntegrityError, and aCACTUS_HASH_PLACEHOLDERsentinel that bypasses verification with a one-time warning during pre-release dev.CactusAssetSpecwith per-filesha256: string(lowercase hex, 64 chars) andsize: number(cheap byte-length pre-check).fetchAssetBytesNodeand BEFOREcache.putinfetchAssetBytesBrowser. Corrupt disk/cache entries are evicted and refetched.path.resolve+path.relativecontainment check at every fs call site, paired withassertSafeModelId/assertSafeFilenameallowlists. Closes the CodeQLjs/path-injectionflags on the asset cache path.assertHexSha256at catalog import time so a malformed entry fails fast.Breaking change — catalog asset shape
CactusCatalogEntry.assets.{weights,vocab,config}changes from a filenamestring to a
CactusAssetSpec({ filename: string; sha256: string; size: number }).External consumers that read
entry.assets.weightsas a filename string mustupdate to
entry.assets.weights.filename. Treat as a minor-or-major semverbump per the package's release convention.
Maintainer action required before release
The catalog asset hashes are placeholders (
CACTUS_HASH_PLACEHOLDER/"TODO_FILL_AT_RELEASE"). Populate with real SHA-256 + size values from the pinned HF revision before cutting a release. Recommend a follow-upproviders/cactus/scripts/hash-catalog.tsscript for automation.Recommend follow-up to require signed commits on
Cactus_ModelCatalog.tsso the trust boundary is anchored at the commit-signing layer.Test plan
Cactus_Integrity.test.tscoverssha256Hex(RFC 6234 "abc" vector),verifySha256happy path, mismatch, malformed expected, placeholder-skip, error class shape.Generated by Claude Code