From 3b83a61266910ea3ec65b255746ea44dcf8588e7 Mon Sep 17 00:00:00 2001 From: Alexander Akait <4567934+alexander-akait@users.noreply.github.com> Date: Sat, 31 Aug 2024 21:40:49 +0300 Subject: [PATCH] fix: ignore status message for HTTP/2 (#53) --- HISTORY.md | 5 + index.js | 5 +- test/support/utils.js | 94 ++++++++++++- test/test.js | 317 +++++++++++++++++++++++++++--------------- 4 files changed, 307 insertions(+), 114 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 702d68d..f80b5cd 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,3 +1,8 @@ +unreleased +================== + + * ignore status message for HTTP/2 (#53) + v1.2.1 / 2024-09-02 ================== diff --git a/index.js b/index.js index ea16fd2..ec34be9 100644 --- a/index.js +++ b/index.js @@ -278,7 +278,10 @@ function send (req, res, status, headers, message) { // response status res.statusCode = status - res.statusMessage = statuses.message[status] + + if (req.httpVersionMajor < 2) { + res.statusMessage = statuses.message[status] + } // remove any content headers res.removeHeader('Content-Encoding') diff --git a/test/support/utils.js b/test/support/utils.js index 72c1fbe..1ff3ff9 100644 --- a/test/support/utils.js +++ b/test/support/utils.js @@ -1,14 +1,25 @@ var assert = require('assert') var finalhandler = require('../..') var http = require('http') + +var http2 + +try { + http2 = require('http2') +} catch (_err) { + // Nothing +} + var request = require('supertest') var SlowWriteStream = require('./sws') exports.assert = assert exports.createError = createError -exports.createServer = createServer +exports.createHTTPServer = createHTTPServer +exports.createHTTP2Server = createHTTP2Server exports.createSlowWriteStream = createSlowWriteStream exports.rawrequest = rawrequest +exports.rawrequestHTTP2 = rawrequestHTTP2 exports.request = request exports.shouldHaveStatusMessage = shouldHaveStatusMessage exports.shouldNotHaveBody = shouldNotHaveBody @@ -26,7 +37,7 @@ function createError (message, props) { return err } -function createServer (err, opts) { +function createHTTPServer (err, opts) { return http.createServer(function (req, res) { var done = finalhandler(req, res, opts) @@ -39,6 +50,19 @@ function createServer (err, opts) { }) } +function createHTTP2Server (err, opts) { + return http2.createServer(function (req, res) { + var done = finalhandler(req, res, opts) + + if (typeof err === 'function') { + err(req, res, done) + return + } + + done(err) + }) +} + function createSlowWriteStream () { return new SlowWriteStream() } @@ -107,6 +131,72 @@ function rawrequest (server) { } } +function rawrequestHTTP2 (server) { + var _headers = {} + var _path + + function expect (status, body, callback) { + if (arguments.length === 2) { + _headers[status.toLowerCase()] = body + return this + } + + server.listen(function onlisten () { + var buf = '' + var resHeaders + var addr = this.address() + var port = addr.port + + var client = http2.connect('http://127.0.0.1:' + port) + var req = client.request({ + ':method': 'GET', + ':path': _path.replace(/http:\/\/localhost/, '') + }) + req.on('error', callback) + req.on('response', function onresponse (responseHeaders) { + resHeaders = responseHeaders + }) + req.on('data', function ondata (s) { buf += s }) + req.on('end', function onend () { + var err = null + + try { + for (var key in _headers) { + assert.strictEqual(resHeaders[key], _headers[key]) + } + + assert.strictEqual(resHeaders[':status'], status) + + if (body instanceof RegExp) { + assert.ok(body.test(buf), 'expected body ' + buf + ' to match ' + body) + } else { + assert.strictEqual(buf, body, 'expected ' + body + ' response body, got ' + buf) + } + } catch (e) { + err = e + } + + req.close() + client.close() + server.close() + callback(err) + }) + }) + } + + function get (path) { + _path = path + + return { + expect: expect + } + } + + return { + get: get + } +} + function shouldHaveStatusMessage (statusMessage) { return function (test) { assert.strictEqual(test.res.statusMessage, statusMessage, 'should have statusMessage "' + statusMessage + '"') diff --git a/test/test.js b/test/test.js index 8e78aa5..3fb931f 100644 --- a/test/test.js +++ b/test/test.js @@ -2,180 +2,200 @@ var Buffer = require('safe-buffer').Buffer var finalhandler = require('..') var http = require('http') + +var http2 + +try { + http2 = require('http2') +} catch (_err) { + // Nothing +} + var utils = require('./support/utils') var assert = utils.assert var createError = utils.createError -var createServer = utils.createServer +var createHTTPServer = utils.createHTTPServer +var createHTTP2Server = utils.createHTTP2Server var createSlowWriteStream = utils.createSlowWriteStream var rawrequest = utils.rawrequest +var rawrequestHTTP2 = utils.rawrequestHTTP2 var request = utils.request var shouldHaveStatusMessage = utils.shouldHaveStatusMessage var shouldNotHaveBody = utils.shouldNotHaveBody var shouldNotHaveHeader = utils.shouldNotHaveHeader -var describeStatusMessage = !/statusMessage/.test(http.IncomingMessage.toString()) - ? describe.skip - : describe +var topDescribe = function (type, createServer) { + var wrapper = function wrapper (req) { + if (type === 'http2') { + return req.http2() + } + + return req + } -describe('finalhandler(req, res)', function () { describe('headers', function () { it('should ignore err.headers without status code', function (done) { - request(createServer(createError('oops!', { + wrapper(request(createServer(createError('oops!', { headers: { 'X-Custom-Header': 'foo' } }))) - .get('/') + .get('/')) .expect(shouldNotHaveHeader('X-Custom-Header')) .expect(500, done) }) it('should ignore err.headers with invalid res.status', function (done) { - request(createServer(createError('oops!', { + wrapper(request(createServer(createError('oops!', { headers: { 'X-Custom-Header': 'foo' }, status: 601 }))) - .get('/') + .get('/')) .expect(shouldNotHaveHeader('X-Custom-Header')) .expect(500, done) }) it('should ignore err.headers with invalid res.statusCode', function (done) { - request(createServer(createError('oops!', { + wrapper(request(createServer(createError('oops!', { headers: { 'X-Custom-Header': 'foo' }, statusCode: 601 }))) - .get('/') + .get('/')) .expect(shouldNotHaveHeader('X-Custom-Header')) .expect(500, done) }) it('should include err.headers with err.status', function (done) { - request(createServer(createError('oops!', { + wrapper(request(createServer(createError('oops!', { headers: { 'X-Custom-Header': 'foo=500', 'X-Custom-Header2': 'bar' }, status: 500 }))) - .get('/') + .get('/')) .expect('X-Custom-Header', 'foo=500') .expect('X-Custom-Header2', 'bar') .expect(500, done) }) it('should include err.headers with err.statusCode', function (done) { - request(createServer(createError('too many requests', { + wrapper(request(createServer(createError('too many requests', { headers: { 'Retry-After': '5' }, statusCode: 429 }))) - .get('/') + .get('/')) .expect('Retry-After', '5') .expect(429, done) }) it('should ignore err.headers when not an object', function (done) { - request(createServer(createError('oops!', { + wrapper(request(createServer(createError('oops!', { headers: 'foobar', statusCode: 500 }))) - .get('/') + .get('/')) .expect(500, done) }) }) describe('status code', function () { it('should 404 on no error', function (done) { - request(createServer()) - .get('/') + wrapper(request(createServer()) + .get('/')) .expect(404, done) }) it('should 500 on error', function (done) { - request(createServer(createError())) - .get('/') + wrapper(request(createServer(createError())) + .get('/')) .expect(500, done) }) it('should use err.statusCode', function (done) { - request(createServer(createError('nope', { + wrapper(request(createServer(createError('nope', { statusCode: 400 }))) - .get('/') + .get('/')) .expect(400, done) }) it('should ignore non-error err.statusCode code', function (done) { - request(createServer(createError('created', { + wrapper(request(createServer(createError('created', { statusCode: 201 }))) - .get('/') + .get('/')) .expect(500, done) }) it('should ignore non-numeric err.statusCode', function (done) { - request(createServer(createError('oops', { + wrapper(request(createServer(createError('oops', { statusCode: 'oh no' }))) - .get('/') + .get('/')) .expect(500, done) }) it('should use err.status', function (done) { - request(createServer(createError('nope', { + wrapper(request(createServer(createError('nope', { status: 400 }))) - .get('/') + .get('/')) .expect(400, done) }) it('should use err.status over err.statusCode', function (done) { - request(createServer(createError('nope', { + wrapper(request(createServer(createError('nope', { status: 400, statusCode: 401 }))) - .get('/') + .get('/')) .expect(400, done) }) it('should set status to 500 when err.status < 400', function (done) { - request(createServer(createError('oops', { + wrapper(request(createServer(createError('oops', { status: 202 }))) - .get('/') + .get('/')) .expect(500, done) }) it('should set status to 500 when err.status > 599', function (done) { - request(createServer(createError('oops', { + wrapper(request(createServer(createError('oops', { status: 601 }))) - .get('/') + .get('/')) .expect(500, done) }) it('should use err.statusCode over invalid err.status', function (done) { - request(createServer(createError('nope', { + wrapper(request(createServer(createError('nope', { status: 50, statusCode: 410 }))) - .get('/') + .get('/')) .expect(410, done) }) it('should ignore non-error err.status code', function (done) { - request(createServer(createError('created', { + wrapper(request(createServer(createError('created', { status: 201 }))) - .get('/') + .get('/')) .expect(500, done) }) it('should ignore non-numeric err.status', function (done) { - request(createServer(createError('oops', { + wrapper(request(createServer(createError('oops', { status: 'oh no' }))) - .get('/') + .get('/')) .expect(500, done) }) }) + // http2 does not support status message + var describeStatusMessage = !/statusMessage/.test(http.IncomingMessage.toString()) || type === 'http2' + ? describe.skip + : describe + describeStatusMessage('status message', function () { it('should be "Not Found" on no error', function (done) { request(createServer()) @@ -215,13 +235,13 @@ describe('finalhandler(req, res)', function () { describe('404 response', function () { it('should include method and pathname', function (done) { - request(createServer()) - .get('/foo') + wrapper(request(createServer()) + .get('/foo')) .expect(404, /
Cannot GET \/foo<\/pre>/, done)
     })
 
     it('should escape method and pathname characters', function (done) {
-      rawrequest(createServer())
+      (type === 'http2' ? rawrequestHTTP2 : rawrequest)(createServer())
         .get('/')
         .expect(404, /
Cannot GET \/%3Cla'me%3E<\/pre>/, done)
     })
@@ -232,8 +252,8 @@ describe('finalhandler(req, res)', function () {
         next()
       })
 
-      request(server)
-        .get('/foo')
+      wrapper(request(server)
+        .get('/foo'))
         .expect(404, /
Cannot GET resource<\/pre>/, done)
     })
 
