Skip to content

Commit 393ec2b

Browse files
MarshallOfSoundMarshallOfSound
authored
feat: replace make-fetch-happen with built-in fetch (#3302)
* feat: replace make-fetch-happen with built-in fetch Use Node's built-in fetch for downloading headers/tarballs, with undici's EnvHttpProxyAgent providing --proxy, --noproxy and --cafile support (plus http_proxy/https_proxy/no_proxy env var handling). Drops 36 transitive dependencies. * fixup: address review feedback - Guard Readable.fromWeb against null body (204/304 responses) - Add socket error handlers to CONNECT tunnel in proxy test - Destroy socket in noproxy test's CONNECT handler so a regression fails fast instead of hanging * ci: fix Python lint and npm install in tests workflow Apply f-string fix from gyp-next#337 to resolve ruff F507 in simple_copy.py, and add npm@~11.10.0 pre-install step from #3300 to work around npm/cli#9151. * fix: apply cafile to proxied TLS connections EnvHttpProxyAgent forwards opts to an internal ProxyAgent for proxied requests, which reads origin TLS config from requestTls rather than connect. Set connect/requestTls/proxyTls so the custom CA is honored on both direct and proxied paths. Adds a test covering https origin behind an HTTP CONNECT proxy with a custom CA.
1 parent 19da158 commit 393ec2b

5 files changed

Lines changed: 133 additions & 19 deletions

File tree

‎.github/workflows/tests.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,9 @@ jobs:
6767
with:
6868
node-version: 22.x
6969
- name: Update npm
70-
run: npm install npm@latest -g
70+
run: |
71+
npm install npm@~11.10.0 -g # Workaround for https://github.com/npm/cli/issues/9151
72+
npm install npm@latest -g
7173
- name: Install Dependencies
7274
run: npm install
7375
- name: Pack

‎gyp/pylib/gyp/simple_copy.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ def deepcopy(x):
2424
return _deepcopy_dispatch[type(x)](x)
2525
except KeyError:
2626
raise Error(
27-
"Unsupported type %s for deepcopy. Use copy.deepcopy "
28-
+ "or expand simple_copy support." % type(x)
27+
f"Unsupported type {type(x)} for deepcopy. Use copy.deepcopy "
28+
+ "or expand simple_copy support."
2929
)
3030

3131

‎lib/download.js‎

Lines changed: 55 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
const fetch = require('make-fetch-happen')
1+
const { Readable } = require('stream')
2+
const { EnvHttpProxyAgent } = require('undici')
23
const { promises: fs } = require('graceful-fs')
34
const log = require('./log')
45

@@ -10,19 +11,65 @@ async function download (gyp, url) {
1011
'User-Agent': `node-gyp v${gyp.version} (node ${process.version})`,
1112
Connection: 'keep-alive'
1213
},
13-
proxy: gyp.opts.proxy,
14-
noProxy: gyp.opts.noproxy
14+
dispatcher: await createDispatcher(gyp)
1515
}
1616

17-
const cafile = gyp.opts.cafile
18-
if (cafile) {
19-
requestOpts.ca = await readCAFile(cafile)
17+
let res
18+
try {
19+
res = await fetch(url, requestOpts)
20+
} catch (err) {
21+
// Built-in fetch wraps low-level errors in "TypeError: fetch failed" with
22+
// the underlying error on .cause. Callers inspect .code (e.g. ENOTFOUND).
23+
if (err.cause) {
24+
throw err.cause
25+
}
26+
throw err
2027
}
2128

22-
const res = await fetch(url, requestOpts)
2329
log.http(res.status, res.url)
2430

25-
return res
31+
const body = res.body ? Readable.fromWeb(res.body) : Readable.from([])
32+
return {
33+
status: res.status,
34+
url: res.url,
35+
body,
36+
text: async () => {
37+
let data = ''
38+
body.setEncoding('utf8')
39+
for await (const chunk of body) {
40+
data += chunk
41+
}
42+
return data
43+
}
44+
}
45+
}
46+
47+
async function createDispatcher (gyp) {
48+
const env = process.env
49+
const hasProxyEnv = env.http_proxy || env.HTTP_PROXY || env.https_proxy || env.HTTPS_PROXY
50+
if (!gyp.opts.proxy && !gyp.opts.cafile && !hasProxyEnv) {
51+
return undefined
52+
}
53+
54+
const opts = {}
55+
if (gyp.opts.cafile) {
56+
const ca = await readCAFile(gyp.opts.cafile)
57+
// EnvHttpProxyAgent forwards opts to both its internal Agent (direct) and
58+
// ProxyAgent (proxied). Agent reads TLS config from `connect`; ProxyAgent
59+
// reads it from `requestTls` (origin) / `proxyTls` (proxy). Set all three
60+
// so the custom CA is applied regardless of which path a request takes.
61+
opts.connect = { ca }
62+
opts.requestTls = { ca }
63+
opts.proxyTls = { ca }
64+
}
65+
if (gyp.opts.proxy) {
66+
opts.httpProxy = gyp.opts.proxy
67+
opts.httpsProxy = gyp.opts.proxy
68+
}
69+
if (gyp.opts.noproxy) {
70+
opts.noProxy = gyp.opts.noproxy
71+
}
72+
return new EnvHttpProxyAgent(opts)
2673
}
2774

