Skip to content

Commit 3805b8f

Browse files
deepview-autofixclaudeChALkeR
authored andcommitted
fix(socks5-proxy-agent): use per-origin pools to prevent cross-origin routing (#5041)
The Socks5ProxyAgent stored a single Pool keyed implicitly to the first origin it saw and reused it for every subsequent request. When a second request targeted a different origin, it was dispatched through the existing pool whose connect callback tunnelled to the original origin, causing the request to reach the wrong host (and potentially leaking headers/credentials intended for origin B to origin A). Track pools in a Map keyed by origin so each origin gets its own pool and SOCKS5 tunnel. Signed-off-by: Nikita Skovoroda <chalkerx@gmail.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Nikita Skovoroda <chalkerx@gmail.com> (cherry picked from commit a516f87)
1 parent 85a2405 commit 3805b8f

2 files changed

Lines changed: 63 additions & 12 deletions

File tree

lib/dispatcher/socks5-proxy-agent.js

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ const debug = debuglog('undici:socks5-proxy')
1717
const kProxyUrl = Symbol('proxy url')
1818
const kProxyHeaders = Symbol('proxy headers')
1919
const kProxyAuth = Symbol('proxy auth')
20-
const kPool = Symbol('pool')
20+
const kPools = Symbol('pools')
2121
const kConnector = Symbol('connector')
2222

2323
// Static flag to ensure warning is only emitted once per process
@@ -65,8 +65,8 @@ class Socks5ProxyAgent extends DispatcherBase {
6565
servername: options.proxyTls?.servername || url.hostname
6666
})
6767

68-
// Pool for the actual HTTP connections (with SOCKS5 tunnel connect function)
69-
this[kPool] = null
68+
// Pools for the actual HTTP connections (with SOCKS5 tunnel connect function), keyed by origin
69+
this[kPools] = new Map()
7070
}
7171

7272
/**
@@ -177,9 +177,11 @@ class Socks5ProxyAgent extends DispatcherBase {
177177
debug('dispatching request to', origin, 'via SOCKS5')
178178

179179
try {
180-
// Create Pool with custom connect function if we don't have one yet
181-
if (!this[kPool] || this[kPool].destroyed || this[kPool].closed) {
182-
this[kPool] = new Pool(origin, {
180+
const originKey = String(origin)
181+
let pool = this[kPools].get(originKey)
182+
// Create a Pool per origin so requests are not routed to the wrong host
183+
if (!pool || pool.destroyed || pool.closed) {
184+
pool = new Pool(origin, {
183185
pipelining: opts.pipelining,
184186
connections: opts.connections,
185187
connect: async (connectOpts, callback) => {
@@ -219,10 +221,11 @@ class Socks5ProxyAgent extends DispatcherBase {
219221
}
220222
}
221223
})
224+
this[kPools].set(originKey, pool)
222225
}
223226

224-
// Dispatch the request through the pool
225-
return this[kPool][kDispatch](opts, handler)
227+
// Dispatch the request through the per-origin pool
228+
return pool[kDispatch](opts, handler)
226229
} catch (err) {
227230
debug('dispatch error:', err)
228231
if (typeof handler.onError === 'function') {
@@ -234,15 +237,21 @@ class Socks5ProxyAgent extends DispatcherBase {
234237
}
235238

236239
async [kClose] () {
237-
if (this[kPool]) {
238-
await this[kPool].close()
240+
const closePromises = []
241+
for (const pool of this[kPools].values()) {
242+
closePromises.push(pool.close())
239243
}
244+
this[kPools].clear()
245+
await Promise.all(closePromises)
240246
}
241247

242248
async [kDestroy] (err) {
243-
if (this[kPool]) {
244-
await this[kPool].destroy(err)
249+
const destroyPromises = []
250+
for (const pool of this[kPools].values()) {
251+
destroyPromises.push(pool.destroy(err))
245252
}
253+
this[kPools].clear()
254+
await Promise.all(destroyPromises)
246255
}
247256
}
248257

test/socks5-proxy-agent.js

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,48 @@ test('Socks5ProxyAgent - multiple requests through same proxy', async (t) => {
225225
await p.completed
226226
})
227227

228+
test('Socks5ProxyAgent - requests to different origins are routed correctly', async (t) => {
229+
const p = tspl(t, { plan: 4 })
230+
231+
// Create two distinct target servers
232+
const serverA = createServer((req, res) => {
233+
res.writeHead(200, { 'content-type': 'application/json' })
234+
res.end(JSON.stringify({ server: 'A', path: req.url }))
235+
})
236+
const serverB = createServer((req, res) => {
237+
res.writeHead(200, { 'content-type': 'application/json' })
238+
res.end(JSON.stringify({ server: 'B', path: req.url }))
239+
})
240+
241+
await new Promise((resolve) => serverA.listen(0, '127.0.0.1', resolve))
242+
await new Promise((resolve) => serverB.listen(0, '127.0.0.1', resolve))
243+
const portA = serverA.address().port
244+
const portB = serverB.address().port
245+
246+
const socksServer = new TestSocks5Server()
247+
const socksAddress = await socksServer.listen()
248+
249+
try {
250+
const proxyWrapper = new Socks5ProxyAgent(`socks5://127.0.0.1:${socksAddress.port}`)
251+
252+
// First request goes to server A — establishes a pool
253+
const respA = await request(`http://127.0.0.1:${portA}/a`, { dispatcher: proxyWrapper })
254+
p.equal(respA.statusCode, 200)
255+
p.deepEqual(await respA.body.json(), { server: 'A', path: '/a' })
256+
257+
// Second request goes to server B — must NOT reuse the pool from origin A
258+
const respB = await request(`http://127.0.0.1:${portB}/b`, { dispatcher: proxyWrapper })
259+
p.equal(respB.statusCode, 200)
260+
p.deepEqual(await respB.body.json(), { server: 'B', path: '/b' }, 'request to origin B must reach server B, not server A')
261+
} finally {
262+
await socksServer.close()
263+
serverA.close()
264+
serverB.close()
265+
}
266+
267+
await p.completed
268+
})
269+
228270
test('Socks5ProxyAgent - connection failure', async (t) => {
229271
const p = tspl(t, { plan: 1 })
230272

0 commit comments

Comments
 (0)