fix(socks5): enforce authenticated state before CONNECT (#5097)

trivikr · web-flow · commit 1dec8810cd9c · 2026-04-24T07:19:40.000+02:00
diff --git a/lib/core/socks5-client.js b/lib/core/socks5-client.js
@@ -51,6 +51,7 @@ const STATES = {
   INITIAL: 'initial',
   HANDSHAKING: 'handshaking',
   AUTHENTICATING: 'authenticating',
+  AUTHENTICATED: 'authenticated',
   CONNECTING: 'connecting',
   CONNECTED: 'connected',
   ERROR: 'error',
@@ -139,6 +140,11 @@ class Socks5Client extends EventEmitter {
     }
   }
 
+  markAuthenticated () {
+    this.state = STATES.AUTHENTICATED
+    this.emit('authenticated')
+  }
+
   /**
    * Start the SOCKS5 handshake
    */
@@ -189,7 +195,7 @@ class Socks5Client extends EventEmitter {
     debug('server selected auth method', method)
 
     if (method === AUTH_METHODS.NO_AUTH) {
-      this.emit('authenticated')
+      this.markAuthenticated()
     } else if (method === AUTH_METHODS.USERNAME_PASSWORD) {
       this.state = STATES.AUTHENTICATING
       this.sendAuthRequest()
@@ -254,7 +260,7 @@ class Socks5Client extends EventEmitter {
 
     this.buffer = this.buffer.subarray(2)
     debug('authentication successful')
-    this.emit('authenticated')
+    this.markAuthenticated()
   }
 
   /**
@@ -263,8 +269,12 @@ class Socks5Client extends EventEmitter {
    * @param {number} port - Target port
    */
   connect (address, port) {
-    if (this.state === STATES.CONNECTED) {
-      throw new InvalidArgumentError('Already connected')
+    if (this.state === STATES.CONNECTING || this.state === STATES.CONNECTED) {
+      throw new InvalidArgumentError('Connection already in progress')
+    }
+
+    if (this.state !== STATES.AUTHENTICATED) {
+      throw new InvalidArgumentError('Client must be authenticated before CONNECT')
     }
 
     debug('connecting to', address, port)
diff --git a/lib/dispatcher/socks5-proxy-agent.js b/lib/dispatcher/socks5-proxy-agent.js
@@ -6,7 +6,7 @@ const { URL } = require('node:url')
 let tls // include tls conditionally since it is not always available
 const DispatcherBase = require('./dispatcher-base')
 const { InvalidArgumentError } = require('../core/errors')
-const { Socks5Client } = require('../core/socks5-client')
+const { Socks5Client, STATES } = require('../core/socks5-client')
 const { kDispatch, kClose, kDestroy } = require('../core/symbols')
 const Pool = require('./pool')
 const buildConnector = require('../core/connect')
@@ -133,7 +133,7 @@ class Socks5ProxyAgent extends DispatcherBase {
     }
 
     // Check if already authenticated (for NO_AUTH method)
-    if (socks5Client.state === 'authenticated') {
+    if (socks5Client.state === STATES.AUTHENTICATED) {
       clearTimeout(authenticationTimeout)
       authenticationReady.resolve()
     } else {
diff --git a/test/socks5-client.js b/test/socks5-client.js
@@ -51,15 +51,15 @@ test('Socks5Client - handshake flow', async (t) => {
   p.equal(client.state, STATES.INITIAL, 'should start in INITIAL state')
 
   client.on('authenticated', () => {
-    p.equal(client.state, STATES.HANDSHAKING, 'should be in HANDSHAKING state after auth')
+    p.equal(client.state, STATES.AUTHENTICATED, 'should be in AUTHENTICATED state after auth')
     p.ok(true, 'should emit authenticated event')
   })
 
   await client.handshake()
 
   // Wait for the authenticated event
   await new Promise((resolve) => {
-    if (client.state !== STATES.HANDSHAKING) {
+    if (client.state === STATES.AUTHENTICATED) {
       resolve()
     } else {
       client.once('authenticated', resolve)
@@ -72,6 +72,37 @@ test('Socks5Client - handshake flow', async (t) => {
   await p.completed
 })
 
+test('Socks5Client - connect requires authenticated state', async (t) => {
+  const p = tspl(t, { plan: 2 })
+
+  class MockSocket {
+    constructor () {
+      this.destroyed = false
+      this.writes = []
+    }
+
+    on () {}
+
+    write (chunk) {
+      this.writes.push(chunk)
+    }
+
+    destroy () {
+      this.destroyed = true
+    }
+  }
+
+  const socket = new MockSocket()
+  const client = new Socks5Client(socket)
+
+  p.throws(() => {
+    client.connect('example.com', 80)
+  }, InvalidArgumentError, 'should reject connect before authentication')
+  p.equal(socket.writes.length, 0, 'should not write CONNECT before authentication')
+
+  await p.completed
+})
+
 test('Socks5Client - username/password authentication', async (t) => {
   const p = tspl(t, { plan: 7 })
 
