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

Polyfilling HostEnsureCanCompileStrings #120

Closed
mikesamuel opened this issue Jan 20, 2019 · 7 comments
Closed

Polyfilling HostEnsureCanCompileStrings #120

mikesamuel opened this issue Jan 20, 2019 · 7 comments
Labels

Comments

@mikesamuel
Copy link
Collaborator

mikesamuel commented Jan 20, 2019

Collecting issues for polyfilling https://wicg.github.io/trusted-types/dist/spec/#string-compilation

Modify HostEnsureCanCompileStrings algorithm, adding the following steps before step 1:

IIUC, this is meant to require TrustedScript inputs to new Function and eval.

We would need to change the behavior of new Function(x) without changing basic identities like (function () {}) instanceof Function.

https://gist.github.com/mikesamuel/4f3696975ec47d09de50dc3f4328dfe9 does that by creating a proxy over Function that traps [[Construct]] and [[Apply]].


The polyfill should also preserve the eval function / eval operator distinction from:

  • window.eval does PerformEval with direct=false
  • eval uses direct=true
    and direct affects whether the lexical environment (step 9) is the top of the stack or the global environment.

This difference can be seen in

const x = 'global';
function f(direct) {
  const x = 'local';
  return direct ? eval('x') : window.eval('x');
}
f(true) // -> 'local'
f(false) // -> 'global'

This is trickier because a bare eval(...) is only the eval operator when step 4.a of the bare function call finds that eval in scope is the same as the Realm's original eval value:

If SameValue(func, %eval%) is true

We could fake that by reassigning global.eval while being careful to not open a window for recursive calls to eval in the script body by prepending something to the script body, but that would still not get the wrapper function off the top of the environment stack.

@mikesamuel mikesamuel changed the title Polyfilling HostEnsureCanCompileStrings. Polyfilling HostEnsureCanCompileStrings Jan 20, 2019
@koto
Copy link
Member

koto commented Jan 21, 2019

I like the Function constructor polyfill. Can we check if this doesn't break existing code?

As for 'eval', I'm afraid that polyfilling it will introduce tricky functional bugs in the applications using the polyfill. I'm not yet convinced this is the right way to go; it might be more fitting to just mention that this is a known gap for the polyfill, and assume that eval is otherwise sparsely used and easy to find with other means.

@mikesamuel
Copy link
Collaborator Author

@koto

Can we check if this doesn't break existing code?

Do you want to try out a canary project with the patch?
Or do you want a PR that adds it and some tests.

As for 'eval',

To make sure I understand, Function can be called implicitly as via ({})['constructor']['constructor'](code)(), but eval requires the use of the eval (unreserved) keyword.

So our polyfill security caveats could recommend banning eval via linter since the polyfill handles the one that is easy to overlook.

@mikesamuel
Copy link
Collaborator Author

I have tried proxying Function before so have some indication that legacy code doesn't just go up in flames.

I use a Function proxy to restrict dynamic code loading to certain modules in Node.js.

That works with a bunch of legacy Node.js code.

@koto
Copy link
Member

koto commented Jan 24, 2019

OK, that works for Function(). To make sure I understand the eval concern: we can effectively polyfill window.eval, but not the operator (because of the scoping difference), but that's a problem, because most of the time the platform will choose the operator in place of the function. I think this may cause the polyfill to introduce functional bugs that are tricky to debug.

That's an implementation concern and I'm OK for the polyfill to cover less sinks than the native implementation if the alternative is to break applications in confusing ways. That said, for now I think we have to pause this anyway - as the Chrome implementation doesn't yet cover Function() nor [window.]eval(), let's resolve this first.

@mikesamuel
Copy link
Collaborator Author

mikesamuel commented Mar 28, 2019

https://mikesamuel.github.io/proposal-hostensurecancompilestrings-passthru/ aims to address the lack of context in the host.

@koto
Copy link
Member

koto commented Aug 11, 2019

I moved the spec-centric issues to #207. I propose we postpone the polyfilling until after the spec issues are resolved. We won't be able to polyfill everything, so I'm thinking on whether we should attempt to in the TT polyfill, given that we'd likely run into performance, or some stack trace introspection issues. To demonstrate feasibility, we can probably create a Babel plugin. WDYT, @mikesamuel ?

@mikesamuel
Copy link
Collaborator Author

@koto, +1

@koto koto closed this as completed Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants