Skip to content

fix(env-http-proxy-agent): ignore trailing dots when matching no_proxy - #5637

Open
pacocartones wants to merge 1 commit into
nodejs:mainfrom
pacocartones:fix/env-http-proxy-agent-trailing-dot
Open

fix(env-http-proxy-agent): ignore trailing dots when matching no_proxy#5637
pacocartones wants to merge 1 commit into
nodejs:mainfrom
pacocartones:fix/env-http-proxy-agent-trailing-dot

Conversation

@pacocartones

Copy link
Copy Markdown
Contributor

This relates to...

No open issue. This is the same class of no_proxy matching bug as #5623 (bare IPv6 addresses), which landed in this same file last week, so I followed that PR's shape: one small fix plus a test in the existing describe('no_proxy') block.

Not filed through the security process, deliberately — see the note at the end of the Rationale.

Rationale

A trailing dot is the fully qualified form of a domain name (the RFC 1034 root label): example.com. and example.com are the same name. EnvHttpProxyAgent compares host strings, and neither side of the comparison normalises that dot, so the two forms never match each other.

Concretely, on main:

  • with no_proxy=example.com, a request to http://example.com./ (or https://, or with an explicit port, or to http://sub.example.com./) is routed through the proxy instead of going direct;
  • with no_proxy=example.com., that entry matches nothing at all — not even http://example.com/.

The trailing dot is not something people type by accident. It is how you pin a name to the DNS root and skip search domain expansion — Kubernetes' own guidance is to use svc.cluster.local. in latency-sensitive paths for exactly this reason, and plenty of internal tooling emits FQDNs in that form. The user-visible symptom is that traffic meant to stay inside the network is handed to the corporate proxy, which normally can't reach the internal host, so the request fails to connect (or, at best, takes a slower path).

Standalone reproduction, fully local, no network needed. From an undici checkout:

const { createServer } = require('node:http')
const { EnvHttpProxyAgent } = require('./index.js')

const server = createServer((req, res) => res.end('direct'))
server.listen(0, '127.0.0.1', async () => {
  const { port } = server.address()
  const agent = new EnvHttpProxyAgent({
    httpProxy: 'http://127.0.0.1:1/', // nothing is listening here
    noProxy: '127.0.0.1,localhost,example.test'
  })
  for (const origin of ['http://localhost:' + port, 'http://localhost.:' + port]) {
    try {
      const res = await agent.request({ origin, path: '/', method: 'GET' })
      console.log(origin, '->', await res.body.text())
    } catch (err) {
      console.log(origin, '-> ERROR', err.code ?? err.message)
    }
  }
  await agent.close()
  server.close()
})
# on main
http://localhost:43453  -> direct
http://localhost.:43453 -> ERROR ECONNREFUSED     <-- sent to the dead proxy
# with this PR
http://localhost:43447  -> direct
http://localhost.:43447 -> direct

Prior art. curl ignores a trailing dot on both sides of the no_proxy comparison — on the entry (lib/proxy.c, "ignore trailing dots in the token to check") and on the request hostname ("ignore trailing dots in the hostname"). This PR does the same thing in the same two places, which is why it touches both #getProxyAgentForUrl and #parseNoProxy.

On framing: I am explicitly not presenting this as a security issue. SECURITY.md puts "any proxy server configured by the application, runtime, or environment" inside the trusted set and states that undici's proxy support "is not intended to ... bypass organizational, regulatory, or legal controls". Sending a request to the configured, trusted proxy is therefore not a boundary violation in undici's threat model. This is a correctness bug and a parity gap with curl; the practical impact is connectivity, and the scope is limited to hosts written with a trailing dot. Low to medium impact, and I'd rather say so than oversell it.

Changes

Both sides of the no_proxy comparison now drop a single trailing dot before matching:

  • #getProxyAgentForUrl — the request host, alongside the existing port-suffix and IPv6-bracket stripping.
  • #parseNoProxy — each no_proxy entry, alongside the existing leading-dot / *. stripping.
  • docs/docs/api/EnvHttpProxyAgent.md — one sentence documenting the behaviour.

Notes on the edges, since this is host-matching code:

  • CONTRIBUTING.md asks for a test/issue-XXXX.js reproduction. There is no issue here to number, and createEnvHttpProxyAgentWithMocks plus the whole describe('no_proxy') block already live in test/env-http-proxy-agent.js, so the test goes there — same as fix(env-http-proxy-agent): match bare IPv6 addresses in no_proxy #5623 did in this file last week. The standalone script above is the CONTRIBUTING-style reproduction; happy to add it as a file if you'd rather have one.
  • Only one dot is stripped, matching curl (a single check, not a loop). example.com.. becomes example.com. on both sides, so those still match each other consistently.
  • On the request side, length > 1 leaves the degenerate host . untouched (new URL('http://./') does parse, host === .). On the entry side nothing new happens for . or *.: the existing leading-dot strip already turns them into an empty entry, before and after this change.
  • IPv4 literals never reach this code with a dot — the WHATWG host parser already canonicalises new URL('http://127.0.0.1./').host to 127.0.0.1.
  • IPv6 literals cannot carry a trailing dot at all — new URL('http://[::1]./') throws ERR_INVALID_URL. The bare-IPv6 handling added in fix(env-http-proxy-agent): match bare IPv6 addresses in no_proxy #5623 is untouched.
  • The two sides use different shapes on purpose. #getProxyAgentForUrl runs on every dispatch, so it uses a charCodeAt check instead of a third regex — /^(.+)\.$/ has a capture group and backtracks over the whole string on the common (no-dot) case, which measured ~2.3x the cost of the existing two-regex chain on this line (139 ns -> 320 ns/call vs 145 ns for the charCode check, Node v24.18.1, 2M iterations). #parseNoProxy runs once per no_proxy change, not per request, so it keeps the regex style of the line it sits on.

