Fix `keepAlive` for http-proxy-agent #169

jportner · 2023-05-15T17:14:02Z

I was testing the new keepAlive option, and it turns out that while it works for https-proxy-agent, it doesn't work with http-proxy-agent.

I use Smokescreen, and when testing keepAlive with http-proxy-agent, the first request succeeded, but all subsequent requests failed with an error: "This is a proxy server. Does not respond to non-proxy requests"

Turns out, the first request used an absolute URL (http://localhost:3000/foo) while all subsequent requests used relative URLs (/foo). HTTP proxy servers require that HTTP requests use absolute URLs, which is why this happens. (HTTPS requests are not impacted because they are encrypted, and regular non-MITM proxies cannot read the URL).

The http-proxy-agent package already had logic to rewrite the relative URL to an absolute URL:

proxy-agents/packages/http-proxy-agent/src/index.ts

Lines 88 to 90 in 2f835a4

    
           // Change the `http.ClientRequest` instance's "path" field 
        
           // to the absolute path of the URL that will be requested. 
        
           req.path = String(url);

However, that only happens on connect(), so it only worked on the first request. This PR makes the following changes:

Updates existing tests and adds additional tests to demonstrate the problem: 1467942 (three tests now fail)
Fixes the problem, and now the tests all pass: 6f3bc0d

* All existing keepAlive tests now assert OK status * Added http-proxy-agent keepAlive test * Added pac-proxy-agent keepAlive tests * Added proxy-agent keepAlive test and cache tests

changeset-bot · 2023-05-15T17:14:05Z

🦋 Changeset detected

Latest commit: a841e14

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
http-proxy-agent	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2023-05-15T17:14:06Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
proxy-agents	✅ Ready (Inspect)	Visit Preview	May 16, 2023 4:20am

jportner · 2023-05-15T17:43:20Z

packages/http-proxy-agent/src/index.ts

+	addRequest(req: InternalClientRequest, opts: AgentConnectOpts): void {
+		this.setRequestProps(req, opts);
+		// @ts-expect-error `addRequest()` isn't defined in `@types/node`
+		super.addRequest(req, opts);
+	}

+	async connect(
+		req: InternalClientRequest,
+		opts: AgentConnectOpts
+	): Promise<net.Socket> {
+		if (this.setRequestPropsOnConnect) {
+			this.setRequestProps(req, opts);
+		}
 		// Create a socket connection to the proxy server.
 		let socket: net.Socket;


This is the primary change to fix the problem. I noticed that when keepAlive is enabled, the call stack for the first request is Agent.addRequest() -> Agent.createSocket() -> Agent.connect(), and all subsequent requests only call Agent.addRequest().

Before: HttpProxyAgent.connect() rewrites the path and add headers for only the first request

After:

In "http-proxy-agent": HttpProxyAgent.addRequest() will rewrite the path and add headers for all requests

In "proxy-agent" and "pac-proxy-agent": we need to use the setRequestPropsOnConnect option to cause HttpProxyAgent.connect() to rewrite the path and add headers for the first request, and then we have the right context to rewrite the path and add headers for all subsequent requests.

It's not ideal to have the second code path with the setRequestPropsOnConnect option, but because of the way that "proxy-agent" and "pac-proxy-agent" abstract out the usage of "http-proxy-agent", I couldn't see a cleaner way to do this.

I tweaked this in 2ddd92f in order to descope this PR and maintain full backwards compatibility with ProxyAgent/PacProxyAgent, see #169 (comment)

jportner · 2023-05-15T17:46:02Z

packages/proxy-agent/src/index.ts

@@ -41,7 +39,7 @@ export const proxies: {
 	'pac-ftp': [PacProxyAgent, PacProxyAgent],
 	'pac-http': [PacProxyAgent, PacProxyAgent],
 	'pac-https': [PacProxyAgent, PacProxyAgent],
-};
+} satisfies { [P in ValidProtocol]: [AgentConstructor, AgentConstructor] };


Using the satisfies operator introduced in TS 4.9 allows us to preserve more specific type information on the proxies object. That means that // @ts-expect-error meh… below is no longer necessary, I was able to remove it 😄

Nice! This may constitute a breaking change though. If we're relying on any TS 4.9+ features already then that wouldn't be the case.

Oh interesting, I hadn't thought of that, just assumed it would be OK because this repo uses TS 5.0.

I just checked the contents of packages/proxy-agent/dist/index.d.ts, and it looks like satisfies operator has been stripped out of the compiled type definition:

/** * Supported proxy types. */ export declare const proxies: { http: [typeof HttpProxyAgent, typeof HttpsProxyAgent]; https: [typeof HttpProxyAgent, typeof HttpsProxyAgent]; socks: [typeof SocksProxyAgent, typeof SocksProxyAgent]; socks4: [typeof SocksProxyAgent, typeof SocksProxyAgent]; socks4a: [typeof SocksProxyAgent, typeof SocksProxyAgent]; socks5: [typeof SocksProxyAgent, typeof SocksProxyAgent]; socks5h: [typeof SocksProxyAgent, typeof SocksProxyAgent]; 'pac-data': [typeof PacProxyAgent, typeof PacProxyAgent]; 'pac-file': [typeof PacProxyAgent, typeof PacProxyAgent]; 'pac-ftp': [typeof PacProxyAgent, typeof PacProxyAgent]; 'pac-http': [typeof PacProxyAgent, typeof PacProxyAgent]; 'pac-https': [typeof PacProxyAgent, typeof PacProxyAgent]; };

I guess operators like this are just used at compile time. TIL!

packages/pac-proxy-agent/src/index.ts

packages/proxy-agent/src/index.ts

packages/proxy-agent/test/test.ts

This test was fixed in 6f3bc0d. I forgot to remove the comment in that commit.

TooTallNate · 2023-05-15T21:09:35Z

Thank you for identifying the primary bug in http-proxy-agent, and for this PR, and for all the descriptive comments! I do have an ask though, since this seems to be addressing a few different issues in multiple modules: can we break this up into a few smaller more focused PRs?

http-proxy-agent fix (I actually think the setRequestPropsOnConnect thing won't be necessary, see next point).
pac-proxy-agent seems to only address the http-proxy-agent module case. In actuality, keepAlive is pretty broken in general for that module right now. It needs an agent caching mechanism like proxy-agent has.
satisfies could be its own PR.
Some of the agent-base changes seem like they might be affected by [agent-base] Allow for never relying on stack trace #170.

jportner · 2023-05-15T21:29:11Z

Thank you for identifying the primary bug in http-proxy-agent, and for this PR, and for all the descriptive comments!

Sure thing, I'm glad I can contribute!

I do have an ask though, since this seems to be addressing a few different issues in multiple modules: can we break this up into a few smaller more focused PRs?

Yeah... I set out to do one thing and it ballooned a bit. I'm happy to split this up, I'll take a crack at it tomorrow morning.

jportner · 2023-05-16T03:56:37Z

packages/http-proxy-agent/src/index.ts

-		const base = `${protocol}//${hostname}`;
+		const base = `http://${hostname}`;


It seemed redundant to have const protocol = opts.secureEndpoint ? 'https:' : 'http:'; and use that to generate the URL, when HttpProxyAgent should only be used for HTTP URLs. In any case, the secureEndpoint option is always undefined Agent.addRequest() (except for when ProxyAgent is used).

I simplified it here by hard-coding the protocol, I think that's OK.

If we're going to do this, then the function should throw an error when opts.secureEndpoint === true. However, I think it is valid to do HTTPS requests (though less ideal, especially over an external network). I believe squid works when passing an https URL for example.

jportner · 2023-05-16T03:58:33Z

packages/http-proxy-agent/src/index.ts

+	addRequest(req: HttpProxyAgentClientRequest, opts: AgentConnectOpts): void {
+		this.setRequestProps(req, opts);
+		// @ts-expect-error `addRequest()` isn't defined in `@types/node`
+		super.addRequest(req, opts);
+	}
+
+	async connect(
+		req: HttpProxyAgentClientRequest,
+		opts: AgentConnectOpts
+	): Promise<net.Socket> {
+		if (!req._updatedPropsForProxyAgent) {
+			// Maintain backwards compatibility with ProxyAgent and
+			// PacProxyAgent, which both rely on HttpProxyAgent to set request
+			// props inside the connect() function.
+			this.setRequestProps(req, opts);
+		}


I changed this to work in a fully backwards-compatible way.
ProxyAgent and PacProxyAgent still function and all tests pass.

jportner · 2023-05-16T04:04:57Z

packages/http-proxy-agent/src/index.ts

-		req._implicitHeader();
+		if (!req._header) {
+			req._implicitHeader();
+		}


I had to add this conditional because now that we are setting the headers before addRequest(), it causes req._header to be non-null here, which will cause req._implicitHeader() to throw an exception.

Alternatively, I could do just allow connect() to rewrite the request URL and set headers, even if it's already happened in addRequest():

diff --git a/packages/http-proxy-agent/src/index.ts b/packages/http-proxy-agent/src/index.ts index 16d5b51..807e4c5 100644 --- a/packages/http-proxy-agent/src/index.ts +++ b/packages/http-proxy-agent/src/index.ts @@ -29,7 +29,6 @@ interface HttpProxyAgentClientRequest extends http.ClientRequest { }[]; _header?: string | null; _implicitHeader(): void; - _updatedPropsForProxyAgent?: boolean; } function isHTTPS(protocol?: string | null): boolean { @@ -104,8 +103,6 @@ export class HttpProxyAgent<Uri extends string> extends Agent { this.keepAlive ? 'Keep-Alive' : 'close' ); } - - req._updatedPropsForProxyAgent = true; } addRequest(req: HttpProxyAgentClientRequest, opts: AgentConnectOpts): void { @@ -118,12 +115,10 @@ export class HttpProxyAgent<Uri extends string> extends Agent { req: HttpProxyAgentClientRequest, opts: AgentConnectOpts ): Promise<net.Socket> { - if (!req._updatedPropsForProxyAgent) { - // Maintain backwards compatibility with ProxyAgent and - // PacProxyAgent, which both rely on HttpProxyAgent to set request - // props inside the connect() function. - this.setRequestProps(req, opts); - } + // Maintain backwards compatibility with ProxyAgent and + // PacProxyAgent, which both rely on HttpProxyAgent to set request + // props inside the connect() function. + this.setRequestProps(req, opts); // Create a socket connection to the proxy server. let socket: net.Socket; @@ -141,9 +136,7 @@ export class HttpProxyAgent<Uri extends string> extends Agent { let first: string; let endOfHeaders: number; debug('Regenerating stored HTTP header string for request'); - if (!req._header) { - req._implicitHeader(); - } + req._implicitHeader(); if (req.outputData && req.outputData.length > 0) { // Node >= 12 debug(

What do you think?

jportner · 2023-05-16T04:05:31Z

packages/pac-proxy-agent/test/test.ts

+					agent,
+				});
+				expect(res2.headers.connection).toEqual('keep-alive');
+				// expect(res2.statusCode).toEqual(200); // TODO: this assertion will cause this test to fail; uncomment in a follow-on PR to fix keepAlive for PacProxyAgent+HTTP


I left the new tests and test assertions intact.
I simply commented out the assertions that are currently failing.

jportner · 2023-05-16T04:14:45Z

I do have an ask though, since this seems to be addressing a few different issues in multiple modules: can we break this up into a few smaller more focused PRs?

Yeah... I set out to do one thing and it ballooned a bit. I'm happy to split this up, I'll take a crack at it tomorrow morning.

OK, I descoped this PR to focus on fixing HttpProxyAgent as requested, this is ready for review whenever you have time.

Also, remove auto-formatting changes in other packages

Part of #169.

Based on #169.

TooTallNate · 2023-05-24T19:19:36Z

Hey @jportner! Sorry for the delayed review. I dove into your branch to get a better understanding. I'm proposing this slight altercation instead: #190 (still giving you credit as the commit author). Let me know what you think!

Based on #169. Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

jportner · 2023-05-24T22:52:04Z

Hey @jportner! Sorry for the delayed review. I dove into your branch to get a better understanding. I'm proposing this slight altercation instead: #190 (still giving you credit as the commit author). Let me know what you think!

Thanks, looks good to me! I like your approach of checking the path instead of setting a flag on the request object.

TooTallNate · 2023-05-25T00:16:47Z

Looking forward to any more follow-up PRs from you as well 😃

jportner added 2 commits May 15, 2023 11:31

pnpm format

3b50fc1

Update tests

1467942

* All existing keepAlive tests now assert OK status * Added http-proxy-agent keepAlive test * Added pac-proxy-agent keepAlive tests * Added proxy-agent keepAlive test and cache tests

vercel bot deployed to Preview May 15, 2023 17:14 View deployment

vercel bot deployed to Preview May 15, 2023 17:17 View deployment

jportner added 2 commits May 15, 2023 13:42

Fix http-proxy-agent keepAlive

6f3bc0d

Add changeset

a546c05

jportner force-pushed the fix-http-proxy-agent-keepalive branch from 14b1490 to a546c05 Compare May 15, 2023 17:42

vercel bot deployed to Preview May 15, 2023 17:43 View deployment

jportner commented May 15, 2023

View reviewed changes

jportner marked this pull request as ready for review May 15, 2023 17:56

Remove outdated comment

1170a79

This test was fixed in 6f3bc0d. I forgot to remove the comment in that commit.

vercel bot deployed to Preview May 15, 2023 20:28 View deployment

TooTallNate added the http-proxy-agent label May 15, 2023

Descope changes

bac3018

vercel bot deployed to Preview May 16, 2023 03:52 View deployment

Remove debug statement

2ddd92f

vercel bot deployed to Preview May 16, 2023 03:58 View deployment

jportner commented May 16, 2023

View reviewed changes

jportner requested a review from TooTallNate May 16, 2023 04:09

Reformat

a841e14

Also, remove auto-formatting changes in other packages

vercel bot deployed to Preview May 16, 2023 04:20 View deployment

TooTallNate added a commit that referenced this pull request May 24, 2023

[agent-base] Add isSecureEndpoint() as an instance method

1773a4e

Part of #169.

TooTallNate mentioned this pull request May 24, 2023

[agent-base] Add isSecureEndpoint() as an instance method #186

Merged

TooTallNate added a commit that referenced this pull request May 24, 2023

[agent-base] Add isSecureEndpoint() as an instance method

4cd803b

Part of #169.

TooTallNate added a commit that referenced this pull request May 24, 2023

[agent-base] Add isSecureEndpoint() as an instance method (#186)

da699b1

Part of #169.

TooTallNate pushed a commit that referenced this pull request May 24, 2023

[http-proxy-agent] Fix keepAlive: true

7bf9c4f

Based on #169.

TooTallNate mentioned this pull request May 24, 2023

[http-proxy-agent] Fix keepAlive: true #190

Merged

TooTallNate added a commit that referenced this pull request May 24, 2023

[http-proxy-agent] Fix keepAlive: true (#190)

eb6906b

Based on #169. Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>

jportner closed this May 24, 2023

jportner deleted the fix-http-proxy-agent-keepalive branch May 24, 2023 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `keepAlive` for http-proxy-agent #169

Fix `keepAlive` for http-proxy-agent #169

jportner commented May 15, 2023 •

edited

changeset-bot bot commented May 15, 2023 •

edited

vercel bot commented May 15, 2023 •

edited

jportner May 15, 2023

jportner May 16, 2023

jportner May 15, 2023

TooTallNate May 15, 2023

jportner May 15, 2023

TooTallNate May 15, 2023

TooTallNate commented May 15, 2023

jportner commented May 15, 2023

jportner May 16, 2023 •

edited

TooTallNate May 24, 2023

jportner May 16, 2023

jportner May 16, 2023

jportner May 16, 2023

jportner commented May 16, 2023

TooTallNate commented May 24, 2023

jportner commented May 24, 2023

TooTallNate commented May 25, 2023

	// Change the `http.ClientRequest` instance's "path" field
	// to the absolute path of the URL that will be requested.
	req.path = String(url);

		const base = `${protocol}//${hostname}`;
		const base = `http://${hostname}`;

Fix keepAlive for http-proxy-agent #169

Fix keepAlive for http-proxy-agent #169

Conversation

jportner commented May 15, 2023 • edited

changeset-bot bot commented May 15, 2023 • edited

🦋 Changeset detected

vercel bot commented May 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TooTallNate commented May 15, 2023

jportner commented May 15, 2023

jportner May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner commented May 16, 2023

TooTallNate commented May 24, 2023

jportner commented May 24, 2023

TooTallNate commented May 25, 2023

Fix `keepAlive` for http-proxy-agent #169

Fix `keepAlive` for http-proxy-agent #169

jportner commented May 15, 2023 •

edited

changeset-bot bot commented May 15, 2023 •

edited

vercel bot commented May 15, 2023 •

edited

jportner May 16, 2023 •

edited