ProxyAgent URI parser and validator is broken

[timursevimli]

Bug Description

Checking the string uri in the ProxyAgent class does not work consistently. It may consider some non-existent IP addresses valid and sometimes not valid. In addition, in some cases it can change the IP address that was transmitted via string uri to ProxyAgent.

Reproducible By

'use strict';

const { fetch, ProxyAgent } = require('undici');

const URL = 'https://google.com';

const proxyUri1 = 'http://123.123.0123.123:8080'; // Valid for ProxyAgent
const proxyUri2 = 'http://0123.123.123.123:8080'; // Valid too
const proxyUri3 = 'http://085.26.146.169:8080'; // Invalid
const proxyUri4 = 'http://85.026.0146.169:8080'; // Valid too but convert ip to 85.22.102.169 ??

const proxyAgent = new ProxyAgent(proxyUri4);

(async () => {
  const res = await fetch(URL, { dispatcher: proxyAgent });
  console.log(await res.text());
})();

Environment

macOS Sonoma 14.3.1, Node.js v20.11.0, undici@6.6.2

Additional

Example of error output using proxyUri3 variable

node:internal/url:775
    this.#updateContext(bindingUrl.parse(input, base));
                                   ^

TypeError: Invalid URL
    at new URL (node:internal/url:775:36)
    at new ProxyAgent (/Users/timursevimli/Programming/Projects/deneme/node_modules/undici/lib/proxy-agent.js:68:25)
    at Object.<anonymous> (/Users/timursevimli/Programming/Projects/deneme/issue.js:12:20)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Module.load (node:internal/modules/cjs/loader:1207:32)
    at Module._load (node:internal/modules/cjs/loader:1023:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:135:12)
    at node:internal/main/run_main_module:28:49 {
  code: 'ERR_INVALID_URL',
  input: 'http://085.26.146.169:8080'
}

Node.js v20.11.0

Example of error output using proxyUri4 variable

/Users/timursevimli/Programming/Projects/deneme/node_modules/undici/index.js:103
      Error.captureStackTrace(err, this)
            ^

TypeError: fetch failed
    at fetch (/Users/timursevimli/Programming/Projects/deneme/node_modules/undici/index.js:103:13)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async /Users/timursevimli/Programming/Projects/deneme/issue.js:15:15 {
  [cause]: Error: connect ECONNREFUSED 85.22.102.169:8080
      at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1595:16) {
    errno: -61,
    code: 'ECONNREFUSED',
    syscall: 'connect',
    address: '85.22.102.169',
    port: 8080
  }
}

Node.js v20.11.0

[Uzlopak]

The leading 0 in an ipv4 octet means that the number is a octal value. Thats why 026 is converted to decimal 22. The same with 085. The notation is defined somewhere in the tcp/ip specification.
Nodejs itself does not accept octal values for ipv4 octets. Check is isIPv4 from node:net.

Maybe we should use isIPv4 from node:net and validate the IP if somebody passes octal notation?!

[rossilor95]

Hey @timursevimli! Under the hood, ProxyAgent uses the Node implementation of the WHATWG URL object (reference).

Unfortunately, the inconsistent behaviours you experienced can be replicated in the JS console of any browser or in any Node.js environment. For example:

const url3 = new URL('http://085.26.146.169:8080');
const url4 = new URL('http://85.026.0146.169:8080');

[rossilor95]

@Uzlopak I can work on this. In your opinion, it would be better to update parseURL from core/util and use it in ProxyAgent to parse input URIs or we just add a check in ProxyAgent?

[timursevimli]

The leading 0 in an ipv4 octet means that the number is a octal value. Thats why 026 is converted to decimal 22. The same with 085. The notation is defined somewhere in the tcp/ip specification. Nodejs itself does not accept octal values for ipv4 octets. Check is isIPv4 from node:net.

Maybe we should use isIPv4 from node:net and validate the IP if somebody passes octal notation?!

Thanks for the advice. I already use the isIPv4 check from node:net before calling the ProxyAgent constructor.

However, it seems to me that this check could be included in the ProxyAgent constructor, as you also suggested. This way, we can prevent incorrect connections in cases where IP address validation is not used.

[metcoder95]

Given that WHATWG URL already covers that, I'm not sure there's something else that should be done for this, as the error in the sense of being a bad URL.
Potentially, a documentation update will be better as ProxyAgent already parses the provided URL into a WHATWG one, which will lead to this error.

[metcoder95]

Closed by #2893