@@ -245,35 +265,35 @@ describe('finalhandler(req, res)', function () {
         next()
       })
 
-      request(server)
-        .get('/foo/bar')
+      wrapper(request(server)
+        .get('/foo/bar'))
         .expect(404, /
Cannot GET \/foo\/bar<\/pre>/, done)
     })
 
     it('should include pathname only', function (done) {
-      rawrequest(createServer())
+      (type === 'http2' ? rawrequestHTTP2 : rawrequest)(createServer())
         .get('http://localhost/foo?bar=1')
         .expect(404, /
Cannot GET \/foo<\/pre>/, done)
     })
 
     it('should handle HEAD', function (done) {
-      request(createServer())
-        .head('/foo')
+      wrapper(request(createServer())
+        .head('/foo'))
         .expect(404)
         .expect(shouldNotHaveBody())
         .end(done)
     })
 
     it('should include X-Content-Type-Options header', function (done) {
-      request(createServer())
-        .get('/foo')
+      wrapper(request(createServer())
+        .get('/foo'))
         .expect('X-Content-Type-Options', 'nosniff')
         .expect(404, done)
     })
 
     it('should include Content-Security-Policy header', function (done) {
-      request(createServer())
-        .get('/foo')
+      wrapper(request(createServer())
+        .get('/foo'))
         .expect('Content-Security-Policy', "default-src 'none'")
         .expect(404, done)
     })
@@ -281,7 +301,7 @@ describe('finalhandler(req, res)', function () {
     it('should not hang/error if there is a request body', function (done) {
       var buf = Buffer.alloc(1024 * 16, '.')
       var server = createServer()
-      var test = request(server).post('/foo')
+      var test = wrapper(request(server).post('/foo'))
       test.write(buf)
       test.write(buf)
       test.write(buf)
@@ -291,42 +311,42 @@ describe('finalhandler(req, res)', function () {
 
   describe('error response', function () {
     it('should include error stack', function (done) {
-      request(createServer(createError('boom!')))
-        .get('/foo')
+      wrapper(request(createServer(createError('boom!')))
+        .get('/foo'))
         .expect(500, /
Error: boom!
   at/, done) }) it('should handle HEAD', function (done) { - request(createServer(createError('boom!'))) - .head('/foo') + wrapper(request(createServer(createError('boom!'))) + .head('/foo')) .expect(500) .expect(shouldNotHaveBody()) .end(done) }) it('should include X-Content-Type-Options header', function (done) { - request(createServer(createError('boom!'))) - .get('/foo') + wrapper(request(createServer(createError('boom!'))) + .get('/foo')) .expect('X-Content-Type-Options', 'nosniff') .expect(500, done) }) it('should includeContent-Security-Policy header', function (done) { - request(createServer(createError('boom!'))) - .get('/foo') + wrapper(request(createServer(createError('boom!'))) + .get('/foo')) .expect('Content-Security-Policy', "default-src 'none'") .expect(500, done) }) it('should handle non-error-objects', function (done) { - request(createServer('lame string')) - .get('/foo') + wrapper(request(createServer('lame string')) + .get('/foo')) .expect(500, /
lame string<\/pre>/, done)
     })
 
     it('should handle null prototype objects', function (done) {
-      request(createServer(Object.create(null)))
-        .get('/foo')
+      wrapper(request(createServer(Object.create(null)))
+        .get('/foo'))
         .expect(500, /
Internal Server Error<\/pre>/, done)
     })
 
@@ -334,10 +354,10 @@ describe('finalhandler(req, res)', function () {
       var err = createError('boom!', {
         status: 501
       })
-      request(createServer(err, {
+      wrapper(request(createServer(err, {
         env: 'production'
       }))
-        .get('/foo')
+        .get('/foo'))
         .expect(501, /
Not Implemented<\/pre>/, done)
     })
 
@@ -345,7 +365,7 @@ describe('finalhandler(req, res)', function () {
       it('should not hang/error when unread', function (done) {
         var buf = Buffer.alloc(1024 * 16, '.')
         var server = createServer(new Error('boom!'))
-        var test = request(server).post('/foo')
+        var test = wrapper(request(server).post('/foo'))
         test.write(buf)
         test.write(buf)
         test.write(buf)
@@ -361,7 +381,7 @@ describe('finalhandler(req, res)', function () {
           })
         })
         var stream = createSlowWriteStream()
-        var test = request(server).post('/foo')
+        var test = wrapper(request(server).post('/foo'))
         test.write(buf)
         test.write(buf)
         test.write(buf)
@@ -377,7 +397,7 @@ describe('finalhandler(req, res)', function () {
           })
           req.resume()
         })
-        var test = request(server).post('/foo')
+        var test = wrapper(request(server).post('/foo'))
         test.write(buf)
         test.write(buf)
         test.write(buf)
@@ -387,31 +407,37 @@ describe('finalhandler(req, res)', function () {
 
     describe('when res.statusCode set', function () {
       it('should keep when >= 400', function (done) {
-        var server = http.createServer(function (req, res) {
+        var server = createServer(function (req, res) {
           var done = finalhandler(req, res)
           res.statusCode = 503
           done(new Error('oops'))
         })
 
-        request(server)
-          .get('/foo')
+        wrapper(request(server)
+          .get('/foo'))
           .expect(503, done)
       })
 
       it('should convert to 500 is not a number', function (done) {
-        var server = http.createServer(function (req, res) {
+        // http2 does not support non numeric status code
+        if (type === 'http2') {
+          done()
+          return
+        }
+
+        var server = createServer(function (req, res) {
           var done = finalhandler(req, res)
           res.statusCode = 'oh no'
           done(new Error('oops'))
         })
 
-        request(server)
-          .get('/foo')
+        wrapper(request(server)
+          .get('/foo'))
           .expect(500, done)
       })
 
       it('should override with err.status', function (done) {
-        var server = http.createServer(function (req, res) {
+        var server = createServer(function (req, res) {
           var done = finalhandler(req, res)
           var err = createError('oops', {
             status: 414,
@@ -420,8 +446,8 @@ describe('finalhandler(req, res)', function () {
           done(err)
         })
 
-        request(server)
-          .get('/foo')
+        wrapper(request(server)
+          .get('/foo'))
           .expect(414, done)
       })
 
@@ -429,24 +455,30 @@ describe('finalhandler(req, res)', function () {
         var err = createError('boom!', {
           status: 509
         })
-        request(createServer(err, {
+        wrapper(request(createServer(err, {
           env: 'production'
         }))
-          .get('/foo')
+          .get('/foo'))
           .expect(509, /
Bandwidth Limit Exceeded<\/pre>/, done)
       })
     })
 
     describe('when res.statusCode undefined', function () {
       it('should set to 500', function (done) {
-        var server = http.createServer(function (req, res) {
+        // http2 does not support non numeric status code
+        if (type === 'http2') {
+          done()
+          return
+        }
+
+        var server = createServer(function (req, res) {
           var done = finalhandler(req, res)
           res.statusCode = undefined
           done(new Error('oops'))
         })
 
-        request(server)
-          .get('/foo')
+        wrapper(request(server)
+          .get('/foo'))
           .expect(500, done)
       })
     })
@@ -454,29 +486,29 @@ describe('finalhandler(req, res)', function () {
 
   describe('headers set', function () {
     it('should persist set headers', function (done) {
-      var server = http.createServer(function (req, res) {
+      var server = createServer(function (req, res) {
         var done = finalhandler(req, res)
         res.setHeader('Server', 'foobar')
         done()
       })
 
-      request(server)
-        .get('/foo')
+      wrapper(request(server)
+        .get('/foo'))
         .expect(404)
         .expect('Server', 'foobar')
         .end(done)
     })
 
     it('should override content-type and length', function (done) {
-      var server = http.createServer(function (req, res) {
+      var server = createServer(function (req, res) {
         var done = finalhandler(req, res)
         res.setHeader('Content-Type', 'image/png')
         res.setHeader('Content-Length', '50')
         done()
       })
 
-      request(server)
-        .get('/foo')
+      wrapper(request(server)
+        .get('/foo'))
         .expect(404)
         .expect('Content-Type', 'text/html; charset=utf-8')
         .expect('Content-Length', '142')
@@ -484,7 +516,7 @@ describe('finalhandler(req, res)', function () {
     })
 
     it('should remove other content headers', function (done) {
-      var server = http.createServer(function (req, res) {
+      var server = createServer(function (req, res) {
         var done = finalhandler(req, res)
         res.setHeader('Content-Encoding', 'gzip')
         res.setHeader('Content-Language', 'jp')
@@ -492,8 +524,8 @@ describe('finalhandler(req, res)', function () {
         done()
       })
 
-      request(server)
-        .get('/foo')
+      wrapper(request(server)
+        .get('/foo'))
         .expect(404)
         .expect(shouldNotHaveHeader('Content-Encoding'))
         .expect(shouldNotHaveHeader('Content-Language'))
@@ -504,7 +536,7 @@ describe('finalhandler(req, res)', function () {
 
   describe('request started', function () {
     it('should not respond', function (done) {
-      var server = http.createServer(function (req, res) {
+      var server = createServer(function (req, res) {
         var done = finalhandler(req, res)
         res.statusCode = 301
         res.write('0')
@@ -514,13 +546,13 @@ describe('finalhandler(req, res)', function () {
         })
       })
 
-      request(server)
-        .get('/foo')
+      wrapper(request(server)
+        .get('/foo'))
         .expect(301, '01', done)
     })
 
     it('should terminate on error', function (done) {
-      var server = http.createServer(function (req, res) {
+      var server = createServer(function (req, res) {
         var done = finalhandler(req, res)
         res.statusCode = 301
         res.write('0')
@@ -533,8 +565,8 @@ describe('finalhandler(req, res)', function () {
         })
       })
 
-      request(server)
-        .get('/foo')
+      wrapper(request(server)
+        .get('/foo'))
         .on('request', function onrequest (test) {
           test.req.on('response', function onresponse (res) {
             if (res.listeners('error').length > 0) {
@@ -563,8 +595,8 @@ describe('finalhandler(req, res)', function () {
         error = e
       }
 
-      request(createServer(err, { onerror: log }))
-        .get('/')
+      wrapper(request(createServer(err, { onerror: log }))
+        .get('/'))
         .end(function () {
           assert.equal(error, err)
           done()
@@ -575,7 +607,7 @@ describe('finalhandler(req, res)', function () {
   if (parseInt(process.version.split('.')[0].replace(/^v/, ''), 10) > 11) {
     describe('req.socket', function () {
       it('should not throw when socket is null', function (done) {
-        request(createServer(function (req, res, next) {
+        wrapper(request(createServer(function (req, res, next) {
           res.statusCode = 200
           res.end('ok')
           process.nextTick(function () {
@@ -583,13 +615,76 @@ describe('finalhandler(req, res)', function () {
             next(new Error())
           })
         }))
-          .get('/')
-          .end(function () {
-            assert.strictEqual(this.res.statusCode, 200)
-            assert.strictEqual(this.res.text, 'ok')
-            done()
+          .get('/'))
+          .expect(200)
+          .end(function (err) {
+            done(err)
           })
       })
     })
   }
-})
+
+  describe('no deprecation warnings', function () {
+    it('should respond 404 on no error', function (done) {
+      var warned = false
+
+      process.once('warning', function (warning) {
+        if (/The http2 module is an experimental API/.test(warning)) return
+        assert.fail(warning)
+      })
+
+      wrapper(request(createServer())
+        .head('/foo'))
+        .expect(404)
+        .end(function (err) {
+          assert.strictEqual(warned, false)
+          done(err)
+        })
+    })
+
+    it('should respond 500 on error', function (done) {
+      var warned = false
+
+      process.once('warning', function (warning) {
+        if (/The http2 module is an experimental API/.test(warning)) return
+        assert.fail(warning)
+      })
+
+      var err = createError()
+
+      wrapper(request(createServer(function (req, res) {
+        var done = finalhandler(req, res)
+
+        if (typeof err === 'function') {
+          err(req, res, done)
+          return
+        }
+
+        done(err)
+      }))
+        .head('/foo'))
+        .expect(500)
+        .end(function (err, res) {
+          assert.strictEqual(warned, false)
+          done(err)
+        })
+    })
+  })
+}
+
+var servers = [
+  ['http', createHTTPServer]
+]
+
+var nodeVersion = process.versions.node.split('.').map(Number)
+
+// `superagent` only supports `http2` since Node.js@10
+if (http2 && nodeVersion[0] >= 10) {
+  servers.push(['http2', createHTTP2Server])
+}
+
+for (var i = 0; i < servers.length; i++) {
+  var tests = topDescribe.bind(undefined, servers[i][0], servers[i][1])
+
+  describe(servers[i][0], tests)
+}