feat: add intrinsic support for SOCKS proxies #1966

DirtyHairy · 2025-02-05T17:44:14Z

This PR adds support for establishing TCP and TLS connections through a SOCKS proxy. Proxy support is enabled with a new socks configuration parameter that takes a curl-style SOCKS URL, or by setting an environment variable (MQTTJS_SOCKS_PROXY).

Supported schemes (and protocols) are socks5:// , socks5h:// , socks4:// and socks4a:// .

robertsLando · 2025-02-06T09:23:42Z

src/lib/connect/index.ts

+	if (isBrowser || opts.unixSocket) {
+		opts.socks = undefined
+	}
+
+	if (
+		!isBrowser &&
+		!opts.unixSocket &&
+		opts.socks === undefined &&
+		typeof process !== 'undefined'
+	) {
+		opts.socks = process.env['MQTTJS_SOCKS_PROXY']
+	}


why? Could you remove the env var here?

I added the environment variable in order to provide an easy way to route unmodified applications that use mqtt.js through a proxy. In or case the culprit is Node-RED.

It would be definitely preferable if these had support for using a proxy themselves, but configuring a proxy is one of those networking options where it is not uncommon to offer configuration through the environment.

src/lib/client.ts

src/lib/connect/socks.ts

src/lib/connect/index.ts

Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>

src/lib/connect/socks.ts

robertsLando · 2025-02-07T07:59:38Z

@DirtyHairy Would it be possible to test part of this?

DirtyHairy · 2025-02-07T13:35:59Z

Sure, done 😏

DirtyHairy · 2025-02-14T10:27:56Z

Just checking in to see whether there is something still missing.

robertsLando

Tests are failing

robertsLando · 2025-02-24T16:16:19Z

src/lib/connect/socks.ts

+function fatal<T>(e: T): T {
+	try {
+		if ((e as any).code === undefined) (e as any).code = 'SOCKS'
+		return e
+	} catch {
+		return e
+	}
+}


check ErrorWithReasonCode and do something like that instead:

