MetaMask / MetaMask/snaps

Wrap response body streams to ensure security of a teardown process

Open
#681 2 comments 0 reactions 0 assignees View on GitHub
type-bug type-research type-security
Dominant language
TypeScript
Stars
853
Forks
662
Avg merge
2d 8h
Merged PRs (30d)
8

Description

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` should be processed somehow like this:
```js
return this.#ogResponse.body?.pipeThrough(
new BodyTransformStream(),
) as ReadableStream;
```
Where `BodyTransformStream` class should be something like:
```js
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](https://developer.mozilla.org/en-US/docs/Web/API/Streams_API/Using_readable_streams#pipe_chains)
[Pipe through example](https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream/pipeThrough#examples)
[Transform stream example](https://github.com/mdn/dom-examples/blob/master/streams/png-transform-stream/png-transform-stream.js#L73)

Known issues:
- ~~Jest does not work well with streams so it makes it difficult for testing (`ReferenceError: ReadableStream is not defined`)~~
- ~~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()`~~
- ~~Importing something like `import { ReadableStream } from 'node:stream/web';` is not working with the TypeScript project setup for unknown reason.~~
- Testing / mocking fetch that returns a response with all WHATWG Fetch API methods (_pipeThrough_).

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

Contributor guide

Open the contributing guide

Research direction

Start in packages/execution-environments/src/common/endowments/network.ts and review the ResponseWrapper implementation from PR 661, then compare with the WIP PR 685. Use the Web Streams API documentation linked in the issue and inspect existing fetch tests or mocks. Done means pipeThrough, pipeTo, tee, and getReader().read() stop sending data after teardown without closing.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
networking, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.