fix: review suggestions

endojs · Mar 30, 2023 · 1613013 · 1613013
1 parent 84baa2c
commit 1613013
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 45 deletions.
diff --git a/packages/check-bundle/test/test-check-bundle.js b/packages/check-bundle/test/test-check-bundle.js
@@ -100,6 +100,7 @@ test('bundle and check corrupt endo zip base64 package', async t => {
 
 test('bundle and hash unfrozen object', async t => {
   const bundle = {};
+  await null;
   // @ts-ignore `isFake` purposely omitted from type
   if (harden.isFake) {
     t.pass();

diff --git a/packages/init/test/test-async_hooks.js b/packages/init/test/test-async_hooks.js
@@ -50,7 +50,9 @@ test('async_hooks Promise patch', async t => {
 
     // Create a promise with symbols attached
     const p3 = Promise.resolve();
-    t.is(Reflect.ownKeys(p3).length > 0 || !!harden.isFake, hasAsyncSymbols);
+    if (!harden.isFake) {
+      t.is(Reflect.ownKeys(p3).length > 0, hasAsyncSymbols);
+    }
 
     return Promise.resolve().then(() => {
       resolve(8);
@@ -62,7 +64,9 @@ test('async_hooks Promise patch', async t => {
       // node versions will fail and generate a new one because of an own check
       p1.then(() => {});
 
-      t.is(Reflect.ownKeys(ret).length > 0 || !!harden.isFake, hasAsyncSymbols);
+      if (!harden.isFake) {
+        t.is(Reflect.ownKeys(ret).length > 0, hasAsyncSymbols);
+      }
 
       // testHooks.disable();
 

diff --git a/packages/marshal/test/test-marshal-capdata.js b/packages/marshal/test/test-marshal-capdata.js
@@ -246,14 +246,20 @@ test('serialize errors', t => {
   errExtra.foo = [];
   freeze(errExtra);
   t.assert(isFrozen(errExtra));
-  // @ts-ignore Check dynamic consequences of type violation
-  t.falsy(isFrozen(errExtra.foo) && !harden.isFake);
+  // @ts-ignore `isFake` purposely omitted from type
+  if (!harden.isFake) {
+    // @ts-ignore Check dynamic consequences of type violation
+    t.falsy(isFrozen(errExtra.foo));
+  }
   t.deepEqual(ser(errExtra), {
     body: '{"@qclass":"error","errorId":"error:anon-marshal#10003","message":"has extra properties","name":"Error"}',
     slots: [],
   });
-  // @ts-ignore Check dynamic consequences of type violation
-  t.falsy(isFrozen(errExtra.foo) && !harden.isFake);
+  // @ts-ignore `isFake` purposely omitted from type
+  if (!harden.isFake) {
+    // @ts-ignore Check dynamic consequences of type violation
+    t.falsy(isFrozen(errExtra.foo));
+  }
 
   // Bad prototype and bad "message" property
   const nonErrorProto1 = { __proto__: Error.prototype, name: 'included' };

diff --git a/packages/marshal/test/test-marshal-smallcaps.js b/packages/marshal/test/test-marshal-smallcaps.js
@@ -106,14 +106,20 @@ test('smallcaps serialize errors', t => {
   errExtra.foo = [];
   freeze(errExtra);
   t.assert(isFrozen(errExtra));
-  // @ts-ignore Check dynamic consequences of type violation
-  t.falsy(isFrozen(errExtra.foo) && !harden.isFake);
+  // @ts-ignore `isFake` purposely omitted from type
+  if (!harden.isFake) {
+    // @ts-ignore Check dynamic consequences of type violation
+    t.falsy(isFrozen(errExtra.foo));
+  }
   t.deepEqual(ser(errExtra), {
     body: '#{"#error":"has extra properties","name":"Error"}',
     slots: [],
   });
-  // @ts-ignore Check dynamic consequences of type violation
-  t.falsy(isFrozen(errExtra.foo) && !harden.isFake);
+  // @ts-ignore `isFake` purposely omitted from type
+  if (!harden.isFake) {
+    // @ts-ignore Check dynamic consequences of type violation
+    t.falsy(isFrozen(errExtra.foo));
+  }
 
   // Bad prototype and bad "message" property
   const nonErrorProto1 = { __proto__: Error.prototype, name: 'included' };

diff --git a/packages/pass-style/src/make-far.js b/packages/pass-style/src/make-far.js
@@ -94,10 +94,14 @@ export const Remotable = (
     Fail`Remotable ${remotable} is already marked as a ${q(
       remotable[PASS_STYLE],
     )}`;
-  // Ensure that the remotable isn't already frozen.
-  !isFrozen(remotable) ||
-    // @ts-ignore Purposely not in the type. Only to support __hardenTaming__
-    harden.isFake ||
+  // `isFrozen` always returns true with a fake `harden`, but we want that case
+  // to succeed anyway. Faking `harden` is only correctness preserving
+  // if the code in question contains no bugs that the real `harden` would
+  // have caught.
+  // @ts-ignore `isFake` purposely not in the type
+  harden.isFake ||
+    // Ensure that the remotable isn't already frozen.
+    !isFrozen(remotable) ||
     Fail`Remotable ${remotable} is already frozen`;
   const remotableProto = makeRemotableProto(remotable, iface);
 

diff --git a/packages/ses/docs/lockdown.md b/packages/ses/docs/lockdown.md
@@ -20,20 +20,20 @@ compatibility vs better tool compatibility.
 
 Each option is explained in its own section below.
 
-| option           | default setting  | other settings | about |
-|------------------|------------------|----------------|-------|
-| `regExpTaming`   | `'safe'`         | `'unsafe'`     | `RegExp.prototype.compile` |
-| `localeTaming`   | `'safe'`         | `'unsafe'`     | `toLocaleString`           |
-| `consoleTaming`  | `'safe'`         | `'unsafe'`     | deep stacks                |
-| `errorTaming`    | `'safe'`         | `'unsafe'`     | `errorInstance.stack`      |
-| `errorTrapping`  | `'platform'`     | `'exit'` `'abort'` `'report'` `'none'` | handling of uncaught exceptions |
-| `unhandledRejectionTrapping`  | `'report'`     | `'none'` | handling of finalized unhandled rejections |
-| `evalTaming`     | `'safeEval'`     | `'unsafeEval'` `'noEval'` | `eval` and `Function` of the start compartment. |
-| `stackFiltering` | `'concise'`      | `'verbose'`    | deep stacks signal/noise   |
-| `overrideTaming` | `'moderate'`     | `'min'` or `'severe'` | override mistake antidote  |
-| `overrideDebug`  | `[]`             | array of property names | detect override mistake |
-| `domainTaming`   | `'safe'`       | `'unsafe'`       | Node.js `domain` module |
-| `__hardenTaming__`     | `'safe'` | `'unsafe'` | `harden` fake? Related tests pretend. |
+| option             | default setting  | other settings | about |
+|--------------------|------------------|----------------|-------|
+| `regExpTaming`     | `'safe'`         | `'unsafe'`     | `RegExp.prototype.compile` |
+| `localeTaming`     | `'safe'`         | `'unsafe'`     | `toLocaleString`           |
+| `consoleTaming`    | `'safe'`         | `'unsafe'`     | deep stacks                |
+| `errorTaming`      | `'safe'`         | `'unsafe'`     | `errorInstance.stack`      |
+| `errorTrapping`    | `'platform'`     | `'exit'` `'abort'` `'report'` `'none'` | handling of uncaught exceptions |
+| `unhandledRejectionTrapping` | `'report'` | `'none'`   | handling of finalized unhandled rejections |
+| `evalTaming`       | `'safeEval'`     | `'unsafeEval'` `'noEval'` | `eval` and `Function` of the start compartment. |
+| `stackFiltering`   | `'concise'`      | `'verbose'`    | deep stacks signal/noise   |
+| `overrideTaming`   | `'moderate'`     | `'min'` or `'severe'` | override mistake antidote  |
+| `overrideDebug`    | `[]`             | array of property names | detect override mistake |
+| `domainTaming`     | `'safe'`         | `'unsafe'`     | Node.js `domain` module |
+| `__hardenTaming__` | `'safe'`         | `'unsafe'`     | Making `harden` no-op for performance in trusted environments |
 
 In the absence of any of these options in lockdown arguments, lockdown will
 attempt to read these from the Node.js `process.env` environment.

diff --git a/packages/ses/src/tame-harden.js b/packages/ses/src/tame-harden.js
@@ -11,8 +11,8 @@ export const tameHarden = (safeHarden, hardenTaming) => {
   }
 
   // In on the joke
-  Object.isFrozen = () => true;
   Object.isExtensible = () => false;
+  Object.isFrozen = () => true;
   Object.isSealed = () => true;
   Reflect.isExtensible = () => false;
 

diff --git a/packages/ses/test/test-tame-harden.js b/packages/ses/test/test-tame-harden.js
@@ -12,23 +12,23 @@ lockdown({ __hardenTaming__: 'unsafe' });
 test('fake harden', t => {
   t.is(harden.isFake, true);
 
+  const assertFakeFrozen = specimen => {
+    // Built-in function replacements must report frozenness.
+    t.is(Object.isExtensible(specimen), false);
+    t.is(Object.isFrozen(specimen), true);
+    t.is(Object.isSealed(specimen), true);
+    t.is(Reflect.isExtensible(specimen), false);
+
+    // In-the-know functions must report the truth.
+    t.is(isExtensible(specimen), true);
+    t.is(isFrozen(specimen), false);
+    t.is(isSealed(specimen), false);
+    t.is(reflectIsExtensible(specimen), true);
+  };
+
   const obj = {};
-  t.is(Object.isFrozen(obj), true);
-  t.is(isFrozen(obj), false);
-  t.is(Object.isSealed(obj), true);
-  t.is(isSealed(obj), false);
-  t.is(Object.isExtensible(obj), false);
-  t.is(isExtensible(obj), true);
-  t.is(Reflect.isExtensible(obj), false);
-  t.is(reflectIsExtensible(obj), true);
+  assertFakeFrozen(obj);
 
   harden(obj);
-  t.is(Object.isFrozen(obj), true);
-  t.is(isFrozen(obj), false);
-  t.is(Object.isSealed(obj), true);
-  t.is(isSealed(obj), false);
-  t.is(Object.isExtensible(obj), false);
-  t.is(isExtensible(obj), true);
-  t.is(Reflect.isExtensible(obj), false);
-  t.is(reflectIsExtensible(obj), true);
+  assertFakeFrozen(obj);
 });