From 6721654127d59c173f25c6a9a163aa8c7c1a933e Mon Sep 17 00:00:00 2001 From: Gil Pedersen Date: Tue, 7 Nov 2023 12:56:32 +0100 Subject: [PATCH 1/3] Don't return error response when clientError is a pipelined HPE_INVALID_METHOD --- lib/core.js | 12 +++++++++ lib/request.js | 12 +++------ test/request.js | 69 +++++++++++++++++++++++++++++++++++++------------ 3 files changed, 68 insertions(+), 25 deletions(-) diff --git a/lib/core.js b/lib/core.js index 09feebfd7..9ed6f4d0b 100755 --- a/lib/core.js +++ b/lib/core.js @@ -557,12 +557,24 @@ exports = module.exports = internals.Core = class { if (socket.readable) { const request = this.settings.operations.cleanStop && this.actives.get(socket); if (request) { + + // If a request is available, it means that the connection and parsing has progressed far enough to have created the request. + + if (err.code === 'HPE_INVALID_METHOD') { + + // This parser error is for a pipelined request. Schedule destroy once current request is done. + + request.raw.res.once('close', () => socket.destroy(err)); + return; + } + const error = Boom.badRequest(); error.output.headers = { connection: 'close' }; request._reply(error); } else { socket.end(internals.badRequestResponse); + socket.destroy(err); } } else { diff --git a/lib/request.js b/lib/request.js index 6efaa1511..ffddd4aca 100755 --- a/lib/request.js +++ b/lib/request.js @@ -493,7 +493,7 @@ exports = module.exports = internals.Request = class { this._eventContext.request = null; // Disable req events - if (!Boom.isBoom(this.response)) { + if (this.response._close) { if (this.response.statusCode === 500 && this.response._error) { @@ -501,9 +501,7 @@ exports = module.exports = internals.Request = class { this._log(tags, this.response._error, 'error'); } - if (this.response._close) { - this.response._close(); - } + this.response._close(); } this.info.completed = Date.now(); @@ -522,13 +520,11 @@ exports = module.exports = internals.Request = class { this.response !== response && this.response.source !== response.source) { - this.response._close(); + this.response._close?.(); } if (this.info.completed) { - if (response._close) { - response._close(); - } + response._close?.(); return; } diff --git a/test/request.js b/test/request.js index 62cf6309c..8c5f1a961 100755 --- a/test/request.js +++ b/test/request.js @@ -2276,38 +2276,29 @@ describe('Request', () => { await server.stop({ timeout: 1 }); }); - it('returns a logged bad request error when parser fails after request is setup', async () => { + it('returns normal response when parser fails with bad method after request is setup', async () => { const server = Hapi.server({ routes: { timeout: { server: false } } }); - server.route({ path: '/', method: 'GET', handler: Hoek.block }); + server.route({ path: '/', method: 'GET', handler: () => 'ok' }); await server.start(); const log = server.events.once('response'); const client = Net.connect(server.info.port); - const clientEnded = new Promise((resolve, reject) => { - - let response = ''; - client.on('data', (chunk) => { - - response = response + chunk.toString(); - }); - - client.on('end', () => resolve(response)); - client.on('error', reject); - }); + const clientEnded = Wreck.read(client); await new Promise((resolve) => client.on('connect', resolve)); client.write('GET / HTTP/1.1\r\nHost: test\r\nContent-Length: 0\r\n\r\n\r\ninvalid data'); const [request] = await log; - expect(request.response.statusCode).to.equal(400); - const clientResponse = await clientEnded; - expect(clientResponse).to.contain('400 Bad Request'); + expect(request.response.statusCode).to.equal(200); + expect(request.response.source).to.equal('ok'); + const clientResponse = (await clientEnded).toString(); + expect(clientResponse).to.contain('HTTP/1.1 200 OK'); await server.stop({ timeout: 1 }); }); - it('returns a logged bad request error when parser fails after request is setup (cleanStop false)', async () => { + it('returns a bad request when parser fails after request is setup (cleanStop false)', async () => { const server = Hapi.server({ routes: { timeout: { server: false } }, operations: { cleanStop: false } }); server.route({ path: '/', method: 'GET', handler: Hoek.block }); @@ -2335,6 +2326,50 @@ describe('Request', () => { await server.stop({ timeout: 1 }); }); + it('returns a bad request for POST request when chunked parsing fails', async () => { + + const server = Hapi.server({ routes: { timeout: { server: false } } }); + server.route({ path: '/', method: 'POST', handler: () => 'ok', options: { payload: { parse: true } } }); + await server.start(); + + const log = server.events.once('response'); + const client = Net.connect(server.info.port); + const clientEnded = Wreck.read(client); + + await new Promise((resolve) => client.on('connect', resolve)); + client.write('POST / HTTP/1.1\r\nHost: test\r\nTransfer-Encoding: chunked\r\n\r\n'); + await Hoek.wait(10); + client.write('not chunked\r\n'); + + const [request] = await log; + expect(request.response.statusCode).to.equal(400); + expect(request.response.source).to.contain({ error: 'Bad Request' }); + const clientResponse = (await clientEnded).toString(); + expect(clientResponse).to.contain('400 Bad Request'); + + await server.stop({ timeout: 1 }); + }); + + it('returns a bad request for POST request when chunked parsing fails (cleanStop false)', async () => { + + const server = Hapi.server({ routes: { timeout: { server: false } }, operations: { cleanStop: false } }); + server.route({ path: '/', method: 'POST', handler: () => 'ok', options: { payload: { parse: true } } }); + await server.start(); + + const client = Net.connect(server.info.port); + const clientEnded = Wreck.read(client); + + await new Promise((resolve) => client.on('connect', resolve)); + client.write('POST / HTTP/1.1\r\nHost: test\r\nTransfer-Encoding: chunked\r\n\r\n'); + await Hoek.wait(10); + client.write('not chunked\r\n'); + + const clientResponse = (await clientEnded).toString(); + expect(clientResponse).to.contain('400 Bad Request'); + + await server.stop({ timeout: 1 }); + }); + it('does not return an error when server is responding when the timeout occurs', async () => { let ended = false; From 0bce91fc45eece57d7680fb310fdeae2cf49b81b Mon Sep 17 00:00:00 2001 From: Gil Pedersen Date: Tue, 7 Nov 2023 13:42:18 +0100 Subject: [PATCH 2/3] Actually respond with code 400 to pipelined request --- lib/core.js | 9 ++++++- test/request.js | 63 +++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/lib/core.js b/lib/core.js index 9ed6f4d0b..70564ee25 100755 --- a/lib/core.js +++ b/lib/core.js @@ -564,7 +564,14 @@ exports = module.exports = internals.Core = class { // This parser error is for a pipelined request. Schedule destroy once current request is done. - request.raw.res.once('close', () => socket.destroy(err)); + request.raw.res.once('close', () => { + + if (socket.readable) { + socket.end(internals.badRequestResponse); + } + + socket.destroy(err); + }); return; } diff --git a/test/request.js b/test/request.js index 8c5f1a961..fa7b034e9 100755 --- a/test/request.js +++ b/test/request.js @@ -2279,7 +2279,7 @@ describe('Request', () => { it('returns normal response when parser fails with bad method after request is setup', async () => { const server = Hapi.server({ routes: { timeout: { server: false } } }); - server.route({ path: '/', method: 'GET', handler: () => 'ok' }); + server.route({ path: '/', method: 'GET', handler: () => 'PAYLOAD' }); await server.start(); const log = server.events.once('response'); @@ -2287,14 +2287,43 @@ describe('Request', () => { const clientEnded = Wreck.read(client); await new Promise((resolve) => client.on('connect', resolve)); - client.write('GET / HTTP/1.1\r\nHost: test\r\nContent-Length: 0\r\n\r\n\r\ninvalid data'); + client.write('GET / HTTP/1.1\r\nHost: test\r\nContent-Length: 0\r\n\r\ninvalid data'); const [request] = await log; expect(request.response.statusCode).to.equal(200); - expect(request.response.source).to.equal('ok'); + expect(request.response.source).to.equal('PAYLOAD'); const clientResponse = (await clientEnded).toString(); expect(clientResponse).to.contain('HTTP/1.1 200 OK'); + const nextResponse = clientResponse.slice(clientResponse.indexOf('PAYLOAD') + 7); + expect(nextResponse).to.startWith('HTTP/1.1 400 Bad Request'); + + await server.stop({ timeout: 1 }); + }); + + it('returns nothing when parser fails with bad method after request is setup and the connection is closed', async () => { + + const server = Hapi.server({ routes: { timeout: { server: false } } }); + server.route({ path: '/', method: 'GET', handler: (request, h) => { + + request.raw.res.destroy(); + return h.abandon; + } }); + + await server.start(); + + const log = server.events.once('response'); + const client = Net.connect(server.info.port); + const clientEnded = Wreck.read(client); + + await new Promise((resolve) => client.on('connect', resolve)); + client.write('GET / HTTP/1.1\r\nHost: test\r\nContent-Length: 0\r\n\r\n\r\ninvalid data'); + + const [request] = await log; + expect(request.response.statusCode).to.be.undefined(); + const clientResponse = (await clientEnded).toString(); + expect(clientResponse).to.equal(''); + await server.stop({ timeout: 1 }); }); @@ -2318,7 +2347,7 @@ describe('Request', () => { }); await new Promise((resolve) => client.on('connect', resolve)); - client.write('GET / HTTP/1.1\r\nHost: test\nContent-Length: 0\r\n\r\n\r\ninvalid data'); + client.write('GET / HTTP/1.1\r\nHost: test\nContent-Length: 0\r\n\r\ninvalid data'); const clientResponse = await clientEnded; expect(clientResponse).to.contain('400 Bad Request'); @@ -2370,6 +2399,32 @@ describe('Request', () => { await server.stop({ timeout: 1 }); }); + it('returns a bad request for POST request when chunked parsing fails', async () => { + + const server = Hapi.server({ routes: { timeout: { server: false } } }); + server.route({ path: '/', method: 'POST', handler: () => 'ok', options: { payload: { parse: true } } }); + await server.start(); + + const log = server.events.once('response'); + const client = Net.connect(server.info.port); + const clientEnded = Wreck.read(client); + + await new Promise((resolve) => client.on('connect', resolve)); + client.write('POST / HTTP/1.1\r\nHost: test\r\nContent-Length: 5\r\n\r\n'); + await Hoek.wait(10); + client.write('111A1'); // Doesn't work if 'A' is replaced with '1' !?! + client.write('\Q\r\n'); // Extra bytes considered to be start of next request + client.end(); + + const [request] = await log; + expect(request.response.statusCode).to.equal(400); + expect(request.response.source).to.contain({ error: 'Bad Request' }); + const clientResponse = (await clientEnded).toString(); + expect(clientResponse).to.contain('400 Bad Request'); + + await server.stop({ timeout: 1 }); + }); + it('does not return an error when server is responding when the timeout occurs', async () => { let ended = false; From 74cfa9a9c5dd9b0c78eec1f2b82ec1748f9c5195 Mon Sep 17 00:00:00 2001 From: Gil Pedersen Date: Mon, 29 Jan 2024 22:55:43 +0100 Subject: [PATCH 3/3] Don't destroy() after end() --- lib/core.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/core.js b/lib/core.js index 70564ee25..43329536f 100755 --- a/lib/core.js +++ b/lib/core.js @@ -569,8 +569,9 @@ exports = module.exports = internals.Core = class { if (socket.readable) { socket.end(internals.badRequestResponse); } - - socket.destroy(err); + else { + socket.destroy(err); + } }); return; } @@ -581,7 +582,6 @@ exports = module.exports = internals.Core = class { } else { socket.end(internals.badRequestResponse); - socket.destroy(err); } } else {