Skip to content

Conversation

@caitp
Copy link
Contributor

@caitp caitp commented Jul 16, 2025

This refactors the way the legacy unsecured behaviour is implemented, by wrapping the Proxy client in a wrapper which rewrites requests, and handles errors. This will also insert authentication headers in each request.

CURRENTLY:

For nonsecure channels, clients allocated by the pool all delegate to the proxy client, and reuse the same HttpContext. This is probably not a good thing, but at least in cases where only one request is performed at a time, it seems harmless enough. In order to make this ready to land, this probably needs to handle simultaneous concurrent requests and have a test for that.

This relates to...

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

The changes lgtm, and I'm aligned with them; tho this seems like a breaking change that might be better to keep disabled by default and change it once in the next major

Comment on lines 67 to 71
// FIXME: remove wrapper from the pool
}

async [kDestroy] () {
await this.#client.destroy()
// FIXME: remove wrapper from the pool
Copy link
Member

Choose a reason for hiding this comment

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

why not close the this.#client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as noted in the PR description, this.#client is currently the original Proxy client -- which I think is actually a huge problem, but I haven't written a test yet to demonstrate that it is, which is probably next up. Maybe it's actually fine to reuse the same HttpContext for concurrent requests, but I suspect it isn't

But given that it is the Proxy server client, closing it breaks the ProxyAgent

@caitp
Copy link
Contributor Author

caitp commented Jul 22, 2025

I've attempted to add a test for this reuse of the HttpContext, and haven't been able to reproduce any issues yet (although the test case is pretty small) -- so maybe it actually does work?

but still need to look into the other CI failures.

@caitp caitp force-pushed the NoTunnelByDefault branch from 9cb8d42 to 8881567 Compare July 23, 2025 20:25
caitp added 3 commits July 23, 2025 16:25
…ons (nodejs#4180)

This refactors the way the legacy unsecured behaviour is implemented, by wrapping the Proxy client in a wrapper which rewrites requests, and handles errors. This will also insert authentication headers in each request.
@caitp
Copy link
Contributor Author

caitp commented Jul 23, 2025

summary of the changes:

  • factory can now be passed to ProxyAgent again, which was needed to get tests using MockAgent etc working. By default, it will use something lifted from Agent.js's defaultFactory, which takes options.connections into account to decide whether to creates a Pool or a Client.
  • Http1ProxyWrapper no longer reuses the existing connection to the Proxy, but instead creates a new one, and retains it for the life time of the ProxyWrapper. The wrapper is responsible for rewriting requests in the format of HTTP1 non-tunneling proxy requests
  • proxyTunnel now defaults to true again, so the non-tunneling becomes opt-in again (was already the case in the last update) -- however now, tests have been updated to explicitly specify the proxyTunnel value, so that it's less of a change needed when flipping the flag.

@metcoder95 metcoder95 requested a review from mcollina July 24, 2025 08:03
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit b7513d4 into nodejs:main Jul 24, 2025
27 checks passed
@github-actions github-actions bot mentioned this pull request Jul 31, 2025
@JoshMock JoshMock mentioned this pull request Aug 7, 2025
@github-actions
Copy link
Contributor

The backport to v6.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-4340-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b7513d4df62e9d1c1ecc34c3a418bd402e3c8432
# Push it to GitHub
git push --set-upstream origin backport-4340-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.x

Then, create a pull request where the base branch is v6.x and the compare/head branch is backport-4340-to-v6.x.

metcoder95 pushed a commit that referenced this pull request Aug 21, 2025
…ons (#4180) (#4340)

* feat(ProxyAgent) improve Curl-y behavior in HTTP->HTTP Proxy connections (#4180)

This refactors the way the legacy unsecured behaviour is implemented, by wrapping the Proxy client in a wrapper which rewrites requests, and handles errors. This will also insert authentication headers in each request.

* add a test to attempt multiple concurrent connections with a single HttpContext

* be explicit about proxyTunnel status in each ProxyAgent test

(cherry picked from commit b7513d4)
@github-actions
Copy link
Contributor

The backport to v6.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-4340-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b7513d4df62e9d1c1ecc34c3a418bd402e3c8432
# Push it to GitHub
git push --set-upstream origin backport-4340-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.x

Then, create a pull request where the base branch is v6.x and the compare/head branch is backport-4340-to-v6.x.

Uzlopak pushed a commit that referenced this pull request Aug 22, 2025
…ons (#4180) (#4340) (#4445)

* feat(ProxyAgent) improve Curl-y behavior in HTTP->HTTP Proxy connections (#4180) (#4340)

* test: adjust v18 expectations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants