Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

Restrict Max Message Size #158

Closed
maschad opened this issue May 9, 2023 · 2 comments
Closed

Restrict Max Message Size #158

maschad opened this issue May 9, 2023 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@maschad
Copy link
Member

maschad commented May 9, 2023

          Currently, it's not practical to use `RTCDataChannel` for messages larger than 64 KiB (16 KiB if you want to support cross-browser exchange of data - That is why in the [spec states](https://github.com/libp2p/specs/tree/master/webrtc#multiplexing) that encoded messages including their length prefix **MUST NOT** exceed 16kiB to support all major browsers.) 

The problem arises from the fact that SCTP—the protocol used for sending and receiving data on an RTCDataChannel—was originally designed for use as a signaling protocol. There is a [proposal](https://datatracker.ietf.org/doc/html/rfc8260) which will make it possible to send messages with essentially no size limitations, but for now perhaps we should just set that max limit to 16Kb to avoid _silent_ failures for consumers.

Originally posted by @maschad in #144 (comment)

We should throw an error if it the message size exceeds this limit

@maschad maschad self-assigned this May 9, 2023
@maschad maschad added the bug Something isn't working label May 9, 2023
@marcus-pousette
Copy link
Contributor

marcus-pousette commented May 9, 2023

Hey! can you check out the PR #147 ? It is solving this problem without putting any restrictions on message size, instead since we are doing ordered messages, and use 'it-length-prefixed', we can chunk the message up to appropriate sizes and aggregate them on the other end without any issues. The message size option I provided there is basically the chunk size.

If you don't like that route, just putting a message size error will not be enough (from the information I gathered) you also needs to make sure that the underlying buffer of webrtc does not exceed this, since message seems to be sent in batches (?) or there are other limits at play here. See this thread.

I am trying to update the PR but the build is breaking in main in this repository so I can't resolve merge conflicts at the moment.

@maschad
Copy link
Member Author

maschad commented May 9, 2023

See my comment on the previous issue.

If you don't like that route, just putting a message size error will not be enough (from the information I gathered) you also needs to make sure that the underlying buffer of webrtc does not exceed this, since message seems to be sent in batches (?) or there are other limits at play here. See feross/simple-peer#393 thread.

Good find, I think that fix should address this issue.

main CI seems to be working at b3cb445 please rebase off that commit.

github-actions bot pushed a commit that referenced this issue May 17, 2023
## [2.0.3](v2.0.2...v2.0.3) (2023-05-17)

### Bug Fixes

* restrict message sizes to 16kb ([#147](#147)) ([aca4422](aca4422)), closes [#144](#144) [#158](#158)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

2 participants