Skip to content
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

Include the string to be compiled in the call to `HostEnsureCanCompileStrings` #938

Open
mikewest opened this issue Jun 21, 2017 · 8 comments · May be fixed by #1498
Open

Include the string to be compiled in the call to `HostEnsureCanCompileStrings` #938

mikewest opened this issue Jun 21, 2017 · 8 comments · May be fixed by #1498
Labels

Comments

@mikewest
Copy link

@mikewest mikewest commented Jun 21, 2017

To improve the quality of CSP reports, it would be helpful for HostEnsureCanCompileStrings() to include the string to be compiled as an argument. HostEnsureCanCompileStrings(callerRealm, calleeRealm, source) seems ideal. :)

The goal is to ensure that we can include a sample of the script which violates the policy when generating a CSP violation report. We're doing this for inline <script>...</script> blocks today, and layering eval() and the like on as well would be helpful.

@domenic
Copy link
Member

@domenic domenic commented Jun 21, 2017

Sounds good in theory. The only reason we didn't do this at the time was because it wasn't needed, IIRC.

There are a couple of tricky things:

  • How do you want to handle non-strings? Currently in a CSP-restricted environment, eval(nonString) will throw. (I wonder if we have tests for that?) One refactoring would be to ensure things are a string before passing them to HostEnsureCanCompileStrings. If we do that, then behavior will change, and we'll bail out before ever hitting HostEnsureCanCompileStrings.
  • Similarly, for Function() and its various friends (GeneratorFunction(), AsyncFunction()), do we handle arg coercion before or after HostEnsureCanCompileStrings? This would change which error is thrown when doing e.g. Function({ toString() { throw 5; }).
  • For Function() and friends, what text should be passed? Should it be the body text of the function, which is directly passed as an argument? Or perhaps we should re-serialize the entire function, after we've assembled the various arguments to Function() into an actual function? The former would probably work for your purposes, although it's a bit weird to be doing checks on something that isn't standalone evaluatable source code. Maybe it would help if we named the new parameter something like sourceTextHint or proximateSourceText instead of just sourceText, with a reasonable explanation.
@koto
Copy link

@koto koto commented Jan 18, 2019

This issue also surfaced when creating the trusted types spec draft. In short, we're trying to figure out if eval could accept stringifiable objects, and the decision in HostEnsureCanCompileStrings would be based on their value (essentially, we would optionally reject all values that are not of a given type).

@koto
Copy link

@koto koto commented Jan 24, 2019

Actually, having looked at it, for Trusted Types in specific, we might need to be able to validate and coerce a given 'eval' argument to a string before e.g. PerformEval is called. The issue is that eval returns its input value if it's not a string:

eval(new String('foo'))
String {'foo'}

The use case we have in Trusted Types is to be able to, in the host environment, allow

eval(ASpecificObjectWrappingAString("code-to-compile"))

but optionally disallow:

eval("code-to-compile")

or even transform it to :

eval("modified-code-to-compile")

Theoretically, this can still be done in HostEnsureCanCompileString if it accepted an object passed by the caller as-is and returned a tuple of boolean value and the (potentially coerced to string) code to compile. Spec-wise this would require small changes to a few HostEnsureCanCompileString callsites, namely:

I'm not sure how involved the change is, both in spec and the implementations.

@mikesamuel
Copy link
Member

@mikesamuel mikesamuel commented Jan 24, 2019

Similarly, for Function() and its various friends (GeneratorFunction(), AsyncFunction()), do we handle arg coercion before or after HostEnsureCanCompileStrings? This would change which error is thrown when doing e.g. Function({ toString() { throw 5; }).

Excellent question.

  1. If coercion happens too early, then HostEnsureCanCompileStrings lacks context to distinguish between a trusted script and a raw string.
  2. If coercion happens after CanCompileStrings, then a HostEnsureCanCompileStrings implementation can't guarantee that the stringified form observes is the same as the stringified form that the parser sees.

It seems that for mkwst's diagnostic purposes, (2) is not a breaker, but for TT, (1) is a breaker.

Per the questions about functions, those seem like non-issues since, as koto points out, HostEnsureCanCompileStrings is never called.

$ npx node@10 --disallow_code_generation_from_strings -e 'console.log(eval(() => {}))'
[Function]

$ npx node@10 --disallow_code_generation_from_strings -e 'console.log(eval("() => {}"))'
[eval]:1
console.log(eval("() => {}"))
        ^

EvalError: Code generation from strings disallowed for this context

Perhaps we could tweak the eval algo so that if the input is a string or has a particular symbol property then it is stringified to avoid breaking code that depends on the current behavior where eval is identity except for strings.

@koto
Copy link

@koto koto commented Aug 11, 2019

@ljharb

This comment has been hidden.

@mikesamuel

This comment has been hidden.

@ljharb ljharb added the proposal label Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants