Use a net.Socket instead of a plain EventEmitter for replaying proxy errors (TooTallNate#83)

lpinca · TooTallNate · commit 4c0d37ef90a2 · 2019-10-16T18:00:17.000-07:00
* Run CI on pull requests * Use a `Duplex` instead of a plain `EventEmitter` Fixes: TooTallNate#81 * Use a new and closed `net.Socket` instead of a `Duplex`
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -1,6 +1,6 @@
 name: Node CI
 
-on: [push]
+on: [push, pull_request]
 
 jobs:
   build:
diff --git a/index.js b/index.js
@@ -2,10 +2,11 @@
  * Module dependencies.
  */
 
+var assert = require('assert');
 var net = require('net');
 var tls = require('tls');
 var url = require('url');
-var events = require('events');
+var stream = require('stream');
 var Agent = require('agent-base');
 var inherits = require('util').inherits;
 var debug = require('debug')('https-proxy-agent');
@@ -161,14 +162,16 @@ HttpsProxyAgent.prototype.callback = function connect(req, opts, fn) {
 			// that the node core `http` can parse and handle the error status code
 			cleanup();
 
-			// the original socket is closed, and a "fake socket" EventEmitter is
+			// the original socket is closed, and a new closed socket is
 			// returned instead, so that the proxy doesn't get the HTTP request
 			// written to it (which may contain `Authorization` headers or other
 			// sensitive data).
 			//
 			// See: https://hackerone.com/reports/541502
 			socket.destroy();
-			socket = new events.EventEmitter();
+			socket = new net.Socket();
+			socket.readable = true;
+
 
 			// save a reference to the concat'd Buffer for the `onsocket` callback
 			buffers = buffered;
@@ -182,15 +185,11 @@ HttpsProxyAgent.prototype.callback = function connect(req, opts, fn) {
 
 	function onsocket(socket) {
 		debug('replaying proxy buffer for failed request');
+		assert(socket.listenerCount('data') > 0);
 
 		// replay the "buffers" Buffer onto the `socket`, since at this point
 		// the HTTP module machinery has been hooked up for the user
-		if (socket.listenerCount('data') > 0) {
-			socket.emit('data', buffers);
-		} else {
-			// never?
-			throw new Error('should not happen...');
-		}
+		socket.push(buffers);
 
 		// nullify the cached Buffer instance
 		buffers = null;
diff --git a/test/test.js b/test/test.js
@@ -234,6 +234,25 @@ describe('HttpsProxyAgent', function() {
 				done();
 			});
 		});
+		it('should not error if the proxy responds with 407 and the request is aborted', function(done) {
+			proxy.authenticate = function(req, fn) {
+				fn(null, false);
+			};
+
+			const proxyUri =
+				process.env.HTTP_PROXY ||
+				process.env.http_proxy ||
+				'http://127.0.0.1:' + proxyPort;
+
+			const req = http.get({
+				agent: new HttpsProxyAgent(proxyUri)
+			}, function(res) {
+				assert.equal(407, res.statusCode);
+				req.abort();
+			});
+
+			req.on('abort', done);
+		});
 		it('should emit an "error" event on the `http.ClientRequest` if the proxy does not exist', function(done) {
 			// port 4 is a reserved, but "unassigned" port
 			var proxyUri = 'http://127.0.0.1:4';
