feat: add contentTypeNegotiation option #131

Phillip9587 · 2025-12-17T18:21:58Z

This is a backward-compatible version of #85.

It contains 2 commits:

feat: add contentTypeNegotiation: Adds a contentTypeNegotiation option that enables content type negotiation for error responses based on the Accept header. When enabled, responses will be formatted as either text/plain or text/html according to client preferences.
feat: add defaultContentType option to configure the default response: Adds a defaultContentType option to configure the default response type when content negotiation is disabled or no preferred type can be determined.

The current behaviour (alway returning text/html responses) is unchanged but the user can configure it if he wants to. I added 2 warning in the README which states that the options default will be changed in the next major.

socket-security · 2025-12-17T18:22:15Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	negotiator@1.0.0

View full report

Copilot

Pull request overview

This PR adds backward-compatible content type negotiation functionality to finalhandler, allowing error responses to be formatted as either text/plain or text/html based on client preferences. The current behavior (always returning text/html) is preserved as the default but can be configured via two new options.

Added contentTypeNegotiation option to enable/disable Accept header-based content negotiation
Added defaultContentType option to configure the fallback content type
Refactored HTML generation to return Buffers and support both HTML and plain text responses

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
index.js	Core implementation: adds negotiator dependency, implements content type negotiation logic, refactors response body generation to support both HTML and plain text formats
test/test.js	Comprehensive test coverage for both new options across various Accept header scenarios for 404 and error responses
package.json	Adds negotiator as a new dependency
README.md	Documents the two new options with warnings about future default changes
HISTORY.md	Records the new features in the unreleased section

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T18:30:54Z

test/test.js

+      it('should respond according to quality when contentTypeNegotiation is true', function (done) {
+        var server = createServer(null, { contentTypeNegotiation: true })
+        wrapper(request(server)
+          .get('/foo'))
+          .set('Accept', 'text/html, application/*;q=0.2, image/jpeg;q=0.8')
+          .expect('Content-Type', 'text/html; charset=utf-8')
+          .expect(404, /<html/, done)
+      })


There are two test cases with the identical name "should respond according to quality when contentTypeNegotiation is true" within the same describe block. The second test should have a more specific name that distinguishes it from the first, such as "should prefer text/html when it has higher quality" or "should respond with text/html when quality is higher".

Copilot · 2025-12-17T18:30:55Z

README.md

+responses will use `text/plain` or `text/html` based on the client's preferences. Defaults
+to `false`.


The documentation for contentTypeNegotiation should clarify the behavior when no Accept header is present. Based on the test at line 433, when content negotiation is enabled and there's no Accept header, the response defaults to 'text/plain' (the first item in AVAILABLE_MEDIA_TYPES). This should be explicitly documented to avoid confusion.

Suggested change

responses will use `text/plain` or `text/html` based on the client's preferences. Defaults

to `false`.

responses will use `text/plain` or `text/html` based on the client's preferences. When

enabled and the request does not include an `Accept` header, the response defaults to

`text/plain` (the first available media type). Defaults to `false`.

Copilot · 2025-12-17T18:30:55Z

index.js

    }

+    let preferredType
+    // If text/plain fallback is enabled, negotiate content type


The comment "If text/plain fallback is enabled" is misleading. The option is named contentTypeNegotiation, not "text/plain fallback", and it enables negotiation between both text/plain and text/html. Consider updating the comment to "If content type negotiation is enabled" for clarity.

Suggested change

// If text/plain fallback is enabled, negotiate content type

// If content type negotiation is enabled, negotiate content type

Copilot · 2025-12-17T18:30:55Z

index.js

+        contentType = HTML_CONTENT_TYPE
+        break
+      case 'text/plain':
+        // default to plain text


The comment "default to plain text" is misleading in this context. This case is reached when preferredType is 'text/plain' OR when defaultContentType is 'text/plain' (and no preference was determined). The comment should be removed or clarified to "text/plain response" to avoid confusion.

Suggested change

// default to plain text

// text/plain response

Copilot · 2025-12-17T18:30:55Z

test/test.js

+    describe('when no Accept header', function () {
+      it('should respond with plain text when contentTypeNegotiation is true', function (done) {
+        var server = createServer(null, { contentTypeNegotiation: true })
+        wrapper(request(server)
+          .get('/foo'))
+          .expect('Content-Type', 'text/plain; charset=utf-8')
+          .expect(404, 'Cannot GET /foo', done)
+      })
+
+      it('should respond with HTML when contentTypeNegotiation is false', function (done) {
+        var server = createServer(null, { contentTypeNegotiation: false })
+        wrapper(request(server)
+          .get('/foo'))
+          .expect('Content-Type', 'text/html; charset=utf-8')
+          .expect(404, /<html/, done)
+      })
+
+      it('should respond with HTML when contentTypeNegotiation is not set', function (done) {
+        var server = createServer()
+        wrapper(request(server)
+          .get('/foo'))
+          .expect('Content-Type', 'text/html; charset=utf-8')
+          .expect(404, /<html/, done)
+      })
+    })


