feat: add https proxy server implementation #626
Conversation
github-actions
bot
added
t-core-services
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
bliuchak
added
medium priority
jirimoravcik
left a comment
jirimoravcik
left a comment
There was a problem hiding this comment.
Thanks for the PR, looks great, had a few comments
There was a problem hiding this comment.
(opt) This file would benefit from a split into multiple ones.
There was a problem hiding this comment.
Yep, you already have https-stress-test.js, so maybe the new nice https scenarios could be stand-alone files as well?
There was a problem hiding this comment.
Fine, I'll take a look at it.
tobice
left a comment
tobice
left a comment
There was a problem hiding this comment.
Honestly I have no idea how proxy-chain works and my general knowledge of proxies is still limited 😄 So I'm leaving the actual functionality review to Jirka and Ludvík.
From my PoV the application code change is small, well documented and easy to read. No concerns there.
The tests, especially the new ones, would benefit from splitting into files. Also one scenario caught my attention due to its nested structure. I'm not sure why that's needed.
Finally, I'd expand on the PR description. You can e.g. add there the nice diagram from the original PR.
| * Connection statistics for bandwidth tracking. | ||
| */ | ||
| export type ConnectionStats = { | ||
| // Bytes sent from proxy to client. |
There was a problem hiding this comment.
I'm a bit confused by these descriptions. Is this one even correct? Shouldn't it be from client to proxy?
Also Bytes received from client to proxy. is not very good English, which further complicates the readability.
I'd do:
Bytes sent by client to proxyBytes received by proxy from client
This should be clear and unambiguous.
| } | ||
|
|
||
| // Apply secure TLS defaults (user options can override). | ||
| const secureDefaults: https.ServerOptions = { |
There was a problem hiding this comment.
| const secureDefaults: https.ServerOptions = { | |
| const effectiveOptions: https.ServerOptions = { |
(nit) IMHO these are no longer defaults.
| const DEFAULT_AUTH_REALM = 'ProxyChain'; | ||
| const DEFAULT_PROXY_SERVER_PORT = 8000; | ||
|
|
||
| const HTTPS_DEFAULTS = { |
There was a problem hiding this comment.
| const HTTPS_DEFAULTS = { | |
| const HTTPS_DEFAULT_OPTIONS = { |
(opt) Just more consistent naming across the code...
| if (this.serverType === 'https') { | ||
| // For HTTPS: Track only post-TLS-handshake sockets (secureConnection). | ||
| // This ensures we track the TLS-wrapped socket with correct bytesRead/bytesWritten. | ||
| this.server.on('secureConnection', this.onConnection.bind(this)); |
There was a problem hiding this comment.
What happens if the server is HTTPs but somebody tries to establish an insecure connection? Is that even possible? 🤔
|
|
||
| // Not specific for https but still worth to have. | ||
| it('handles 100 concurrent CONNECT tunnels with data verification', async () => { | ||
| const TUNNELS = 100; |
There was a problem hiding this comment.
| const TUNNELS = 100; | |
| const TUNNEL_COUNT = 100; |
(opt)(nit)
Dtto below for REQUESTS.
| } | ||
| }); | ||
|
|
||
| it('handles TLS handshake failures gracefully and continues accepting connections', function (done) { |
There was a problem hiding this comment.
(opt) The describe has only one it. Maybe you can inline it?
| tlsErrors.push(error); | ||
| }); | ||
|
|
||
| server.listen().then(() => { |
There was a problem hiding this comment.
Why not make the test async and then do await server.listen()? 🤔 In general the structure of this test is super weird and confusing with all the nesting and relying on done.
Maybe you can promisify the socket communication instead to keep the structure more flat?
await server.listen();
const badSocked = tls.connect({ ... });
await new Promise((resolve) => badSocket.on('close', resolve));
await promiseDelay(1000); // We have utility like this somewhere(I can see you are doing that below.)
There was a problem hiding this comment.
Yep, you already have https-stress-test.js, so maybe the new nice https scenarios could be stand-alone files as well?
3 participants
This PR represents simplification of #602. It contains changes related only to HTTPS proxy server support.
Scope:
Out Of Scope (should be fixed separately):
got-scrapting.Note: I might miss some edge cases for HTTPS server proxy, however I think that current coverage is good enough and covers core logic fully (80/20) and is backward compatible (HTTP proxy server tests pass).