Skip to content

fix(typescript): vendor ZapCode on every target and cap padStart/padEnd - #2909

Merged
chaliy merged 2 commits into
mainfrom
claude/project-thread-hcbtls
Nov 10, 2026
Merged

chaliy merged 2 commits into
mainfrom
claude/project-thread-hcbtls

Conversation

@chaliy

@chaliy chaliy commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Requested by Михайло · project thread

What changed

  • ts/node padStart/padEnd with a target length above 10,000,000 now fail the run with an allocation-limit error (exit != 0) instead of aborting the whole host process. Same cap upstream already applies to repeat.
  • Native and WASI builds now use the in-tree ZapCode 1.5.1 copy that the browser build already used (renamed zapcode_browser → zapcode_vendor). The registry zapcode-core dependency is gone. The fix is a marked bashkit fork: edit in that copy.
  • The drift check (scripts/check_zapcode_vendor.py, CI) lists fork edits explicitly and fails on any unlisted marker or drifted edit, on top of the existing upstream hash check.
  • Knowledge: ZapCode runtime "Vendored source" decision, TM-TS-025, oxc pin rationale, log entry.

Why

Security audit run-1 F5 (High), re-confirmed on main during the pre-release check: zapcode-core 1.5.1, which is still the latest on crates.io, collects pad.chars().cycle().take(target_len) unchecked. Because of that, 'a'.padStart(2**40) reserves about 1 TiB, and the failed allocation aborts the process along with every tenant in it. The only way to fix it is to patch ZapCode, and a crates.io fork (like bashkit-monty) isn't needed because the copy ships inside bashkit.

Before / After

Before, with a wheel built from main (BashTool(typescript=True)):

$ ts -e "console.log('a'.padStart(2**40).length)"
memory allocation of 1099511627775 bytes failed
(process aborted)

New test threat_ts_pad_huge_length_fails_cleanly before the fix: process didn't exit successfully ... (signal: 6, SIGABRT).
After: the test passes. Both methods fail cleanly with empty stdout, and normal padding ('7'.padStart(3,'0'), multi-char pad) still prints 007|abxyx.

Local runs: cargo clippy -p bashkit --features typescript --all-targets -D warnings is clean, the typescript/ts/zapcode lib and integration tests pass (348 + 337), the typescript_external_functions example runs, and the drift check passes (it fails when the fork edit is altered). Cargo.lock only loses zapcode-core.

Risk

  • Medium-low
  • Native TypeScript now compiles from the vendored source. It's the same upstream 1.5.1 code, byte-checked by hash, plus the clock shim, which is plain std::time on native. Wasm targets are covered by CI.
  • Public ZapcodeValue is now the vendored type on every target. It's the same shape, only the crate path differs.

Checklist

  • Tests added or updated
  • Backward compatibility considered (internal; no API shape change)

🤖 Generated with Claude Code

zapcode-core 1.5.1 collects padStart/padEnd padding with no size check, so 'a'.padStart(2**40) reserves ~1 TiB and the failed allocation aborts the host process with every tenant (security audit F5). Native builds now use the in-tree ZapCode copy the browser build already used (renamed zapcode_browser -> zapcode_vendor) and drop the registry crate; the copy carries a marked fork edit capping the target length at 10,000,000 like upstream repeat. The drift check lists fork edits and rejects unlisted markers. TM-TS-025.
@chaliy chaliy self-assigned this Nov 10, 2026
@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
bashkit 7277537 Commit Preview URL

Branch Preview URL
Nov 10 2026, 08:56 PM

@bashkit-pr-reviewer bashkit-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the vendored-ZapCode + padStart/padEnd cap change. The security goal holds: a huge length now returns AllocationLimitExceeded (exit 1, clean stderr) instead of aborting the host, the vendor inventory is hash-pinned with license retained and no host FS/net/process access, and the threat-model test covers both methods plus the happy path. Two worth-considering unit inconsistencies below; nothing blocking.

@bashkit-pr-reviewer

Copy link
Copy Markdown

Review summary (head 72775378)

Verdict: Sound fix, no blockers. Vendoring ZapCode 1.5.1 in-tree on every target plus a padStart/padEnd allocation cap resolves the host-abort DoS (TM-TS-025) the same way the existing repeat/concat caps do, and the new threat-model test plus the vendor-integrity check cover the changed behavior.

Findings: 2 × P2 (both posted inline, non-blocking) — the new cap compares a char-count target_len against byte-length s.len(), so non-ASCII receivers mis-pad vs real JS and a 4-byte pad char can allocate ~40 MB past the documented 10M budget. Suggested fix: cap resulting byte size like repeat does.

What I read beyond the diff: AGENTS.md (failing-test + decision-comment + knowledge rules — all followed), knowledge/runtimes/zapcode-runtime.md vendor section, zapcode_vendor README/license/upstream-sha256.json, vm/builtins.rs pad/repeat/concat/length/arg_int code, vm/mod.rs error propagation, builtins/typescript.rs error formatting (clean exit-1 path), lib.rs decision comment, scripts/check_zapcode_vendor.py, and the threat-model test. Verified the vendored tree has no std::fs/net/process/env usage. The second push only re-resolves the Cargo.lock merge conflict; the fix itself is unchanged.

Not reviewed: full per-hunk diff lines for Cargo.toml, .github/workflows/ci.yml, and Cargo.lock (saw only the file list); no build/test run per project setup (reasoned from code).

@chaliy
chaliy merged commit bfa0997 into main Nov 10, 2026
57 checks passed
@chaliy
chaliy deleted the claude/project-thread-hcbtls branch November 10, 2026 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant