Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Oct 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
@@ -1,6 +1,6 @@
name: Node CI

on: [push]
on: [push, pull_request]

jobs:
build:
Expand Down
17 changes: 8 additions & 9 deletions index.js
Expand Up @@ -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');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var stream = require('stream');

var Agent = require('agent-base');
var inherits = require('util').inherits;
var debug = require('debug')('https-proxy-agent');
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
19 changes: 19 additions & 0 deletions test/test.js
Expand Up @@ -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';
Expand Down