fix(env-http-proxy-agent): ignore trailing dots when matching no_proxy - #5637
fix(env-http-proxy-agent): ignore trailing dots when matching no_proxy#5637pacocartones wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
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>
0d6ccfb to
0bc7658
Compare
|
Rebased onto current The reason for the rebase: CodeQL was red here, and it was not this change.
No rush on the review — flagging this only so the red check is not read as a problem with the patch. |
|
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 — The part worth flagging: Locally on a clean
|
This relates to...
No open issue. This is the same class of
no_proxymatching 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 existingdescribe('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.andexample.comare the same name.EnvHttpProxyAgentcompares host strings, and neither side of the comparison normalises that dot, so the two forms never match each other.Concretely, on
main:no_proxy=example.com, a request tohttp://example.com./(orhttps://, or with an explicit port, or tohttp://sub.example.com./) is routed through the proxy instead of going direct;no_proxy=example.com., that entry matches nothing at all — not evenhttp://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
searchdomain expansion — Kubernetes' own guidance is to usesvc.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:
Prior art. curl ignores a trailing dot on both sides of the
no_proxycomparison — 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#getProxyAgentForUrland#parseNoProxy.On framing: I am explicitly not presenting this as a security issue.
SECURITY.mdputs "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_proxycomparison now drop a single trailing dot before matching:#getProxyAgentForUrl— the request host, alongside the existing port-suffix and IPv6-bracket stripping.#parseNoProxy— eachno_proxyentry, 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:
test/issue-XXXX.jsreproduction. There is no issue here to number, andcreateEnvHttpProxyAgentWithMocksplus the wholedescribe('no_proxy')block already live intest/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.example.com..becomesexample.com.on both sides, so those still match each other consistently.length > 1leaves 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.new URL('http://127.0.0.1./').hostto127.0.0.1.new URL('http://[::1]./')throwsERR_INVALID_URL. The bare-IPv6 handling added in fix(env-http-proxy-agent): match bare IPv6 addresses in no_proxy #5623 is untouched.#getProxyAgentForUrlruns on every dispatch, so it uses acharCodeAtcheck 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).#parseNoProxyruns once perno_proxychange, not per request, so it keeps the regex style of the line it sits on.Features
N/A
Bug Fixes
EnvHttpProxyAgentnow honoursno_proxyfor hosts written in fully qualified form with a trailing dot (http://example.com./withno_proxy=example.com), including subdomains andhost:portentries.no_proxyentry 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 intohostname.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
Results on
main@ ae4a3e3, Node v24.18.1:test/env-http-proxy-agent.jshttp://example.com./)So each hunk is independently necessary and independently covered by an assertion.
npm run test:unitis 1489 tests / 0 failures / 4 skipped with the patch applied, andnpm run lintis 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.