Skip to content

Commit c5669fb

Browse files
XadillaXaduh95
XadillaX
authored and
aduh95
committed
http: preserve socket errors as response error causes
Keep the original socket error as the cause of ECONNRESET errors emitted when an HTTP response closes before completion. Preserve the existing aborted message, error code, and event order. Leave cause absent when there is no underlying error. Allow ConnResetException to accept Error options, document the behavior, and cover TLS record errors, TCP resets, explicit destruction, and premature closure without a socket error. This follows investigation of #66001 and addresses lost error context. The original TLS decryption failure remains unresolved. Refs: #66001 Signed-off-by: XadillaX <[email protected]> PR-URL: #66061 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Tim Perry <[email protected]>
1 parent 2c409d9 commit c5669fb

5 files changed

Lines changed: 181 additions & 4 deletions

File tree

‎doc/api/http.md‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4272,6 +4272,13 @@ the following events will be emitted in the following order:
42724272
`'Error: aborted'` and code `'ECONNRESET'`
42734273
* `'close'` on the `res` object
42744274
4275+
If a socket error (such as a TLS error) causes the premature close, that error
4276+
is emitted on the request before the close. The error emitted on the incomplete
4277+
response retains the message `'aborted'` and code `'ECONNRESET'`, with the original
4278+
socket error available as its `cause`. This also applies when the original socket
4279+
error has code `'ECONNRESET'`. If no underlying error is available, the response
4280+
error has no `cause` property.
4281+
42754282
If `req.destroy()` is called before a socket is assigned, the following
42764283
events will be emitted in the following order:
42774284
@@ -4299,7 +4306,8 @@ events will be emitted in the following order:
42994306
* `'aborted'` on the `res` object
43004307
* `'close'`
43014308
* `'error'` on the `res` object with an error with message `'Error: aborted'`
4302-
and code `'ECONNRESET'`, or the error with which `req.destroy()` was called
4309+
and code `'ECONNRESET'`. If an error was passed to `req.destroy()`, it is
4310+
available as the response error's `cause`.
43034311
* `'close'` on the `res` object
43044312
43054313
If `req.abort()` is called before a socket is assigned, the following

