fix: add native function to create preload script #13032
Conversation
|
💖 Thanks for opening this pull request! 💖 We use Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
There was a problem hiding this comment.
I'm not really sure how to add a test for this. I guess a custom protocol might work?
One test that you could add is to verify that the preload wrapper function doesn't have access to the local scope where it is executed. In other words, it should behave as eval when it is called as a different name (source):
If you use the eval function indirectly, by invoking it via a reference other than eval, as of ECMAScript 5 it works in the global scope rather than the local scope. This means, for instance, that function declarations create global functions, and that the code being evaluated doesn't have access to local variables within the scope where it's being called.
Other than that, the existing sandbox tests already verify preload is still working.
|
Also, please rebase to ensure everything is working on top of latest master, sandbox loading was changed recently. |
spec/webview-spec.js
Outdated
| @@ -248,6 +248,37 @@ describe('<webview> tag', function () { | |||
| }) | |||
| }) | |||
|
|
|||
| it('runs in the correct context', async () => { | |||
There was a problem hiding this comment.
I didn't see an existing test for the unsandboxed version, but maybe there's one I've missed?
There was a problem hiding this comment.
Unsandboxed preload exposes everything by default so it is not needed.
There was a problem hiding this comment.
Technically, the test title should be "run in the correct scope". Context denotes the global JS environment, and both the page and preload run in the same context.
|
@tarruda can this be merged? |
|
@PalmerAL I believe so, but I don't have permissions to merge it. Can you push again to trigger a new CI run? Maybe after all tests are passing it will be merged. |
|
The mac failures seem to be because of #13316, everything else is passing. |
|
@zcbenz (or someone else) can this be merged? |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
@tarruda shouldn't this be backported to 3.0? |
Fixes #9276.
It looks like
file:///anddata:URL's are always able to use eval() regardless of what the CSP is, so I'm not really sure how to add a test for this. I guess a custom protocol might work?