1736622 - Resolving a Promise with a WebIDL type can lead to unexpected results when Object.prototype.then is defined

Status: RESOLVED INVALID

(Core :: DOM: Bindings (WebIDL), defect)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

In at least two specifications, Streams and Web Locks, there are points where a Promise is resolved with a WebIDL type (ReadableStreamDefaultReader.read, LockManager.query).

In implementation, we end up doing promise->MaybeResolve(idlDict). When we resolve an idlDict, we convert the result to a JS object first (using DictType::ToObjectInternal), then resolve the Promise with the JS object.

The problem is that if Object.prototype.then is set, the result of that resolve can be rewritten arbitrarily (see the special handling of the then property in the Promises specification (Steps 9-12)), since the converted dictionary objects have Object as their prototype.

For example this test case for Streams explicitly tests for this possibility by trying to use Object.prototype.then to replace a 'hello' with a 'bye' (the test passes in the JS streams implementation because it follows an older non-WebIDL version of the spec that had an explicit boolean to avoid this issue), requiring that a promise resolved with a WebIDL type not resolve using Object.prototype.then.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Note: The current checked in code in the DOM Streams implementation doesn't use an IDL type, and suffers from this bug. However, if I were to patch ChunkSteps to look like this instead, it would still fail for the above reason:

  void ChunkSteps(JSContext* aCx, JS::Handle<JS::Value> aChunk,
                  ErrorResult& aRv) override {
    // Step 1.
    RootedDictionary<ReadableStreamDefaultReadResult> result(aCx);
    result.mDone.Construct(false);
    result.mValue = aChunk;
    mPromise->MaybeResolve(result);
  }

Comment 2

[Peter Van der Beken [:peterv]]

In https://webidl.spec.whatwg.org/#resolve, step 2 is "Let value be the result of converting x to an ECMAScript value" and step 3 is "Perform ! Call(p.[[Resolve]], undefined, « value »)". Isn't that exactly what we're doing?

Comment 3

[Anne (:annevk)]

I think I figured out what might be going wrong here. Fetch doesn't use read() on a stream directly. The text() method on a response invokes https://fetch.spec.whatwg.org/#concept-body-consume-body. That uses https://fetch.spec.whatwg.org/#fully-reading-body-as-promise to read the body. After getting a reader that uses https://streams.spec.whatwg.org/#readablestreamdefaultreader-read-all-bytes. Now while these algorithms do use promises, they don't have anything that can be hooked by Object.prototype.then. It does seem like there's a problem though as https://webidl.spec.whatwg.org/#resolve doesn't really account for a byte sequence (a specification type): https://github.com/whatwg/streams/issues/1178.

Hope this helps.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Yeah, I think this is not a WebIDL issue. Definitely a streams specification issue.

Thanks for the investigation everyone, closing this as invalid.