test: Added test for RedirectHandler upgrade handling.

lib/api/api-connect.js

@@ -1,6 +1,6 @@
 'use strict'
 
-const { InvalidArgumentError, RequestAbortedError } = require('../core/errors')
+const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors')
 const { AsyncResource } = require('async_hooks')
 const util = require('../core/util')
 const { addSignal, removeSignal } = require('./abort-signal')
@@ -34,6 +34,10 @@ class ConnectHandler extends AsyncResource {
     this.abort = abort
   }
 
+  onHeaders () {
+    throw new SocketError('bad connect')
+  }
+
   onUpgrade (statusCode, headers, socket) {
     const { callback, opaque } = this
 

lib/api/api-upgrade.js

@@ -1,6 +1,6 @@
 'use strict'
 
-const { InvalidArgumentError, RequestAbortedError } = require('../core/errors')
+const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors')
 const { AsyncResource } = require('async_hooks')
 const util = require('../core/util')
 const { addSignal, removeSignal } = require('./abort-signal')
@@ -35,6 +35,10 @@ class UpgradeHandler extends AsyncResource {
     this.abort = abort
   }
 
+  onHeaders () {
+    throw new SocketError('bad upgrade')
+  }
+
   onUpgrade (statusCode, headers, socket) {
     const { callback, opaque } = this
 

lib/client.js

@@ -556,7 +556,7 @@ class Parser {
     assert(socket === client[kSocket])
     assert(!socket.isPaused())
     assert(socket._handle && socket._handle.reading)
-    assert(request.upgrade)
+    assert(request.upgrade || request.method === 'CONNECT')
 
     this.statusCode = null
     this.shouldKeepAlive = null
@@ -614,7 +614,7 @@ class Parser {
       return -1
     }
 
-    if (request.upgrade !== true && upgrade !== Boolean(request.upgrade)) {
+    if (request.upgrade && !request.upgrade) {
       util.destroy(socket, new SocketError('bad upgrade'))
       return -1
     }
@@ -649,7 +649,13 @@ class Parser {
     this.statusCode = statusCode
     this.shouldKeepAlive = shouldKeepAlive
 
-    if (request.upgrade) {
+    if (request.method === 'CONNECT' && statusCode >= 200 && statusCode < 300) {
+      assert(client[kRunning] === 1)
+      this.upgrade = true
+      return 2
+    }
+
+    if (upgrade) {
       assert(client[kRunning] === 1)
       this.upgrade = true
       return 2
@@ -1149,7 +1155,7 @@ function _resume (client, sync) {
       return
     }
 
-    if (client[kRunning] > 0 && request.upgrade) {
+    if (client[kRunning] > 0 && (request.upgrade || request.method === 'CONNECT')) {
       // Don't dispatch an upgrade until all preceeding requests have completed.
       // A misbehaving server might upgrade the connection before all pipelined
       // request has completed.
@@ -1272,7 +1278,7 @@ function write (client, request) {
     socket[kReset] = true
   }
 
-  if (upgrade) {
+  if (upgrade || method === 'CONNECT') {
     // On CONNECT or upgrade, block pipeline from dispatching further
     // requests on this connection.
 
@@ -1287,7 +1293,7 @@ function write (client, request) {
 
   let header
 
-  if (typeof upgrade === 'string') {
+  if (upgrade) {
     header = `${method} ${path} HTTP/1.1\r\nconnection: upgrade\r\nupgrade: ${upgrade}\r\n`
   } else if (client[kPipelining]) {
     header = `${method} ${path} HTTP/1.1\r\nconnection: keep-alive\r\n`

lib/core/request.js

@@ -64,7 +64,7 @@ class Request {
 
     this.aborted = false
 
-    this.upgrade = upgrade || method === 'CONNECT' || null
+    this.upgrade = upgrade || null
 
     this.path = path
 
@@ -101,7 +101,7 @@ class Request {
       throw new InvalidArgumentError('invalid onError method')
     }
 
-    if (this.upgrade) {
+    if (this.upgrade || this.method === 'CONNECT') {
       if (typeof handler.onUpgrade !== 'function') {
         throw new InvalidArgumentError('invalid onUpgrade method')
       }
@@ -124,29 +124,35 @@ class Request {
 
   onConnect (abort) {
     assert(!this.aborted)
+    assert(!this.completed)
+
     return this[kHandler].onConnect(abort)
   }
 
   onHeaders (statusCode, headers, resume) {
     assert(!this.aborted)
+    assert(!this.completed)
+
     return this[kHandler].onHeaders(statusCode, headers, resume)
   }
 
   onData (chunk) {
     assert(!this.aborted)
-    assert(!this.upgrade)
+    assert(!this.completed)
+
     return this[kHandler].onData(chunk)
   }
 
   onUpgrade (statusCode, headers, socket) {
     assert(!this.aborted)
-    assert(this.upgrade)
+    assert(!this.completed)
+
     return this[kHandler].onUpgrade(statusCode, headers, socket)
   }
 
   onComplete (trailers) {
     assert(!this.aborted)
-    assert(!this.upgrade)
+
     this.completed = true
     return this[kHandler].onComplete(trailers)
   }

test/client-connect.js

@@ -53,8 +53,8 @@ test('basic connect', (t) => {
   })
 })
 
-test('connect >=300 should not error', (t) => {
-  t.plan(1)
+test('connect >=300 should error', (t) => {
+  t.plan(2)
 
   const server = http.createServer((c) => {
     t.fail()
@@ -77,11 +77,14 @@ test('connect >=300 should not error', (t) => {
     const client = new Client(`http://localhost:${server.address().port}`)
     t.teardown(client.destroy.bind(client))
 
-    const { statusCode, socket } = await client.connect({
-      path: '/'
-    })
-    t.equal(statusCode, 300)
-    socket.destroy()
+    try {
+      await client.connect({
+        path: '/'
+      })
+    } catch (err) {
+      t.equal(err.code, 'UND_ERR_SOCKET')
+      t.equal(err.message, 'bad connect')
+    }
   })
 })
 

test/redirect-upgrade.js

@@ -0,0 +1,34 @@
+'use strict'
+
+const t = require('tap')
+const { upgrade } = require('..')
+const { startServer } = require('./utils/redirecting-servers')
+
+t.test('should upgrade the connection when no redirects are present', async t => {
+  t.plan(2)
+
+  const server = await startServer(t, (req, res) => {
+    if (req.url === '/') {
+      res.statusCode = 301
+      res.setHeader('Location', `http://${server}/end`)
+      res.end('REDIRECT')
+      return
+    }
+
+    res.statusCode = 101
+    res.setHeader('Connection', 'upgrade')
+    res.setHeader('Upgrade', req.headers.upgrade)
+    res.end('')
+  })
+
+  const { headers, socket } = await upgrade(`http://${server}/`, {
+    method: 'GET',
+    protocol: 'foo/1',
+    maxRedirections: 10
+  })
+
+  socket.end()
+
+  t.equal(headers.connection, 'upgrade')
+  t.equal(headers.upgrade, 'foo/1')
+})

test/utils/redirecting-servers.js

@@ -153,6 +153,7 @@ async function startRedirectingChainServers (t) {
 }
 
 module.exports = {
+  startServer,
   startRedirectingServer,
   startRedirectingWithBodyServer,
   startRedirectingWithoutLocationServer,