Closed Bug 1209107 Opened 5 years ago Closed 5 years ago

Don't expose module environment objects

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

ModuleEnvironmentObject is a type of scope object and is not meant to be exposed to JS in general.  Currently it can be obtained by accessing the |environment| property on a ModuleObject and is used by the self-hosted parts of the module implementation and by the tests.

There should be an intrinsic so that self-hosted JS can access this and testing functions so that tests can check its state, but the object itself should not be exposed like this.

All other ModuleObject properties are either getters for immutable objects or idempotent methods.
Duplicate of this bug: 1209008
Duplicate of this bug: 1208890
Attachment #8666742 - Flags: review?(shu)
Comment on attachment 8666742 [details] [diff] [review]
bug1209107-environment

Review of attachment 8666742 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/ModuleObject.cpp
@@ +762,5 @@
>  js::InitModuleClass(JSContext* cx, HandleObject obj)
>  {
>      static const JSPropertySpec protoAccessors[] = {
> +        //JS_PSG("initialEnvironment", ModuleObject_initialEnvironmentGetter, 0),
> +        //JS_PSG("environment", ModuleObject_environmentGetter, 0),

Should delete instead of commenting out.

::: js/src/builtin/TestingFunctions.cpp
@@ +2902,5 @@
> +        array->initDenseElement(i, StringValue(JSID_TO_STRING(ids[i])));
> +
> +    args.rval().setObject(*array);
> +    return true;
> +}

You already wrote the code for GetModuleEnvironmentNames and GetModuleEnvironmentValue so I have no objections to leaving them in. I don't understand why they were needed though, couldn't you have left the tests the same and only changed uses of module.environment to use the GetModuleEnvironment testing function?
Attachment #8666742 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #4)

> couldn't you have left the
> tests the same and only changed uses of module.environment to use the
> GetModuleEnvironment testing function?

The problem is that the fuzzers can then get hold of the scope object and manipulate it in ways we don't support (e.g. add or delete properties).  I've lost the bug where this was happening now, but the point is that these objects are not meant to be exposed to arbitrary script anyway.
Can this land soon? The fuzzbugs for this (duped here) are appearing long enough to be on the verge of being fuzzblockers.
Flags: needinfo?(jcoppeard)
https://hg.mozilla.org/mozilla-central/rev/1ac68e528d12
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Flags: needinfo?(jcoppeard)
Depends on: 1228575
Depends on: 1394493
You need to log in before you can comment on or make changes to this bug.