Skip to content

fix(js): require branded token for filesystem external imports#1602

Closed
chaliy wants to merge 1 commit intomainfrom
2026-05-08-propose-fix-for-filesystem-external-vulnerability
Closed

fix(js): require branded token for filesystem external imports#1602
chaliy wants to merge 1 commit intomainfrom
2026-05-08-propose-fix-for-filesystem-external-vulnerability

Conversation

@chaliy
Copy link
Copy Markdown
Contributor

@chaliy chaliy commented May 8, 2026

Motivation

  • The Node External import path accepted arbitrary N-API External values and then interpreted their raw pointer as a bashkit filesystem ABI handle, allowing non-bashkit externals to reach unsafe native import code.
  • This can lead to invalid memory reads, vtable dereferences, or invocation of attacker-controlled callbacks when an External from another native addon is misinterpreted.
  • Mitigate at the JS boundary by ensuring only tokens minted by this module's export path are forwarded to native import.

Description

  • Add an in-process branded token wrapper implemented with a module-local WeakSet and FileSystemExternalToken interface to mark provenance in crates/bashkit-js/wrapper.ts.
  • Implement createFileSystemExternalToken and isFileSystemExternalToken and use them so FileSystem.toExternal() returns a branded frozen token and FileSystem.fromExternal() rejects any value not created by toExternal().
  • Change FileSystem.external cached type to the branded token and unwrap the native external only after JS-level validation.
  • Update the JS test expectation in crates/bashkit-js/__test__/vfs.spec.ts to assert the new explicit provenance error message.

Testing

  • Attempted cargo check -p bashkit-js, but the invocation failed in this environment while fetching/compiling unrelated dependencies; the change is TypeScript-only and does not touch the native import path.
  • Ran the JS test command for the updated spec, but ava is not installed in this environment so the test run could not be executed here.
  • Changes were committed locally with message fix(js): require branded token for filesystem external imports.

Codex Task

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 8, 2026

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 040ea3a Commit Preview URL May 08 2026, 09:16 AM

@chaliy chaliy force-pushed the 2026-05-08-propose-fix-for-filesystem-external-vulnerability branch from 39e6486 to c870e71 Compare May 8, 2026 09:09
@chaliy chaliy force-pushed the 2026-05-08-propose-fix-for-filesystem-external-vulnerability branch from c870e71 to 040ea3a Compare May 8, 2026 09:16
Copy link
Copy Markdown
Contributor Author

chaliy commented May 8, 2026

This PR's branded-token approach breaks the documented public ABI interop with downstream NAPI consumers.

The fixture crates/bashkit-js/test-fixtures/random-fs/ produces an External via the supported bashkit::interop::fs::export_filesystem ABI from a separate native addon. After this change, FileSystem.fromExternal(...) rejects that External because the token's WeakSet is module-local and only contains tokens minted by bashkit-js itself. The interop.spec.ts test fails on node 20/22/24/latest, confirming the regression against legitimate downstream usage.

Even setting interop aside, the JS-layer brand check is bypassable: an attacker who can pass a value through this code path can equally well call bashkit-js's own toExternal() first and forward the result. The unsafe-pointer concerns from #1579 must be mitigated at the native ABI layer (e.g. magic/version validation inside import_filesystem), not in wrapper.ts.

Closing pending a redesign.


Generated by Claude Code

@chaliy chaliy closed this May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant