protobuf.js icon indicating copy to clipboard operation
protobuf.js copied to clipboard

Support HardenedJS

Open michaelfig opened this issue 3 years ago • 1 comments

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch [email protected] and @protobufjs/[email protected] for the project I'm working on.

I'm using your code within a Compartment created by the HardenedJS shim (https://github.com/endojs/endo/tree/HEAD/packages/ses) and it cannot faithfully implement direct-eval (the special form using the scope as it exists in the caller), so it bails when it detects the eval(...) syntax. However, because @protobufjs/inquire works properly even when eval is indirect eval (just using the global scope), the only change needed is to avoid triggering the suspect syntax by surrounding eval with parens.

I went further and found some JS portability issues in protobufjs/src/util/minimal.js.

  • Within a Compartment, the portable way to retrieve the global this is globalThis.
  • If you try assigning to an object with a frozen prototype, and you shadow one of the frozen properties, it won't create the new property, and under strict mode will throw an exception. The workaround is to use Object.defineProperty.

It would be great if you could make this change upstream, as it shouldn't affect any of your other users, except to make it portable to Hardened JS.

Here is the diff that solved my problem:

diff --git a/node_modules/@protobufjs/inquire/index.js b/node_modules/@protobufjs/inquire/index.js
index 33778b5..a7aee2d 100644
--- a/node_modules/@protobufjs/inquire/index.js
+++ b/node_modules/@protobufjs/inquire/index.js
@@ -9,7 +9,7 @@ module.exports = inquire;
  */
 function inquire(moduleName) {
     try {
-        var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval
+        var mod = (eval)("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval
         if (mod && (mod.length || Object.keys(mod).length))
             return mod;
     } catch (e) {} // eslint-disable-line no-empty
diff --git a/node_modules/protobufjs/src/util/minimal.js b/node_modules/protobufjs/src/util/minimal.js
index 3c406de..59880a0 100644
--- a/node_modules/protobufjs/src/util/minimal.js
+++ b/node_modules/protobufjs/src/util/minimal.js
@@ -41,7 +41,8 @@ util.isNode = Boolean(typeof global !== "undefined"
  * @memberof util
  * @type {Object}
  */
-util.global = util.isNode && global
+util.global = typeof globalThis !== "undefined" && globalThis
+	   || util.isNode && global
            || typeof window !== "undefined" && window
            || typeof self   !== "undefined" && self
            || this; // eslint-disable-line no-invalid-this
@@ -280,13 +281,13 @@ function newError(name) {
             merge(this, properties);
     }
 
-    (CustomError.prototype = Object.create(Error.prototype)).constructor = CustomError;
+    Object.defineProperty(CustomError.prototype = Object.create(Error.prototype), "constructor", { value: CustomError });
 
     Object.defineProperty(CustomError.prototype, "name", { get: function() { return name; } });
 
-    CustomError.prototype.toString = function toString() {
+    Object.defineProperty(CustomError.prototype, "toString", { value: function toString() {
         return this.name + ": " + this.message;
-    };
+    } });
 
     return CustomError;
 }

This issue body was partially generated by patch-package.

michaelfig avatar May 10 '22 07:05 michaelfig

@sofisl if I send a PR would you accept this change? It's strictly safer and would unbreak this package in hardened JS environments.

turadg avatar Nov 05 '24 18:11 turadg