fix(captp): ensure Sync(x) never returns a thenable

packages/captp/lib/captp.js

@@ -44,6 +44,14 @@ const STARTING_SEQUENCE_NUMBER = 23;
  * @property {GiveSyncReply} giveSyncReply if specified, enable this CapTP to
  * serve objects marked with exportAsSyncable to synchronous clients
  */
+
+/**
+ * @param {any} maybeThenable
+ * @returns {boolean}
+ */
+const isThenable = maybeThenable =>
+  maybeThenable && typeof maybeThenable.then === 'function';
+
 /**
  * Create a CapTP connection.
  *
@@ -599,10 +607,9 @@ export function makeCapTP(ourId, rawSend, bootstrapObj = undefined, opts = {}) {
       const it = takeSyncReply(implMethod, slot, implArgs);
       const nextSync = prep => {
         const status = it.next(prep);
-        assert.typeof(
-          /** @type {any} */ (status).then,
-          'undefined',
-          X`takeSyncReply must be a synchronous Generator but it.next() returned a Thenable ${status}`,
+        assert(
+          !isThenable(status),
+          X`takeSyncReply must be a fully-synchronous Generator but it.next() returned a Thenable ${status}`,
         );
         return status;
       };
@@ -616,6 +623,7 @@ export function makeCapTP(ourId, rawSend, bootstrapObj = undefined, opts = {}) {
         if (firstTime) {
           // Mark the send as "sync", but handle it asynchronously on the other
           // side.
+          firstTime = false;
           send({
             type: 'CTP_CALL',
             epoch,
@@ -624,7 +632,6 @@ export function makeCapTP(ourId, rawSend, bootstrapObj = undefined, opts = {}) {
             target: slot,
             method,
           });
-          firstTime = false;
         } else {
           // We weren't done fetching the entire result in the first iteration,
           // so thread through the iterator values into the "next" messages to
@@ -638,12 +645,16 @@ export function makeCapTP(ourId, rawSend, bootstrapObj = undefined, opts = {}) {
             data: status.value,
           });
         }
-        status = nextSync(false);
+        status = nextSync(firstTime);
       }
 
       // We've finished claiming the return value.
       const [isReject, serialized] = status.value;
       const value = unserialize(serialized);
+      assert(
+        !isThenable(value),
+        X`Sync() reply must not be a Thenable; have ${value}`,
+      );
       if (isReject) {
         throw value;
       }
@@ -718,6 +729,7 @@ export function makeLoopback(ourId, opts = {}) {
         isException = true;
         value = e;
       }
+      harden(value);
       // eslint-disable-next-line no-use-before-define
       return [isException, farSerialize(value)];
     },

packages/captp/test/synclib.js

@@ -1,3 +1,6 @@
+// @ts-check
+/* global setTimeout */
+
 import { assert, details as X } from '@agoric/assert';
 import { Far } from '@agoric/marshal';
 import { E, makeCapTP } from '../lib/captp';
@@ -11,14 +14,17 @@ export function createHostBootstrap(exportAsSyncable) {
           getN() {
             return n;
           },
+          getPromise() {
+            return new Promise(resolve => setTimeout(() => resolve(n), 10));
+          },
         }),
       );
       return syncable;
     },
   });
 }
 
-export async function runSyncTests(t, Sync, bs) {
+export async function runSyncTests(t, Sync, bs, unwrapsPromises) {
   // Demonstrate async compatibility of syncable.
   const pn = E(E(bs).getSyncable(3)).getN();
   t.is(Promise.resolve(pn), pn);
@@ -35,6 +41,16 @@ export async function runSyncTests(t, Sync, bs) {
   const s = await ps;
   t.is(Sync(s).getN(), 4);
 
+  // Try Sync unwrapping of a promise.
+  if (unwrapsPromises) {
+    t.is(Sync(s).getPromise(), 4);
+  } else {
+    t.throws(() => Sync(s).getPromise(), {
+      instanceOf: Error,
+      message: /reply must not be a Thenable/,
+    });
+  }
+
   // Demonstrate Sync fails on an unmarked remotable.
   const b = await bs;
   t.throws(() => Sync(b).getSyncable(5), {
@@ -45,7 +61,7 @@ export async function runSyncTests(t, Sync, bs) {
 
 function createGuestBootstrap(Sync, other) {
   return Far('tests', {
-    async runSyncTests() {
+    async runSyncTests(unwrapsPromises) {
       const mockT = {
         is(a, b) {
           assert.equal(a, b, X`${a} !== ${b}`);
@@ -60,7 +76,7 @@ function createGuestBootstrap(Sync, other) {
           assert.fail(X`Thunk did not throw: ${ret}`);
         },
       };
-      await runSyncTests(mockT, Sync, other);
+      await runSyncTests(mockT, Sync, other, unwrapsPromises);
       return true;
     },
   });

packages/captp/test/test-sync.js

@@ -31,9 +31,9 @@ const makeWorkerTests = isHost => async t => {
   const bs = getBootstrap();
   // console.error('have bs', bs);
   if (Sync) {
-    await runSyncTests(t, Sync, bs);
+    await runSyncTests(t, Sync, bs, true);
   } else {
-    t.assert(await E(bs).runSyncTests());
+    t.assert(await E(bs).runSyncTests(true));
   }
 };
 
@@ -43,5 +43,5 @@ test('try Node.js worker syncable, main guest', makeWorkerTests(false));
 test('try restricted loopback syncable', async t => {
   const { makeFar, Sync, exportAsSyncable } = makeLoopback('us');
   const bs = makeFar(createHostBootstrap(exportAsSyncable));
-  await runSyncTests(t, Sync, bs);
+  await runSyncTests(t, Sync, bs, false);
 });