Skip to content
This repository was archived by the owner on Mar 13, 2025. It is now read-only.

Commit a353f62

Browse files
committed
Override Symbol.hasInstances without Proxy
1 parent 63befac commit a353f62

File tree

9 files changed

+201
-113
lines changed

9 files changed

+201
-113
lines changed

packages/core/src/plugins/core.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,11 @@ export class CorePlugin extends Plugin<CoreOptions> implements CoreOptions {
473473
// could be used as an escape hatch if behaviour needs to be different
474474
// locally for any reason
475475
MINIFLARE: true,
476+
477+
// Object, Function, Array, Promise, RegExp, Error, EvalError, RangeError,
478+
// ReferenceError, SyntaxError, TypeError and URIError are intentionally
479+
// omitted. See packages/runner-vm/src/instanceof.ts for a detailed
480+
// explanation of why.
476481
};
477482

478483
// Process module rules if modules mode was enabled

packages/core/test/plugins/bindings.spec.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -513,7 +513,6 @@ test("BindingsPlugin: propagates error if service throws", async (t) => {
513513
}
514514
);
515515
await t.throwsAsync(mf.dispatchFetch("http://localhost/"), {
516-
instanceOf: Error,
517516
message: "oops",
518517
});
519518
});

packages/jest-environment-miniflare/src/index.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import {
1111
} from "@miniflare/durable-objects";
1212
import { HTMLRewriterPlugin } from "@miniflare/html-rewriter";
1313
import { KVPlugin } from "@miniflare/kv";
14-
import { VMScriptRunner, makeProxiedGlobals } from "@miniflare/runner-vm";
14+
import { VMScriptRunner, defineHasInstances } from "@miniflare/runner-vm";
1515
import { Context, NoOpLog } from "@miniflare/shared";
1616
import { SitesPlugin } from "@miniflare/sites";
1717
import { WebSocketPlugin } from "@miniflare/web-sockets";
@@ -68,10 +68,12 @@ export default class MiniflareEnvironment implements JestEnvironment {
6868
this.config = config;
6969
// Intentionally allowing code generation as some coverage tools require it
7070
this.context = vm.createContext({});
71-
this.scriptRunner = new VMScriptRunner(
72-
this.context,
73-
/* blockCodeGeneration */ false
74-
);
71+
// Make sure we define custom [Symbol.hasInstance]s for primitives so
72+
// cross-realm instanceof works correctly. This is done automatically
73+
// when running scripts using @miniflare/runner-vm, but we might not be
74+
// using Durable Objects, so may never do this.
75+
defineHasInstances(this.context);
76+
this.scriptRunner = new VMScriptRunner(this.context);
7577

7678
const global = (this.global = vm.runInContext("this", this.context));
7779
global.global = global;
@@ -174,7 +176,6 @@ export default class MiniflareEnvironment implements JestEnvironment {
174176
mfGlobalScope.self = global;
175177
// Make sure Miniflare's global scope is assigned to Jest's global context,
176178
// even if we didn't run a script because we had no Durable Objects
177-
Object.assign(global, makeProxiedGlobals(/* blockCodeGeneration */ false));
178179
Object.assign(global, mfGlobalScope);
179180

180181
// Add a way of getting bindings in modules mode to allow seeding data.

packages/jest-environment-miniflare/test/fixtures/core.worker.spec.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,3 +39,11 @@ describe("uses Jest fake timers", () => {
3939
test("can generate secure random numbers", () => {
4040
crypto.getRandomValues(new Uint8Array(8));
4141
});
42+
43+
test("Object instanceof checks succeed", () => {
44+
expect(new Uint8Array() instanceof Object).toBe(true);
45+
expect({} instanceof Object).toBe(true);
46+
expect({}.constructor === Object).toBe(true);
47+
expect(new Object({ a: 1 })).toEqual({ a: 1 });
48+
expect(Object.getPrototypeOf({}) === Object.prototype);
49+
});

packages/runner-vm/src/index.ts

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,15 @@ import {
88
ScriptRunnerResult,
99
} from "@miniflare/shared";
1010
import { VMScriptRunnerError } from "./error";
11+
import { defineHasInstances } from "./instanceof";
1112
import { ModuleLinker } from "./linker";
12-
import { makeProxiedGlobals } from "./proxied";
1313

1414
export * from "./error";
15-
export * from "./proxied";
15+
export * from "./instanceof";
1616

1717
// noinspection JSMethodCanBeStatic
1818
export class VMScriptRunner implements ScriptRunner {
19-
constructor(
20-
private context?: vm.Context,
21-
private blockCodeGeneration = true
22-
) {}
19+
constructor(private context?: vm.Context) {}
2320

2421
private runAsScript(context: vm.Context, blueprint: ScriptBlueprint) {
2522
const script = new vm.Script(blueprint.code, {
@@ -59,22 +56,20 @@ export class VMScriptRunner implements ScriptRunner {
5956
const linker =
6057
modulesRules && new ModuleLinker(modulesRules, additionalModules ?? {});
6158

62-
// Add proxied globals so cross-realm instanceof works correctly.
63-
// globalScope will be fresh for each call of run so it's fine to mutate it.
64-
Object.assign(globalScope, makeProxiedGlobals(this.blockCodeGeneration));
65-
6659
let context = this.context;
6760
if (context) {
6861
// This path is for custom test environments, where this.context is only
6962
// used once.
7063
Object.assign(context, globalScope);
7164
} else {
7265
// Create a new context with eval/new Function/WASM compile disabled
73-
const allow = !this.blockCodeGeneration;
7466
context = vm.createContext(globalScope, {
75-
codeGeneration: { strings: allow, wasm: allow },
67+
codeGeneration: { strings: false, wasm: false },
7668
});
7769
}
70+
// Define custom [Symbol.hasInstance]s for primitives so cross-realm
71+
// instanceof works correctly.
72+
defineHasInstances(context);
7873

7974
// Keep track of module namespaces and total script size
8075
let exports: Context = {};
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
/* eslint-disable @typescript-eslint/ban-types */
2+
// Object, Function, Array, Promise, RegExp, Error, EvalError, RangeError,
3+
// ReferenceError, SyntaxError, TypeError and URIError are intentionally
4+
// not passed into the sandbox. There's a trade-off here, ideally we'd like all
5+
// these checks to pass:
6+
//
7+
// ```js
8+
// import vm from "vm";
9+
//
10+
// const ctx1 = vm.createContext({ objectFunction: () => ({}) });
11+
//
12+
// vm.runInContext("({}) instanceof Object", ctx1); // true
13+
// vm.runInContext("({}).constructor === Object", ctx1); // true
14+
//
15+
// vm.runInContext("objectFunction() instanceof Object", ctx1); // false
16+
// vm.runInContext("objectFunction().constructor === Object", ctx1); // false
17+
//
18+
// const ctx2 = vm.createContext({ Object: Object, objectFunction: () => ({}) });
19+
//
20+
// vm.runInContext("({}) instanceof Object", ctx2); // false
21+
// vm.runInContext("({}).constructor === Object", ctx2); // false
22+
//
23+
// vm.runInContext("objectFunction() instanceof Object", ctx2); // true
24+
// vm.runInContext("objectFunction().constructor === Object", ctx2); // true
25+
// ```
26+
//
27+
// wasm-bindgen (a tool used to make compiling Rust to WebAssembly easier),
28+
// often generates code that looks like `value instanceof Object`.
29+
// We'd like this check to succeed for objects generated outside the worker
30+
// (e.g. Workers runtime APIs), and inside user code (instances of classes we
31+
// pass in e.g. `new Uint8Array()`, and literals e.g. `{}`). To do this, we
32+
// override the `[Symbol.hasInstance]` property of primitive classes like
33+
// `Object` so `instanceof` performs a cross-realm check.
34+
// See `defineHasInstancesScript` later in this file.
35+
//
36+
// Historically, we used to do this by proxying the primitive classes instead:
37+
//
38+
// ```js
39+
// function isObject(value) {
40+
// return value !== null && typeof value === "object";
41+
// }
42+
//
43+
// const ObjectProxy = new Proxy(Object, {
44+
// get(target, property, receiver) {
45+
// if (property === Symbol.hasInstance) return isObject;
46+
// return Reflect.get(target, property, receiver);
47+
// },
48+
// });
49+
//
50+
// const ctx3 = vm.createContext({
51+
// Object: ObjectProxy,
52+
// objectFunction: () => ({}),
53+
// });
54+
//
55+
// vm.runInContext("({}) instanceof Object", ctx3); // true
56+
// vm.runInContext("({}).constructor === Object", ctx3); // false
57+
//
58+
// vm.runInContext("objectFunction() instanceof Object", ctx3); // true
59+
// vm.runInContext("objectFunction().constructor === Object", ctx3); // false
60+
// ```
61+
//
62+
// The problem with this option is that the `constructor`/`prototype` checks
63+
// fail, because we're passing in the `Object` from the outer realm.
64+
// These are used quite a lot in JS, and this was the cause of several issues:
65+
// - https://github.com/cloudflare/miniflare/issues/109
66+
// - https://github.com/cloudflare/miniflare/issues/137
67+
// - https://github.com/cloudflare/miniflare/issues/141
68+
// - https://github.com/cloudflare/wrangler2/issues/91
69+
//
70+
// The new behaviour still has the issue `constructor`/`prototype`/`instanceof`
71+
// checks for `Object`s created outside the sandbox would fail, but I think
72+
// that's less likely to be a problem.
73+
74+
import util from "util";
75+
import vm from "vm";
76+
77+
// https://nodejs.org/api/util.html#util_util_isobject_object
78+
function isObject(value: any): value is Object {
79+
return value !== null && typeof value === "object";
80+
}
81+
82+
// https://nodejs.org/api/util.html#util_util_isfunction_object
83+
function isFunction(value: any): value is Function {
84+
return typeof value === "function";
85+
}
86+
87+
function isError<Ctor extends ErrorConstructor>(errorCtor: Ctor) {
88+
const name = errorCtor.prototype.name;
89+
return function (value: any): value is InstanceType<Ctor> {
90+
if (!util.types.isNativeError(value)) return false;
91+
// Traverse up prototype chain and check for matching name
92+
let prototype = value;
93+
while ((prototype = Object.getPrototypeOf(prototype)) !== null) {
94+
if (prototype.name === name) return true;
95+
}
96+
return false;
97+
};
98+
}
99+
100+
const types = {
101+
isObject,
102+
isFunction,
103+
isArray: Array.isArray,
104+
isPromise: util.types.isPromise,
105+
isRegExp: util.types.isRegExp,
106+
isError,
107+
};
108+
109+
const defineHasInstancesScript = new vm.Script(
110+
`(function(types) {
111+
// Only define properties once, will throw if we try doing this twice
112+
if (Object[Symbol.hasInstance] === types.isObject) return;
113+
Object.defineProperty(Object, Symbol.hasInstance, { value: types.isObject });
114+
Object.defineProperty(Function, Symbol.hasInstance, { value: types.isFunction });
115+
Object.defineProperty(Array, Symbol.hasInstance, { value: types.isArray });
116+
Object.defineProperty(Promise, Symbol.hasInstance, { value: types.isPromise });
117+
Object.defineProperty(RegExp, Symbol.hasInstance, { value: types.isRegExp });
118+
Object.defineProperty(Error, Symbol.hasInstance, { value: types.isError(Error) });
119+
Object.defineProperty(EvalError, Symbol.hasInstance, { value: types.isError(EvalError) });
120+
Object.defineProperty(RangeError, Symbol.hasInstance, { value: types.isError(RangeError) });
121+
Object.defineProperty(ReferenceError, Symbol.hasInstance, { value: types.isError(ReferenceError) });
122+
Object.defineProperty(SyntaxError, Symbol.hasInstance, { value: types.isError(SyntaxError) });
123+
Object.defineProperty(TypeError, Symbol.hasInstance, { value: types.isError(TypeError) });
124+
Object.defineProperty(URIError, Symbol.hasInstance, { value: types.isError(URIError) });
125+
})`,
126+
{ filename: "<defineHasInstancesScript>" }
127+
);
128+
129+
// This is called on each new vm.Context before executing arbitrary user code
130+
export function defineHasInstances(ctx: vm.Context): void {
131+
defineHasInstancesScript.runInContext(ctx)(types);
132+
}

packages/runner-vm/src/proxied.ts

Lines changed: 0 additions & 75 deletions
This file was deleted.

packages/runner-vm/test/index.spec.ts

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ test("VMScriptRunner: run: includes module file name in stack traces", async (t)
5252
}
5353
});
5454
test("VMScriptRunner: run: disallows dynamic JavaScript execution", async (t) => {
55+
// noinspection JSUnusedGlobalSymbols
5556
const globals = { callback: () => t.fail() };
5657
const expectations: ThrowsExpectation = {
5758
message: "Code generation from strings disallowed for this context",
@@ -97,30 +98,46 @@ test("VMScriptRunner: run: disallows WebAssembly compilation", async (t) => {
9798
"WebAssembly.compile(): Wasm code generation disallowed by embedder",
9899
});
99100
});
100-
test("VMScriptRunner: run: allows dynamic code generation if enabled", async (t) => {
101-
const runner = new VMScriptRunner(undefined, false);
102-
const addModule = await fs.readFile(path.join(fixturesPath, "add.wasm"));
103-
await runner.run({}, { code: 'eval("1 + 1")', filePath: "test.js" });
104-
await runner.run({}, { code: 'new Function("1 + 1")', filePath: "test.js" });
105-
await runner.run(
106-
{ addModule },
107-
{ code: "await WebAssembly.compile(addModule)", filePath: "test.js" },
108-
[]
109-
);
110-
t.pass();
111-
});
112101
test("VMScriptRunner: run: supports cross-realm instanceof", async (t) => {
113102
const result = await runner.run(
114-
{ outsideRegexp: /a/ },
103+
{ outsideObject: {}, outsideRegexp: /a/ },
115104
{
116105
code: `
117-
export const outsideInstanceOf = outsideRegexp instanceof RegExp;
118-
export const insideInstanceOf = /a/ instanceof RegExp;
106+
// Simulating wasm-bindgen
107+
export const outsideInstanceOf = outsideObject instanceof Object;
108+
export const insideInstanceOf = {} instanceof Object;
109+
110+
export const outsideRegExpInstanceOf = outsideRegexp instanceof RegExp;
111+
export const insideRegExpInstanceOf = /a/ instanceof RegExp;
112+
113+
// https://github.com/cloudflare/miniflare/issues/109
114+
// https://github.com/cloudflare/miniflare/issues/141
115+
export const outsideConstructor = outsideObject.constructor === Object;
116+
export const insideConstructor = {}.constructor === Object;
117+
118+
// https://github.com/cloudflare/miniflare/issues/137
119+
export const newObject = new Object({ a: 1 });
120+
121+
// https://github.com/cloudflare/wrangler2/issues/91
122+
export const outsidePrototype = Object.getPrototypeOf(outsideObject) === Object.prototype;
123+
export const insidePrototype = Object.getPrototypeOf({}) === Object.prototype;
119124
`,
120125
filePath: "test.js",
121126
},
122127
[]
123128
);
129+
124130
t.true(result.exports.outsideInstanceOf);
125131
t.true(result.exports.insideInstanceOf);
132+
133+
t.true(result.exports.outsideRegExpInstanceOf);
134+
t.true(result.exports.insideRegExpInstanceOf);
135+
136+
t.false(result.exports.outsideConstructor); // :(
137+
t.true(result.exports.insideConstructor);
138+
139+
t.deepEqual(result.exports.newObject, { a: 1 });
140+
141+
t.false(result.exports.outsidePrototype); // :(
142+
t.true(result.exports.insidePrototype);
126143
});

0 commit comments

Comments
 (0)