Skip to content

Commit f4c31d6

Browse files
mcollinaUlisesGascon
mcollina
and
UlisesGascon
authored
fix: guard idle socket validation to skip fresh sockets (#5400)
* fix: guard idle socket validation to skip fresh sockets References: GHSA-35p6-xmwp-9g52 Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Ulises Gascon <ulisesgascongonzalez@gmail.com> * test: update issue 810 idle validation expectations * test: stabilize idle validation coverage * test: avoid issue 810 hangs on older Node.js * test: stabilize parser error cleanup * test: stabilize fetch encoding sockets * test: use explicit loopback in fetch encoding * test: use explicit loopback in fetch cookies * test: use explicit loopback in fetch h2 cookies --------- Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Ulises Gascon <ulisesgascongonzalez@gmail.com>
1 parent 768beac commit f4c31d6

6 files changed

Lines changed: 281 additions & 144 deletions

File tree

‎lib/dispatcher/client-h1.js‎

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ const EMPTY_BUF = Buffer.alloc(0)
5757
const FastBuffer = Buffer[Symbol.species]
5858
const addListener = util.addListener
5959
const removeAllListeners = util.removeAllListeners
60+
const kIdleSocketValidation = Symbol('kIdleSocketValidation')
61+
const kIdleSocketValidationTimeout = Symbol('kIdleSocketValidationTimeout')
62+
const kSocketUsed = Symbol('kSocketUsed')
6063

6164
let extractBody
6265

@@ -371,6 +374,11 @@ class Parser {
371374
return -1
372375
}
373376

377+
if (client[kRunning] === 0) {
378+
util.destroy(socket, new SocketError('bad response', util.getSocketInfo(socket)))
379+
return -1
380+
}
381+
374382
const request = client[kQueue][client[kRunningIdx]]
375383
if (!request) {
376384
return -1
@@ -474,6 +482,11 @@ class Parser {
474482
return -1
475483
}
476484

485+
if (client[kRunning] === 0) {
486+
util.destroy(socket, new SocketError('bad response', util.getSocketInfo(socket)))
487+
return -1
488+
}
489+
477490
const request = client[kQueue][client[kRunningIdx]]
478491

479492
/* istanbul ignore next: difficult to make a test case for */
@@ -647,6 +660,7 @@ class Parser {
647660
request.onComplete(headers)
648661

649662
client[kQueue][client[kRunningIdx]++] = null
663+
socket[kSocketUsed] = true
650664

651665
if (socket[kWriting]) {
652666
assert(client[kRunning] === 0)
@@ -705,6 +719,9 @@ async function connectH1 (client, socket) {
705719
socket[kWriting] = false
706720
socket[kReset] = false
707721
socket[kBlocking] = false
722+
socket[kIdleSocketValidation] = 0
723+
socket[kIdleSocketValidationTimeout] = null
724+
socket[kSocketUsed] = false
708725
socket[kParser] = new Parser(client, socket, llhttpInstance)
709726

710727
addListener(socket, 'error', function (err) {
@@ -751,6 +768,8 @@ async function connectH1 (client, socket) {
751768
const client = this[kClient]
752769
const parser = this[kParser]
753770

771+
clearIdleSocketValidation(this)
772+
754773
if (parser) {
755774
if (!this[kError] && parser.statusCode && !parser.shouldKeepAlive) {
756775
this[kError] = parser.finish() || this[kError]
@@ -816,7 +835,7 @@ async function connectH1 (client, socket) {
816835
return socket.destroyed
817836
},
818837
busy (request) {
819-
if (socket[kWriting] || socket[kReset] || socket[kBlocking]) {
838+
if (socket[kWriting] || socket[kReset] || socket[kBlocking] || socket[kIdleSocketValidation] === 1) {
820839
return true
821840
}
822841

@@ -854,6 +873,31 @@ async function connectH1 (client, socket) {
854873
}
855874
}
856875

876+
function clearIdleSocketValidation (socket) {
877+
if (socket[kIdleSocketValidationTimeout]) {
878+
clearTimeout(socket[kIdleSocketValidationTimeout])
879+
socket[kIdleSocketValidationTimeout] = null
880+
}
881+
882+
socket[kIdleSocketValidation] = 0
883+
}
884+
885+
function scheduleIdleSocketValidation (client, socket) {
886+
socket[kIdleSocketValidation] = 1
887+
socket[kIdleSocketValidationTimeout] = setTimeout(() => {
888+
socket[kIdleSocketValidationTimeout] = null
889+
socket[kIdleSocketValidation] = 2
890+
891+
if (client[kSocket] === socket && !socket.destroyed) {
892+
client[kResume]()
893+
}
894+
}, 0)
895+
socket[kIdleSocketValidationTimeout].unref?.()
896+
}
897+
898+
/**
899+
* @param {import('./client.js')} client
900+
*/
857901
function resumeH1 (client) {
858902
const socket = client[kSocket]
859903

@@ -868,6 +912,32 @@ function resumeH1 (client) {
868912
socket[kNoRef] = false
869913
}
870914

915+
if (client[kRunning] === 0 && client[kPending] > 0 && socket[kSocketUsed]) {
916+
if (socket[kIdleSocketValidation] === 0) {
917+
scheduleIdleSocketValidation(client, socket)
918+
socket[kParser].readMore()
919+
if (socket.destroyed) {
920+
return
921+
}
922+
return
923+
}
924+
925+
if (socket[kIdleSocketValidation] === 1) {
926+
socket[kParser].readMore()
927+
if (socket.destroyed) {
928+
return
929+
}
930+
return
931+
}
932+
}
933+
934+
if (client[kRunning] === 0) {
935+
socket[kParser].readMore()
936+
if (socket.destroyed) {
937+
return
938+
}
939+
}
940+
871941
if (client[kSize] === 0) {
872942
if (socket[kParser].timeoutType !== TIMEOUT_KEEP_ALIVE) {
873943
socket[kParser].setTimeout(client[kKeepAliveTimeoutValue], TIMEOUT_KEEP_ALIVE)
@@ -961,6 +1031,7 @@ function writeH1 (client, request) {
9611031
}
9621032

9631033
const socket = client[kSocket]
1034+
clearIdleSocketValidation(socket)
9641035

9651036
const abort = (err) => {
9661037
if (request.aborted || request.completed) {

‎test/client-errors.js‎

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,46 @@
11
'use strict'
22

3-
const { tspl } = require('@matteo.collina/tspl')
4-
const { test, after } = require('node:test')
5-
const { Client } = require('..')
3+
const assert = require('node:assert')
4+
const { test } = require('node:test')
5+
const { once } = require('node:events')
6+
const { Client, errors } = require('..')
67
const net = require('node:net')
78

8-
// TODO: move to test/node-test/client-connect.js
9-
test('parser error', async (t) => {
10-
t = tspl(t, { plan: 2 })
11-
12-
const server = net.createServer()
13-
server.once('connection', (socket) => {
14-
socket.write('asd\n\r213123')
9+
function closeServer (server) {
10+
return new Promise((resolve, reject) => {
11+
server.close((err) => err ? reject(err) : resolve())
1512
})
16-
after(() => server.close())
17-
18-
server.listen(0, () => {
19-
const client = new Client(`http://localhost:${server.address().port}`)
20-
after(() => client.destroy())
13+
}
2114

15+
function request (client) {
16+
return new Promise((resolve, reject) => {
2217
client.request({ path: '/', method: 'GET' }, (err) => {
23-
t.ok(err)
24-
client.close((err) => {
25-
t.ifError(err)
26-
})
18+
if (err) {
19+
reject(err)
20+
} else {
21+
resolve()
22+
}
2723
})
2824
})
25+
}
26+
27+
// TODO: move to test/node-test/client-connect.js
28+
test('parser error', async () => {
29+
const server = net.createServer((socket) => {
30+
socket.once('data', () => {
31+
socket.end('asd\n\r213123')
32+
})
33+
})
34+
35+
server.listen(0)
36+
await once(server, 'listening')
37+
38+
const client = new Client(`http://localhost:${server.address().port}`)
2939

30-
await t.completed
40+
try {
41+
await assert.rejects(request(client), errors.HTTPParserError)
42+
} finally {
43+
await client.destroy()
44+
await closeServer(server)
45+
}
3146
})

‎test/fetch/cookies.js‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,17 +15,18 @@ test('Can receive set-cookie headers from a server using fetch - issue #1262', a
1515
const server = createServer((req, res) => {
1616
res.setHeader('set-cookie', 'name=value; Domain=example.com')
1717
res.end()
18-
}).listen(0)
18+
}).listen(0, '127.0.0.1')
1919

2020
t.after(closeServerAsPromise(server))
2121
await once(server, 'listening')
2222

23-
const response = await fetch(`http://localhost:${server.address().port}`)
23+
const response = await fetch(`http://127.0.0.1:${server.address().port}`, { keepalive: false })
2424

2525
assert.strictEqual(response.headers.get('set-cookie'), 'name=value; Domain=example.com')
2626

27-
const response2 = await fetch(`http://localhost:${server.address().port}`, {
28-
credentials: 'include'
27+
const response2 = await fetch(`http://127.0.0.1:${server.address().port}`, {
28+
credentials: 'include',
29+
keepalive: false
2930
})
3031

3132
assert.strictEqual(response2.headers.get('set-cookie'), 'name=value; Domain=example.com')
@@ -35,7 +36,7 @@ test('Can send cookies to a server with fetch - issue #1463', async (t) => {
3536
const server = createServer((req, res) => {
3637
assert.strictEqual(req.headers.cookie, 'value')
3738
res.end()
38-
}).listen(0)
39+
}).listen(0, '127.0.0.1')
3940

4041
t.after(closeServerAsPromise(server))
4142
await once(server, 'listening')
@@ -47,7 +48,7 @@ test('Can send cookies to a server with fetch - issue #1463', async (t) => {
4748
]
4849

4950
for (const headers of headersInit) {
50-
await fetch(`http://localhost:${server.address().port}`, { headers })
51+
await fetch(`http://127.0.0.1:${server.address().port}`, { headers, keepalive: false })
5152
}
5253
})
5354

@@ -57,12 +58,13 @@ test('Cookie header is delimited with a semicolon rather than a comma - issue #1
5758
const server = createServer((req, res) => {
5859
strictEqual(req.headers.cookie, 'FOO=lorem-ipsum-dolor-sit-amet; BAR=the-quick-brown-fox')
5960
res.end()
60-
}).listen(0)
61+
}).listen(0, '127.0.0.1')
6162

6263
t.after(closeServerAsPromise(server))
6364
await once(server, 'listening')
6465

65-
await fetch(`http://localhost:${server.address().port}`, {
66+
await fetch(`http://127.0.0.1:${server.address().port}`, {
67+
keepalive: false,
6668
headers: [
6769
['cookie', 'FOO=lorem-ipsum-dolor-sit-amet'],
6870
['cookie', 'BAR=the-quick-brown-fox']
@@ -83,18 +85,18 @@ test('Can receive set-cookie headers from a http2 server using fetch - issue #28
8385
stream.end('test')
8486
})
8587

86-
server.listen()
88+
server.listen(0, '127.0.0.1')
8789
await once(server, 'listening')
8890

89-
const client = new Client(`https://localhost:${server.address().port}`, {
91+
const client = new Client(`https://127.0.0.1:${server.address().port}`, {
9092
connect: {
9193
rejectUnauthorized: false
9294
},
9395
allowH2: true
9496
})
9597

9698
const response = await fetch(
97-
`https://localhost:${server.address().port}/`,
99+
`https://127.0.0.1:${server.address().port}/`,
98100
// Needs to be passed to disable the reject unauthorized
99101
{
100102
method: 'GET',

‎test/fetch/encoding.js‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,12 @@ test('content-encoding header is case-iNsENsITIve', async (t) => {
2323

2424
gzip.write(text)
2525
gzip.end()
26-
}).listen(0)
26+
}).listen(0, '127.0.0.1')
2727

2828
t.after(closeServerAsPromise(server))
2929
await once(server, 'listening')
3030

31-
const response = await fetch(`http://localhost:${server.address().port}`)
31+
const response = await fetch(`http://127.0.0.1:${server.address().port}`, { keepalive: false })
3232

3333
assert.strictEqual(await response.text(), text)
3434
assert.strictEqual(response.headers.get('content-encoding'), contentCodings)
@@ -49,12 +49,12 @@ test('response decompression according to content-encoding should be handled in
4949

5050
deflate.write(text)
5151
deflate.end()
52-
}).listen(0)
52+
}).listen(0, '127.0.0.1')
5353

5454
t.after(closeServerAsPromise(server))
5555
await once(server, 'listening')
5656

57-
const response = await fetch(`http://localhost:${server.address().port}`)
57+
const response = await fetch(`http://127.0.0.1:${server.address().port}`, { keepalive: false })
5858

5959
assert.strictEqual(await response.text(), text)
6060
})
@@ -78,15 +78,15 @@ describe('content-encoding chain limit', () => {
7878
})
7979
res.end('test')
8080
})
81-
await once(server.listen(0), 'listening')
81+
await once(server.listen(0, '127.0.0.1'), 'listening')
8282
})
8383

8484
after(() => {
8585
server.close()
8686
})
8787

8888
test(`should allow exactly ${MAX_CONTENT_ENCODINGS} content-encodings`, async (t) => {
89-
const response = await fetch(`http://localhost:${server.address().port}`, {
89+
const response = await fetch(`http://127.0.0.1:${server.address().port}`, {
9090
keepalive: false,
9191
headers: { 'x-encoding-count': String(MAX_CONTENT_ENCODINGS) }
9292
})
@@ -98,7 +98,7 @@ describe('content-encoding chain limit', () => {
9898

9999
test(`should reject more than ${MAX_CONTENT_ENCODINGS} content-encodings`, async (t) => {
100100
await assert.rejects(
101-
fetch(`http://localhost:${server.address().port}`, {
101+
fetch(`http://127.0.0.1:${server.address().port}`, {
102102
keepalive: false,
103103
headers: { 'x-encoding-count': String(MAX_CONTENT_ENCODINGS + 1) }
104104
}),
@@ -111,7 +111,7 @@ describe('content-encoding chain limit', () => {
111111

112112
test('should reject excessive content-encoding chains', async (t) => {
113113
await assert.rejects(
114-
fetch(`http://localhost:${server.address().port}`, {
114+
fetch(`http://127.0.0.1:${server.address().port}`, {
115115
keepalive: false,
116116
headers: { 'x-encoding-count': '100' }
117117
}),

0 commit comments

Comments
 (0)