From fe889eaeead123a841bc4dc017af7ad1d5dcc293 Mon Sep 17 00:00:00 2001
From: Branislav Katreniak <katreniak@gmail.com>
Date: Tue, 12 Jul 2022 18:07:42 +0200
Subject: [PATCH] fix: default clientError replies on reused connection (#4101)
 (#4133)

When fastify server receives request with invalid url
in a reused connection, it closes the connection
instead of 400 Bad Request reply.

The closed connection is then propagated by load balancer (ALB)
as 502 error. This turns client errors into closely monitored
server errors.

`socket.bytesWritten` is never going to be 0 on reused connection.

Co-authored-by: Branislav Katreniak <bkatreniak@slido.com>
---
 fastify.js                 |  2 +-
 test/request-error.test.js | 45 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/fastify.js b/fastify.js
index 476c2a51e8..7769e927a8 100644
--- a/fastify.js
+++ b/fastify.js
@@ -621,7 +621,7 @@ function fastify (options) {
     // https://github.com/nodejs/node/blob/6ca23d7846cb47e84fd344543e394e50938540be/lib/_http_server.js#L666
 
     // If the socket is not writable, there is no reason to try to send data.
-    if (socket.writable && socket.bytesWritten === 0) {
+    if (socket.writable) {
       socket.write(`HTTP/1.1 400 Bad Request\r\nContent-Length: ${body.length}\r\nContent-Type: application/json\r\n\r\n${body}`)
     }
     socket.destroy(err)
diff --git a/test/request-error.test.js b/test/request-error.test.js
index 15924c7b34..a3be6e1870 100644
--- a/test/request-error.test.js
+++ b/test/request-error.test.js
@@ -150,7 +150,7 @@ test('default clientError handler ignores sockets in destroyed state', t => {
 })
 
 test('default clientError handler destroys sockets in writable state', t => {
-  t.plan(1)
+  t.plan(2)
 
   const fastify = Fastify({
     bodyLimit: 1,
@@ -166,6 +166,9 @@ test('default clientError handler destroys sockets in writable state', t => {
     },
     destroy () {
       t.pass('destroy should be called')
+    },
+    write (response) {
+      t.match(response, /^HTTP\/1.1 400 Bad Request/)
     }
   })
 })
@@ -186,6 +189,9 @@ test('default clientError handler destroys http sockets in non-writable state',
     },
     destroy () {
       t.pass('destroy should be called')
+    },
+    write (response) {
+      t.fail('write should not be called')
     }
   })
 })
@@ -270,3 +276,40 @@ test('encapsulated error handler binding', t => {
     t.equal(fastify.hello, undefined)
   })
 })
+
+test('default clientError replies with bad request on reused keep-alive connection', t => {
+  t.plan(2)
+
+  let response = ''
+
+  const fastify = Fastify({
+    bodyLimit: 1,
+    keepAliveTimeout: 100
+  })
+
+  fastify.get('/', (request, reply) => {
+    reply.send('OK\n')
+  })
+
+  fastify.listen({ port: 0 }, function (err) {
+    t.error(err)
+    fastify.server.unref()
+
+    const client = connect(fastify.server.address().port)
+
+    client.on('data', chunk => {
+      response += chunk.toString('utf-8')
+    })
+
+    client.on('end', () => {
+      t.match(response, /^HTTP\/1.1 200 OK.*HTTP\/1.1 400 Bad Request/s)
+    })
+
+    client.resume()
+    client.write('GET / HTTP/1.1\r\n')
+    client.write('\r\n\r\n')
+    client.write('GET /?a b HTTP/1.1\r\n')
+    client.write('Connection: close\r\n')
+    client.write('\r\n\r\n')
+  })
+})