fix this binding in Function constructor

[Jack-Works]

• fix: this binding in Function

Other unrelated fixes are move to other prs

• fix: pollution to outside world Function.prototype.constructor (see fix global function get polluted close #52 #63 )
• disabled rejectHtmlComments and rejectImportExpressions (see allow configurable Realm transforms #42 )
• non-strict assignment (see Eval sloppy globals #33 )
• disabled alwaysThrowHandler on WKWebKit (see fix: getPrototypeOf trap get called in safari #62 )

[Jack-Works]

I have fixed my patch on this

[erights]

Hi @Jack-Works , reviewing this PR, I see that we did not have enough test cases to cover my concerns. See #18 where I propose to add these.

Aside from that concern, the other concern I have with this PR is it attempts to emulate one aspect of sloppy (non-strict) behavior when the Function text does not begin with "use strict". I don't think this is a good thing to do. The shim can only make strict functions. The user of the shim should be aware of this. We should not try to blur that distinction by having it partially emulate a sloppy function.

Given these reactions, I must apologize for having left in the // todo: fix "this" binding in Function().. With these extra tests, I no longer see anything wrong with the current this binding behavior. I am sorry if this led to an unnecessary attempt to fix something that wasn't broken.

[erights]

Looking over your PR again, I realize that #18 still doesn't have the needed extra test. Hold on.

[erights]

Added

  t.equal(F.call(undefined), undefined);

which would fool

return Reflect.apply(f, this || globalThis, arguments);

into returning globalThis. I doubt there is any general way for a function to distinguish between getting a this-binding from its caller implicitly vs explicitly.

[Jack-Works]

I'm loading my whole application into the Realm and I found it is broken. Debugging it, I found it is breaking in the lodash library.

Lodash detects global variables in a wrong way (they write them before we have globalThis in Javascript) and "return this" is just a quite common technique in the classic JavaScript world.

At least 2 dependencies in my project use this pattern. If new Function cannot simulate a sloppy mode, or let just say, getting "this", I think it's not a correct way of emulating a sandbox.

[Jack-Works]

To be more concise.

I'm working on a project that provides a "polyfill" to WebExtension API. In browser extensions, content scripts run in an isolated world and I'm implementing it by Realm shim.

Then I put my extension code into this emulated web extension environment, its breaking due to "this" in Function is undefined.

If I let other extensions run in my environment, they may crash due to cannot get the global object. Giving them the global object is not harmful and improve the compatibility with the standard "new Function" behavior.

[Jack-Works]

I'm curious about the default strict behavior, is that a by-design in the spec, or due to technical restrictions code in sloppy mode may leak out of the sandbox?

[erights]

Hi @Jack-Works , thanks. This helps a lot.

If we provided a globalThis, emulating the proposal on a per-compartment basis, would that help?

Could you point me at the relevant places in lodash or other libraries that probe for the global? Are they using the Function constructor both for this purpose as well as other purposes? (If only for this, you could provide a special purpose Function endowment that did just this one job, shadowing the Function provided by the shim.)

Emulating the WebExtension API is cool! Eager to hear more about this project. Is there anything publicly visible to look at?

[erights]

The strict-only behavior is required by the technique we use to emulate the proposal, the so-called 8 magic lines of code https://www.youtube.com/watch?v=mSNxsn0pK74&list=PLKr-mvz8uvUg70w0yKGfytaDqxiIBNo_L&t=799 . If we allowed the user to write a sloppy function, they could use it to obtain the genuine global object which would totally break the illusion the shim is trying to create.

As for the proposal, it is likely to support both modes, with sloppy likely the default and strict-only being an option. Anticipating that, the shim should probably insist that the strict-only option be passed in, throwing otherwise. Thus, code that works with the shim would continue to work with the full proposal even if strict-only is not the default.

[erights]

Reopening until we understand the issue better, and hopefully succeed somehow at not breaking your use case.

[erights]

We must find a different technique to address this use case. See #17 (comment)

[Jack-Works]

1. Adding globalThis is good, but that doesn't help.
   I'm adding window and globalThis to the realm but lodash still breaks.

I have fixed lodash by detecting globalThis(lodash/lodash#4347) but I realize there are thousands of libraries doing Function('return this') even Function('x = y') and I can't fix them one by one to get my usage working.

Facebook's regenerator-runtime library is commonly used in transpile async function into ES5/ES6.

Standalone runtime for Regenerator-compiled generator and async functions.

In https://github.com/facebook/regenerator/blob/master/packages/regenerator-runtime/runtime.js#L725
they expect that library runs in sloppy mode and if code accidentally runs in strict mode, they will run Function("r", "regeneratorRuntime = r")(runtime) to set a global variable.

With my forked version of Realms, all of the code works properly now.

[Jack-Works]

2. About webextension-shim
   I'm currently working on it and I will open source it(maybe in 1 or 2 months) until I resolve the prototype protect and two-way isolation. WebExtension-shim is trying to implement WebExtension on iOS (on Android we have GeckoView that supports WebExtension) by WKWebView.

Content script is running in a context that shares DOMs but isolates JS environment. (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision)
Now I'm just copying every Web API into the realm but it corrupt outside by modifying DOMs prototype.
For example if I set HTMLElement.prototype.a = 1 it should not affect outside of the sandbox but it does now.

Now realms assuming outside of the sandbox is safe, and inside of the sandbox is dangerous. In my case, both outside AND inside of the sandbox are dangerous. I need to prevent attacks to the content in the sandbox(in browser extension environment we have a high permission browser API) from the outside of the sandbox.
Realms just using the global object so if the code in the main frame hijacks eval or Proxy, content in the sandbox will leaks (which should not happen).
I'm preparing to solve this problem by wrap a with+Proxy outside of all codes of WebExtension-shim to save all global reference.

[Jack-Works]

Excepts commits in this pr, I forked to another repo to make some changes that I think may not be accepted by this project but I need them to let realms works for my project.

1. I disabled rejectHtmlComments and rejectImportExpressions in our production because it has false positive throws when I'm running an uncompressed version of react in the sandbox (which import clause is in the comment)
2. I allow non-strict assignment in new Function by a hack way to let regenerator-runtime library runs normally (In Function("r", "regeneratorRuntime = r")(runtime) they are not using this)
3. I disabled alwaysThrowHandler because it always throws on WKWebKit for unknown reason.

[erights]

On https://github.com/DimensionDev/realms-shim/commit/be2c2728d266035bbff7786f5d25f274b90c30fe
could you show some code snippets where you ran into these problems? @michaelfig and I have been going back and forth about how to integrate parser-based alternatives to this rejection logic, possibly under non-default options settings. At some risk, tolerable under a non-default option setting, we could allow apparent import expressions that appear only in comments or literal strings.

Where did you run into html-like comments? This one really surprises me. Thanks.

[erights]

On https://github.com/DimensionDev/realms-shim/commit/44ab25eca8d928a070f813fd9dcd1216f31c00db
I like the idea of a non-default option allowing global assignment to not-previously-existing global variables. We've encountered the need for this ourselves.

[erights]

On https://github.com/DimensionDev/realms-shim/commit/55963b0b26c92235123afb0a95c251e0f48fd59d I have filed a bug against WebKit/Safari/JSC on this. Will try to find. I think we will accept your fix here.

[michaelfig]

Where did you run into html-like comments? This one really surprises me. Thanks.

I found that the string "-->" occurred in Agoric/ERTP, where it appeared as somebody writing an ASCII-art arrow as part of the documentation in a Javascript comment. It was caught by the realms-shim HTML-comment rejector.

Perhaps we should only be concerned with "<!--" and not "-->".

[erights]

Perhaps we should only be concerned with "".

No, we need to be equally concerned with both. The Javascript parsing rule for these is that each is treated separately as a line comment. They are not bracketed.

Because this hazard itself makes ambiguous whether we are even in a comment or literal string or whatever, it would be quite hazardous to do a parser-based-only rejection of these. We could, but we would need to very carefully review it. On that one, we should also ask @waldemarhorwat to double check our work. We should still reject these by default. The non-default options setting should have a clear name discouraging its use.

[erights]

@Jack-Works why is allowNonStrictModeAssignmentTimes a count rather than a flag?

[erights]

Also, see https://github.com/DimensionDev/realms-shim/commit/55963b0b26c92235123afb0a95c251e0f48fd59d#commitcomment-34258398

[erights]

@Jack-Works what is your current status/understanding of your need for access to global this?

[Jack-Works]

On DimensionDev@be2c272
could you show some code snippets where you ran into these problems? @michaelfig and I have been going back and forth about how to integrate parser-based alternatives to this rejection logic, possibly under non-default options settings. At some risk, tolerable under a non-default option setting, we could allow apparent import expressions that appear only in comments or literal strings.

Where did you run into html-like comments? This one really surprises me. Thanks.

Try to evaluate React and ReactDOM the unminified version, both import-like and html-like comments appear.

The import-like comment is teaching you how to async import a React Component and load it.

The html-like comment is inside of a picture (or table?) in comments

[Jack-Works]

@Jack-Works why is allowNonStrictModeAssignmentTimes a count rather than a flag?

Yeah I said it's a hack way to resolve this problem.
If I set it as a flag, it will not work because the actual code is not running in the current context of eval.
It goes through eval -> new Function -> eval -> ... then the actual non-strict code we want to run.
I found when I run realm.evaluate("new Function('x = 1')"), the 5 is the counter number I need to make it work properly.

If relams want to support non-strict assignment, there must be a better implementation than mine.

[Jack-Works]

@Jack-Works what is your current status/understanding of your need for access to global this?

I'm not writing code for relamed environment, I'm creating a realm that can run the existing code. So my usage is the community's usage, they will write all bad design like new Function('x = a')(y)

(and they are not using globalThis. sad)

[erights]

1. Adding globalThis is good, but that doesn't help.
   I'm adding window and globalThis to the realm but lodash still breaks.

I have fixed lodash by detecting globalThis(lodash/lodash#4347) but I realize there are thousands of libraries doing Function('return this') even Function('x = y') and I can't fix them one by one to get my usage working.

@Jack-Works, thanks for fixing this issue in lodash! @jdalton, thanks for merging this in so quickly!

Facebook's regenerator-runtime library is commonly used in transpile async function into ES5/ES6.

Standalone runtime for Regenerator-compiled generator and async functions.

In https://github.com/facebook/regenerator/blob/master/packages/regenerator-runtime/runtime.js#L725
they expect that library runs in sloppy mode and if code accidentally runs in strict mode, they will run Function("r", "regeneratorRuntime = r")(runtime) to set a global variable.

With my forked version of Realms, all of the code works properly now.

In any JavaScript in which any of this is relevant, the platform will already support both generators and async function. So why shim them? I ask because the comment you link to has the generous invitation:

// [...] If
// you've misconfigured your bundler to force strict mode and applied a
// CSP to forbid Function, and you're not willing to fix either of those
// problems, please detail your unique predicament in a GitHub issue.

Even though this doesn't quite fit our constraints, I'd be happy to post an issue explaining our constraints if relevant. But given the purpose of regenerator, I'm unclear on how I'd claim that our constraints are relevant.

[Jack-Works]

In any JavaScript in which any of this is relevant, the platform will already support both generators and async function. So why shim them? I ask because the comment you link to has the generous invitation:

I'm not transpile my code to ES5, but the libraries I'm using, its dependencies, its dependencies' dependencies, ... etc may transpile their code into ES5 and use regenerator-runtime. I can't control that..

[Jack-Works]

With 969f1fe I fixed that Realms shim will pollute outside world Function.prototype.constructor which cause a failure in my production env

[Jack-Works]

@erights I have fixed the tests.

[erights]

From #17 (comment)

Excepts commits in this pr, I forked to another repo to make some changes that I think may not be accepted by this project but I need them to let realms works for my project.

1. I disabled rejectHtmlComments and rejectImportExpressions in our production because it has false positive throws when I'm running an uncompressed version of react in the sandbox (which import clause is in the comment)

We plan to make these syntactic rejections switchable. As always, the default will be the safe setting --- reject --- but you'll be able to turn these off by an option setting.

2. I allow non-strict assignment in new Function by a hack way to let regenerator-runtime library runs normally (In Function("r", "regeneratorRuntime = r")(runtime) they are not using this)

The "sloppyGlobalsMode" option does this.

3. I disabled alwaysThrowHandler because it always throws on WKWebKit for unknown reason.

We merged your separate PR on this issue. Thanks!

[erights]

Regarding the remaining this binding issue, with a globalThis, could you fix this yourself in user code, after the realms shim creates a safe environment, by creating your own Function constructor, and then endowing problematic code with that Function constructor rather than the original. Since it would be constructed by user code within a safe new realm, it would not be able to introduce an isolation break. Perhaps something like:

const InnerFunction = Function;  // the post initialization safe one
function OuterFunction(...args) {
  const inner = new InnerFunction(...args);
  return Reflect.apply(inner, this || globalThis, args);
}

As this would be a one-off that doesn't really shim any behavior we would propose, I suggest that it should remain as post-initialization user code, rather than incorporating it as an option in the realms-shim.

If so, can we close this?

[Jack-Works]

Regarding the remaining this binding issue, with a globalThis, could you fix this yourself in user code, after the realms shim creates a safe environment, by creating your own Function constructor, and then endowing problematic code with that Function constructor rather than the original. Since it would be constructed by user code within a safe new realm, it would not be able to introduce an isolation break. Perhaps something like:

const InnerFunction = Function;  // the post initialization safe one
function OuterFunction(...args) {
  const inner = new InnerFunction(...args);
  return Reflect.apply(inner, this || globalThis, args);
}

As this would be a one-off that doesn't really shim any behavior we would propose, I suggest that it should remain as post-initialization user code, rather than incorporating it as an option in the realms-shim.

If so, can we close this?

Seems good to me, I'll try it if it's working in our production

[jfparadis]

@Jack-Works Is there anything I can help you with in this pull request? Let me know!

[Jack-Works]

Ah I didn't touch the web extension polyfill these days.

[Jack-Works]

@Jack-Works Is there anything I can help you with in this pull request? Let me know!

That works well in our project I guess

[erights]

Hi @Jack-Works , does #17 (comment) work for you? Can we close this?

[Jack-Works]

It may works for us 🧐🧐

I tried it in our project and it seems no error has thrown. I don't know if it is fixed

[erights]

Ok, @Jack-Works , I'll close this. But if you run into a problem, please reopen. Thanks.