FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

websocket: handle empty fragments and stream limits · nodejs/undici@c5ed787 · GitHub

/ undici Public

Commit c5ed787

Browse files
authored and
mcollina
committed
websocket: handle empty fragments and stream limits
Treat zero-byte frames as real fragments so fragmented messages can start with an empty frame and empty continuations still count toward maxFragments. Pass dispatcher WebSocket limits through to WebSocketStream's parser, add regression coverage for WebSocket and WebSocketStream fragment limits, make the fragment close tests wait for both endpoints, and fix the Client docs typo for maxFragments. Co-authored-by: Ulises Gascon <ulisesgascongonzalez@gmail.com>
1 parent e114e77 commit c5ed787

5 files changed

Lines changed: 175 additions & 9 deletions

File tree

‎docs/docs/api/Client.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ Returns: `Client`
2525
* **maxHeaderSize** `number | null` (optional) - Default: `--max-http-header-size` or `16384` - The maximum length of request headers in bytes. Defaults to Node.js' --max-http-header-size or 16KiB.
2626
* **maxResponseSize** `number | null` (optional) - Default: `-1` - The maximum length of response body in bytes. Set to `-1` to disable.
2727
* **webSocket** `WebSocketOptions` (optional) - WebSocket-specific configuration options.
28-
* **maxFragments** `number` (optional) - Defailt: `131072` - Maximum number of fragments in a message. Set to 0 to disable the limit.
28+
* **maxFragments** `number` (optional) - Default: `131072` - Maximum number of fragments in a message. Set to 0 to disable the limit.
2929
* **maxPayloadSize** `number` (optional) - Default: `134217728` (128 MB) - Maximum allowed payload size in bytes for WebSocket messages. Applied to uncompressed messages, compressed frame payloads, and decompressed (permessage-deflate) messages. Set to 0 to disable the limit.
3030
* **pipelining** `number | null` (optional) - Default: `1` - The amount of concurrent requests to be sent over the single TCP/TLS connection according to [RFC7230](https://tools.ietf.org/html/rfc7230#section-6.3.2). Carefully consider your workload and environment before enabling concurrent requests as pipelining may reduce performance if used incorrectly. Pipelining is sensitive to network stack settings as well as head of line blocking caused by e.g. long running requests. Set to `0` to disable keep-alive connections. This option has no effect once HTTP/2 is negotiated — see `maxConcurrentStreams` for the h2 dispatch ceiling.
3131
* **connect** `ConnectOptions | Function | null` (optional) - Default: `null` - Configures how undici establishes TCP/TLS connections. Accepts two forms:

‎lib/web/websocket/receiver.js‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ class ByteParser extends Writable {
3939
/** @type {import('./websocket').Handler} */
4040
#handler
4141

42-
4342
/** @type {number} */
4443
#maxFragments
4544

@@ -247,7 +246,7 @@ class ByteParser extends Writable {
247246
this.#state = parserStates.INFO
248247
} else {
249248
if (!this.#info.compressed) {
250-
if (body.length && !this.writeFragments(body)) {
249+
if (!this.writeFragments(body)) {
251250
return
252251
}
253252

@@ -271,7 +270,7 @@ class ByteParser extends Writable {
271270
return
272271
}
273272

274-
if (data.length && !this.writeFragments(data)) {
273+
if (!this.writeFragments(data)) {
275274
return
276275
}
277276

‎lib/web/websocket/stream/websocketstream.js‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,14 @@ class WebSocketStream {
258258
#onConnectionEstablished (response, parsedExtensions) {
259259
this.#handler.socket = response.socket
260260

261-
const parser = new ByteParser(this.#handler, parsedExtensions)
261+
// Get options from dispatcher options
262+
const maxFragments = this.#handler.controller.dispatcher?.webSocketOptions?.maxFragments
263+
const maxPayloadSize = this.#handler.controller.dispatcher?.webSocketOptions?.maxPayloadSize
264+
265+
const parser = new ByteParser(this.#handler, parsedExtensions, {
266+
maxFragments,
267+
maxPayloadSize
268+
})
262269
parser.on('drain', () => this.#handler.onParserDrain())
263270
parser.on('error', (err) => this.#handler.onParserError(err))
264271

‎test/websocket/fragments.js‎

Lines changed: 106 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,15 @@ test('Fragmented frame with a ping frame in the middle of it', (t) => {
4343
test('Too many fragments (uncompressed)', (t, done) => {
4444
t.plan(4)
4545

46+
function maybeDone () {
47+
if (++maybeDone.callCount === 2) {
48+
agent.close()
49+
server.close(done)
50+
}
51+
}
52+
53+
maybeDone.callCount = 0
54+
4655
const agent = new Agent({
4756
webSocket: {
4857
maxFragments: 3
@@ -61,15 +70,15 @@ test('Too many fragments (uncompressed)', (t, done) => {
6170

6271
client.addEventListener('close', (event) => {
6372
t.assert.deepStrictEqual(event.code, 1006)
73+
maybeDone()
6474
})
6575
})
6676

6777
server.on('connection', (ws) => {
6878
ws.on('close', (code, reason) => {
6979
t.assert.deepStrictEqual(code, 1008)
7080
t.assert.deepStrictEqual(reason.toString(), 'Too many message fragments')
71-
agent.close()
72-
server.close(done)
81+
maybeDone()
7382
})
7483

7584
const fragment = Buffer.from('a')
@@ -85,6 +94,15 @@ test('Too many fragments (uncompressed)', (t, done) => {
8594
test('Too many fragments (compressed)', (t, done) => {
8695
t.plan(4)
8796

97+
function maybeDone () {
98+
if (++maybeDone.callCount === 2) {
99+
agent.close()
100+
server.close(done)
101+
}
102+
}
103+
104+
maybeDone.callCount = 0
105+
88106
const agent = new Agent({
89107
webSocket: {
90108
maxFragments: 3
@@ -106,15 +124,15 @@ test('Too many fragments (compressed)', (t, done) => {
106124

107125
client.addEventListener('close', (event) => {
108126
t.assert.deepStrictEqual(event.code, 1006)
127+
maybeDone()
109128
})
110129
})
111130

112131
server.on('connection', (ws) => {
113132
ws.on('close', (code, reason) => {
114133
t.assert.deepStrictEqual(code, 1008)
115134
t.assert.deepStrictEqual(reason.toString(), 'Too many message fragments')
116-
agent.close()
117-
server.close(done)
135+
maybeDone()
118136
})
119137

120138
const fragment = Buffer.from('a')
@@ -126,3 +144,87 @@ test('Too many fragments (compressed)', (t, done) => {
126144
ws.send(fragment, options)
127145
})
128146
})
147+
148+
test('Empty first fragment followed by non-empty continuation delivers the message', (t) => {
149+
// RFC 6455 §5.4 allows zero-byte fragments. A conforming server that opens
150+
// a fragmented message with an empty frame must be honored: the parser must
151+
// recognize the in-progress fragmented message when the continuation arrives.
152+
const server = new WebSocketServer({ port: 0 })
153+
154+
server.on('connection', (ws) => {
155+
ws.send('', { fin: false }) // Text frame fin=0, len=0
156+
ws.send('hello', { fin: true }) // Continuation fin=1, "hello"
157+
})
158+
159+
after(() => {
160+
for (const client of server.clients) {
161+
client.close()
162+
}
163+
164+
server.close()
165+
})
166+
167+
const ws = new WebSocket(`ws://localhost:${server.address().port}`)
168+
169+
return new Promise((resolve) => {
170+
ws.addEventListener('message', ({ data }) => {
171+
t.assert.strictEqual(data, 'hello')
172+
173+
ws.close()
174+
resolve()
175+
})
176+
})
177+
})
178+
179+
test('Too many empty fragments triggers close 1008', (t, done) => {
180+
// Empty fragments must still count toward maxFragments; otherwise a
181+
// peer can flood zero-byte continuation frames forever.
182+
t.plan(4)
183+
184+
function maybeDone () {
185+
if (++maybeDone.callCount === 2) {
186+
agent.close()
187+
server.close(done)
188+
}
189+
}
190+
191+
maybeDone.callCount = 0
192+
193+
const agent = new Agent({
194+
webSocket: {
195+
maxFragments: 3
196+
}
197+
})
198+
199+
const server = new WebSocketServer({ port: 0 }, () => {
200+
const { port } = server.address()
201+
const client = new WebSocket(`ws://127.0.0.1:${port}`, {
202+
dispatcher: agent
203+
})
204+
205+
client.addEventListener('error', (event) => {
206+
t.assert.ok(true)
207+
})
208+
209+
client.addEventListener('close', (event) => {
210+
t.assert.deepStrictEqual(event.code, 1006)
211+
maybeDone()
212+
})
213+
})
214+
215+
server.on('connection', (ws) => {
216+
ws.on('close', (code, reason) => {
217+
t.assert.deepStrictEqual(code, 1008)
218+
t.assert.deepStrictEqual(reason.toString(), 'Too many message fragments')
219+
maybeDone()
220+
})
221+
222+
const fragment = ''
223+
const options = { fin: false }
224+
225+
ws.send(fragment, options) // Text frame fin=0, len=0
226+
ws.send(fragment, options) // Continuation fin=0, len=0
227+
ws.send(fragment, options) // Continuation fin=0, len=0
228+
ws.send(fragment, options) // Continuation fin=0, len=0
229+
})
230+
})

‎test/websocket/stream/too-many-fragments.js‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
'use strict'
2+
3+
const { test } = require('node:test')
4+
const { WebSocketServer } = require('ws')
5+
const {
6+
Agent,
7+
WebSocketStream,
8+
getGlobalDispatcher,
9+
setGlobalDispatcher
10+
} = require('../../..')
11+
12+
test('Too many fragments via WebSocketStream triggers close 1008', async (t) => {
13+
// WebSocketStream reads its dispatcher from the global one (its options
14+
// dictionary doesn't accept a dispatcher), so we swap it for the duration
15+
// of this test. The fragment-count limit must apply to WebSocketStream
16+
// the same way it applies to the WebSocket API.
17+
const previous = getGlobalDispatcher()
18+
const agent = new Agent({
19+
webSocket: {
20+
maxFragments: 3
21+
}
22+
})
23+
setGlobalDispatcher(agent)
24+
25+
const server = new WebSocketServer({ port: 0 })
26+
27+
t.after(async () => {
28+
setGlobalDispatcher(previous)
29+
await agent.close()
30+
server.close()
31+
})
32+
33+
const serverClose = new Promise((resolve) => {
34+
server.on('connection', (ws) => {
35+
ws.on('close', (code, reason) => {
36+
resolve({ code, reason: reason.toString() })
37+
})
38+
39+
const fragment = Buffer.from('a')
40+
const options = { fin: false }
41+
42+
ws.send(fragment, options)
43+
ws.send(fragment, options)
44+
ws.send(fragment, options)
45+
ws.send(fragment, options)
46+
})
47+
})
48+
49+
const wss = new WebSocketStream(`ws://localhost:${server.address().port}`)
50+
51+
// The connection will be failed by the parser; both `opened` and `closed`
52+
// settle. We only care that the server observed the policy-violation close.
53+
await Promise.allSettled([wss.opened, wss.closed])
54+
55+
const observed = await serverClose
56+
t.assert.deepStrictEqual(observed.code, 1008)
57+
t.assert.deepStrictEqual(observed.reason, 'Too many message fragments')
58+
})

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL