1288284 - Content scripts window.eval runs code in the page, not in the content script

Status: RESOLVED INVALID

(WebExtensions :: Untriaged, defect, P3)

Description

[Rob Wu [:robwu]]

Attachment: window-eval.zip

1. Load attachment at about:debugging
2. Visit example.com
3. Look at the console.

Expected: The lines are all true statements.
Actual:
Content script: Should be 1: undefined <------ WRONG
Content script: Should be 2: 2
Page script: Should be undefined: 1 <---- WRONG
Page script: Should be undefined: undefined

The first and the third line use window.eval,
the second and fourth line use eval.

eval executes in the current scope, window.eval executes in the page's scope.

---

eval and window.eval already have different semantics:
1. window.eval runs code in the global context.
2. eval runs code in the local context.
The execution context should be the content script. If there is a desire to run code in the page, then it should be more explicit. Implicitly switching contexts depending on how a function is called is just confusing.


I also found that window.Function and Function behave differently, but let's focus on eval for now.

Firefox version: Firefox 47.0.1, Firefox Nightly 50 (2016-07-20)

Comment 1

[Kris Maglione [:kmag]]

`window.eval` runs code in the context of the window object it's bound to, not in the context of the current global. iframe.contentWindow.eval, for instance, runs code in the context of that iframe's window. Unbound eval (`(0, eval)(...)`) evaluates code in the context of its incumbent global.

Those are the same semantics in play here. I don't think it makes sense to change them. Being able to evaluate code in the context of the content window is a useful feature.

Comment 2

[Rob Wu [:robwu]]

(In reply to Kris Maglione [:kmag] from comment #1)
> `window.eval` runs code in the context of the window object it's bound to,
> not in the context of the current global. iframe.contentWindow.eval, for
> instance, runs code in the context of that iframe's window. Unbound eval
> (`(0, eval)(...)`) evaluates code in the context of its incumbent global.

In normal web pages global scope === window. Idem in Chrome's, (old) Opera's and Safari's content script.
Many articles describe window.eval vs eval as I mentioned, including our own MDN [1].
The only doc that I found which describes WebExtensions's current semantics for window.eval is Greasemonkey [2] (and this is not even presented as a Firefox-thing, but GM). And I only found this after Googling for "window.eval Greasemonkey"; searching for "window.eval Firefox" only provides results supporting the other behavior.

> Those are the same semantics in play here. I don't think it makes sense to
> change them.

window.eval in content scripts differ from window.eval on the web. We should design APIs in a way that result in the least surprises, and crossing boundaries with security implications should not be dead-easy. Repurposing an existing API (window.eval) to switch execution environments does not fit with that.

> Being able to evaluate code in the context of the content
> window is a useful feature.

I agree, and this is already possible via <script>. To communicate back (aka the return value of eval), events, DOM or helpers from bug 1280482 can be used.

In my tests (see comment 3 below), I found that window.eval / {window.,}{setTimeout,setInterval} / window.Function all run code in the page's context. I don't see a legitimate use for the setTimeout/setInterval behavior, only downsides.

If it is too late to fix/change the behavior of window.eval and such, then their semantics must be clearly documented, including pitfalls.


[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval
[2] https://wiki.greasespot.net/Content_Script_Injection

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1288284 - Add tests for eval and friends in content script

Review commit: https://reviewboard.mozilla.org/r/65854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65854/

Comment 4

[Andy McKay]

Rob, it looks like this patch didn't have a reviewer on it and got dropped, would it be worth trying to clean and ask for review? Also sounds like you want dev-doc-needed based on comment 2?

Comment 5

[Rob Wu [:robwu]]

(In reply to Andy McKay [:andym] from comment #4)
> Rob, it looks like this patch didn't have a reviewer on it and got dropped,
> would it be worth trying to clean and ask for review?

The patch adds tests that demonstrate the behavior that I expected, not the actual behavior.
Since Kris closed this bug as INVALID, I think that there is no use in reviving the patch.

> Also sounds like you want dev-doc-needed based on comment 2?

It's unfortunate that this issue has been stuck around for so long. Now add-on devs can reasonably expect that this is the expected behavior.

In Chrome the logic is simple: Content scripts can never implicitly run code in a context with different security/privilege semantics with ordinary JS functions (eval etc.).
In Firefox, it is possible to inadvertently run code in an execution context with different security/privilege semantics (via window.eval etc).

This difference of window.eval in content scripts between browsers should be documented somewhere.
Perhaps a subsection at the Content scripts article [1], linked from [2] (in case people search for "eval" and only read the docs for "eval" instead of "Content scripts").


[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_scripts
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval

Comment 6

[Will Bamberg [:wbamberg]]

I've added a section here:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_scripts#Using_eval()_in_content_scripts

Let me know if this covers it.

For this:

> linked from [2] (in case people search for "eval" and only read the docs for "eval" instead of "Content scripts").
> ...
> [2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval

NI Florian, who owns the JS docs. I'm not sure it is needed, personally.

Comment 7

[Florian Scholz (Open Web Docs)]

(In reply to Will Bamberg [:wbamberg] from comment #6)
> > linked from [2] (in case people search for "eval" and only read the docs for "eval" instead of "Content scripts").
> > ...
> > [2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval
> 
> NI Florian, who owns the JS docs. I'm not sure it is needed, personally.

The JS docs try to be neutral about their environment (web, node.js, addons, etc) especially for example code. So I don't want to add an WebExtension-specific example to the page. I've linked to Will's section from "See also", though. 

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#See_also

Comment 8

[Will Bamberg [:wbamberg]]

Sorry Rob, forgot to NI you for the changes linked in comment 6.

Comment 9

[Rob Wu [:robwu]]

Looks good. I edited the doc to add validation in the "message" event, to encourage developers to always validate messages in global "message" events.