Update boxes to work in SES environments

[nicolo-ribaudo]

• Fix Spec description of Box typo #255
• Make .unbox a static method
• Disallow primitives in boxes (Should boxes be allowed to contain primitives? #258)
• Add the realm check
• Don't automatically unwrap boxes in JSON.stringify, similarly to how BigInts aren't automatically serialized

[nicolo-ribaudo]

We could potentially add a non-normative note that _box_.[[Realm]] is only accessed here and in BoxSameValue, and that since we only need its identity (and not its contents) engines are could just use an UUID so that boxes don't prevent realms to be garbage-collected.

Or is it too much into the "let's not write optimizations in the spec" territory?

[sjrd]

• Disallow primitives in boxes

Disallowing primitives in boxes will make it even harder to write generic code that manipulates values of generic types and puts them in tuples and records. With that + the fact that boxes need to be explicit will now require the author of generic code to perform tests on all input values and different code paths for primitives than for non-primitives.

[sjrd]

IIUC, this also rejects Boxes in Boxes. It was already argued at #238 that Box(Box(x)) should be allowed and distinguishable from Box(x). An argument that you agreed with and approved.

[acutmore]

I kept the current JSON.stringify semantics so that it transparently unwraps boxes, but it throws when unwrapping boxes coming from different realms

At first that sounded good, being able to JSON.stringify boxes without needed to provide a replacer when staying within one realm sounds ergonomic. But I think this breaks the security for compartments that are replacing Box.unbox with a custom function per compartment.

[acutmore]

Should we also mention the [[Realm]] internal slot here? And remove the 'any ECMAScript value' ?

I think for every object and realm pair, there is a unique corresponding Box value. But not sure how to say that in emca-speak.

[ljharb]

I remain very unconvinced that anything should have per-realm semantics, including unbox.

[Jack-Works]

Why unboxing across realms is an SES hazard?

[nicolo-ribaudo]

@Jack-Works

• Because all the currently deployed membranes between two iframe-based realms check if typeof x === "object" || typeof x === "function"
• Because the Stage 3 ShadowRealm proposal (which is the first proposal to allow creating realms withing ECMAScript) allows passing primitives across the realm boundary, but not objects. When passing a record containing a box, there are two options:
  1. Throw early, preventing the whole record from being passed
  2. Allow the record to be passed, but throw when trying to read the nested object

(1) makes it impossible to pass a record containing a box even if you don't actually care about the box, forcing developers to clone R&T structures way more often.

[ljharb]

If Box can’t contain any JS value, it’s name should probably not be “Box”, because that strongly implies it’s generic - dare i say monadic.

[nicolo-ribaudo]

Tbh I would be happy to change the name, mostly because we have already failed to communicate different times about the overloaded meaning of "unbox" (object->primitive vs box->contents).

[mhofman]

I kept the current JSON.stringify semantics so that it transparently unwraps boxes, but it throws when unwrapping boxes coming from different realms. Users can provide a custom replacer to unbox boxes with the correct .unbox function, or to omit them.

As @acutmore already pointed out, it'd break the ability to virtualize Box and Box.unbox (unless you also patch JSON.stringify to wrap the serializer)

[mhofman]

I'm still unclear how realms are threaded through the spec, but I believe we settled on comparing the realm stored by the box with the realm which created the Box.unbox intrinsic. This reads to me that the operation uses the current executing realm.

This is an important distinction to avoid making Box.unbox sensitive to the calling realm.

In particular:

const otherUnbox = makeLegacyRealm().globalThis.Box.unbox;
const box = Box({});

assert.throws(() => otherUnbox(box));

[nicolo-ribaudo]

I believe built-in functions are identical to normal functions when it comes to realms handling.

When you evaluate otherUnbox(box), steps 4-5 of PrepareForOrdinaryCall sets the "current execution context" (which stores the "current Realm") to the realm associated to the unbox function.

[mhofman]

My bad, I confused execution context and environment record

[Jack-Works]

Thanks, but why disallowing Boxing primitives? Is there any harm there?

[nicolo-ribaudo]

I'm pulling out the "only objects" restriction from this PR, so that we can discuss more about it.

EDIT: #257 (comment)

[mhofman]

As I mentioned earlier, at the very least if we allow primitives in Boxes, we need to only disallow unboxing based on realm if the content is an object, not a primitive. Otherwise there is no way to remember the mapping necessary for ShadowRealm for records that do not contain recursively contain an object (can't put them in WeakMap)

So this PR should either disallow primitives, or it should check the realm only for object content.

As for the virtualization of boxes in compartments, I think JSON.stringify and toString should be consistent: either neither unbox the content or they both do. I do however have an idea to remove the auto unboxing constraint while not breaking virtualization, but I'll follow up on that in #254.

[mhofman]

This would make it completely untenable for membranes, as there's be no way to remember the mapping of a record/tuple which only contains boxes of primitives.

[nicolo-ribaudo]

Why does it need to be remembered? Can't membranes rebuild the box every time, since it doesn't have identity?

[mhofman]

That means every time you send a record/tuple through the membrane that Box.containsBox but does not Object.containsObject, you have to recursively unbox all the boxes, push their content, and remap the record/tuple/box on the other side, without being able to remember this work. That would be incredibly inefficient.

[mhofman]

As discussed in the SES meeting, to keep the Box semantics of being both non-transparent and requiring permission to unbox through access to Box.unbox, any type of content (primitive or objects) has to have a realm check.

We also realized that while there is no way around the impossibility to memoize the result of mapping a box of primitive cross-realm, a fully transparent membrane doesn't have to eagerly map boxes when processing a record/tuple/box, but can defer it to a replaced Box.unbox that is membrane aware. The limitation is in the case of realms n > 2, the membrane has to try each realm's Box.unbox sequentially until it find the right one (which could be optimized my sorting by last realm interacted with).

[mhofman]

This may cause issues with virtualization of Box.unbox in compartments, I need to think more about it.

[nicolo-ribaudo]

I can make it return just Box().

[nicolo-ribaudo]

@mhofman With this toString, code in one compartment can execute code on another compartment, by doing something like

// comparment A
const box = Box({
  toString() {
    return "Hello from compartment A!";
  }
});

// compartment B
const box = getBoxFromCompartmentA();
String(box); // logs "Box(Hello from compartment A!)"

However, toString can only return primitives and not objects: is this an authority escalation (if that's the correct term)?

It might cause problems in membranes that want to distort the .toString of a box's content, but this can be worked around by replacing the box ahead of time.

[mhofman]

Edit: this is wrong, ignore.

Not with the virtualization a Compartment would do to Box. See #254 (comment)

function toString() {
  const target = unwrap(this);
  // Perform https://tc39.es/ecma262/#sec-ordinarytoprimitive
  for (const methodName of ['toString', 'valueOf']) {
    const method = target[methodName];
    if (typeof method === 'function') {
      const result = method.call(target);
      if (Object(result) !== result) {
        return result;
      }
    }
  }
  throw new TypeError();
}

Looking at this I probably forgot the fallback to toStringTag, but that's a small detail.

Edit: Nope, I'm not fully awake I guess, if the real target object has no toString or valueOf, it should throw, and it's Object.prototype.toString that takes care of the rest.

[mhofman]

Ignore above comment, I the virtualization of toString is not possible.

[nicolo-ribaudo]

I have moved the discussion about primitives in boxes to #258, since it was happening in multiple parallel threads.

I have marked the comments about that topic in this PR as "outdated" since this PR doesn't disallow them anymore, so that it's easier to see the arguments about the other topics covered by this PR.

[mhofman]

Since boxes are not objects, should it return Box() ?

Suggested change

	1. Return *"[object Box]"*.
	1. Return *"Box()"*.

[jridgewell]

I don't understand why explicit realm checks are necessary. If the values are objects, then SameValue shouldn't already return false? And if they're primitives, it shouldn't matter what realm they were created in.

[ljharb]

if they're primitives, it shouldn't matter what realm they were created in.

this is my stance, but this PR is explicitly trying to change it so that you can't unbox a Box primitive unless you have the Box constructor from the realm it was created in.

[mhofman]

To keep object graphs separate for ShadowRealm, and enforce existing security-critical assumptions of membranes between legacy realms, a Box of object cannot be unboxable across a Realm boundary.

The model is then to say that a Box is realm-local primitive opaque wrapper for a value. I originally wanted to allow primitives to be unboxed cross-realm as they wouldn't break the above realm constraints, but @erights pointed out that users may assume that if a box requires permission (through access to Box.unbox) to open, it should apply regardless of the content, as a user may wish to put a sensitive primitive value in it (e.g. unique symbol).

If a box can only be unboxed using the local realm's Box.unbox, it has to be different from another realm's box containing the same value.

[jridgewell]

To keep object graphs separate for ShadowRealm,

How does a boxed object cross the realm boundary? Shouldn't that just throw?

enforce existing security-critical assumptions of membranes between legacy realms,

What's the failure case?

[acutmore]

To keep object graphs separate for ShadowRealm, and enforce existing security-critical assumptions of membranes between legacy realms, a Box of object cannot be unboxable across a Realm boundary.

To further explain the two situations that have been presented as motivating cases for uniquely pairing each box constructor with its unbox function.

“Legacy realms”:

• Existing code that is not patched in time, running in newer environments.
• An application that is creating realms (e.g same-origin-iFrames)
• with all values passed between them being checked for 'isObject', then all object values are secured (denied or wrapped in a Proxy), and all other values passing directly across.
• And also not restricting globalThis with an allowList (that would remove the unrecognised Box constructor).

Here code in one realm could now give another realm direct access to its objects by putting one in a Tuple using a Box. And now when that value is passed across it will appear as a primitive: Object(tuple) !== tuple so it will pass through as is.

“ShadowRealms”:

Being able to call shadow-realm proxied functions with Records and Tuples would be very useful, e.g. the named argument pattern.

For protection shadowRealms do not allow directly passing objects.

What should happen if a record being passed to a shadowRealm contains a Box with an object inside it? One option is only boxless R&T can be used, otherwise they will throw. Another option is they can be passed across, but not unboxed. This second option could be useful in situations where it’s useful to preserve identity across the boundary but still restrict access.

[jridgewell]

This is much less ergonomic to use. It's not clear to me why it needs to be a static method instead of a instance method.

[mhofman]

Is Box.unbox(box) that much less ergonomic than box.unbox()?
Also nit, box being a primitive, it wouldn't per-se be an instance method, but a prototype method of the Box object wrapper. Users are still free to install their own:

Box.prototype.unbox = function unbox() {
  return Box.unbox(this);
};

[jridgewell]

Is Box.unbox(box) that much less ergonomic than box.unbox()?

In an OOP oriented language, considerably less.

[acutmore]

Potentially a use case for pipeline |> ?