MetaMask / snaps

Extend the functionality of MetaMask using Snaps
https://metamask.io/snaps/
Other
724 stars 557 forks source link

Wrap response body streams to ensure security of a teardown process #681

Open david0xd opened 2 years ago

david0xd commented 2 years ago

This is second part of the following ticket: https://github.com/MetaMask/snaps-skunkworks/issues/574 (Teardown can be escaped by late returning promises)

It is left to wrap up response body streams using transformers and cancel/stop them in the process when teardown is triggered.

res.body.pipeThrough(), res.body.pipeTo(), res.body.tee() and res.body.getReader().read() should all stop sending data after teardown without closing.

The target is network endowment (fetch): https://github.com/MetaMask/snaps-skunkworks/blob/main/packages/execution-environments/src/common/endowments/network.ts

ResponseWrapper is implemented in the following PR: https://github.com/MetaMask/snaps-skunkworks/pull/661

Part with get body(): ReadableStream<Uint8Array> should be processed somehow like this:

return this.#ogResponse.body?.pipeThrough(
      new BodyTransformStream(),
    ) as ReadableStream<Uint8Array>;

Where BodyTransformStream class should be something like:

class BodyTransformStream {
  readable;
  writable;

  constructor() {
    this.readable = new ReadableStream({
      start(controller) {
        // TODO: implement teardown
      },
      cancel(reason) {
        // TODO: implement teardown
      },
      // TODO: Check other methods as well
    });

    this.writable = new WritableStream({});
  }
}

Some useful documentation with examples: Using Readable Streams / Pipe chains Pipe through example Transform stream example

Known issues:

Some documentation that might be useful for Web Streams in NodeJS: https://nodejs.org/api/webstreams.html

WIP pull request: https://github.com/MetaMask/snaps-skunkworks/pull/685

rekmarks commented 2 years ago
  • Jest does not work well with streams so it makes it difficult for testing (ReferenceError: ReadableStream is not defined)

This may be because the tests use jsdom. You could create a new test file and add a new entry to the projects array in jest.config.js that only runs that file under the default Jest environment. If you need to import ReadableStream via Node 16, you can just add it to the global environment in a Jest setup file. See the Jest docs for details.

  • NodeJS v16 supports Web Streams API, but not without import? Try with node repl: let rs = new ReadableStream()
  • NodeJS v18 supports Web Streams API, without import. Try with node repl: let rs = new ReadableStream()

At runtime, we can either control the Node version, or we'll be in the browser. The important thing is just to get the tests working. So, for these, do whatever you need to do!

  • Importing something like import { ReadableStream } from 'node:stream/web'; is not working with the TypeScript project setup for unknown reason.

It could be that we're on an old version of @types/node. You could try bumping that, although we'll have to make sure doing so doesn't cause other problems. Otherwise, any reasonable solution you can find on Stackoverflow will do.

david0xd commented 2 years ago

Found out that it can be resolved by importing import { ReadableStream, WritableStream } from 'node:stream/web'; and updating @types/node to the latest. Not sure why it was not working previously since the same types existed in the previously used version of @types/node. Now other issues are to be resolved.