Don't proxy primitive instanceof checks by default

mrbbot · mrbbot · commit 720794accee7 · 2022-01-03T12:57:07.000Z
This was initially added to make it easier to run Rust workers in Miniflare, as `wasm-bindgen` frequently generates this code. However, the implementation caused `Object` prototype/constructor checks to fail in JavaScript. The previous behaviour can be restored with the `--proxy-primitive`/`miniflare.proxy_primitive_instanceof`/ `proxyPrimitiveInstanceOf` CLI/`wrangler.toml`/API option. Closes #109 Closes #137 Closes #141 Closes cloudflare/workers-sdk#91
diff --git a/packages/cli-parser/test/help.spec.ts b/packages/cli-parser/test/help.spec.ts
@@ -56,6 +56,7 @@ Core Options:
      --global-timers         Allow setting timers outside handlers     [boolean]
      --global-random         Allow secure random generation outside    [boolean]
                              handlers
+     --proxy-primitive       Proxy primitives' instanceof (for WASM)   [boolean]
 
 Test Options:
  -b, --boolean-option        Boolean option                            [boolean]
diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts
@@ -697,8 +697,12 @@ export class MiniflareCore<
       const setupWatch = this.#setupWatch!.get(name);
       if (setupWatch) addAll(newWatchPaths, setupWatch);
     }
-    const { modules, processedModuleRules, logUnhandledRejections } =
-      this.#instances!.CorePlugin;
+    const {
+      modules,
+      processedModuleRules,
+      logUnhandledRejections,
+      proxyPrimitiveInstanceOf,
+    } = this.#instances!.CorePlugin;
 
     // Clean up process-wide promise rejection event listeners
     this.#globalScope?.[kDispose]();
@@ -734,7 +738,8 @@ export class MiniflareCore<
         globalScope,
         script,
         rules,
-        additionalModules
+        additionalModules,
+        proxyPrimitiveInstanceOf
       );
 
       this.#scriptWatchPaths.clear();
diff --git a/packages/core/src/plugins/core.ts b/packages/core/src/plugins/core.ts
@@ -102,6 +102,7 @@ export interface CoreOptions {
   globalAsyncIO?: boolean;
   globalTimers?: boolean;
   globalRandom?: boolean;
+  proxyPrimitiveInstanceOf?: boolean;
 }
 
 function mapMountEntries(
@@ -339,6 +340,15 @@ export class CorePlugin extends Plugin<CoreOptions> implements CoreOptions {
   })
   globalRandom?: boolean;
 
+  @Option({
+    type: OptionType.BOOLEAN,
+    name: "proxy-primitive",
+    description: "Proxy primitives' instanceof (for WASM)",
+    logName: "Proxy Primitives' instanceof",
+    fromWrangler: ({ miniflare }) => miniflare?.proxy_primitive_instanceof,
+  })
+  proxyPrimitiveInstanceOf?: boolean;
+
   readonly processedModuleRules: ProcessedModuleRule[] = [];
 
   readonly upstreamURL?: URL;