‎lib/_http_client.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -731,7 +731,8 @@ function socketCloseListener() {
731731
if (res) {
732732
// Socket closed before we emitted 'end' below.
733733
if (!res.complete) {
734-
res.destroy(new ConnResetException('aborted'));
734+
res.destroy(new ConnResetException('aborted',
735+
req[kError] ? { cause: req[kError] } : undefined));
735736
}
736737
req._closed = true;
737738
req.emit('close');
@@ -772,6 +773,9 @@ function socketErrorListener(err) {
772773
// and we need to make sure we don't double-fire the error event.
773774
socket._hadError = true;
774775
emitErrorEvent(req, err);
776+
// Preserve the socket error for an incomplete response, including TLS
777+
// errors that are emitted without setting socket.errored.
778+
req[kError] ||= err;
775779
}
776780

777781
const parser = socket.parser;

‎lib/internal/errors.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -832,8 +832,8 @@ class DNSException extends Error {
832832
}
833833

834834
class ConnResetException extends Error {
835-
constructor(msg) {
836-
super(msg);
835+
constructor(msg, options) {
836+
super(msg, options);
837837
this.code = 'ECONNRESET';
838838
}
839839

‎test/parallel/test-http-response-socket-error.js‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const http = require('http');
6+
const { Writable, pipeline } = require('stream');
7+
8+
for (const method of ['emit', 'emitAndDestroy', 'socketDestroy', 'requestDestroy', 'reset', 'close']) {
9+
let error = method === 'close' || method === 'reset' ? null : new Error('Socket failure');
10+
let serverSocket;
11+
const server = http.createServer(common.mustCall((req, res) => {
12+
serverSocket = req.socket;
13+
res.writeHead(200, { 'Content-Length': 100 });
14+
res.write('partial body');
15+
}));
16+
17+
server.listen(0, common.mustCall(() => {
18+
const req = http.get({
19+
port: server.address().port,
20+
agent: false,
21+
}, common.mustCall((res) => {
22+
res.on('aborted', common.mustCall());
23+
res.on('close', common.mustCall());
24+
pipeline(res, new Writable({
25+
write(chunk, encoding, callback) {
26+
callback();
27+
},
28+
}), common.mustCall((err) => {
29+
assert.strictEqual(res.complete, false);
30+
assert.strictEqual(res.aborted, true);
31+
assert.strictEqual(err.code, 'ECONNRESET');
32+
assert.strictEqual(err.message, 'aborted');
33+
assert.strictEqual(res.errored, err);
34+
if (error) {
35+
assert.notStrictEqual(err, error);
36+
assert.strictEqual(err.cause, error);
37+
assert.deepStrictEqual(Object.getOwnPropertyDescriptor(err, 'cause'), {
38+
value: error,
39+
writable: true,
40+
enumerable: false,
41+
configurable: true,
42+
});
43+
} else {
44+
assert.strictEqual(Object.hasOwn(err, 'cause'), false);
45+
}
46+
server.close(common.mustCall());
47+
}));
48+
49+
switch (method) {
50+
case 'emit':
51+
case 'emitAndDestroy':
52+
// TLSSocket can emit an error without setting socket.errored.
53+
req.socket.emit('error', error);
54+
break;
55+
case 'socketDestroy':
56+
req.socket.destroy(error);
57+
break;
58+
case 'requestDestroy':
59+
req.destroy(error);
60+
break;
61+
case 'reset':
62+
serverSocket.resetAndDestroy();
63+
break;
64+
case 'close':
65+
req.socket.destroy();
66+
break;
67+
}
68+
}));
69+
req.on('error', method !== 'close' ? common.mustCall((err) => {
70+
if (method === 'reset') {
71+
assert.strictEqual(err.code, 'ECONNRESET');
72+
assert.strictEqual(err.syscall, 'read');
73+
error = err;
74+
} else {
75+
assert.strictEqual(err, error);
76+
}
77+
if (method === 'emitAndDestroy')
78+
req.destroy();
79+
}) : common.mustNotCall());
80+
req.on('close', common.mustCall());
81+
}));
82+
}

‎test/parallel/test-https-response-tls-error.js‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const assert = require('assert');
8+
const https = require('https');
9+
const { Writable, pipeline } = require('stream');
10+
const fixtures = require('../common/fixtures');
11+
12+
// Verify error propagation after receiving response headers. The invalid
13+
// record deliberately causes a TLS error; it does not reproduce #66001's
14+
// failure to decrypt an intact stream under backpressure.
15+
for (const version of ['TLSv1.2', 'TLSv1.3']) {
16+
let transport;
17+
let requestError;
18+
const events = [];
19+
const server = https.createServer({
20+
key: fixtures.readKey('agent1-key.pem'),
21+
cert: fixtures.readKey('agent1-cert.pem'),
22+
minVersion: version,
23+
maxVersion: version,
24+
}, common.mustCall((req, res) => {
25+
res.writeHead(200, { 'Content-Length': 100 });
26+
res.write('partial body');
27+
}));
28+
server.on('connection', common.mustCall((socket) => {
29+
transport = socket;
30+
}));
31+
32+
server.listen(0, common.mustCall(() => {
33+
const req = https.get({
34+
port: server.address().port,
35+
rejectUnauthorized: false,
36+
agent: false,
37+
}, common.mustCall((res) => {
38+
assert.strictEqual(res.complete, false);
39+
res.on('aborted', common.mustCall(() => events.push('aborted')));
40+
res.on('error', common.mustCall((err) => {
41+
events.push('response error');
42+
assert.strictEqual(err.code, 'ECONNRESET');
43+
assert.strictEqual(err.message, 'aborted');
44+
assert.strictEqual(err.cause, requestError);
45+
}));
46+
res.on('close', common.mustCall(() => {
47+
events.push('response close');
48+
assert.deepStrictEqual(events, [
49+
'request error',
50+
'aborted',
51+
'request close',
52+
'response error',
53+
'response close',
54+
]);
55+
}));
56+
57+
pipeline(res, new Writable({
58+
write(chunk, encoding, callback) {
59+
callback();
60+
},
61+
}), common.mustCall((err) => {
62+
assert.strictEqual(err, res.errored);
63+
assert.strictEqual(err.cause, requestError);
64+
assert.strictEqual(res.complete, false);
65+
assert.strictEqual(res.aborted, true);
66+
server.close(common.mustCall());
67+
transport.destroy();
68+
}));
69+
70+
// Bypass the server's TLSSocket to send an invalid application-data
71+
// record after the client has received part of the HTTP response.
72+
const record = Buffer.alloc(37);
73+
record.set([23, 3, 3, 0, 32]);
74+
transport.write(record);
75+
}));
76+
req.on('error', common.mustCall((err) => {
77+
events.push('request error');
78+
assert.match(err.code, /^ERR_SSL_/);
79+
requestError = err;
80+
}));
81+
req.on('close', common.mustCall(() => events.push('request close')));
82+
}));
83+
}

0 commit comments

Comments
 (0)