@uppy/companion: do not use unsafe call to JSON.stringify

[aduh95]

Calling JSON.stringify on some objects (e.g. self-referencing objects) can lead to a TypeError; instead we can use util.inspect that is able to deal with those, and is basically guaranteed to never throw. It also means the output will be more readable by humans – but no as readable for machines.

[github-actions]

Diff output files

diff --git a/packages/@uppy/companion/lib/server/emitter/redis-emitter.js b/packages/@uppy/companion/lib/server/emitter/redis-emitter.js
index aa4f768..62796bc 100644
--- a/packages/@uppy/companion/lib/server/emitter/redis-emitter.js
+++ b/packages/@uppy/companion/lib/server/emitter/redis-emitter.js
@@ -1,7 +1,12 @@
 "use strict";
 Object.defineProperty(exports, "__esModule", { value: true });
 const { EventEmitter } = require("node:events");
+const { default: safeStringify } = require("fast-safe-stringify");
 const logger = require("../logger");
+function replacer(key, value) {
+  // Remove the circular structure and internal ones
+  return key[0] === "_" || value === "[Circular]" ? undefined : value;
+}
 /**
  * This module simulates the builtin events.EventEmitter but with the use of redis.
  * This is useful for when companion is running on multiple instances and events need
@@ -134,7 +139,7 @@ module.exports = (redisClient, redisPubSubScope) => {
    * @param {string} eventName name of the event
    */
   function emit(eventName, ...args) {
-    runWhenConnected(() => publisher.publish(getPrefixedEventName(eventName), JSON.stringify(args)));
+    runWhenConnected(() => publisher.publish(getPrefixedEventName(eventName), safeStringify(args, replacer)));
   }
   /**
    * Remove all listeners of an event

The following build files have been modified and might affect the actual diff:

yarn.lock

[Murderlon]

It's a bit inefficient but should we temporarily add try/catch with JSON.stringify to this change as well so we can log the eventName and object?

I'd rather work towards not sending massive circular objects over redis than to silence the problem.

[mifi]

Isn’t this a breaking change if the output from util.inspect is not the same as that from JSON.stringify? Wouldn’t using JSON.parse on an string that has been util.inspect’ed possibly fail? (I assume that JSON.stringify was not added here in order to make it human readable).
I agree we should prevent this from happening in the first place by making sure we dont try to publish such complex objects, however maybe a try/catch would be best for now if we want to be able to identify/eliminate such cases?

[Murderlon]

I don't think it's breaking, in fact it's less breaking as the current code crashed in production and this won't.

[mifi]

I mean breaking as in api breaking: if util.inspect changes the api and someone is consuming the event on redis, then it could break their implementation

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher

🚮 Removed packages: npm/[email protected]), npm/[email protected]), npm/[email protected]), npm/[email protected]), npm/[email protected])

View full report↗︎

[mifi]

better to put replacer inside the function call so typescript can automatically infer the types for key and value, but we can also do that later when refactoring to typescript

[mifi]

Suggested change

	// Remove the circular structure and internal ones
	// Remove the circular references and internal properties
	// so that they don't get exposed to the browser (uppy client)

[Murderlon]

Shall we also stop sending the entire req object in emits in this PR?

Over Slack we concluded that it wouldn't be a breaking change.

[mifi]

You mean stop sending the whole Error object and instead just send message? Yesterday I agreed yes, but now I realised that I misread the code and I thought it was only message being used by Uppy. However I can also see that the whole error object structure is being passed as cause for the created error, meaning somebody could be depending on cause when catching an error. If we can consider removing cause as not being a breaking change, then sure we can do it.

uppy/packages/@uppy/companion-client/src/RequestClient.ts

Line 493 in a750140

cause: payload.error,

[Murderlon]

Discussed just now. Let's keep things for now and not block this PR any further.