-
Notifications
You must be signed in to change notification settings - Fork 30.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Promise.resolve('import("node:fs")').then(eval)
throws TypeError: Invalid host defined options
#49726
Comments
Can you copy paste the output of your console instead of sending screen shots? Screenshots can be very unhelpful for e.g. folks using screen readers and/or folks with visual disabilities. The following works on most1 runtimes but Node.js: Promise.resolve(`import("data:text/javascript,")`).then(eval).then(console.log, console.error) Footnotes
|
Promise.resolve(
import("node:fs")).then(eval)
throws TypeError: Invalid host defined options
Promise.resolve(
import("node:fs")).then(eval)
throws
TypeError: Invalid host defined options`
Promise.resolve(
import("node:fs")).then(eval)
throws
TypeError: Invalid host defined options`Promise.resolve('import("node:fs")').then(eval)
throws TypeError: Invalid host defined options
This comment was marked as off-topic.
This comment was marked as off-topic.
The two spots I could find from a GitHub search for "Invalid host defined options" are these: Lines 572 to 576 in b64f620
Lines 1252 to 1255 in b64f620
|
This comment was marked as off-topic.
This comment was marked as off-topic.
The following check is failing ( Lines 568 to 569 in 26c8858
Here's a backtrace (maybe I need a debug build):
|
node/deps/v8/src/execution/isolate.cc Lines 5113 to 5116 in c2cd744
node/deps/v8/src/runtime/runtime-module.cc Lines 36 to 37 in c2cd744
|
This is likely a V8 issue, I can also reproduce it with d8. The problem probably comes from that V8 is unable to retrieve the referrer/origin when code is |
Also this does not only affect host-defined options, for example V8 would not be able to show the script name if an error is thrown from code being eval'ed directly from a microstask because of the same cause. I'll open an upstream issue. |
Upstream issue https://bugs.chromium.org/p/v8/issues/detail?id=14336 |
@joyeecheung naive question: if it's a V8 problem then why doesn't Deno (also V8-based) have this issue? 🤔 is deno doing some magic or something? jcbhmr@PIG-2016:~$ deno
Deno 1.37.0
exit using ctrl+d, ctrl+c, or close()
REPL is running with all permissions allowed.
To specify permissions, run `deno repl` with allow flags.
> Promise.resolve('import("node:fs")').then(eval).then(console.log)
[Module: null prototype] {
Dir: [class Dir],
Dirent: [class Dirent],
... |
FWIW Chromium is also not affected. Even if it is not a V8 issue in the end, the patch that will fix d8 will probably contain all the needed info to fix it in Node.js, so I think our best course of action is to wait for V8 team for a fix. |
My guess is, Deno or Chromium may not absolutely require the host defined options for the module resolution to work. If the referrer or host defined options are unknown they'll fall back to something else (for example, Chromium can fallback to a default referrer with an empty URL and somehow still work with that). Whereas Node.js enforces that it needs to know about the host defined options to look up for a callback and some data that was originally saved for the referrer script (because e.g. the vm APIs of Node.js allows customization of the dynamic import handling for each different Promise.resolve('throw new Error("test")')
.then(eval)
.catch(e => console.log(e.stack)); You can see that neither Deno nor Chromium is able to infer the origin (the script is listed as One possible workaround might be, we can do basically what #48655 does for shadow realms - when we don't have host-defined options at hand to look up for the callback and data, we just fall back to the default loader with the |
Actually just realized that this bug may affect the integration of shadow realm too because we are operating under the assumption that an empty host defined options means that the import comes from |
Actually the Deno/Chromium behavior of falling back to an empty base URL may be quite surprising. If you create a directory structure:
and load Promise.resolve('import("./mod.js")').then((i) => eval(i)).then(i => console.log(i.hello)); It prints Promise.resolve('import("./mod.js")').then(eval).then(i => console.log(i.hello)); Due to the fallback, it's going to print |
I think printing edit: after seeing the results of the test below idk what to think 😵 this is what I'm getting at: // these should be different; direct vs indirect eval
Promise.resolve('import("./mod.js")').then((i) => eval(i)).then(i => console.log(i.hello));
Promise.resolve('import("./mod.js")').then((i) => globalThis.eval(i)).then(i => console.log(i.hello)); // these should act the same; both indirect eval
Promise.resolve('import("./mod.js")').then((i) => globalThis.eval(i)).then(i => console.log(i.hello));
Promise.resolve('import("./mod.js")').then(eval).then(i => console.log(i.hello));
// ...only problem is that this ^^^^ throws in current Node.js 😢 csb: https://codesandbox.io/s/infallible-elbakyan-2ctpgp?file=/hello/world/main.js console.clear();
Promise.resolve('import("./mod.js")')
.then((i) => eval(i))
.then((i) => console.log("eval()", i.default));
Promise.resolve('import("./mod.js")')
.then((i) => globalThis.eval(i))
.then((i) => console.log("globalThis.eval()", i.default));
Promise.resolve('import("./mod.js")')
.then(eval)
.then((i) => console.log(".then(eval)", i.default));
Promise.resolve('import("./mod.js")')
.then((x) => new Function("return " + x)())
.then((i) => console.log("new Function()", i.default)); actually now that i tried it I'm surprised that idk if this is what i read about direct/indirect eval: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#direct_and_indirect_eval |
Direct eval call from a function sets the current execution context to be its surrounding module/script, which would be used as the referrer (i.e. host defined options) in the dynamic import calls. On the other hand, eval function call from a microtask sets the current execution context to be
I believe the condition here is identical to |
This is not only limited to eval, for example this can also be reproduced with Promise.resolve(['import("./mod.mjs")'])
.then(Reflect.construct.bind(null, Function))
.then(Function.prototype.call.bind(Function.prototype.call))
.then(m => console.log(m.hello)); |
I think the answer to whether we should fallback comes down to:
|
Actually I think the editor notes does not apply to microtasks, from https://262.ecma-international.org/14.0/#sec-hostenqueuepromisejob the implementation should restore the active script from when the microtask is queued
|
@joyeecheung this is interesting. I tested on Chrome 117, Firefox 119, and Safari 17, none of them respected this statement. They resolve eval import calls relative to the origin of the document, rather than the initiating module. I believe either the ecma262 has to be corrected to reflect the web reality, or the browsers have to fix the behavior. |
I’m confused; why would anyone expect this to work? This would attempt to eval the toString of the module namespace object, and you simply can’t assume eval of any function will be portable. |
@ljharb it's not about eval-ing the toString result of a module namespace. The problem here is how to resolve the specifier when the eval function is called with a string script containing dynamic imports and the eval function is invoked by built-in callbacks directly. |
I just noticed the HTML spec deals with this exact situation in https://html.spec.whatwg.org/multipage/webappapis.html#hostmakejobcallback |
Interesting, apparently WHATWG spec is explicitly saying that in this case the referrer should be the original script (and, on the contrary, if it's from a |
@joyeecheung they took this to TC39 at one point but TC39 members had some concerns about the behavior because it could lead to an exploit by giving extra permissions to the eval'd string From July 2020: https://docs.google.com/presentation/d/19S97ZqhibJABqzeP5ZU6Flk6TVgWzXuvJWFbNTTfpWs/edit#slide=id.p |
Submitted tc39/ecma262#3195 to propagate active scripts with arbitrary microtask callbacks (like FinalizationRegistry callbacks) in ecma262. |
A referrer can be a Script Record, a Cyclic Module Record, or a Realm Record as defined in https://tc39.es/ecma262/#sec-HostLoadImportedModule. Add support for dynamic import calls with a realm as the referrer and allow specifying an `importModuleDynamically` callback in `vm.createContext`. PR-URL: #50360 Refs: #49726 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
A referrer can be a Script Record, a Cyclic Module Record, or a Realm Record as defined in https://tc39.es/ecma262/#sec-HostLoadImportedModule. Add support for dynamic import calls with a realm as the referrer and allow specifying an `importModuleDynamically` callback in `vm.createContext`. PR-URL: nodejs#50360 Refs: nodejs#49726 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
A referrer can be a Script Record, a Cyclic Module Record, or a Realm Record as defined in https://tc39.es/ecma262/#sec-HostLoadImportedModule. Add support for dynamic import calls with a realm as the referrer and allow specifying an `importModuleDynamically` callback in `vm.createContext`. PR-URL: nodejs#50360 Refs: nodejs#49726 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
A referrer can be a Script Record, a Cyclic Module Record, or a Realm Record as defined in https://tc39.es/ecma262/#sec-HostLoadImportedModule. Add support for dynamic import calls with a realm as the referrer and allow specifying an `importModuleDynamically` callback in `vm.createContext`. PR-URL: #50360 Refs: #49726 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
A referrer can be a Script Record, a Cyclic Module Record, or a Realm Record as defined in https://tc39.es/ecma262/#sec-HostLoadImportedModule. Add support for dynamic import calls with a realm as the referrer and allow specifying an `importModuleDynamically` callback in `vm.createContext`. PR-URL: #50360 Refs: #49726 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
A referrer can be a Script Record, a Cyclic Module Record, or a Realm Record as defined in https://tc39.es/ecma262/#sec-HostLoadImportedModule. Add support for dynamic import calls with a realm as the referrer and allow specifying an `importModuleDynamically` callback in `vm.createContext`. PR-URL: #50360 Refs: #49726 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Version
v20.7.0
Platform
Linux PIG-2016 5.15.90.1-microsoft-standard-WSL2 #1 SMP Fri Jan 27 02:56:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Subsystem
esm
What steps will reproduce the bug?
node -e 'Promise.resolve(`import("node:fs")`).then(eval)'
How often does it reproduce? Is there a required condition?
No response
What is the expected behavior? Why is that the expected behavior?
it shouldn't throw.
What do you see instead?
these DONT throw
these DO throw
Additional information
deno version of the same thing DOESN'T throw:
The text was updated successfully, but these errors were encountered: