Correctly parse JSON contentType in responseError interceptor (#3892)

Signed-off-by: Matteo Collina <hello@matteocollina.com>
nodejs · Nov 27, 2024 · c178ca1 · c178ca1
1 parent 513e213
commit c178ca1
Show file tree

Hide file tree

Showing 2 changed files with 92 additions and 2 deletions.
diff --git a/lib/interceptor/response-error.js b/lib/interceptor/response-error.js
@@ -27,6 +27,10 @@ class ResponseErrorHandler extends DecoratorHandler {
     return this.#handler.onConnect(abort)
   }
 
+  #checkContentType (contentType) {
+    return this.#contentType.indexOf(contentType) === 0
+  }
+
   onHeaders (statusCode, rawHeaders, resume, statusMessage, headers = parseHeaders(rawHeaders)) {
     this.#statusCode = statusCode
     this.#headers = headers
@@ -36,7 +40,7 @@ class ResponseErrorHandler extends DecoratorHandler {
       return this.#handler.onHeaders(statusCode, rawHeaders, resume, statusMessage, headers)
     }
 
-    if (this.#contentType === 'application/json' || this.#contentType === 'text/plain') {
+    if (this.#checkContentType('application/json') || this.#checkContentType('text/plain')) {
       this.#decoder = new TextDecoder('utf-8')
     }
   }
@@ -53,7 +57,7 @@ class ResponseErrorHandler extends DecoratorHandler {
     if (this.#statusCode >= 400) {
       this.#body += this.#decoder?.decode(undefined, { stream: false }) ?? ''
 
-      if (this.#contentType === 'application/json') {
+      if (this.#checkContentType('application/json')) {
         try {
           this.#body = JSON.parse(this.#body)
         } catch {

diff --git a/test/interceptors/response-error.js b/test/interceptors/response-error.js
@@ -82,3 +82,89 @@ test('should not throw error for ok response', async () => {
   assert.equal(response.statusCode, 200)
   assert.equal(await response.body.text(), 'hello')
 })
+
+test('should throw error for error response, parsing JSON', async () => {
+  const server = createServer()
+
+  server.on('request', (req, res) => {
+    res.writeHead(400, { 'content-type': 'application/json; charset=utf-8' })
+    res.end(JSON.stringify({ message: 'Bad Request' }))
+  })
+
+  server.listen(0)
+
+  await once(server, 'listening')
+
+  const client = new Client(
+    `http://localhost:${server.address().port}`
+  ).compose(responseError())
+
+  after(async () => {
+    await client.close()
+    server.close()
+
+    await once(server, 'close')
+  })
+
+  let error
+  try {
+    await client.request({
+      method: 'GET',
+      path: '/',
+      headers: {
+        'content-type': 'text/plain'
+      }
+    })
+  } catch (err) {
+    error = err
+  }
+
+  assert.equal(error.statusCode, 400)
+  assert.equal(error.message, 'Response Error')
+  assert.deepStrictEqual(error.body, {
+    message: 'Bad Request'
+  })
+})
+
+test('should throw error for error response, parsing JSON without charset', async () => {
+  const server = createServer()
+
+  server.on('request', (req, res) => {
+    res.writeHead(400, { 'content-type': 'application/json' })
+    res.end(JSON.stringify({ message: 'Bad Request' }))
+  })
+
+  server.listen(0)
+
+  await once(server, 'listening')
+
+  const client = new Client(
+    `http://localhost:${server.address().port}`
+  ).compose(responseError())
+
+  after(async () => {
+    await client.close()
+    server.close()
+
+    await once(server, 'close')
+  })
+
+  let error
+  try {
+    await client.request({
+      method: 'GET',
+      path: '/',
+      headers: {
+        'content-type': 'text/plain'
+      }
+    })
+  } catch (err) {
+    error = err
+  }
+
+  assert.equal(error.statusCode, 400)
+  assert.equal(error.message, 'Response Error')
+  assert.deepStrictEqual(error.body, {
+    message: 'Bad Request'
+  })
+})