-
Notifications
You must be signed in to change notification settings - Fork 289
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
Implement an UnsafeEval
binding
#1338
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
import { | ||
strictEqual, | ||
ok, | ||
throws | ||
} from 'node:assert'; | ||
|
||
export const basics = { | ||
test(ctx, env) { | ||
strictEqual(env.unsafe.eval('1'), 1); | ||
|
||
// eval does not capture outer scope. | ||
let m = 1; | ||
throws(() => env.unsafe.eval('m')); | ||
|
||
throws(() => env.unsafe.eval(' throw new Error("boom"); ', 'foo'), { | ||
message: 'boom', | ||
stack: /at foo/ | ||
}); | ||
|
||
// Regular dynamic eval is still not allowed | ||
throws(() => eval('')); | ||
} | ||
}; | ||
|
||
export const newFunction = { | ||
test(ctx, env) { | ||
const fn = env.unsafe.newFunction('return m', 'bar', 'm'); | ||
strictEqual(fn.length, 1); | ||
strictEqual(fn.name, 'bar'); | ||
strictEqual(fn(), undefined); | ||
strictEqual(fn(1), 1); | ||
strictEqual(fn(fn), fn); | ||
} | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
using Workerd = import "/workerd/workerd.capnp"; | ||
|
||
const unitTests :Workerd.Config = ( | ||
services = [ | ||
( name = "unsafe-test", | ||
worker = ( | ||
modules = [ | ||
(name = "worker", esModule = embed "unsafe-test.js") | ||
], | ||
compatibilityDate = "2023-01-15", | ||
compatibilityFlags = ["nodejs_compat", "experimental"], | ||
bindings = [ | ||
(name = "unsafe", unsafeEval = void ) | ||
] | ||
) | ||
), | ||
], | ||
); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
#include "unsafe.h" | ||
|
||
namespace workerd::api { | ||
|
||
namespace { | ||
static constexpr auto EVAL_STR = "eval"_kjc; | ||
inline kj::StringPtr getName(jsg::Optional<kj::String>& name, kj::StringPtr def) { | ||
return name.map([](kj::String& str) { | ||
return str.asPtr(); | ||
}).orDefault(def); | ||
} | ||
} // namespace | ||
|
||
jsg::JsValue UnsafeEval::eval(jsg::Lock& js, kj::String script, | ||
jsg::Optional<kj::String> name) { | ||
js.setAllowEval(true); | ||
KJ_DEFER(js.setAllowEval(false)); | ||
auto compiled = jsg::NonModuleScript::compile(script, js, getName(name, EVAL_STR)); | ||
return jsg::JsValue(compiled.runAndReturn(js.v8Context())); | ||
} | ||
|
||
UnsafeEval::UnsafeEvalFunction UnsafeEval::newFunction( | ||
jsg::Lock& js, | ||
jsg::JsString script, | ||
jsg::Optional<kj::String> name, | ||
jsg::Arguments<jsg::JsRef<jsg::JsString>> args, | ||
const jsg::TypeHandler<UnsafeEvalFunction>& handler) { | ||
js.setAllowEval(true); | ||
KJ_DEFER(js.setAllowEval(false)); | ||
|
||
auto nameStr = js.str(getName(name, EVAL_STR)); | ||
v8::ScriptOrigin origin(js.v8Isolate, nameStr); | ||
v8::ScriptCompiler::Source source(script, origin); | ||
|
||
auto argNames = KJ_MAP(arg, args) { | ||
return v8::Local<v8::String>(arg.getHandle(js)); | ||
}; | ||
|
||
auto fn = jsg::check(v8::ScriptCompiler::CompileFunction( | ||
js.v8Context(), &source, argNames.size(), argNames.begin(), 0, nullptr)); | ||
fn->SetName(nameStr); | ||
|
||
return KJ_ASSERT_NONNULL(handler.tryUnwrap(js, fn)); | ||
} | ||
|
||
} // namespace workerd::api |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
#pragma once | ||
|
||
#include <workerd/jsg/jsg.h> | ||
|
||
namespace workerd::api { | ||
|
||
// A special binding object that allows for dynamic evaluation. | ||
class UnsafeEval: public jsg::Object { | ||
public: | ||
UnsafeEval() = default; | ||
|
||
// A non-capturing eval. Compile and evaluates the given script, returning whatever | ||
// value is returned by the script. This version of eval intentionally does not | ||
// capture any part of the outer scope other than globalThis and globally scoped | ||
// variables. The optional `name` will appear in stack traces for any errors thrown. | ||
// | ||
// console.log(env.unsafe.eval('1 + 1')); // prints 2 | ||
// | ||
jsg::JsValue eval(jsg::Lock& js, kj::String script, jsg::Optional<kj::String> name); | ||
|
||
using UnsafeEvalFunction = jsg::Function<jsg::Value(jsg::Arguments<jsg::Value>)>; | ||
|
||
// Compiles and returns a new Function using the given script. The function does not | ||
// capture any part of the outer scope other than globalThis and globally scoped | ||
// variables. The optional `name` will be set as the name of the function and will | ||
// appear in stack traces for any errors thrown. An optional list of argument names | ||
// can be passed in. | ||
// | ||
// const fn = env.unsafe.newFunction('return m', 'foo', 'm'); | ||
// console.log(fn(1)); // prints 1 | ||
// | ||
UnsafeEvalFunction newFunction( | ||
jsg::Lock& js, | ||
jsg::JsString script, | ||
jsg::Optional<kj::String> name, | ||
jsg::Arguments<jsg::JsRef<jsg::JsString>> args, | ||
const jsg::TypeHandler<UnsafeEvalFunction>& handler); | ||
|
||
JSG_RESOURCE_TYPE(UnsafeEval) { | ||
JSG_METHOD(eval); | ||
JSG_METHOD(newFunction); | ||
} | ||
}; | ||
|
||
#define EW_UNSAFE_ISOLATE_TYPES \ | ||
api::UnsafeEval | ||
|
||
} // namespace workerd::api |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasnell Is this call actually necessary? Since we're not actually invoking the JS eval API in this code -- we're calling the C++ APIs directly -- I suspect there's no need to enable eval here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specifically to allow for cases where the script being eval'd itself contains nested regular
eval()
expressions, e.g.While this specific example is a bit silly, it's not unlikely that nested evals could be included in the script passed into the
unsafe.eval(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that that was the case @jasnell. I think it's undesirable to recursively allow eval to be used by unprivileged code.
We use dynamic execution in a specialty/privileged code that powers local development workflows/ Outside of this specialty code, we don't want people to use eval as that could user provided code to run locally but not in production.
I propose that we remove this recursive feature. If the privileged code has a need to expose eval to userland code, it can do so via
globalThis.eval = env.unsafe.eval
before evaling user's code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globalThis.eval = env.unsafe.eval
wouldn't actually work directly, that would end up with a "Illegal invocation" error, but you can get close withglobalThis.eval = function(code) { return env.unsafe.eval(code); }
. But this is not a pattern that we should ever recommend either way.I'm not sure how this would allow use of eval outside of this scope? The regular
eval()
would only be allowed within the script passed intoenv.unsafe.eval(...)
, which is only enabled with the binding that only exists in the local runtime. There's nothing here that would allow that to be used in production and nothing that would alloweval()
to be used outside of this specific binding.For instance, even with the binding enabled, the following still throws an error:
But
Even if I tried something silly like the following, trying to break out of the restriction we still get an appropriate error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I agree with @IgorMinar here. Users could feasibly try to use
eval()
/new Function()
in the top-level. Passing this user code toenv.unsafe.eval
would succeed in local development/testing, where it would fail when deployed.As a concrete example, imagine a user wanted to use Ajv for JSON-schema validation. They build the validator in the top-level so they can reuse it across functions. This won't work when deployed, as
new Function()
is disabled, but would work if the user code was passed directly toenv.unsafe.eval()
....whereas the following would pass...
Admittedly, the user code would probably be wrapped in a
function() { ... }
like your last code snippet, so this wouldn't be an issue. Even so, I'm not sure this should be the default behaviour.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why this is a problem. Using
Ajv
like this at the top level, the user would still need to callunsafe.eval(...)
to use it, and we would only allow foreval(...)
andnew Function(...)
within the synchronous execution of that call. ANY worker code that is usingunsafe.eval(...)
, whether it contains a nested eval or not, will not work when deployed to production because we cannot deploy a worker with theunsafe
binding at all -- it simply doesn't and won't exist outside of the local workerd environment.