There's no test coverage for the scenario where defaultContentType: 'text/plain' is set but contentTypeNegotiation is false (or not set). According to the implementation at line 154, this should result in plain text responses. Consider adding tests to verify that defaultContentType works independently of contentTypeNegotiation, such as a test that sets only defaultContentType: 'text/plain' and expects plain text responses.

Copilot · 2025-12-17T18:30:56Z

index.js

  // get error callback
  var onerror = opts.onerror

+  // fallback response content type negotiation enabled


The comment "fallback response content type negotiation enabled" is unclear. Consider revising to "content type negotiation enabled" for clarity, as "fallback" is ambiguous in this context.

Suggested change

// fallback response content type negotiation enabled

// response content type negotiation enabled

Copilot · 2025-12-17T18:30:56Z

index.js

+        // default to plain text
+        body = Buffer.from(msg, 'utf8')
+        contentType = TEXT_CONTENT_TYPE
+        break


The switch statement lacks a default case, which could result in body and contentType being undefined if an unexpected value is used. While the validation at line 91-93 should prevent this, adding a default case would make the code more defensive and prevent potential runtime errors if the validation logic changes in the future.

Suggested change

break

break

default:

// Defensive fallback: use the configured defaultContentType

if (defaultContentType === 'text/plain') {

body = Buffer.from(msg, 'utf8')

contentType = TEXT_CONTENT_TYPE

} else {

body = createHtmlBody(msg)

contentType = HTML_CONTENT_TYPE

}

break

Copilot · 2025-12-17T18:30:56Z

index.js

 /**
 * Create a minimal HTML document.
 *
 * @param {string} message
 * @private
 */


The JSDoc comment for this function says "Create a minimal HTML document", but the function has been renamed from createHtmlDocument to createHtmlBody and now returns a Buffer instead of a string. The documentation should be updated to clarify that it returns a Buffer containing the HTML body.

Copilot · 2025-12-17T18:30:56Z

test/test.js

+      it('should respond according to quality when contentTypeNegotiation is true', function (done) {
+        var server = createServer(createError('boom!'), { contentTypeNegotiation: true })
+        wrapper(request(server)
+          .get('/foo'))
+          .set('Accept', 'text/html, application/*;q=0.2, image/jpeg;q=0.8')
+          .expect('Content-Type', 'text/html; charset=utf-8')
+          .expect(500, /<html/, done)
+      })


There are two test cases with the identical name "should respond according to quality when contentTypeNegotiation is true" within the same describe block. The second test should have a more specific name that distinguishes it from the first, such as "should prefer text/html when it has higher quality" or "should respond with text/html when quality is higher".

Phillip9587 added 2 commits December 17, 2025 18:49

feat: add contentTypeNegotiation option

4418c67

feat: add defaultContentType option to configure the default response

22bd52c

Phillip9587 requested a review from wesleytodd December 17, 2025 18:22

Phillip9587 requested review from UlisesGascon, bjohansebas and Copilot December 17, 2025 18:22

Copilot started reviewing on behalf of Phillip9587 December 17, 2025 18:23 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat: add contentTypeNegotiation option #131

feat: add contentTypeNegotiation option #131

Uh oh!

Phillip9587 commented Dec 17, 2025

Uh oh!

socket-security bot commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		responses will use `text/plain` or `text/html` based on the client's preferences. Defaults
		to `false`.

-responses will use `text/plain` or `text/html` based on the client's preferences. Defaults
-to `false`.
+responses will use `text/plain` or `text/html` based on the client's preferences. When
+enabled and the request does not include an `Accept` header, the response defaults to
+`text/plain` (the first available media type). Defaults to `false`.

	// If text/plain fallback is enabled, negotiate content type
	// If content type negotiation is enabled, negotiate content type

	// fallback response content type negotiation enabled
	// response content type negotiation enabled

-        break
+        break
+      default:
+        // Defensive fallback: use the configured defaultContentType
+        if (defaultContentType === 'text/plain') {
+          body = Buffer.from(msg, 'utf8')
+          contentType = TEXT_CONTENT_TYPE
+        } else {
+          body = createHtmlBody(msg)
+          contentType = HTML_CONTENT_TYPE
+        }
+        break

Uh oh!

feat: add contentTypeNegotiation option #131

Are you sure you want to change the base?

feat: add contentTypeNegotiation option #131

Uh oh!

Conversation

Phillip9587 commented Dec 17, 2025

Uh oh!

socket-security bot commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant