Eval in specialpowersAPI.js

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: vinoth, Assigned: vinoth)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
As part of Bug 1473549, we are in the process of adding an assertion to make sure that eval() is not executed with system principal.

Eval() has been used in specialpowersAPI.js, we need few information about whether this eval() can be replaced or prefs need to be added for all tests that are using this specialpowersAPI.js
(Assignee)

Comment 1

9 months ago
Hey Geoff Brown,

Please see comment 1. Latest change to specialpowersAPI.js related to eval has been made by you as part of Bug 1367780 (https://hg.mozilla.org/mozilla-central/annotate/4e56a2f51ad739ca52046723448f3129a58f1666/testing/specialpowers/content/specialpowersAPI.js#l41) 

Please let us know whether we can change this file for replacing the eval with someother workaround for getting the global this object. Let me know incase you have any comments on this.

Thanks.
Flags: needinfo?(gbrown)
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
I only added the eslint comment on that line. Better to ask :bz, who added the eval in

https://hg.mozilla.org/mozilla-central/rev/322ea4cf5b8e
Flags: needinfo?(gbrown) → needinfo?(bzbarsky)
I think bug 1444973 changed the way browser-test.js so now it's always loaded with a Window on its scope chain.  But it's possible we're still loading specialpowersAPI.js from other non-Window contexts...  And "this" is still not the global when specialpowersAPI.js is loaded from browser-tests.js.

I think there are basically two options here.  One is to explicitly check for each name here and import it if needed:

  try { DOMParser; } catch (e) { Cu.importGlobalProperties(["DOMParser"]); }

and the same for File, InspectorUtils, NodeFilter.  The bareword get is important, because that will throw of the property is not defined, instead of returning undefined.

Another option is to do:

  try {
      Cu.importGlobalProperties(["DOMParser", "File", "InspectorUtils", "NodeFilter"]);
  } catch (e) {
      /* We must be in a Window scope; we already have all those defined. */
  }

or so.
Flags: needinfo?(bzbarsky)
Comment hidden (spam)
Comment hidden (spam)
(Assignee)

Comment 7

8 months ago
Comment on attachment 9004182 [details]
Bug 1482347 - Usage of Eval in specialpowersAPI.js removed.

Hey Jonathan,
Since christoph is on vacation, Please kindly review the patch and let me know if you have any comments.

I have made the changes to specialpowersAPI.js as per comment 3. 
And try server push for this changes are,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b61da9d33b749a401e01284894762b10465ea386

I am also doubtful whether the few failures in try push are related to this patch changes. Correct me if I am wrong.
Attachment #9004182 - Flags: review?(jkt)
Comment on attachment 9004182 [details]
Bug 1482347 - Usage of Eval in specialpowersAPI.js removed.

R+ on phab but as noted there you need a peer also. :)
Attachment #9004182 - Flags: review?(jkt) → review+
(Assignee)

Comment 9

8 months ago
Comment on attachment 9004182 [details]
Bug 1482347 - Usage of Eval in specialpowersAPI.js removed.

I have made the changes to specialpowersAPI.js as per comment 3. 
And new try server push for this changes are,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2aeba7c28be33f0008011ee015ac37526e68c894

Please kindly take a look and let me know incase of changes.
Attachment #9004182 - Flags: review?(bzbarsky)
Comment on attachment 9004182 [details]
Bug 1482347 - Usage of Eval in specialpowersAPI.js removed.

Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.
Attachment #9004182 - Flags: review+
Comment on attachment 9004182 [details]
Bug 1482347 - Usage of Eval in specialpowersAPI.js removed.

Jonathan Kingston [:jkt] has been removed from the revision.
Attachment #9004182 - Flags: review+
Attachment #9004182 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 12

8 months ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8fe981dd967
Usage of Eval in specialpowersAPI.js removed. r=bzbarsky
Keywords: checkin-needed

Comment 13

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f8fe981dd967
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.