Add binding to napi_run_script #692

branchseer · 2021-03-07T11:29:18Z

No description provided.

kjvalencik

Thanks for the contribution! This seems very straight forward, but I would like to discuss the API a bit further. Specifically if this method should be marked unsafe.

crates/neon-runtime/src/napi/string.rs

kjvalencik · 2021-03-08T15:10:32Z

src/types/mod.rs

@@ -360,6 +360,16 @@ impl JsString {
        }
    }

+    #[cfg(feature = "napi-1")]
+    pub fn run_as_script<'a, C: Context<'a>>(self, cx: &mut C) -> JsResult<'a, JsValue> {


@dherman What do you think about marking this as unsafe? Even though it's not unsafe by Rust's definition,eval is a very common exploit vector and it could be helpful to signal to users that only trusted input should be used.

rustls had a similar case: they wanted to add an option that disables cert verification, which was very unsafe, but not rust unsafe. What they do is put it behind a feature flag and explicitly say "dangerous" on the api.

I like the idea of adding dangerous to the function name itself. For example, React has a dangerouslySetInnerHTML function. We could call this, dangerously_run_script.

Adding the feature flag seems like additional complexity without any benefit. Unlike with rustls, the run_script API will still exist even if we feature flag it because it's part of Node.

I think marking unsafe when it's not about Rust's safety guarantees is generally considered poor form in the Rust ecosystem, so I agree that's not the best approach. Rather than a prefix, how about a module? Some possibilities:

neon::danger::run_script

neon::advanced::run_script

neon::unchecked::run_script

neon::reflect::run_script

I should mention, I also agree the feature flag is the wrong approach because it's viral. If a library wants to use the feature they should be able to do it without requiring downstream clients to have to enable the feature flag, and then those clients end up enabling it for everyone anyway, which isn't great either.

Update: @kjvalencik and I spoke this morning, and we came to this suggestion for the naming and documentation:

It shouldn't be available in any convenience forms (not on Context or anywhere else)

neon::reflect::eval is a good name for it: it shows that it's a reflection API, not something to reach for casually, and it connects it to JS's eval, which is in fact what it is (technically speaking, it's JS's "indirect eval" but you can't implement direct eval as anything other that a syntax in pure JavaScript source code) and also brings along the mindshare from the JS ecosystem about the dangers of eval

The API docs should clearly explain that use of eval can be a security risk and should not be used casually

@patr0nus How does that sound?

@dherman @kjvalencik

Putting it under neon::reflect sounds good to me.

However I prefer the name run_script rather than eval, since it's a direct binding to napi_run_script, and as you mentioned, it's not quite the same as eval in JS. For example, one might assume neon::reflect::eval can access local variables like JS eval can.

Also I would suggest the function should accept JsString instead of AsRef<str>. If a user wants to be run a script repeatedly, he/she wouldn't want a JsString to be created every time.

TL;DR: I would prefer the function signature to be:

fn neon::reflect::run_script<'a, 'b, C: Context<'a>>(cx: &mut C, script: Handle<'b, JsString>) -> JsResult<'a, JsValue>.

In my opinion, the N-API naming is not optimal. It is identical to indirect eval (as defined by the ECMA spec).

Considering direct eval is not possible in a native context, eval seems more appropriate.

Neon already diverges from n-api in a number of places. For example, Root instead of reference.

Additionally, eval carries the stigma of danger associated with the term, while run_script does not. I think this is very important.

I like accepting JsString. This is somewhat outside the scope of this PR, but we could improve the API by adding a ToJsString trait implemented for Handle<JsString>, &JsString, and anything that's AsRef<str> and making eval generic.

That seems reasonable. I have pushed the changes.
Though I didn't include the documentation which explains the security risk, as I am not a native speaker, so not really suitable for explaining this kind of serious stuff.

No problem! I am going to be working on API docs next so I can follow up in a future PR with some documentation. Thanks so much for your work!

dherman

Edit: (moved this to the appropriate sub-thread)

# Conflicts: # crates/neon-runtime/src/napi/bindings/functions.rs

# Conflicts: # crates/neon-runtime/src/napi/bindings/functions.rs # src/lib.rs

kjvalencik · 2021-03-15T20:35:12Z

@patr0nus Thanks for the contribution!

Add binding to napi_run_script

2e7a34a

kjvalencik reviewed Mar 8, 2021

View reviewed changes

Test run_string_as_script with invalid syntax

272fe0d

dherman reviewed Mar 12, 2021

View reviewed changes

branchseer and others added 5 commits March 13, 2021 21:42

Merge remote-tracking branch 'upstream/main' into main

2918268

# Conflicts: # crates/neon-runtime/src/napi/bindings/functions.rs

Move the run_script binding to neon::reflect::eval

397c734

Update mod.rs

bd795ea

Merge remote-tracking branch 'upstream/main' into main

64ee165

# Conflicts: # crates/neon-runtime/src/napi/bindings/functions.rs # src/lib.rs

cargo fmt

ccbc591

kjvalencik approved these changes Mar 15, 2021

View reviewed changes

kjvalencik merged commit b10a779 into neon-bindings:main Mar 15, 2021

This was referenced Jun 6, 2021

Update dependency neon-cli to ^0.10.0 amilajack/os-type#7

Open

Update dependency neon-cli to ^0.10.0 amilajack/neon-benchmarks#56

Open

Update dependency neon-cli to ^0.10.1 sqlite-js/sqlite#57

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add binding to napi_run_script #692

Add binding to napi_run_script #692

branchseer commented Mar 7, 2021

kjvalencik left a comment

kjvalencik Mar 8, 2021 •

edited

Loading

branchseer Mar 10, 2021 •

edited

Loading

kjvalencik Mar 11, 2021

dherman Mar 11, 2021

dherman Mar 11, 2021

dherman Mar 12, 2021 •

edited

Loading

branchseer Mar 13, 2021

kjvalencik Mar 13, 2021 •

edited

Loading

branchseer Mar 13, 2021

dherman Mar 13, 2021

dherman left a comment •

edited

Loading

kjvalencik commented Mar 15, 2021

Add binding to napi_run_script #692

Add binding to napi_run_script #692

Conversation

branchseer commented Mar 7, 2021

kjvalencik left a comment

Choose a reason for hiding this comment

kjvalencik Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

branchseer Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

kjvalencik Mar 11, 2021

Choose a reason for hiding this comment

dherman Mar 11, 2021

Choose a reason for hiding this comment

dherman Mar 11, 2021

Choose a reason for hiding this comment

dherman Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

branchseer Mar 13, 2021

Choose a reason for hiding this comment

kjvalencik Mar 13, 2021 • edited Loading

Choose a reason for hiding this comment

branchseer Mar 13, 2021

Choose a reason for hiding this comment

dherman Mar 13, 2021

Choose a reason for hiding this comment

dherman left a comment • edited Loading

Choose a reason for hiding this comment

kjvalencik commented Mar 15, 2021

kjvalencik Mar 8, 2021 •

edited

Loading

branchseer Mar 10, 2021 •

edited

Loading

dherman Mar 12, 2021 •

edited

Loading

kjvalencik Mar 13, 2021 •

edited

Loading

dherman left a comment •

edited

Loading