chore: added tests for client protocol

Author: aryanjassal

src/client/errors.ts

@@ -23,6 +23,11 @@ class ErrorClientInvalidHeader<T> extends ErrorClient<T> {
   exitCode = sysexits.USAGE;
 }
 
+class ErrorClientProtocolError<T> extends ErrorClient<T> {
+  static description = 'Data does not match the protocol requirements';
+  exitCode = sysexits.USAGE;
+}
+
 class ErrorClientService<T> extends ErrorClient<T> {}
 
 class ErrorClientServiceRunning<T> extends ErrorClientService<T> {
@@ -51,6 +56,7 @@ export {
   ErrorClientAuthFormat,
   ErrorClientAuthDenied,
   ErrorClientInvalidHeader,
+  ErrorClientProtocolError,
   ErrorClientService,
   ErrorClientServiceRunning,
   ErrorClientServiceNotRunning,

src/client/handlers/VaultsSecretsRemove.ts

@@ -35,11 +35,13 @@ class VaultsSecretsRemove extends DuplexHandler<
     const { db, vaultManager }: { db: DB; vaultManager: VaultManager } =
       this.container;
     // Extract the header message from the iterator
+    const headerMessagePair = await input.next();
     const headerMessage:
       | SecretsRemoveHeaderMessage
-      | SecretIdentifierMessageTagged = (await input.next()).value;
+      | SecretIdentifierMessageTagged = headerMessagePair.value;
+    // Testing if the header is of the expected format
     if (
-      headerMessage == null ||
+      headerMessagePair.done ||
       headerMessage.type !== 'VaultNamesHeaderMessage'
     ) {
       throw new clientErrors.ErrorClientInvalidHeader();
@@ -72,10 +74,12 @@ class VaultsSecretsRemove extends DuplexHandler<
         for (let i = 0; i < efses.length; i++) {
           vaultMap.set(headerMessage!.vaultNames[i], efses[i]);
         }
+        let loopRan = false;
         for await (const message of input) {
-          // Ignoring any header messages
+          loopRan = true;
+          // Header messages should not be seen anymore
           if (message.type === 'VaultNamesHeaderMessage') {
-            throw new clientErrors.ErrorClientInvalidHeader(
+            throw new clientErrors.ErrorClientProtocolError(
               'The header message cannot be sent multiple times',
             );
           }
@@ -116,6 +120,12 @@ class VaultsSecretsRemove extends DuplexHandler<
             }
           }
         }
+        // Content messages must follow header messages
+        if (!loopRan) {
+          throw new clientErrors.ErrorClientProtocolError(
+            'No content messages followed header message',
+          );
+        }
       },
     );
   };

src/git/utils.ts

@@ -218,7 +218,9 @@ async function listObjects({
         }
         return;
       default:
-        utils.never('Invalid type');
+        utils.never(
+          `type must be one of "commit", "tree", "blob", or "tag", got "${type}"`,
+        );
     }
   }
 

src/vaults/VaultInternal.ts

@@ -596,9 +596,6 @@ class VaultInternal {
               await this.createCommit();
             } catch (e_) {
               e = e_;
-            }
-            // This would happen if an error was caught inside the catch block
-            if (e != null) {
               // Error implies dirty state
               await this.cleanWorkingDirectory();
             }

tests/audit/utils.test.ts

@@ -47,6 +47,7 @@ describe('Audit Utils', () => {
       ['f'],
       ['f'],
     ];
+    // @ts-ignore: treating TopicSubPath as string for testing
     const filtered = auditUtils.filterSubPaths(data).map((v) => v.join('.'));
     expect(filtered).toHaveLength(3);
     expect(filtered).toInclude('a.b');

tests/client/handlers/nodes.test.ts

@@ -825,7 +825,6 @@ describe('nodesGetAll', () => {
       manifest: {
         nodesGetAll: new NodesGetAll({
           nodeGraph,
-          keyRing,
         }),
       },
       host: localhost,

tests/client/handlers/vaults.test.ts

@@ -70,6 +70,7 @@ import * as keysUtils from '@/keys/utils';
 import * as nodesUtils from '@/nodes/utils';
 import * as vaultsUtils from '@/vaults/utils';
 import * as vaultsErrors from '@/vaults/errors';
+import * as clientErrors from '@/client/errors';
 import * as networkUtils from '@/network/utils';
 import * as utils from '@/utils';
 import * as testsUtils from '../../utils';
@@ -2356,6 +2357,80 @@ describe('vaultsSecretsRemove', () => {
       recursive: true,
     });
   });
+  test('fails when header is not sent', async () => {
+    // Write paths
+    const response = await rpcClient.methods.vaultsSecretsRemove();
+    const writer = response.writable.getWriter();
+    // Not sending the header message
+    // Content messages
+    await writer.write({
+      type: 'SecretIdentifierMessage',
+      nameOrId: 'invalid',
+      secretName: 'invalid',
+    });
+    await writer.close();
+    // Read response
+    const consumeP = async () => {
+      for await (const _ of response.readable) {
+        // Consume values
+      }
+    };
+    await testsUtils.expectRemoteError(
+      consumeP(),
+      clientErrors.ErrorClientInvalidHeader,
+    );
+  });
+  test('fails when only the header is sent', async () => {
+    const vaultId = await vaultManager.createVault('test-vault');
+    const vaultIdEncoded = vaultsUtils.encodeVaultId(vaultId);
+    // Write paths
+    const response = await rpcClient.methods.vaultsSecretsRemove();
+    const writer = response.writable.getWriter();
+    // Header message
+    await writer.write({
+      type: 'VaultNamesHeaderMessage',
+      vaultNames: [vaultIdEncoded],
+    });
+    // Not sending the content messages
+    await writer.close();
+    // Read response
+    const consumeP = async () => {
+      for await (const _ of response.readable) {
+        // Consume values
+      }
+    };
+    await testsUtils.expectRemoteError(
+      consumeP(),
+      clientErrors.ErrorClientProtocolError,
+    );
+  });
+  test('fails when the header is sent multiple times', async () => {
+    const vaultId = await vaultManager.createVault('test-vault');
+    const vaultIdEncoded = vaultsUtils.encodeVaultId(vaultId);
+    // Write paths
+    const response = await rpcClient.methods.vaultsSecretsRemove();
+    const writer = response.writable.getWriter();
+    // Header message
+    await writer.write({
+      type: 'VaultNamesHeaderMessage',
+      vaultNames: [vaultIdEncoded],
+    });
+    await writer.write({
+      type: 'VaultNamesHeaderMessage',
+      vaultNames: [vaultIdEncoded],
+    });
+    await writer.close();
+    // Read response
+    const consumeP = async () => {
+      for await (const _ of response.readable) {
+        // Consume values
+      }
+    };
+    await testsUtils.expectRemoteError(
+      consumeP(),
+      clientErrors.ErrorClientProtocolError,
+    );
+  });
   test('fails with invalid vault name', async () => {
     // Write paths
     const response = await rpcClient.methods.vaultsSecretsRemove();
@@ -2374,7 +2449,9 @@ describe('vaultsSecretsRemove', () => {
     await writer.close();
     // Read response
     const consumeP = async () => {
-      for await (const _ of response.readable);
+      for await (const _ of response.readable) {
+        // Consume values
+      }
     };
     await testsUtils.expectRemoteError(
       consumeP(),

tests/git/utils.ts

@@ -90,7 +90,8 @@ type NegotiationTestData =
  * @param rest - Random buffer data to be appended to the end to simulate more lines in the stream.
  */
 function generateGitNegotiationLine(data: NegotiationTestData, rest: Buffer) {
-  switch (data.type) {
+  const type = data.type;
+  switch (type) {
     case 'want': {
       // Generate a `want` line that includes `want`, the `objectId` and capabilities
       const line = Buffer.concat([
@@ -123,9 +124,8 @@ function generateGitNegotiationLine(data: NegotiationTestData, rest: Buffer) {
       // Generate an empty buffer to simulate the stream running out of data to process
       return Buffer.alloc(0);
     default:
-      // @ts-ignore: if we're here then data isn't the type we expect
       utils.never(
-        `data.type must be "want", "have", "SEPARATOR", "done", "none", got "${data.type}"`,
+        `data.type must be "want", "have", "SEPARATOR", "done", "none", got "${type}"`,
       );
   }
 }