Skip to content

perf(AUTO-1063): skip parsing hostnames and cache verdicts in isAddressRestricted - #1571

Merged
Kart03 merged 4 commits into
developfrom
claude/auto-1063-rca-05deed
Oct 30, 2026
Merged

Kart03 merged 4 commits into
developfrom
claude/auto-1063-rca-05deed

Conversation

@Kart03

@Kart03 Kart03 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Context

AUTO-1063: per-VU throughput of our cloud performance runs dropped after the restricted-address policy was turned on (#1548 added CIDR support; the performance library started passing a 19-entry policy). On Graviton (c6g) at 25,000 VUs, turning the policy on costs about 6.3% per-VU throughput on a 100 ms endpoint and 6.9% on a 50 ms endpoint (policy off vs on, load test below). A slow endpoint (1,000 ms) shows no difference.

isAddressRestricted runs on every request for the URL's hostname (requester.js:434), and again on each newly resolved address inside the DNS lookup override (core.js:309, core.js:322). I measured each call separately on our load workers: switching off only the hostname check recovers almost all of it (6.0 of 6.4 points in that experiment); switching off only the resolved-address check changes nothing measurable (it runs once per new connection, and connections are kept alive).

The cost is ipaddr.js throwing:

  • For a hostname, ipaddr.process() throws "neither IPv6 nor IPv4 format", and the catch at the bottom of the function turns that into false. Building the error (stack trace) is about 7 µs, on every request.
  • For an IPv4 address, addr.match() throws on each IPv6 range in the policy ("cannot match ipv4 address with non-ipv4 one"). With 3 IPv6 ranges a public IPv4 throws 3 times, about 9.4 µs per call.

Summary of the change

All inside isAddressRestricted in lib/requester/core.js. The policy, the call sites, the DNS-time check and the redirect check are unchanged.

  1. Hostname pre-check. After the exact-match lookup, a host with no : and a character outside [0-9a-fx.] returns false without parsing. Every IPv4 form ipaddr.js 2.x accepts (dotted decimal, octal, 0x hex, short and single-number forms) uses only those characters, and ipaddr.js rejects every colon-free IPv6 form (its IPv6 regex matches strings like deadbeef and 1%eth0, but its part-count check rejects them), so such a host can never match a CIDR entry; the parse would only throw. A generated-corpus unit test asserts this against ipaddr.js, so an upgrade that widens its IPv4 grammar (e.g. 0o octal) fails CI. It stays after the exact-match block so listed names like localhost are still blocked. Anything IP-shaped (0x7f.0.0.1, 2130706433, fe80::1%eth0) still takes the full path, which matters because Node skips the lookup hook for canonical dotted-quad and bare IPv6 hosts (net.isIP ≠ 0), so for those the hostname check is the only guard.
  2. Verdict cache. Verdicts are remembered per lower-cased host string on networkOptions.restrictedVerdicts (a Map capped at 1,000 entries, evicting the least recently used). This needs a stronger assumption than the one the function already makes for restrictedCidrs: a cache hit also skips the exact-match check, so restrictedAddresses must not change at all after the first call, exact-match entries included. The cache is keyed by the exact string checked, never by where a name resolves to, so when DNS starts returning a different address for a name, the resolved-address check sees the new address string and checks it on its own (there is a test for this).

Breaking changes

None for callers. Verdicts are identical for every input in the new and existing tests. networkOptions gains a restrictedVerdicts property, written lazily like restrictedCidrs. An embedder that builds networkOptions from untrusted input should not let that input set restrictedVerdicts (the same applies to restrictedCidrs today); the performance library strips both, and I'll make that explicit in its own PR.

Dependencies

None.

Test plan

  • New unit tests in test/unit/requester-core.test.js (hostname pre-check and verdict cache):

    • 21 blocked and 15 allowed inputs keep the verdicts recorded from the current implementation (hex/octal/short IPv4, IPv4-mapped and IPv4-compatible IPv6, NAT64, 6to4, bracketed IPv6, zone IDs, uppercase, trailing dots, names, localhost).
    • Hostnames never reach ipaddr.process (spy). IP-shaped strings always do.
    • A repeat check with the same networkOptions does not parse again.
    • DNS rebinding: name allowed, first address allowed, then a new blocked address is blocked.
    • Cache stays at or under 1,000 entries after 1,500 distinct addresses and verdicts stay correct, and evicts the least recently used entry.
    • Generated corpus: every colon-free string ipaddr.js accepts still reaches the parser. Patching ipaddr.js to accept 0o octal parts makes this test fail.
    • Before the change, 4 of these 12 fail (no-parse, no-reparse, bounded cache, LRU eviction) and 8 pass; after, all pass.
  • Suites: node npm/test-unit.js 583 passing, 10 pending, 0 failing. node npm/test-integration.js 1,915 passing, 33 pending, 0 failing (includes test/integration/sanity/restricted-addresses.test.js). ESLint clean on both files.

  • Microbenchmark (ns per call, the performance library's 19-CIDR policy, Node 24.14, median of 5 × 200k calls, same verdict before and after):

    Input Before After, repeat After, first call
    Hostname 7,400 ns 95 ns 147 ns
    Public IPv4 10,600 ns 32 ns 11,000 ns
    Private IPv4 6,900 ns 30 ns 7,500 ns
    Loopback 327 ns 31 ns 384 ns

    The first check of an IP literal is no faster (slightly slower, from the cache insert): its cost is the addr.match() throws on IPv6 ranges, which only the cache avoids on repeats.

  • Load test (our load workers, a performance library build that vendors this branch):

    Per-VU requests/second, 100 ms and 50 ms endpoints, fixed profile, 5 min, one worker image with only this change versus the same image without it. 0% errors in every run.

    Graviton (c6g), 25,000 VUs. Workers peak at 92-94% CPU:

    Endpoint Without fix With fix Policy off With fix vs without
    100 ms 1.039, 1.031 1.099, 1.098 1.105 +6.1%
    50 ms 1.058, 1.057 1.131, 1.142 1.136 +7.5%

    AMD (c8a). Workers peak at 74-78% CPU, so there is less to win:

    Endpoint VUs Without fix With fix Change
    100 ms 25,000 3.430 3.443 +0.4%
    50 ms 25,000 3.890 3.964 +1.9%
    100 ms 100,000 2 pairs, total requests 101.6M / 99.0M and 98.2M / 98.1M no measurable difference
    50 ms 100,000 3.712 3.865 +4.1%

    On c6g the fix lands within 0.6% of running with the policy switched off. At 100,000 VUs I compare total requests, because the reported requests/second uses a measurement window that varied between 284 s and 301 s.

Additional notes

  • Base is develop.
  • This touches an SSRF guard, so I'd like a security review in addition to the usual one. The parts worth reading closely are the pre-check condition and its placement after the exact-match block.
  • Not in this PR: matching an address only against ranges of its own family, which would remove the remaining throws for the first check of an IP. The cache already makes repeats cheap, and the load test (hostname targets) shows the fix recovers the full cost, so I'll only add it if IP-literal targets turn out to matter.

Generated with Claude Code

…ssRestricted

A hostname made ipaddr.process() throw on every request (~7 us for the
stack trace), and an IPv4 address threw once per IPv6 range in the
policy. Return early for hosts that cannot be an IP literal, and
remember verdicts per host string on networkOptions.restrictedVerdicts
(bounded at 1,000 entries). Verdicts are unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.94%. Comparing base (4e40645) to head (a6bfb79).
⚠️ Report is 3 commits behind head on develop.

❗ There is a different number of reports uploaded between BASE (4e40645) and HEAD (a6bfb79). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (4e40645) HEAD (a6bfb79)
integration 1 0
legacy 1 0
Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #1571       +/-   ##
============================================
- Coverage    77.61%   46.94%   -30.68%     
============================================
  Files           51       51               
  Lines         4361     4375       +14     
  Branches      1234     1238        +4     
============================================
- Hits          3385     2054     -1331     
- Misses         732     2156     +1424     
+ Partials       244      165       -79     
Flag Coverage Δ
integration ?
legacy ?
unit 46.94% <100.00%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@Kart03
Kart03 marked this pull request as ready for review October 23, 2026 07:18
Comment thread lib/requester/core.js Outdated
* @returns {Boolean} the verdict, so callers can return it directly
*/
rememberAddressVerdict = function (verdicts, host, verdict) {
verdicts.size >= MAX_CACHED_ADDRESS_VERDICTS && verdicts.clear();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Clearing the entire cache on overflow evicts frequently used IP verdicts, bringing back parsing and CIDR matching for active destinations. AUTO-1063 calls for a bounded LRU: evict the least recently used entry and refresh recency on hits.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in a6bfb79. Hits now re-insert the entry (most recently used) and overflow evicts the oldest key instead of clearing the cache. Costs about 33 ns more per hit against the ~7 µs throw it replaces. Added a test that a touched entry survives overflow and the untouched oldest one is evicted.

Kart03 and others added 2 commits October 26, 2026 12:45
…t instead of clearing the cache

Refresh recency on a hit and evict the oldest entry on overflow, so frequently used verdicts survive when the cache fills.

Co-Authored-By: Claude <noreply@anthropic.com>
…v4 form ipaddr.js accepts

Generated-corpus property test: every colon-free string ipaddr.js
accepts must reach the parser, so an ipaddr.js bump that widens its
IPv4 grammar (e.g. 0o octal) fails CI instead of opening a bypass.
Also corrects the pre-check and verdict-cache comments.

Co-Authored-By: Claude <noreply@anthropic.com>
@Kart03
Kart03 merged commit 9942278 into develop Oct 30, 2026
13 of 15 checks passed
@Kart03
Kart03 deleted the claude/auto-1063-rca-05deed branch October 30, 2026 08:36
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.

2 participants