Features

N/A

Bug Fixes

  • EnvHttpProxyAgent now honours no_proxy for hosts written in fully qualified form with a trailing dot (http://example.com./ with no_proxy=example.com), including subdomains and host:port entries.
  • A no_proxy entry written with a trailing dot (no_proxy=example.com.) now matches, instead of silently matching nothing.

Breaking Changes and Deprecations

N/A. No public API change. The only behaviour that changes is host matching for names carrying a trailing dot, which previously never matched and had no useful semantics.

One accidental behaviour does go away: because no_proxy=. stores an empty entry, the subdomain rule currently degenerates into hostname.endsWith('.'), i.e. no_proxy=. today bypasses the proxy for exactly the hosts written with a trailing dot. After this change those hosts no longer end in a dot, so that stops. It had no defensible semantics and no test covers it.

Status


Verification

node --test test/env-http-proxy-agent.js
npm run test:unit
npm run lint

Results on main @ ae4a3e3, Node v24.18.1:

state test/env-http-proxy-agent.js
test only, no fix 36 pass, 1 fail (first assertion, http://example.com./)
test + request-side hunk only 1 fail (the entry-side assertion)
test + entry-side hunk only 1 fail (the request-side assertion)
test + both hunks 37 pass, 0 fail

So each hunk is independently necessary and independently covered by an assertion. npm run test:unit is 1489 tests / 0 failures / 4 skipped with the patch applied, and npm run lint is clean with the cache cleared. The 36 pre-existing tests in the file — leading dot, *., IPv4, bare IPv6, ports, case-insensitivity — are unchanged.

Disclosure: I used an AI assistant while investigating this and drafting the patch. I don't see a policy on this in CONTRIBUTING.md, so I'm mentioning it for transparency. The diff, the test and every number above are the result of running this locally, and I'm happy to defend or rework any part of it.

@codecov-commenter

codecov-commenter commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.44%. Comparing base (dd85997) to head (0bc7658).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5637   +/-   ##
=======================================
  Coverage   93.43%   93.44%           
=======================================
  Files         110      110           
  Lines       38733    38783   +50     
=======================================
+ Hits        36190    36240   +50     
  Misses       2543     2543           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

A trailing dot marks the fully qualified form of a domain name (the RFC 1034
root label), so `example.com.` and `example.com` are the same name. Neither
side of the no_proxy comparison normalised it, so a request to
`http://example.com./` was routed through the proxy even when no_proxy
contained `example.com`, and an entry written as `example.com.` matched
nothing at all.

Drop a single trailing dot on both sides of the comparison, as curl does in
lib/proxy.c. IPv4 literals are already canonicalised by `new URL()` and IPv6
literals cannot end with a dot, so only DNS names are affected.

Refs: https://github.com/curl/curl/blob/master/lib/proxy.c
Signed-off-by: pacocartones <manusanchezhl@gmail.com>
@pacocartones
pacocartones force-pushed the fix/env-http-proxy-agent-trailing-dot branch from 0d6ccfb to 0bc7658 Compare August 7, 2026 00:43
@pacocartones

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (86b6299). The only change versus the version you may have seen is the base — git range-diff shows the commit itself untouched:

 1..13:  ........ =  (13 upstream commits)
 -:  -------- > 14:  0bc7658d fix(env-http-proxy-agent): ignore trailing dots when matching no_proxy

The reason for the rebase: CodeQL was red here, and it was not this change. Autobuild was failing repo-wide at the time — on main pushes and on unrelated PRs (dependabot, ci-nightly, fix-pipelining-test-flake). #5655 (ci: align codeql-action autobuild and analyze with init at v4.37.3) landed since, and this branch was 13 commits behind it, so it kept re-running the broken configuration.

node --test test/env-http-proxy-agent.js on the rebased head: 37/37 passing.

No rush on the review — flagging this only so the red check is not read as a problem with the patch.

@pacocartones

Copy link
Copy Markdown
Contributor Author

Small correction to my note above: the rebase cleared CodeQL, but two test jobs are now red. They are not from this change, and I do not think they are from the rebase either.

Both failures are the same file — test/http2-request-never-settles.js — on Node 24 and 25 / ubuntu. This patch only touches lib/dispatcher/env-http-proxy-agent.js, its test and its doc; nothing in the h2 path.

The part worth flagging: main is failing on that same test right now. Run 31085361670 (push to main, 2026-08-06) fails on three jobs, and the log shows:

✖ /home/runner/work/undici/undici/test/http2-request-never-settles.js (3619.073804ms)

Locally on a clean main checkout (86b6299, no patch of mine applied) it passes 3 runs out of 3, both seeds. So it reads like a timing-sensitive churn test that goes red under CI load rather than a deterministic break — but since it is red on main, it is not something this PR can clear by itself.

node --test test/env-http-proxy-agent.js on this head is still 37/37.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants