-
Notifications
You must be signed in to change notification settings - Fork 21
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
Support HTTPS proxying #5
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for this!
A couple of change requests, if you don't mind :)
index.js
Outdated
if (!options.ssl) { | ||
options.ssl = { | ||
key: __dirname + "/ssl/snakeoil.key", | ||
cert: __dirname + "/ssl/snakeoil.crt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use path.join
here. I haven't tested on Windows, but nice to keep compat if it does work
index.js
Outdated
options.ssl.key = fs.readFileSync(options.ssl.key); | ||
options.ssl.cert = fs.readFileSync(options.ssl.cert); | ||
|
||
var rl = makeRequestListener(har.log.entries, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind renaming this listener
. I prefer more descriptive names
One thing that I haven't documented about this change is that in order to handle HTTPS proxying, we need to open 3 ports total:
The user only need concern themselves with the first port, since that's the one they connect to... but the servers themselves require two more ports which could be in use by other processes. The way those ports are assigned is by adding 1 and 2 to the proxy port. Not sure what the best way to communicate this change is in the README - should this be an option as well? |
Upon further reflection, this is an easily-solved problem - check if the port is available first. If not, keep iterating upward until we find one we can use. If the user manually specifies a port, we bypass this behavior and throw an error instead. WDYT? |
@Stuk I've gone ahead and made the change to randomly assign internal proxying ports. Feeling more confident about merging this now. Let me know if you want any other changes. |
The code looks good! One thing I'm wondering: when running from the command line how does the user know what the https port is? Should there be a default https port and some logging in Sorry for the slow response! |
@Stuk Thanks for taking another look! The user doesn't choose between HTTP or HTTPS proxies - that selection is done by the script based on the incoming request. All they do is specify the port they want the proxy on, and I have a network listener on that port which figures out what kind of request it is. I instantiate that listener here: https://github.com/RickyRomero/server-replay/blob/16c27f68545896430ffa357c6ae103e36aba80df/index.js#L72-L75 The Here's some pseudocode to illustrate what's happening:
|
I'm working on a project right now which requires both HAR replay and HTTPS proxying, so I've added the latter to this library. Let me know if there's anything you need from me to successfully merge this and push an update to npm.