Skip to content

Commit a516f87

Browse files
deepview-autofixclaudeChALkeR
authored
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>
1 parent b935c91 commit a516f87

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
/**
@@ -183,9 +183,11 @@ class Socks5ProxyAgent extends DispatcherBase {
183183
debug('dispatching request to', origin, 'via SOCKS5')
184184

185185
try {
186-
// Create Pool with custom connect function if we don't have one yet
187-
if (!this[kPool] || this[kPool].destroyed || this[kPool].closed) {
188-
this[kPool] = new Pool(origin, {
186+
const originKey = String(origin)
187+
let pool = this[kPools].get(originKey)
188+
// Create a Pool per origin so requests are not routed to the wrong host
189+
if (!pool || pool.destroyed || pool.closed) {
190+
pool = new Pool(origin, {
189191
pipelining: opts.pipelining,
190192
connections: opts.connections,
191193
connect: async (connectOpts, callback) => {
@@ -225,10 +227,11 @@ class Socks5ProxyAgent extends DispatcherBase {
225227
}
226228
}
227229
})
230+
this[kPools].set(originKey, pool)
228231
}
229232

230-
// Dispatch the request through the pool
231-
return this[kPool][kDispatch](opts, handler)
233+
// Dispatch the request through the per-origin pool
234+
return pool[kDispatch](opts, handler)
232235
} catch (err) {
233236
debug('dispatch error:', err)
234237
if (typeof handler.onError === 'function') {
@@ -240,15 +243,21 @@ class Socks5ProxyAgent extends DispatcherBase {
240243
}
241244

242245
async [kClose] () {
243-
if (this[kPool]) {
244-
await this[kPool].close()
246+
const closePromises = []
247+
for (const pool of this[kPools].values()) {
248+
closePromises.push(pool.close())
245249
}
250+
this[kPools].clear()
251+
await Promise.all(closePromises)
246252
}
247253

248254
async [kDestroy] (err) {
249-
if (this[kPool]) {
250-
await this[kPool].destroy(err)
255+
const destroyPromises = []
256+
for (const pool of this[kPools].values()) {
257+
destroyPromises.push(pool.destroy(err))
251258
}
259+
this[kPools].clear()
260+
await Promise.all(destroyPromises)
252261
}
253262
}
254263

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)