2875
async function readCAFile (filename) {

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,12 @@
2525
"env-paths": "^2.2.0",
2626
"exponential-backoff": "^3.1.1",
2727
"graceful-fs": "^4.2.6",
28-
"make-fetch-happen": "^15.0.0",
2928
"nopt": "^9.0.0",
3029
"proc-log": "^6.0.0",
3130
"semver": "^7.3.5",
3231
"tar": "^7.5.4",
3332
"tinyglobby": "^0.2.12",
33+
"undici": "^6.25.0",
3434
"which": "^6.0.0"
3535
},
3636
"engines": {

‎test/test-download.js‎

Lines changed: 72 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const fs = require('fs/promises')
66
const path = require('path')
77
const http = require('http')
88
const https = require('https')
9+
const net = require('net')
910
const install = require('../lib/install')
1011
const { download, readCAFile } = require('../lib/download')
1112
const { FULL_TEST, devDir, platformTimeout } = require('./common')
@@ -69,13 +70,24 @@ describe('download', function () {
6970
})
7071

7172
it('download over http with proxy', async function () {
72-
const server = http.createServer((_, res) => {
73+
const server = http.createServer((req, res) => {
74+
assert.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
7375
res.end('ok')
7476
})
7577

76-
const pserver = http.createServer((req, res) => {
77-
assert.strictEqual(req.headers['user-agent'], `node-gyp v42 (node ${process.version})`)
78-
res.end('proxy ok')
78+
let proxyUsed = false
79+
const pserver = http.createServer()
80+
pserver.on('connect', (req, clientSocket, head) => {
81+
proxyUsed = true
82+
const [targetHost, targetPort] = req.url.split(':')
83+
const serverSocket = net.connect(targetPort, targetHost, () => {
84+
clientSocket.write('HTTP/1.1 200 Connection Established\r\n\r\n')
85+
serverSocket.write(head)
86+
serverSocket.pipe(clientSocket)
87+
clientSocket.pipe(serverSocket)
88+
})
89+
clientSocket.on('error', () => serverSocket.destroy())
90+
serverSocket.on('error', () => clientSocket.destroy())
7991
})
8092

8193
after(() => Promise.all([
@@ -96,7 +108,56 @@ describe('download', function () {
96108
}
97109
const url = `http://${host}:${port}`
98110
const res = await download(gyp, url)
99-
assert.strictEqual(await res.text(), 'proxy ok')
111+
assert.strictEqual(await res.text(), 'ok')
112+
assert.strictEqual(proxyUsed, true)
113+
})
114+
115+
it('download over https with proxy and custom ca', async function () {
116+
const cafile = path.join(__dirname, 'fixtures/ca-proxy.crt')
117+
await fs.writeFile(cafile, certs['ca.crt'], 'utf8')
118+
119+
const server = https.createServer({
120+
ca: await readCAFile(cafile),
121+
cert: certs['server.crt'],
122+
key: certs['server.key']
123+
}, (_, res) => res.end('ok'))
124+
125+
let proxyUsed = false
126+
const pserver = http.createServer()
127+
pserver.on('connect', (req, clientSocket, head) => {
128+
proxyUsed = true
129+
const [targetHost, targetPort] = req.url.split(':')
130+
const serverSocket = net.connect(targetPort, targetHost, () => {
131+
clientSocket.write('HTTP/1.1 200 Connection Established\r\n\r\n')
132+
serverSocket.write(head)
133+
serverSocket.pipe(clientSocket)
134+
clientSocket.pipe(serverSocket)
135+
})
136+
clientSocket.on('error', () => serverSocket.destroy())
137+
serverSocket.on('error', () => clientSocket.destroy())
138+
})
139+
140+
after(async () => {
141+
await new Promise((resolve) => server.close(resolve))
142+
await new Promise((resolve) => pserver.close(resolve))
143+
await fs.unlink(cafile)
144+
})
145+
146+
const host = 'localhost'
147+
await new Promise((resolve) => server.listen(0, host, resolve))
148+
const { port } = server.address()
149+
await new Promise((resolve) => pserver.listen(port + 1, host, resolve))
150+
const gyp = {
151+
opts: {
152+
cafile,
153+
proxy: `http://${host}:${port + 1}`,
154+
noproxy: 'bad'
155+
},
156+
version: '42'
157+
}
158+
const res = await download(gyp, `https://${host}:${port}`)
159+
assert.strictEqual(await res.text(), 'ok')
160+
assert.strictEqual(proxyUsed, true)
100161
})
101162

102163
it('download over http with noproxy', async function () {
@@ -105,8 +166,11 @@ describe('download', function () {
105166
res.end('ok')
106167
})
107168

108-
const pserver = http.createServer((_, res) => {
109-
res.end('proxy ok')
169+
let proxyUsed = false
170+
const pserver = http.createServer()
171+
pserver.on('connect', (_, socket) => {
172+
proxyUsed = true
173+
socket.destroy()
110174
})
111175

112176
after(() => Promise.all([
@@ -128,6 +192,7 @@ describe('download', function () {
128192
const url = `http://${host}:${port}`
129193
const res = await download(gyp, url)
130194
assert.strictEqual(await res.text(), 'ok')
195+
assert.strictEqual(proxyUsed, false)
131196
})
132197

133198
it('download with missing cafile', async function () {

0 commit comments

Comments
 (0)