Don't return error response when clientError is a pipelined HPE_INVAL…

…ID_METHOD
bricss · Mar 13, 2024 · 5bd4bea · 5bd4bea
1 parent 4ca7288
commit 5bd4bea
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 25 deletions.
diff --git 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
@@ -493,17 +493,15 @@ 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) {
 
                 const tags = this.response._error.isDeveloperError ? ['internal', 'implementation', 'error'] : ['internal', 'error'];
                 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
@@ -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;