Usage of Eval() in content-task.js

NEW
Unassigned

Status

()

enhancement
P3
normal
7 months ago
3 months ago

People

(Reporter: vinoth, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-backlog1])

(Reporter)

Description

7 months ago
Usage of eval function along with system principal is being removed from everywhere as part of Bug 1473549. Here eval is used in content-task.js (https://dxr.mozilla.org/mozilla-central/rev/f87eeba88f1cf3f4d41095f7a58cb518a59f844c/testing/mochitest/BrowserTestUtils/content/content-task.js#59), which was added as part of Bug 1107609. 

Looks like around 800 test files using ContentTask.spawn (https://dxr.mozilla.org/mozilla-central/search?q=contenttask.spawn+ext%3Ajs&redirect=false)
(Reporter)

Comment 1

7 months ago
Hi Christoph,

We can potentially remove the eval() and replace it with something like loadSubScript, but that doesn't provide any security and it does mostly the same thing as eval().

Since this script is part of the test environment and ContentTask.spawn was specially added to run the user added functions in the content process, Shall I proceed with adding prefs to these test files for disabling the assertion?
Please let me know your comments.

Thanks.
Flags: needinfo?(ckerschb)
(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)
> Since this script is part of the test environment and ContentTask.spawn was
> specially added to run the user added functions in the content process,
> Shall I proceed with adding prefs to these test files for disabling the
> assertion?
> Please let me know your comments.

Since this test is only used for testing purposes anyway, I guess we should just rewrite it to use something like loadSubScript instead. That seems more feasible (actually reasonable) than annotating more than 800 tests. I assume it's not too much work to replace eval() with something like loadSubscript, or is it?
Flags: needinfo?(ckerschb)
(Reporter)

Comment 3

7 months ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> (In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)
> > Since this script is part of the test environment and ContentTask.spawn was
> > specially added to run the user added functions in the content process,
> > Shall I proceed with adding prefs to these test files for disabling the
> > assertion?
> > Please let me know your comments.
> 
> Since this test is only used for testing purposes anyway, I guess we should
> just rewrite it to use something like loadSubScript instead. That seems more
> feasible (actually reasonable) than annotating more than 800 tests. I assume
> it's not too much work to replace eval() with something like loadSubscript,
> or is it?

Sure I will replace the eval() with something similar to loadSubscript or try to execute this script in sandbox and check if the assertion is not triggering.
(Reporter)

Updated

7 months ago
Component: General → DOM: Security
Product: Testing → Core
Version: Version 3 → unspecified
Whiteboard: [domsecurity-active]
(Reporter)

Comment 4

5 months ago
Hello Gijs,

I am working on a patch to replace eval with loadFrameScript instead[1]. I am kind of stuck during the process. I am getting timeout errors when running this patch in TRY server[2]. I am not sure if the async messages I am sending is correct. 
Please kindly take a look at it and let me know in case you can figure out the problem.

[1] - https://phabricator.services.mozilla.com/D12098
[2] - https://treeherder.mozilla.org/#/jobs?repo=try&revision=36b52cee4cb6051ed0b0e26c7e82402d6bd7f702

Thanks.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 5

5 months ago
I responded over IRC. :-)
Flags: needinfo?(gijskruitbosch+bugs)
(Reporter)

Updated

3 months ago
Assignee: cegvinoth → nobody
Whiteboard: [domsecurity-active] → [domsecurity-backlog1]
You need to log in before you can comment on or make changes to this bug.