From 588f219d2bfa8d97d7e2fe41f6e1a9d33e59e78e Mon Sep 17 00:00:00 2001
From: Branislav Katreniak <bkatreniak@slido.com>
Date: Tue, 12 Jul 2022 14:07:53 +0200
Subject: [PATCH] fix: default clientError replies on reused connection (#4101)

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.
---
 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 7a3829b91e..6f5f39cdc9 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')
+  })
+})