@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[bot]]

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

[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[bot]]

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↗︎

[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.

https://github.com/transloadit/uppy/blob/a7501403354538de1ed9712cab9a67366dfb1134/packages/%40uppy/companion-client/src/RequestClient.ts#L493

[Murderlon]

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