-
Notifications
You must be signed in to change notification settings - Fork 693
grpc-js: Parsing logic of no_grpc_proxy/no_proxy variable(s) is flawed #3039
Copy link
Copy link
Open
Description
Problem description
The logic for checking whether a host matches the no proxy list in hostMatchesNoProxyList is flawed due to the naive endsWith(host) logic.
Reproduction steps
Note these reproduction steps copy the function logic and have no dependency on the package as the relevant internal functions aren't publicly exposed. Parts unrelated to the bug with external dependencies are commented out and a version with a proposed fix is given (i.e. hostMatchesNoProxyListV2). This is done for ease of reproducibility.
Dockerfile:
FROM node:24-slim
WORKDIR /reproduce
RUN npm init -y
COPY reproduce-bug.js .
RUN node reproduce-bug.jsDocker compose:
services:
bug-reproduce:
build:
context: .
dockerfile: Dockerfile
container_name: bug_reproduce
command: node reproduce-bug.jsreproduce-bug.js:
// Mocking the environment
process.env.https_proxy = 'http://proxy.local:3128';
process.env.no_proxy = 'local.com,apple.com';
// Existing grpc-js function
function getNoProxyHostList() {
/* Prefer using 'no_grpc_proxy'. Fallback on 'no_proxy' if it is not set. */
let noProxyStr = process.env.no_grpc_proxy;
let envVar = 'no_grpc_proxy';
if (!noProxyStr) {
noProxyStr = process.env.no_proxy;
envVar = 'no_proxy';
}
if (noProxyStr) {
// trace('No proxy server list set by environment variable ' + envVar);
return noProxyStr.split(',');
} else {
return [];
}
}
// Exsting grpc-js function with bug, modified for ease of reproducibility
function hostMatchesNoProxyList(serverHost) {
for (const host of getNoProxyHostList()) {
// const parsedCIDR = parseCIDR(host);
// host is a CIDR and serverHost is an IP address
// if (isIPv4(serverHost) && parsedCIDR && isIpInCIDR(parsedCIDR, serverHost)) {
// return true;
// } else if (serverHost.endsWith(host)) {
// // host is a single IP or a domain name suffix
// return true;
// }
if (serverHost.endsWith(host)) {
console.log(`[checking against (${host})]: ${serverHost} MATCHES the no proxy host`)
return true
}
}
console.log(`${serverHost} DOES NOT MATCH any of the no proxy hosts`)
return false;
}
// Modified function with proposed fix
function hostMatchesNoProxyListV2(serverHost) {
for (const host of getNoProxyHostList()) {
// const parsedCIDR = parseCIDR(host);
// host is a CIDR and serverHost is an IP address
// if (isIPv4(serverHost) && parsedCIDR && isIpInCIDR(parsedCIDR, serverHost)) {
// return true;
// } else if (serverHost.endsWith(host)) {
// // host is a single IP or a domain name suffix
// return true;
// }
// Proposed fix
if (serverHost === host) return true;
if (serverHost.endsWith('.' + host)) return true;
if (host.startsWith('.') && serverHost.endsWith(host)) return true;
}
console.log(`${serverHost} DOES NOT MATCH any of the no proxy hosts`)
return false;
}
const testHosts = [
'local.com', // Correctly matches
'www.local.com', // Correctly matches
'notlocal.com', // BUG: Should NOT match
'www.notlocal.com', // BUG: Should NOT match
'www.apple.com', // Correctly matches
'www.snapple.com' // BUG: Should NOT match
];
console.log(`NO_PROXY is set to: ${process.env.no_proxy}\n`);
testHosts.forEach(host => {
hostMatchesNoProxyList(host);
});
// test proposed fix
// testHosts.forEach(host => {
// hostMatchesNoProxyListV2(host);
// });Run docker compose up --build
Output :
[checking against (local.com)]: local.com MATCHES the no proxy host
[checking against (local.com)]: www.local.com MATCHES the no proxy host
[checking against (local.com)]: notlocal.com MATCHES the no proxy host <-- BUG
[checking against (local.com)]: www.notlocal.com MATCHES the no proxy host <-- BUG
[checking against (apple.com)]: www.apple.com MATCHES the no proxy host
[checking against (apple.com)]: www.snapple.com MATCHES the no proxy host <--- BUG
Environment
N/A - containerized example given
Additional context
The man pages for curl lay out the logic that is most commonly expected:
Comma-separated list of hosts for which not to use a proxy, if one is specified. The only wildcard is a single "*" character, which matches all hosts, and effectively disables the proxy. Each name in this list is matched as either a domain which contains the hostname, or the hostname itself. For example, "local.com" would match "local.com", "local.com:80", and "www.local.com", but not "www.notlocal.com".
This option overrides the environment variables that disable the proxy ("no_proxy" and "NO_PROXY"). If there is an environment variable disabling a proxy, you can set the no proxy list to "" to override it.
IP addresses specified to this option can be provided using CIDR notation (added in 7.86.0): an appended slash and number specifies the number of network bits out of the address to use in the comparison. For example "192.168.0.0/16" would match all addresses starting with "192.168".
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels