From db18ce89e8def6fab4bcbe69fddf020c00286d6b Mon Sep 17 00:00:00 2001 From: Jordan Paulino Date: Thu, 2 Jul 2026 11:29:26 -0400 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=8D=95=20Harden=20browser-compat=20st?= =?UTF-8?q?ubs=20+=20add=20kiln-edit=20bundle=20smoke=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prevent the class of edit-mode crash where a Node-builtin browser stub is import-unsafe (throws at module evaluation) — the family the safe-buffer #253 crash belongs to — and add a real-bundle smoke test so these regress in CI instead of in an editor's console. #1 CI boot-smoke test (browser-compat.bundle.test.js) - Runs the real kiln-edit plugin pipeline (browser-compat + node-resolve + commonjs) over the libraries that have broken before (safe-buffer, create-hash) and evaluates the emitted bundle in a Buffer-less VM. A stub-shape regression now fails CI. #2 Prefer real browser globals over bespoke shims, at zero bundle cost - Codify the stub import-safety invariant and add a shared preferGlobal() helper (single source of truth for "use globalThis.X, else minimal shim"). - buffer/node-fetch already did this; apply it to url (globalThis.URL / URLSearchParams). No heavyweight polyfills added. Fixes surfaced by the smoke test - string_decoder was an empty stub, but cipher-base (crypto-browserify -> create-hash/createHmac) does `new StringDecoder(enc)` inside .digest(enc): an empty stub throws "StringDecoder is not a constructor" the instant a hash/HMAC is stringified in the browser. Promoted to a minimal rich stub whose StringDecoder delegates to Buffer.toString(encoding). - node:-prefixed rich builtins (node:buffer, node:stream, ...) were listed in SIMPLE_STUBS, forcing them to empty stubs and re-opening the subclass-at-eval crash. resolveId already normalizes the node: prefix, so the explicit entries are removed; rich node: forms now route to the rich stub. Adds safe-buffer + create-hash as devDependencies (smoke-test fixtures only). Co-authored-by: Cursor --- .../plugins/browser-compat.bundle.test.js | 171 ++++++++++++++++++ lib/cmd/vite/plugins/browser-compat.js | 138 +++++++++++--- lib/cmd/vite/plugins/browser-compat.test.js | 76 +++++++- package-lock.json | 12 +- package.json | 4 +- 5 files changed, 369 insertions(+), 32 deletions(-) create mode 100644 lib/cmd/vite/plugins/browser-compat.bundle.test.js diff --git a/lib/cmd/vite/plugins/browser-compat.bundle.test.js b/lib/cmd/vite/plugins/browser-compat.bundle.test.js new file mode 100644 index 0000000..8c3ee8d --- /dev/null +++ b/lib/cmd/vite/plugins/browser-compat.bundle.test.js @@ -0,0 +1,171 @@ +/* eslint-env jest */ + +'use strict'; + +const crypto = require('crypto'); +const vm = require('vm'); +const { rollup } = require('rollup'); +const { nodeResolve } = require('@rollup/plugin-node-resolve'); +const commonjs = require('@rollup/plugin-commonjs'); +const viteBrowserCompatPlugin = require('./browser-compat'); + +// Bundling real npm packages through Rollup is slower than a unit test. +jest.setTimeout(30000); + +// ── why this file exists ────────────────────────────────────────────────────── +// +// browser-compat.test.js unit-tests each stub in isolation. This file is the +// end-to-end guard: it runs the SAME plugin pipeline claycli uses for the +// kiln-edit bundle (browser-compat + node-resolve + commonjs) over the exact +// libraries that have crashed edit mode before, then evaluates the emitted +// bundle in a Buffer-less context that mimics the browser. +// +// The original crash (claycli#253) was `safe-buffer` running +// `SafeBuffer.prototype = Object.create(Buffer.prototype)` at module evaluation +// against a shape-broken buffer stub. `create-hash` is the real path that pulled +// safe-buffer into the kiln bundle (crypto-browserify → create-hash → …), and it +// also drags cipher-base/sha.js/hash-base/readable-stream, which subclass the +// stream and events stubs at eval. If any stub regresses to a non-import-safe +// shape, evaluating this bundle throws — turning "an editor sees a console error" +// into "CI is red". + +/** + * Provide a virtual entry module to Rollup so the fixture source lives inline + * (no temp files) while still resolving real packages from node_modules. + * + * @param {string} code - ESM source for the entry module + * @returns {object} a Rollup plugin + */ +function virtualEntry(code) { + const ID = '\0clay-smoke-entry'; + + return { + name: 'clay-smoke-entry', + resolveId(id) { + return id === ID ? ID : null; + }, + load(id) { + return id === ID ? code : null; + }, + }; +} + +/** + * Bundle an ESM entry through the real kiln-edit browser pipeline and return the + * emitted CJS code. + * + * @param {string} entryCode - ESM source importing the libraries under test + * @returns {Promise} the generated bundle source + */ +async function bundleForBrowser(entryCode) { + const bundle = await rollup({ + input: '\0clay-smoke-entry', + // Silence expected browser-bundle noise (circular deps in readable-stream, + // `this` rewrites in CJS). A stub-shape regression surfaces at eval, not here. + onwarn() {}, + plugins: [ + virtualEntry(entryCode), + viteBrowserCompatPlugin(), + nodeResolve({ browser: true, preferBuiltins: false, exportConditions: ['browser', 'default'] }), + commonjs({ transformMixedEsModules: true, requireReturnsDefault: 'preferred' }), + ], + }); + + try { + const { output } = await bundle.generate({ format: 'cjs', inlineDynamicImports: true, exports: 'named' }); + + return output[0].code; + } finally { + await bundle.close(); + } +} + +/** + * Evaluate a generated CJS bundle in an isolated context that mimics the browser. + * `globalThis` inside the context is the sandbox itself, so omitting `Buffer` + * forces the buffer stub's fallback path — exactly the browser condition that + * crashed edit mode. + * + * @param {string} code - the generated bundle source + * @param {object} [opts] + * @param {boolean} [opts.withBuffer=false] - expose a real global Buffer (tests the prefer-global path) + * @returns {object} the bundle's module.exports + */ +function evalBundle(code, opts) { + const withBuffer = Boolean(opts && opts.withBuffer); + const moduleObj = { exports: {} }; + const sandbox = { + module: moduleObj, + exports: moduleObj.exports, + console, + setTimeout, + clearTimeout, + TextEncoder, + TextDecoder, + process: { + env: {}, + browser: true, + argv: [], + version: 'v18.0.0', + versions: { node: '18.0.0' }, + platform: 'browser', + nextTick: (cb, ...args) => Promise.resolve().then(() => cb(...args)), + }, + }; + + // Only the prefer-global test provides a real Buffer; the default path leaves + // globalThis.Buffer undefined to reproduce the browser. + if (withBuffer) sandbox.Buffer = Buffer; + + vm.createContext(sandbox); + vm.runInContext(code, sandbox, { timeout: 5000 }); + + return moduleObj.exports; +} + +// safe-buffer is the exact library and version (5.2.1) behind claycli#253; +// create-hash is the real crypto path that pulls it into the kiln bundle. +const ENTRY = ` + import { Buffer as SafeBuffer } from 'safe-buffer'; + import createHash from 'create-hash'; + + export var safeBufferType = typeof SafeBuffer; + export var safeBufferFromWorks = typeof SafeBuffer.from === 'function'; + export function makeHash() { return createHash('sha256'); } + export function sha256Hex(input) { return createHash('sha256').update(input).digest('hex'); } +`; + +describe('browser-compat kiln-edit bundle smoke test', () => { + let bundledCode; + + beforeAll(async () => { + bundledCode = await bundleForBrowser(ENTRY); + }); + + it('evaluates the danger libs with no global Buffer without crashing (safe-buffer #253 regression)', () => { + let exports; + + // The whole point: a shape-broken stub throws HERE, at module evaluation. + expect(() => { exports = evalBundle(bundledCode, { withBuffer: false }); }).not.toThrow(); + expect(exports.safeBufferType).toBe('function'); + expect(exports.safeBufferFromWorks).toBe(true); + // Constructing a hash exercises the stream/events stubs (cipher-base/sha.js + // subclass them) and the string_decoder stub, and must not throw. We stop at + // construction on purpose: actually digesting needs a byte-capable Buffer + // (writeUInt32BE, toString(enc)) that the minimal fallback shim cannot + // provide — that path is covered, with a real Buffer, by the next test. + expect(() => exports.makeHash()).not.toThrow(); + }); + + // Real crypto (create-hash / crypto-browserify's createHmac) only computes a + // correct digest when the page exposes a real global Buffer; the buffer stub + // defers to it (rule 2). With no global Buffer, `.digest()` throws + // "writeUInt32BE is not a function" — so any browser crypto feature must ensure + // a Buffer global exists or use Web Crypto (crypto.subtle) instead. + it('computes a correct hash when a real global Buffer is present (prefer-global path is functional)', () => { + const exports = evalBundle(bundledCode, { withBuffer: true }); + const expected = crypto.createHash('sha256').update('abc').digest('hex'); + + expect(exports.sha256Hex('abc')).toBe(expected); + }); +}); diff --git a/lib/cmd/vite/plugins/browser-compat.js b/lib/cmd/vite/plugins/browser-compat.js index f185ba1..3630154 100644 --- a/lib/cmd/vite/plugins/browser-compat.js +++ b/lib/cmd/vite/plugins/browser-compat.js @@ -23,8 +23,32 @@ const path = require('path'); * * Most built-ins are imported but never called in browser code paths; an empty * object is sufficient. A handful of built-ins (events, stream, util, buffer, - * http, https) are subclassed or extended by npm packages — those need a richer - * stub that provides the correct prototype chain so inheritance works. + * string_decoder, http, https, url) are subclassed, instantiated, or extended by + * npm packages — those need a richer stub that provides the correct prototype + * chain (and any constructors consumers instantiate) so nothing throws. + * + * ── Stub import-safety invariant ───────────────────────────────────────────── + * + * Every stub MUST evaluate without throwing when imported. A stub is dead code + * at runtime (guarded by isNode/process.browser), but its module body still runs + * at bundle-eval time. Two rules keep that safe: + * + * 1. Constructor-like built-ins (buffer, events, stream classes, http.Agent) + * are exposed as FUNCTIONS with a real `.prototype`, never plain objects. + * Libraries subclass them (`X.prototype = Object.create(Buffer.prototype)`) + * and feature-detect methods at module-eval; a plain-object stub throws + * "Object prototype may only be an Object or null: undefined" and takes the + * whole bundle down before Kiln boots (the safe-buffer crash fixed in #253). + * + * 2. Prefer the browser's own global over a hand-written shim. A real + * `globalThis.Buffer` / `fetch` / `URL` is correct AND costs zero bundle + * bytes; fall back to a minimal function-shaped shim only when the page + * provides no global. preferGlobal() is the single source of truth for this + * pattern (buffer, node-fetch, url). + * + * browser-compat.bundle.test.js enforces rule 1 by bundling the libraries that + * have broken before (safe-buffer, create-hash) and evaluating the output with + * no global Buffer — a stub-shape regression fails CI, not an editor. * * ── node: prefix ──────────────────────────────────────────────────────────── * @@ -114,23 +138,37 @@ const path = require('path'); const VIRTUAL_PREFIX = '\0clay-vite-compat:'; // Modules that can be safely replaced with an empty object namespace. +// +// Bare specifiers only. `node:`-prefixed ids (node:fs, node:buffer, …) are +// normalized to their bare name in resolveId() and routed here or to RICH_STUBS, +// so the prefixed variants don't need listing. Critically, a rich built-in's +// node: form must NOT appear here: listing e.g. `node:buffer` would short-circuit +// it to an empty stub and re-introduce the subclass-at-eval crash (see the +// import-safety invariant above) that the rich buffer stub exists to prevent. const SIMPLE_STUBS = new Set([ 'assert', 'child_process', 'cluster', 'crypto', 'dgram', 'dns', 'domain', 'fs', 'module', 'net', 'os', 'path', 'perf_hooks', 'punycode', 'querystring', - 'readline', 'repl', 'string_decoder', 'sys', 'timers', 'tls', 'tty', 'url', + 'readline', 'repl', 'sys', 'timers', 'tls', 'tty', 'v8', 'vm', 'worker_threads', 'zlib', 'hiredis', - // node: prefix variants - 'node:path', 'node:fs', 'node:os', 'node:crypto', 'node:url', - 'node:stream', 'node:events', 'node:util', 'node:buffer', 'node:http', 'node:https', - 'node:assert', 'node:child_process', 'node:cluster', 'node:dgram', 'node:dns', - 'node:domain', 'node:module', 'node:net', 'node:perf_hooks', 'node:punycode', - 'node:querystring', 'node:readline', 'node:repl', 'node:string_decoder', 'node:sys', - 'node:timers', 'node:tls', 'node:tty', 'node:v8', 'node:vm', 'node:worker_threads', - 'node:zlib', ]); // Modules that need a richer stub because libraries extend/inherit from them. -const RICH_STUBS = new Set(['events', 'stream', 'util', 'buffer', 'http', 'https', 'node-fetch']); +const RICH_STUBS = new Set(['events', 'stream', 'util', 'buffer', 'string_decoder', 'http', 'https', 'node-fetch', 'url']); + +/** + * Build a "prefer the browser's real global, else fall back" expression for use + * inside a stub's ESM source. Centralizes rule 2 of the import-safety invariant + * (prefer a real, zero-cost browser global) so every constructor-like stub + * applies it identically instead of hand-rolling the globalThis check. + * + * @param {string} globalName - the global to prefer, e.g. 'Buffer', 'URL' + * @param {string} fallbackExpr - JS expression used when the global is absent + * @returns {string} an expression string such as + * "(typeof globalThis !== 'undefined' && globalThis.URL) ? globalThis.URL : (function URL() {})" + */ +function preferGlobal(globalName, fallbackExpr) { + return `(typeof globalThis !== 'undefined' && globalThis.${globalName}) ? globalThis.${globalName} : (${fallbackExpr})`; +} const EVENTS_STUB = ` function EventEmitter() { this._events = this._events || {}; this._maxListeners = 10; } @@ -258,12 +296,52 @@ _clayBuffer.alloc = function(size) { return new Uint8Array(size > 0 ? size : 0); _clayBuffer.allocUnsafe = function(size) { return new Uint8Array(size > 0 ? size : 0); }; _clayBuffer.allocUnsafeSlow = function(size) { return new Uint8Array(size > 0 ? size : 0); }; _clayBuffer.concat = function(list) { return (list || []).reduce(function(a, b) { return Array.from(a).concat(Array.from(b)); }, []); }; -var _Buffer = (typeof globalThis !== 'undefined' && globalThis.Buffer) ? globalThis.Buffer : _clayBuffer; +var _Buffer = ${preferGlobal('Buffer', '_clayBuffer')}; export var Buffer = _Buffer; export function SlowBuffer(size) { return _Buffer.alloc(size > 0 ? size : 0); } export default { Buffer: _Buffer, SlowBuffer: SlowBuffer }; `; +// Browser stub for Node's `url`. +// +// The browser ships the real WHATWG URL/URLSearchParams — prefer them (rule 2: +// correct and zero bundle cost). The fallbacks are function-shaped (rule 1) so +// importing `url` and subclassing/feature-detecting these never throws when no +// global exists (e.g. a non-DOM worker). Legacy url.parse()/format() are +// intentionally omitted — they were absent from the previous empty stub too, so +// this change is purely additive (it only adds the two WHATWG constructors). +const URL_STUB = ` +var _URL = ${preferGlobal('URL', 'function URL() {}')}; +var _URLSearchParams = ${preferGlobal('URLSearchParams', 'function URLSearchParams() {}')}; +export var URL = _URL; +export var URLSearchParams = _URLSearchParams; +export default { URL: _URL, URLSearchParams: _URLSearchParams }; +`; + +// Browser stub for Node's `string_decoder`. +// +// `string_decoder` is both a Node builtin AND a userland npm package, so browser- +// compat intercepts the bare specifier — which means an empty stub silently +// breaks any code that constructs a StringDecoder. cipher-base (the base class +// behind crypto-browserify's create-hash / createHmac) does +// `new StringDecoder(enc).write(buf)` inside `.digest('hex' | 'base64' | …)`; an +// empty stub throws "StringDecoder is not a constructor" the instant a hash or +// HMAC is stringified in the browser. (Caught by browser-compat.bundle.test.js.) +// +// This minimal StringDecoder delegates to the Buffer's own toString(encoding), so +// with a real global Buffer (rule 2) a one-shot `.digest(enc)` returns the right +// string. The full decoder's multi-byte streaming boundary handling is +// unnecessary here — digests write the whole buffer in a single call. +const STRING_DECODER_STUB = ` +export function StringDecoder(encoding) { this.encoding = encoding || 'utf8'; } +StringDecoder.prototype.write = function(buf) { + if (buf == null) return ''; + return typeof buf.toString === 'function' ? buf.toString(this.encoding) : String(buf); +}; +StringDecoder.prototype.end = function(buf) { return buf == null ? '' : this.write(buf); }; +export default { StringDecoder: StringDecoder }; +`; + // node-fetch v1/v2 have no browser field; stub to native fetch so server-only // dependencies (encoding → iconv-lite → safer-buffer) never enter the browser bundle. const NODE_FETCH_STUB = ` @@ -299,17 +377,31 @@ const VITE_BROWSER_EXTERNAL = '__vite-browser-external'; const EMPTY_MODULE = 'export default {}; export {};'; +// Map of rich built-in name → ESM stub source. A lookup table (rather than a +// switch) keeps loadRichStub flat as the set of rich stubs grows. Every name here +// must also be in RICH_STUBS so resolveId routes it to the rich branch. +const RICH_STUB_SOURCES = { + events: EVENTS_STUB, + stream: STREAM_STUB, + util: UTIL_STUB, + buffer: BUFFER_STUB, + url: URL_STUB, + string_decoder: STRING_DECODER_STUB, + http: HTTP_STUB, + https: HTTP_STUB, + 'node-fetch': NODE_FETCH_STUB, +}; + +/** + * Resolve a rich built-in's ESM stub source by bare module name. + * + * @param {string} mod - the bare module name (e.g. 'buffer', 'string_decoder') + * @returns {string} the ESM stub source, or the empty module for unknown names + */ function loadRichStub(mod) { - switch (mod) { - case 'events': return EVENTS_STUB; - case 'stream': return STREAM_STUB; - case 'util': return UTIL_STUB; - case 'buffer': return BUFFER_STUB; - case 'http': - case 'https': return HTTP_STUB; - case 'node-fetch': return NODE_FETCH_STUB; - default: return EMPTY_MODULE; - } + return Object.prototype.hasOwnProperty.call(RICH_STUB_SOURCES, mod) + ? RICH_STUB_SOURCES[mod] + : EMPTY_MODULE; } // Short-circuit Vite's `__vite-browser-external` virtual when lenient mode diff --git a/lib/cmd/vite/plugins/browser-compat.test.js b/lib/cmd/vite/plugins/browser-compat.test.js index 78fd33b..cf2fcae 100644 --- a/lib/cmd/vite/plugins/browser-compat.test.js +++ b/lib/cmd/vite/plugins/browser-compat.test.js @@ -62,8 +62,8 @@ describe('viteBrowserCompatPlugin', () => { const SIMPLE_BUILTINS = [ 'assert', 'child_process', 'cluster', 'crypto', 'dgram', 'dns', 'domain', 'fs', 'module', 'net', 'os', 'path', 'perf_hooks', - 'punycode', 'querystring', 'readline', 'repl', 'string_decoder', 'sys', - 'timers', 'tls', 'tty', 'url', 'v8', 'vm', 'worker_threads', 'zlib', + 'punycode', 'querystring', 'readline', 'repl', 'sys', + 'timers', 'tls', 'tty', 'v8', 'vm', 'worker_threads', 'zlib', 'hiredis', ]; @@ -75,15 +75,27 @@ describe('viteBrowserCompatPlugin', () => { }); it.each([ - 'node:path', 'node:fs', 'node:os', 'node:crypto', 'node:url', - 'node:stream', 'node:events', 'node:util', 'node:buffer', - ])('stubs node:-prefixed built-in "%s"', (id) => { + 'node:path', 'node:fs', 'node:os', 'node:crypto', 'node:assert', 'node:zlib', + ])('stubs node:-prefixed simple built-in "%s"', (id) => { const { resolvedId } = runPlugin(id); expect(resolvedId).not.toBeNull(); expect(resolvedId).toContain('simple:'); }); + // node:-prefixed rich built-ins must route to the rich stub, not an empty one. + // Otherwise a library subclassing e.g. node:buffer's Buffer crashes at module + // evaluation — the same class of bug as the safe-buffer #253 crash. + it.each([ + 'node:buffer', 'node:stream', 'node:events', 'node:util', + 'node:http', 'node:https', 'node:url', + ])('routes node:-prefixed rich built-in "%s" to the rich stub', (id) => { + const { resolvedId } = runPlugin(id); + + expect(resolvedId).not.toBeNull(); + expect(resolvedId).toContain('rich:'); + }); + it('loads an empty ESM namespace for simple stubs', () => { const { code } = runPlugin('fs'); @@ -92,7 +104,7 @@ describe('viteBrowserCompatPlugin', () => { }); describe('resolveId — rich stubs', () => { - const RICH_MODS = ['events', 'stream', 'util', 'buffer', 'http', 'https', 'node-fetch']; + const RICH_MODS = ['events', 'stream', 'util', 'buffer', 'string_decoder', 'http', 'https', 'node-fetch', 'url']; it.each(RICH_MODS)('stubs rich module "%s"', (id) => { const { resolvedId } = runPlugin(id); @@ -213,6 +225,58 @@ describe('viteBrowserCompatPlugin', () => { expect(code).toContain('globalThis.fetch'); }); + + it('url stub exports URL and URLSearchParams', () => { + const { code } = runPlugin('url'); + + expect(code).toContain('export var URL'); + expect(code).toContain('export var URLSearchParams'); + }); + + it('url stub prefers the real global URL/URLSearchParams when present', () => { + const { code } = runPlugin('url'); + const FakeURL = function URL() {}; + const FakeUSP = function URLSearchParams() {}; + const exports = evalEsmStub(code, { URL: FakeURL, URLSearchParams: FakeUSP }); + + expect(exports.URL).toBe(FakeURL); + expect(exports.URLSearchParams).toBe(FakeUSP); + }); + + it('url fallback URL/URLSearchParams are function-shaped when no global exists', () => { + const { code } = runPlugin('url'); + const exports = evalEsmStub(code, {}); // browser path: no global URL + + expect(typeof exports.URL).toBe('function'); + expect(typeof exports.URLSearchParams).toBe('function'); + expect(() => Object.create(exports.URL.prototype)).not.toThrow(); + }); + + // cipher-base (crypto-browserify → create-hash/createHmac) constructs a + // StringDecoder inside .digest(enc); an empty stub throws "not a constructor". + // The decoder delegates to the buffer's own toString(encoding). + it('string_decoder stub exposes a constructable StringDecoder that decodes via Buffer.toString', () => { + const { code } = runPlugin('string_decoder'); + const exports = evalEsmStub(code, {}); + const StringDecoder = exports.StringDecoder; + + expect(typeof StringDecoder).toBe('function'); + + const fakeBuf = { toString: enc => `decoded:${enc}` }; + const decoder = new StringDecoder('hex'); + + expect(decoder.write(fakeBuf)).toBe('decoded:hex'); + expect(decoder.end()).toBe(''); + }); + + // Regression: node:buffer must load the function-shaped buffer stub, never an + // empty one — an empty node:buffer stub reintroduces the #253 subclass crash. + it('node:buffer loads the function-shaped buffer stub, not an empty stub', () => { + const { code } = runPlugin('node:buffer'); + + expect(code).toContain('export var Buffer'); + expect(code).not.toBe('export default {}; export {};'); + }); }); describe('custom stubs (site-specific overrides)', () => { diff --git a/package-lock.json b/package-lock.json index f9e2d32..1f7fa3c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -102,11 +102,13 @@ "@babel/eslint-parser": "^7.14.7", "@babel/plugin-syntax-dynamic-import": "^7.8.3", "coveralls": "^3.0.0", + "create-hash": "^1.2.0", "eslint": "^7.21.0", "jest": "^29.0.0", "jest-fetch-mock": "^3.0.0", "jest-mock-console": "^1.0.0", - "mock-fs": "^4.8.0" + "mock-fs": "^4.8.0", + "safe-buffer": "^5.2.1" }, "engines": { "node": ">=20.0.0" @@ -8028,7 +8030,9 @@ }, "node_modules/create-hash": { "version": "1.2.0", + "resolved": "https://registry.npmjs.org/create-hash/-/create-hash-1.2.0.tgz", "integrity": "sha512-z00bCGNHDG8mHAkP7CtT1qVu+bFQUPjYq/4Iv3C3kWjTFV10zIjfSoeqXo9Asws8gwSHDGj/hl2u4OGIjapeCg==", + "license": "MIT", "dependencies": { "cipher-base": "^1.0.1", "inherits": "^2.0.1", @@ -18222,6 +18226,7 @@ }, "node_modules/safe-buffer": { "version": "5.2.1", + "resolved": "https://registry.npmjs.org/safe-buffer/-/safe-buffer-5.2.1.tgz", "integrity": "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ==", "funding": [ { @@ -18236,7 +18241,8 @@ "type": "consulting", "url": "https://feross.org/support" } - ] + ], + "license": "MIT" }, "node_modules/safe-regex": { "version": "1.1.0", @@ -27292,6 +27298,7 @@ }, "create-hash": { "version": "1.2.0", + "resolved": "https://registry.npmjs.org/create-hash/-/create-hash-1.2.0.tgz", "integrity": "sha512-z00bCGNHDG8mHAkP7CtT1qVu+bFQUPjYq/4Iv3C3kWjTFV10zIjfSoeqXo9Asws8gwSHDGj/hl2u4OGIjapeCg==", "requires": { "cipher-base": "^1.0.1", @@ -34628,6 +34635,7 @@ }, "safe-buffer": { "version": "5.2.1", + "resolved": "https://registry.npmjs.org/safe-buffer/-/safe-buffer-5.2.1.tgz", "integrity": "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ==" }, "safe-regex": { diff --git a/package.json b/package.json index d0fa6da..b7e4625 100644 --- a/package.json +++ b/package.json @@ -62,11 +62,13 @@ "@babel/eslint-parser": "^7.14.7", "@babel/plugin-syntax-dynamic-import": "^7.8.3", "coveralls": "^3.0.0", + "create-hash": "^1.2.0", "eslint": "^7.21.0", "jest": "^29.0.0", "jest-fetch-mock": "^3.0.0", "jest-mock-console": "^1.0.0", - "mock-fs": "^4.8.0" + "mock-fs": "^4.8.0", + "safe-buffer": "^5.2.1" }, "dependencies": { "@babel/core": "^7.24.3", From ab468713a8bf1621805bfc11794af2c64917fed3 Mon Sep 17 00:00:00 2001 From: Jordan Paulino Date: Thu, 2 Jul 2026 11:45:00 -0400 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=8D=95=20Drop=20the=20kiln-edit=20bun?= =?UTF-8?q?dle=20smoke=20test=20from=20this=20PR?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bundle smoke test guards claycli's own stubs against a fixed set of known libraries — it does not (and cannot) catch a sites change that pulls a new Node-builtin-dependent dependency into the kiln bundle, which is how edit mode actually broke. That guard belongs against a consuming site's real bundle (sites CI), not here. Removes browser-compat.bundle.test.js and its smoke-test-only devDependencies (safe-buffer, create-hash). Keeps the stub hardening it surfaced: string_decoder + url rich stubs, the node: routing fix, the import-safety invariant, and preferGlobal() — all covered by browser-compat.test.js. Co-authored-by: Cursor --- .../plugins/browser-compat.bundle.test.js | 171 ------------------ lib/cmd/vite/plugins/browser-compat.js | 10 +- package-lock.json | 12 +- package.json | 4 +- 4 files changed, 9 insertions(+), 188 deletions(-) delete mode 100644 lib/cmd/vite/plugins/browser-compat.bundle.test.js diff --git a/lib/cmd/vite/plugins/browser-compat.bundle.test.js b/lib/cmd/vite/plugins/browser-compat.bundle.test.js deleted file mode 100644 index 8c3ee8d..0000000 --- a/lib/cmd/vite/plugins/browser-compat.bundle.test.js +++ /dev/null @@ -1,171 +0,0 @@ -/* eslint-env jest */ - -'use strict'; - -const crypto = require('crypto'); -const vm = require('vm'); -const { rollup } = require('rollup'); -const { nodeResolve } = require('@rollup/plugin-node-resolve'); -const commonjs = require('@rollup/plugin-commonjs'); -const viteBrowserCompatPlugin = require('./browser-compat'); - -// Bundling real npm packages through Rollup is slower than a unit test. -jest.setTimeout(30000); - -// ── why this file exists ────────────────────────────────────────────────────── -// -// browser-compat.test.js unit-tests each stub in isolation. This file is the -// end-to-end guard: it runs the SAME plugin pipeline claycli uses for the -// kiln-edit bundle (browser-compat + node-resolve + commonjs) over the exact -// libraries that have crashed edit mode before, then evaluates the emitted -// bundle in a Buffer-less context that mimics the browser. -// -// The original crash (claycli#253) was `safe-buffer` running -// `SafeBuffer.prototype = Object.create(Buffer.prototype)` at module evaluation -// against a shape-broken buffer stub. `create-hash` is the real path that pulled -// safe-buffer into the kiln bundle (crypto-browserify → create-hash → …), and it -// also drags cipher-base/sha.js/hash-base/readable-stream, which subclass the -// stream and events stubs at eval. If any stub regresses to a non-import-safe -// shape, evaluating this bundle throws — turning "an editor sees a console error" -// into "CI is red". - -/** - * Provide a virtual entry module to Rollup so the fixture source lives inline - * (no temp files) while still resolving real packages from node_modules. - * - * @param {string} code - ESM source for the entry module - * @returns {object} a Rollup plugin - */ -function virtualEntry(code) { - const ID = '\0clay-smoke-entry'; - - return { - name: 'clay-smoke-entry', - resolveId(id) { - return id === ID ? ID : null; - }, - load(id) { - return id === ID ? code : null; - }, - }; -} - -/** - * Bundle an ESM entry through the real kiln-edit browser pipeline and return the - * emitted CJS code. - * - * @param {string} entryCode - ESM source importing the libraries under test - * @returns {Promise} the generated bundle source - */ -async function bundleForBrowser(entryCode) { - const bundle = await rollup({ - input: '\0clay-smoke-entry', - // Silence expected browser-bundle noise (circular deps in readable-stream, - // `this` rewrites in CJS). A stub-shape regression surfaces at eval, not here. - onwarn() {}, - plugins: [ - virtualEntry(entryCode), - viteBrowserCompatPlugin(), - nodeResolve({ browser: true, preferBuiltins: false, exportConditions: ['browser', 'default'] }), - commonjs({ transformMixedEsModules: true, requireReturnsDefault: 'preferred' }), - ], - }); - - try { - const { output } = await bundle.generate({ format: 'cjs', inlineDynamicImports: true, exports: 'named' }); - - return output[0].code; - } finally { - await bundle.close(); - } -} - -/** - * Evaluate a generated CJS bundle in an isolated context that mimics the browser. - * `globalThis` inside the context is the sandbox itself, so omitting `Buffer` - * forces the buffer stub's fallback path — exactly the browser condition that - * crashed edit mode. - * - * @param {string} code - the generated bundle source - * @param {object} [opts] - * @param {boolean} [opts.withBuffer=false] - expose a real global Buffer (tests the prefer-global path) - * @returns {object} the bundle's module.exports - */ -function evalBundle(code, opts) { - const withBuffer = Boolean(opts && opts.withBuffer); - const moduleObj = { exports: {} }; - const sandbox = { - module: moduleObj, - exports: moduleObj.exports, - console, - setTimeout, - clearTimeout, - TextEncoder, - TextDecoder, - process: { - env: {}, - browser: true, - argv: [], - version: 'v18.0.0', - versions: { node: '18.0.0' }, - platform: 'browser', - nextTick: (cb, ...args) => Promise.resolve().then(() => cb(...args)), - }, - }; - - // Only the prefer-global test provides a real Buffer; the default path leaves - // globalThis.Buffer undefined to reproduce the browser. - if (withBuffer) sandbox.Buffer = Buffer; - - vm.createContext(sandbox); - vm.runInContext(code, sandbox, { timeout: 5000 }); - - return moduleObj.exports; -} - -// safe-buffer is the exact library and version (5.2.1) behind claycli#253; -// create-hash is the real crypto path that pulls it into the kiln bundle. -const ENTRY = ` - import { Buffer as SafeBuffer } from 'safe-buffer'; - import createHash from 'create-hash'; - - export var safeBufferType = typeof SafeBuffer; - export var safeBufferFromWorks = typeof SafeBuffer.from === 'function'; - export function makeHash() { return createHash('sha256'); } - export function sha256Hex(input) { return createHash('sha256').update(input).digest('hex'); } -`; - -describe('browser-compat kiln-edit bundle smoke test', () => { - let bundledCode; - - beforeAll(async () => { - bundledCode = await bundleForBrowser(ENTRY); - }); - - it('evaluates the danger libs with no global Buffer without crashing (safe-buffer #253 regression)', () => { - let exports; - - // The whole point: a shape-broken stub throws HERE, at module evaluation. - expect(() => { exports = evalBundle(bundledCode, { withBuffer: false }); }).not.toThrow(); - expect(exports.safeBufferType).toBe('function'); - expect(exports.safeBufferFromWorks).toBe(true); - // Constructing a hash exercises the stream/events stubs (cipher-base/sha.js - // subclass them) and the string_decoder stub, and must not throw. We stop at - // construction on purpose: actually digesting needs a byte-capable Buffer - // (writeUInt32BE, toString(enc)) that the minimal fallback shim cannot - // provide — that path is covered, with a real Buffer, by the next test. - expect(() => exports.makeHash()).not.toThrow(); - }); - - // Real crypto (create-hash / crypto-browserify's createHmac) only computes a - // correct digest when the page exposes a real global Buffer; the buffer stub - // defers to it (rule 2). With no global Buffer, `.digest()` throws - // "writeUInt32BE is not a function" — so any browser crypto feature must ensure - // a Buffer global exists or use Web Crypto (crypto.subtle) instead. - it('computes a correct hash when a real global Buffer is present (prefer-global path is functional)', () => { - const exports = evalBundle(bundledCode, { withBuffer: true }); - const expected = crypto.createHash('sha256').update('abc').digest('hex'); - - expect(exports.sha256Hex('abc')).toBe(expected); - }); -}); diff --git a/lib/cmd/vite/plugins/browser-compat.js b/lib/cmd/vite/plugins/browser-compat.js index 3630154..6328777 100644 --- a/lib/cmd/vite/plugins/browser-compat.js +++ b/lib/cmd/vite/plugins/browser-compat.js @@ -46,9 +46,11 @@ const path = require('path'); * provides no global. preferGlobal() is the single source of truth for this * pattern (buffer, node-fetch, url). * - * browser-compat.bundle.test.js enforces rule 1 by bundling the libraries that - * have broken before (safe-buffer, create-hash) and evaluating the output with - * no global Buffer — a stub-shape regression fails CI, not an editor. + * Rule 1 is the safe-buffer/#253 crash: an incomplete stub shape throws at eval + * and takes the whole bundle down before Kiln boots. When adding or changing a + * stub, validate that it imports cleanly against the libraries that actually + * reach it (e.g. crypto-browserify's create-hash → safe-buffer chain), not just + * that the specifier resolves. * * ── node: prefix ──────────────────────────────────────────────────────────── * @@ -326,7 +328,7 @@ export default { URL: _URL, URLSearchParams: _URLSearchParams }; // behind crypto-browserify's create-hash / createHmac) does // `new StringDecoder(enc).write(buf)` inside `.digest('hex' | 'base64' | …)`; an // empty stub throws "StringDecoder is not a constructor" the instant a hash or -// HMAC is stringified in the browser. (Caught by browser-compat.bundle.test.js.) +// HMAC is stringified in the browser. // // This minimal StringDecoder delegates to the Buffer's own toString(encoding), so // with a real global Buffer (rule 2) a one-shot `.digest(enc)` returns the right diff --git a/package-lock.json b/package-lock.json index 1f7fa3c..f9e2d32 100644 --- a/package-lock.json +++ b/package-lock.json @@ -102,13 +102,11 @@ "@babel/eslint-parser": "^7.14.7", "@babel/plugin-syntax-dynamic-import": "^7.8.3", "coveralls": "^3.0.0", - "create-hash": "^1.2.0", "eslint": "^7.21.0", "jest": "^29.0.0", "jest-fetch-mock": "^3.0.0", "jest-mock-console": "^1.0.0", - "mock-fs": "^4.8.0", - "safe-buffer": "^5.2.1" + "mock-fs": "^4.8.0" }, "engines": { "node": ">=20.0.0" @@ -8030,9 +8028,7 @@ }, "node_modules/create-hash": { "version": "1.2.0", - "resolved": "https://registry.npmjs.org/create-hash/-/create-hash-1.2.0.tgz", "integrity": "sha512-z00bCGNHDG8mHAkP7CtT1qVu+bFQUPjYq/4Iv3C3kWjTFV10zIjfSoeqXo9Asws8gwSHDGj/hl2u4OGIjapeCg==", - "license": "MIT", "dependencies": { "cipher-base": "^1.0.1", "inherits": "^2.0.1", @@ -18226,7 +18222,6 @@ }, "node_modules/safe-buffer": { "version": "5.2.1", - "resolved": "https://registry.npmjs.org/safe-buffer/-/safe-buffer-5.2.1.tgz", "integrity": "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ==", "funding": [ { @@ -18241,8 +18236,7 @@ "type": "consulting", "url": "https://feross.org/support" } - ], - "license": "MIT" + ] }, "node_modules/safe-regex": { "version": "1.1.0", @@ -27298,7 +27292,6 @@ }, "create-hash": { "version": "1.2.0", - "resolved": "https://registry.npmjs.org/create-hash/-/create-hash-1.2.0.tgz", "integrity": "sha512-z00bCGNHDG8mHAkP7CtT1qVu+bFQUPjYq/4Iv3C3kWjTFV10zIjfSoeqXo9Asws8gwSHDGj/hl2u4OGIjapeCg==", "requires": { "cipher-base": "^1.0.1", @@ -34635,7 +34628,6 @@ }, "safe-buffer": { "version": "5.2.1", - "resolved": "https://registry.npmjs.org/safe-buffer/-/safe-buffer-5.2.1.tgz", "integrity": "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ==" }, "safe-regex": { diff --git a/package.json b/package.json index b7e4625..d0fa6da 100644 --- a/package.json +++ b/package.json @@ -62,13 +62,11 @@ "@babel/eslint-parser": "^7.14.7", "@babel/plugin-syntax-dynamic-import": "^7.8.3", "coveralls": "^3.0.0", - "create-hash": "^1.2.0", "eslint": "^7.21.0", "jest": "^29.0.0", "jest-fetch-mock": "^3.0.0", "jest-mock-console": "^1.0.0", - "mock-fs": "^4.8.0", - "safe-buffer": "^5.2.1" + "mock-fs": "^4.8.0" }, "dependencies": { "@babel/core": "^7.24.3",