export class SocksError extends Error { public code: string public constructor(message: string) { super(message) this.code = 'SOCKS' // We need to set the prototype explicitly Object.setPrototypeOf(this, SocksError.prototype) Object.getPrototypeOf(this).name = SocksError.name } }

I am fine with making that change. However, I still need to keep the function to "decorate" errors from the SOCKS client with .code so that MQTT.js will treat them as fatal (I'd rather not wrap them in another Error as that obscures the original reason).

Should I make the change anyway?

I'd rather not wrap them in another Error as that obscures the original reason

You are not wrapping them twice you just create a specific error instance for sock related errors and that is exactly what you are doing now but in a non-typed way

You are not wrapping them twice you just create a specific error instance for sock related errors and that is exactly what you are doing now but in a non-typed way

I have two code paths that call fatal: one decorates newly created errors, and the other decorates an error that I received and rethrow from socksClient (socks:205). In both cases I set .code in order to keep mqtt.js from ignoring the error ( client.ts:814). The first case can be replaced with a custom error class, the second case only if I wrap the error in my custom error, that's why I went for a decorator.

BTW: why are you manipulating the prototype directly? My understanding is that this used to be necessary for ES5 (as Error always returns a fresh object if called directly, independent of the scope), but for ES2015 the spec explicitly requires Error to be subclassable.

About the first part cannot you do something like:

constructor(messageOrError: string | Error) { if (messageOrError instanceof Error) { super(messageOrError.message); if (messageOrError.stack) { this.stack = messageOrError.stack; } } else { super(messageOrError); } Object.setPrototypeOf(this, SocksError.prototype); this.name = "SocksError"; }

BTW: why are you manipulating the prototype directly? My understanding is that this used to be necessary for ES5 (as Error always returns a fresh object if called directly, independent of the scope), but for ES2015 the spec explicitly requires Error to be subclassable.

From what I know:

Object.setPrototypeOf(this, SocksError.prototype): This used to be necessary in order to have e instanceof Error returning true, without that it used to return false

Object.getPrototypeOf(this).name = SocksError.name without this the name property on the error thrown it's Error and not SocksError

About the first part cannot you do something like:

Compared to that I personally prefer just setting code on the error. But, then again, this is not my project, so my personal preferences are not what counts here 😏 So, if you prefer that, I'll change the code.

From what I know:

I think you can do without that by now (in ES2015), unless you want to support compiling down to ES5:

Welcome to Node.js v23.7.0. Type ".help" for more information. > class FooError extends Error {name = 'FooError'} undefined > foo = new FooError() FooError at REPL2:1:7 at ContextifyScript.runInThisContext (node:vm:137:12) at REPLServer.defaultEval (node:repl:597:22) at bound (node:domain:433:15) at REPLServer.runBound [as eval] (node:domain:444:12) at REPLServer.onLine (node:repl:926:10) at REPLServer.emit (node:events:519:35) at REPLServer.emit (node:domain:489:12) at [_onLine] [as _onLine] (node:internal/readline/interface:416:12) at [_line] [as _line] (node:internal/readline/interface:887:18) > foo.name 'FooError' > foo instanceof FooError true > foo instanceof Error true

But, same here, I can just go ahead and add the prototype mangling.

problem is that foo instanceof Error is false without the second line? Anyway keep it like it is now no problem

Anyway keep it like it is now no problem

Alright, I won't complain about that 😏

problem is that foo instanceof Error is false without the second line?

The issue is foo instanceof FooError which is false for ES5 / "old-school" class definitions (unless you set up the prototype correctly after calling Error). If you extend Error ES2015 style with class / extends the prototype chain is set up correctly. My understanding is that the spec requires Error to check new.target to see whether it is called in a ES2015 constructing context and to behave accordingly (modify the target instead of returning a new object) in this case.

If you run the code in my NodeJS session through TS in ES5 mode foo instanceof FooError is false, and foo instanceof Error is true.

robertsLando · 2025-02-24T16:17:27Z

@DirtyHairy Sorry for late reply but I totally forgot about this

robertsLando · 2025-02-27T16:21:01Z

@DirtyHairy That's not the right approach to fix tests, I suggest you to ensure there are no leaked handlers that prevent test to exit

DirtyHairy · 2025-02-27T16:36:53Z

@robertsLando I don't think the reason are leaking handlers, I think this is a node.js issue similar to nodejs/node#54163 . I got errors because of remaining work on the loop before, and these look differently.

Furthermore I can't reproduce this issue locally with either v20.18.2 (which is what actions is running for 20) or v23.7.0, and for v18.20.7 (again what the CI claims to be running for 18) I do get this exact error for other, existing suites, even on main (without any changes from me, just doing npm install && npm run test:node:

 ▶ keepalive
    ✔ should send ping at keepalive interval (2.114583ms)
    ✔ should not shift ping on publish (1.18025ms)
    ✖ should reschedule pings if publishing at a higher rate than keepalive and reschedulePings===true (1.017125ms)
      'test did not finish before its parent and was cancelled'

    ✖ should not reschedule pings if publishing at a higher rate than keepalive and reschedulePings===false
      'test did not finish before its parent and was cancelled'

    ✖ should shift ping on pingresp when reschedulePings===true
      'test did not finish before its parent and was cancelled'

    ✖ should shift ping on pingresp when reschedulePings===false
      'test did not finish before its parent and was cancelled'

DirtyHairy · 2025-02-27T16:38:33Z

But, anyway, my commit does not fix the tests either 🤷‍♂️ I must admit I am slightly clueless here as I can't reproduce the issue.

DirtyHairy · 2025-02-27T16:41:24Z

Also, not getting this error for any other run on the CI is a pretty strong argument for something being wrong with my tests 😏 I just have no clue what is going on.

robertsLando · 2025-02-27T16:52:45Z

@DirtyHairy Seems you fixed it?

DirtyHairy · 2025-02-27T17:05:07Z

Na, sorry, not yet. But I've got progress: I can reproduce the fail in a Linux VM (I am on a Mac) 😏 With a reproducer I'll get that fixed.

…est is useless on Linux.

DirtyHairy · 2025-02-28T10:20:05Z

Fixed. Unfortunately, be deleting a test: I had a test that tries to assert that errors on socket stream propagate to the stream presented to MQTT.js by closing the server with RST. Unfortunately, this does not translate into an error, but into an EOF in Linux, so the test waited indefinitely.

codecov · 2025-02-28T10:54:52Z

Codecov Report

Attention: Patch coverage is 86.59794% with 13 lines in your changes missing coverage. Please review.

Project coverage is 81.46%. Comparing base (1c19ca0) to head (173b25b).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib/connect/socks.ts	89.15%	5 Missing and 4 partials ⚠️
src/lib/connect/tcp.ts	33.33%	1 Missing and 1 partial ⚠️
src/lib/connect/index.ts	80.00%	0 Missing and 1 partial ⚠️
src/lib/connect/tls.ts	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1966      +/-   ##
==========================================
+ Coverage   81.15%   81.46%   +0.31%     
==========================================
  Files          24       25       +1     
  Lines        1470     1565      +95     
  Branches      349      366      +17     
==========================================
+ Hits         1193     1275      +82     
- Misses        193      199       +6     
- Partials       84       91       +7

Flag	Coverage Δ
unittests	`81.46% <86.59%> (+0.31%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

robertsLando · 2025-03-03T08:20:23Z

@mcollina could you kindly give a check to this please?

wshshra · 2025-04-24T06:28:21Z

@DirtyHairy
I can't use this
I tried:
$env:DEBUG="mqttjs:socks"; node .\index.js

but there is no debug output, and I can't find "mqttjs:socks" in dist

DirtyHairy · 2025-04-24T20:11:50Z

@wshshra What are you trying to do, and what happens, and what do you expect to happen? Can you show some code?

wshshra · 2025-04-24T22:13:05Z

@wshshra What are you trying to do, and what happens, and what do you expect to happen? Can you show some code?

my code:

const options = {
      socksProxy: "socks5://example.com:port",
      ...otherOptions
    };
mqtt.connect(url, options)

the socks not work，then i want debug it, but there is not debug output

Finally I solved it like this

wsOptions: {
      agent,
 }

DirtyHairy added 4 commits February 5, 2025 18:09

Socks proxy support for TCP and TLS connections.

718f901

Documentation.

ca1dedd

Ignore socks for UNIX domain sockets.

7f214c4

Avoid bundling dead code.

a12827b

DirtyHairy changed the title ~~Add intrinsic support for SOCKS proxies~~ feat: add intrinsic support for SOCKS proxies Feb 5, 2025

robertsLando requested changes Feb 6, 2025

View reviewed changes

DirtyHairy added 3 commits February 6, 2025 10:54

Naming.

b132004

Update readme.

2eae2d7

Update lock file.

738c1ea

robertsLando reviewed Feb 6, 2025

View reviewed changes

src/lib/connect/index.ts Show resolved Hide resolved

robertsLando requested a review from mcollina February 6, 2025 10:40

Simplify.

6329624

Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>

robertsLando requested changes Feb 6, 2025

View reviewed changes

src/lib/connect/socks.ts Show resolved Hide resolved

DirtyHairy added 3 commits February 6, 2025 17:36

Trade duplexify against home-grown wrapper.

818abcc

Paranoia, linter.

2b6e203

Minor cleanup.

ddfe42d

Tests.

01f8fd6

robertsLando requested changes Feb 24, 2025

View reviewed changes

Use a fixed port for the tests.

8684fe3

DirtyHairy force-pushed the socks-support branch from 2eacc7b to 8684fe3 Compare February 27, 2025 16:45

DirtyHairy added 2 commits February 28, 2025 11:11

It seems TCP RST causes the remote to close without EOF on Linux -> t…

198bdc2

…est is useless on Linux.

Switch back to dynamic port allocation.

173b25b

robertsLando merged commit ee0fcce into mqttjs:main Mar 4, 2025
7 checks passed

feat: add intrinsic support for SOCKS proxies #1966

feat: add intrinsic support for SOCKS proxies #1966

Uh oh!

Conversation

DirtyHairy commented Feb 5, 2025

Uh oh!

robertsLando Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

DirtyHairy Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertsLando commented Feb 7, 2025

Uh oh!

DirtyHairy commented Feb 7, 2025

Uh oh!

DirtyHairy commented Feb 14, 2025

Uh oh!

robertsLando left a comment

Choose a reason for hiding this comment

Uh oh!

robertsLando Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

DirtyHairy Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

robertsLando Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

DirtyHairy Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertsLando Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

DirtyHairy Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertsLando Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DirtyHairy Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

robertsLando commented Feb 24, 2025

Uh oh!

robertsLando commented Feb 27, 2025

Uh oh!

DirtyHairy commented Feb 27, 2025

Uh oh!

DirtyHairy commented Feb 27, 2025

Uh oh!

DirtyHairy commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertsLando commented Feb 27, 2025

Uh oh!

DirtyHairy commented Feb 27, 2025

Uh oh!

DirtyHairy commented Feb 28, 2025

Uh oh!

codecov bot commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

robertsLando commented Mar 3, 2025

Uh oh!

Uh oh!

wshshra commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirtyHairy Feb 28, 2025 •

edited

Loading

DirtyHairy Mar 3, 2025 •

edited

Loading

robertsLando Mar 4, 2025 •

edited

Loading

DirtyHairy commented Feb 27, 2025 •

edited

Loading

codecov bot commented Feb 28, 2025 •

edited

Loading

wshshra commented Apr 24, 2025 •

edited

Loading

wshshra commented Apr 24, 2025 •

edited

Loading