@@ -473,6 +483,76 @@ export class CorePlugin extends Plugin<CoreOptions> implements CoreOptions {
       // could be used as an escape hatch if behaviour needs to be different
       // locally for any reason
       MINIFLARE: true,
+
+      // Object, Array, Promise, RegExp, Error, EvalError, RangeError,
+      // ReferenceError, SyntaxError, TypeError, URIError and Function are
+      // intentionally omitted. There's a trade-off here, ideally we'd like all
+      // these checks to pass:
+      //
+      // ```js
+      // import vm from "vm";
+      //
+      // const ctx1 = vm.createContext({ objectFunction: () => ({}) });
+      //
+      // vm.runInContext("({}) instanceof Object", ctx1); // true
+      // vm.runInContext("({}).constructor === Object", ctx1); // true
+      //
+      // vm.runInContext("objectFunction() instanceof Object", ctx1); // false
+      // vm.runInContext("objectFunction().constructor === Object", ctx1); // false
+      //
+      // const ctx2 = vm.createContext({ Object: Object, objectFunction: () => ({}) });
+      //
+      // vm.runInContext("({}) instanceof Object", ctx2); // false
+      // vm.runInContext("({}).constructor === Object", ctx2); // false
+      //
+      // vm.runInContext("objectFunction() instanceof Object", ctx2); // true
+      // vm.runInContext("objectFunction().constructor === Object", ctx2); // true
+      // ```
+      //
+      // wasm-bindgen (a tool used to make compiling Rust to WebAssembly easier),
+      // often generates code that looks like `value instanceof Object`.
+      // We'd like this check to succeed for both objects generated outside of
+      // the worker (e.g. KV), and inside user code. This is what the
+      // `proxyPrimitiveInstanceOf` option is for. It replaces `Object` with
+      // a proxy that overrides `Symbol.hasInstance` with a cross-realm check:
+      //
+      // ```js
+      // function isObject(value) {
+      //   return value !== null && typeof value === "object";
+      // }
+      //
+      // const ObjectProxy = new Proxy(Object, {
+      //   get(target, property, receiver) {
+      //     if (property === Symbol.hasInstance) return isObject;
+      //     return Reflect.get(target, property, receiver);
+      //   },
+      // });
+      //
+      // const ctx3 = vm.createContext({
+      //   Object: ObjectProxy,
+      //   objectFunction: () => ({}),
+      // });
+      //
+      // vm.runInContext("({}) instanceof Object", ctx3); // true
+      // vm.runInContext("({}).constructor === Object", ctx3); // false
+      //
+      // vm.runInContext("objectFunction() instanceof Object", ctx3); // true
+      // vm.runInContext("objectFunction().constructor === Object", ctx3); // false
+      // ```
+      //
+      // The problem with this option (it used to be the default) is that the
+      // `constructor`/`prototype` checks fail. These are used quite a lot in
+      // JS, and this was the cause of several issues:
+      // - https://github.com/cloudflare/miniflare/issues/109
+      // - https://github.com/cloudflare/miniflare/issues/137
+      // - https://github.com/cloudflare/miniflare/issues/141
+      // - https://github.com/cloudflare/wrangler2/issues/91
+      //
+      // The new default behaviour (omitting `Object` completely) still has
+      // the issue `constructor`/`prototype`/`instanceof` checks for `Object`s
+      // created outside the sandbox (e.g. KV) would fail, but I think that's
+      // less likely to be an issue, since the return types are always known
+      // in those cases.
     };
 
     // Process module rules if modules mode was enabled
diff --git a/packages/core/test/index.spec.ts b/packages/core/test/index.spec.ts
@@ -677,6 +677,61 @@ test("MiniflareCore: #reload: only runs script if module exports needed when scr
   await mf.getPlugins(); // Allow script to run
   t.true(calledback);
 });
+test("MiniflareCore: #reload: primitive constructor/prototype/instanceof checks succeed", async (t) => {
+  const mf = useMiniflareWithHandler(
+    { BindingsPlugin },
+    { globals: { outsideObject: {} } },
+    (globals) => {
+      const tests = {
+        // Simulating wasm-bindgen
+        outsideInstanceOf: globals.outsideObject instanceof globals.Object,
+        insideInstanceOf: {} instanceof globals.Object,
+
+        // https://github.com/cloudflare/miniflare/issues/109
+        // https://github.com/cloudflare/miniflare/issues/141
+        outsideConstructor:
+          globals.outsideObject.constructor === globals.Object,
+        insideConstructor: {}.constructor === globals.Object,
+
+        // https://github.com/cloudflare/miniflare/issues/137
+        newObject: new globals.Object({ a: 1 }),
+
+        // https://github.com/cloudflare/wrangler2/issues/91
+        outsidePrototype:
+          Object.getPrototypeOf(globals.outsideObject) ===
+          globals.Object.prototype,
+        insidePrototype: Object.getPrototypeOf({}) === globals.Object.prototype,
+      };
+      return new globals.Response(JSON.stringify(tests), {
+        headers: { "Content-Type": "application/json" },
+      });
+    }
+  );
+
+  let res = await mf.dispatchFetch("http://localhost/");
+  t.deepEqual(await res.json(), {
+    outsideInstanceOf: false, // :(
+    insideInstanceOf: true,
+    outsideConstructor: false, // :(
+    insideConstructor: true,
+    newObject: { a: 1 },
+    outsidePrototype: false, // :(
+    insidePrototype: true,
+  });
+
+  // Check with proxyPrimitiveInstanceOf enabled (for Rust development)
+  await mf.setOptions({ proxyPrimitiveInstanceOf: true });
+  res = await mf.dispatchFetch("http://localhost/");
+  t.deepEqual(await res.json(), {
+    outsideInstanceOf: true,
+    insideInstanceOf: true,
+    outsideConstructor: false,
+    insideConstructor: false,
+    newObject: {}, // This is super strange
+    outsidePrototype: true,
+    insidePrototype: false,
+  });
+});
 test("MiniflareCore: #reload: watches files", async (t) => {
   const log = new TestLog();
   const tmp = await useTmp(t);
diff --git a/packages/core/test/plugins/bindings.spec.ts b/packages/core/test/plugins/bindings.spec.ts
@@ -508,6 +508,8 @@ test("BindingsPlugin: propagates error if service throws", async (t) => {
           script: `export default {
             fetch: () => { throw new Error("oops"); },
           }`,
+          // Required for instanceOf assertion below to work
+          proxyPrimitiveInstanceOf: true,
         },
       },
     }
diff --git a/packages/core/test/plugins/core.spec.ts b/packages/core/test/plugins/core.spec.ts
@@ -63,6 +63,7 @@ test("CorePlugin: parses options from argv", (t) => {
     "--global-async-io",
     "--global-timers",
     "--global-random",
+    "--proxy-primitive",
   ]);
   t.deepEqual(options, {
     scriptPath: "script.js",
@@ -105,6 +106,7 @@ test("CorePlugin: parses options from argv", (t) => {
     globalAsyncIO: true,
     globalTimers: true,
     globalRandom: true,
+    proxyPrimitiveInstanceOf: true,
   });
   options = parsePluginArgv(CorePlugin, [
     "-c",
@@ -157,6 +159,7 @@ test("CorePlugin: parses options from wrangler config", async (t) => {
         global_async_io: true,
         global_timers: true,
         global_random: true,
+        proxy_primitive_instanceof: true,
       },
     },
     configDir
@@ -210,6 +213,7 @@ test("CorePlugin: parses options from wrangler config", async (t) => {
     globalAsyncIO: true,
     globalTimers: true,
     globalRandom: true,
+    proxyPrimitiveInstanceOf: true,
   });
   // Check build upload dir defaults to dist
   options = parsePluginWranglerConfig(
@@ -243,6 +247,7 @@ test("CorePlugin: logs options", (t) => {
     globalAsyncIO: true,
     globalTimers: true,
     globalRandom: true,
+    proxyPrimitiveInstanceOf: true,
   });
   t.deepEqual(logs, [
     // script is OptionType.NONE so omitted
@@ -263,6 +268,7 @@ test("CorePlugin: logs options", (t) => {
     "Allow Global Async I/O: true",
     "Allow Global Timers: true",
     "Allow Global Secure Random: true",
+    "Proxy Primitives' instanceof: true",
   ]);
   // Check logs default wrangler config/package paths
   logs = logPluginOptions(CorePlugin, {
diff --git a/packages/jest-environment-miniflare/src/index.ts b/packages/jest-environment-miniflare/src/index.ts
@@ -169,12 +169,18 @@ export default class MiniflareEnvironment implements JestEnvironment {
       }
     );
 
+    const plugins = await mf.getPlugins();
     const mfGlobalScope = await mf.getGlobalScope();
     mfGlobalScope.global = global;
     mfGlobalScope.self = global;
     // Make sure Miniflare's global scope is assigned to Jest's global context,
     // even if we didn't run a script because we had no Durable Objects
-    Object.assign(global, makeProxiedGlobals(/* blockCodeGeneration */ false));
+    if (plugins.CorePlugin.proxyPrimitiveInstanceOf) {
+      Object.assign(
+        global,
+        makeProxiedGlobals(/* blockCodeGeneration */ false)
+      );
+    }
     Object.assign(global, mfGlobalScope);
 
     // Add a way of getting bindings in modules mode to allow seeding data.
diff --git a/packages/jest-environment-miniflare/test/fixtures/autoload/autoload.spec.js b/packages/jest-environment-miniflare/test/fixtures/autoload/autoload.spec.js
@@ -6,6 +6,13 @@ test("auto-loads package.json and wrangler.toml", async () => {
   expect(await res.text()).toBe("test");
 });
 
+test("respects proxy_primitive_instanceof option", async () => {
+  // Should be auto-loaded from wrangler.toml
+  const { OBJECT } = getMiniflareBindings();
+  expect(OBJECT instanceof Object).toBe(true);
+  expect({} instanceof Object).toBe(true);
+});
+
 test("auto-loads .env", () => {
   const { ENV_KEY } = getMiniflareBindings();
   expect(ENV_KEY).toBe("value");
diff --git a/packages/jest-environment-miniflare/test/fixtures/autoload/wrangler.toml b/packages/jest-environment-miniflare/test/fixtures/autoload/wrangler.toml
@@ -5,3 +5,6 @@ bindings = [
 
 [build.upload]
 format = "modules"
+
+[miniflare]
+proxy_primitive_instanceof = true
diff --git a/packages/jest-environment-miniflare/test/fixtures/core.worker.spec.js b/packages/jest-environment-miniflare/test/fixtures/core.worker.spec.js
@@ -39,3 +39,7 @@ describe("uses Jest fake timers", () => {
 test("can generate secure random numbers", () => {
   crypto.getRandomValues(new Uint8Array(8));
 });
+
+test("Object constructor checks succeed", () => {
+  expect({}.constructor === Object).toBe(true);
+});
diff --git a/packages/runner-vm/src/index.ts b/packages/runner-vm/src/index.ts
@@ -46,7 +46,8 @@ export class VMScriptRunner implements ScriptRunner {
     globalScope: Context,
     blueprint: ScriptBlueprint,
     modulesRules?: ProcessedModuleRule[],
-    additionalModules?: AdditionalModules
+    additionalModules?: AdditionalModules,
+    proxyPrimitiveInstanceOf = false
   ): Promise<ScriptRunnerResult> {
     // If we're using modules, make sure --experimental-vm-modules is enabled
     if (modulesRules && !("SourceTextModule" in vm)) {
@@ -59,9 +60,11 @@ export class VMScriptRunner implements ScriptRunner {
     const linker =
       modulesRules && new ModuleLinker(modulesRules, additionalModules ?? {});
 
-    // Add proxied globals so cross-realm instanceof works correctly.
-    // globalScope will be fresh for each call of run so it's fine to mutate it.
-    Object.assign(globalScope, makeProxiedGlobals(this.blockCodeGeneration));
+    if (proxyPrimitiveInstanceOf) {
+      // Add proxied globals so cross-realm instanceof works correctly.
+      // globalScope will be fresh for each call of run so it's fine to mutate it.
+      Object.assign(globalScope, makeProxiedGlobals(this.blockCodeGeneration));
+    }
 
     let context = this.context;
     if (context) {
diff --git a/packages/runner-vm/src/proxied.ts b/packages/runner-vm/src/proxied.ts
@@ -38,6 +38,8 @@ function proxyHasInstance<T extends object>(
   });
 }
 
+// See `CorePlugin` in `/packages/core/src/plugins/core.ts` for an explanation
+// of what this function does and why it's needed.
 // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
 export function makeProxiedGlobals(blockCodeGeneration?: boolean) {
   if (!blockCodeGeneration) blockCodeGeneration = undefined;
diff --git a/packages/runner-vm/test/index.spec.ts b/packages/runner-vm/test/index.spec.ts
@@ -1,6 +1,7 @@
 import fs from "fs/promises";
 import path from "path";
 import { VMScriptRunner } from "@miniflare/runner-vm";
+import { ScriptBlueprint } from "@miniflare/shared";
 import test, { ThrowsExpectation } from "ava";
 
 const fixturesPath = path.join(__dirname, "..", "..", "test", "fixtures");
@@ -110,17 +111,20 @@ test("VMScriptRunner: run: allows dynamic code generation if enabled", async (t)
   t.pass();
 });
 test("VMScriptRunner: run: supports cross-realm instanceof", async (t) => {
-  const result = await runner.run(
-    { outsideRegexp: /a/ },
-    {
-      code: `
+  const global = { outsideRegexp: /a/ };
+  const blueprint: ScriptBlueprint = {
+    code: `
       export const outsideInstanceOf = outsideRegexp instanceof RegExp;
       export const insideInstanceOf = /a/ instanceof RegExp;
       `,
-      filePath: "test.js",
-    },
-    []
-  );
+    filePath: "test.js",
+  };
+
+  let result = await runner.run({ ...global }, blueprint, [], undefined, true);
   t.true(result.exports.outsideInstanceOf);
   t.true(result.exports.insideInstanceOf);
+
+  result = await runner.run({ ...global }, blueprint, []);
+  t.false(result.exports.outsideInstanceOf);
+  t.true(result.exports.insideInstanceOf);
 });
diff --git a/packages/shared/src/runner.ts b/packages/shared/src/runner.ts
@@ -38,6 +38,7 @@ export interface ScriptRunner {
     globalScope: Context,
     blueprint: ScriptBlueprint,
     modulesRules?: ProcessedModuleRule[],
-    additionalModules?: AdditionalModules
+    additionalModules?: AdditionalModules,
+    proxyPrimitiveInstanceOf?: boolean
   ): Promise<ScriptRunnerResult>;
 }
diff --git a/packages/shared/src/wrangler.ts b/packages/shared/src/wrangler.ts
@@ -71,6 +71,7 @@ export interface WranglerEnvironmentConfig {
     global_async_io?: boolean;
     global_timers?: boolean;
     global_random?: boolean;
+    proxy_primitive_instanceof?: boolean;
   }; // inherited
 }
 

Original file line number	Diff line number	Diff line change
`@@ -508,6 +508,8 @@ test("BindingsPlugin: propagates error if service throws", async (t) => {`
`508`	`508`	script: `export default {
`509`	`509`	`fetch: () => { throw new Error("oops"); },`
`510`	`510`	}`,
	`511`	`+ // Required for instanceOf assertion below to work`
	`512`	`+ proxyPrimitiveInstanceOf: true,`
`511`	`513`	`},`
`512`	`514`	`},`
`513`	`515`	`}`