🍕 Harden browser-compat stubs: string_decoder, url, node: import-safety

lib/cmd/vite/plugins/browser-compat.js

@@ -23,8 +23,34 @@ 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).
+ *
+ * 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 ────────────────────────────────────────────────────────────
  *
@@ -114,23 +140,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 +298,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.
+//
+// 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 +379,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

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)', () => {