fix(compass): improve validation of command line arguments COMPASS-7260 (#5363)

Co-authored-by: Sergey Petushkov <[email protected]>
Co-authored-by: Anna Henningsen <[email protected]>
Co-authored-by: Alena Khineika <[email protected]>

Author: 3 people

.evergreen/functions.yml

@@ -83,6 +83,12 @@ variables:
 
 # This is here with the variables because anchors aren't supported across includes
 post:
+  - command: shell.exec
+    params:
+      working_dir: src
+      shell: bash
+      script: |
+        tar czvf all-e2e-logs.tgz packages/compass-e2e-tests/.log
   - command: s3.put
     params:
       <<: *save-artifact-params-private
@@ -92,6 +98,7 @@ post:
         - src/packages/compass-e2e-tests/.nyc_output/coverage.json
         - src/.testserver/logs/*
         - src/.evergreen/logs/*
+        - src/all-e2e-logs.tgz
       remote_file: ${project}/${revision}_${revision_order_id}/${build_variant}/${task_name}
       content_type: text/plain
   - command: s3.put
@@ -367,7 +374,7 @@ functions:
             sleep 20
           done
           echo "SSH connection established after $attempts attempts"
-    
+
           # Write the host info so that it can be used by the signing tool
           if [[ $OSTYPE == "cygwin" ]]; then
             identity_file=$(cygpath -wa "$identity_file")
@@ -566,7 +573,7 @@ functions:
             tar -xvz)
           export COMPASS_CRYPT_LIBRARY_PATH=$(echo $PWD/mongodb-crypt/lib/mongo_*_v1.*)
           npm run test-csfle --workspace mongodb-data-service
-  
+
   verify-artifacts:
     - command: shell.exec
       params:

package-lock.json

@@ -129,6 +129,7 @@ export class Compass {
   logPath?: string;
   userDataPath?: string;
   appName?: string;
+  mainProcessPid?: number;
 
   constructor(
     name: string,
@@ -196,20 +197,17 @@ export class Compass {
 
     // get the app logPath out of electron early in case the app crashes before we
     // close it and load the logs
-    this.logPath = await this.browser.execute(() => {
-      // eslint-disable-next-line @typescript-eslint/no-var-requires
-      return require('electron').ipcRenderer.invoke('compass:logPath');
-    });
-
-    this.userDataPath = await this.browser.execute(() => {
-      // eslint-disable-next-line @typescript-eslint/no-var-requires
-      return require('electron').ipcRenderer.invoke('compass:userDataPath');
-    });
-
-    this.appName = await this.browser.execute(() => {
-      // eslint-disable-next-line @typescript-eslint/no-var-requires
-      return require('electron').ipcRenderer.invoke('compass:appName');
-    });
+    [this.logPath, this.userDataPath, this.appName, this.mainProcessPid] =
+      await this.browser.execute(() => {
+        // eslint-disable-next-line @typescript-eslint/no-var-requires
+        const { ipcRenderer } = require('electron');
+        return Promise.all([
+          ipcRenderer.invoke('compass:logPath'),
+          ipcRenderer.invoke('compass:userDataPath'),
+          ipcRenderer.invoke('compass:appName'),
+          ipcRenderer.invoke('compass:mainProcessPid'),
+        ] as const);
+      });
   }
 
   addDebugger(): void {
@@ -366,14 +364,23 @@ export class Compass {
       }
     }
 
-    debug('Stopping Compass application');
+    const copyCompassLog = async () => {
+      const compassLog = await getCompassLog(this.logPath ?? '');
+      const compassLogPath = path.join(
+        LOG_PATH,
+        `compass-log.${this.name}.log`
+      );
+      debug(`Writing Compass application log to ${compassLogPath}`);
+      await fs.writeFile(compassLogPath, compassLog.raw);
+      return compassLog;
+    };
+
+    // Copy log files before stopping in case that stopping itself fails and needs to be debugged
+    await copyCompassLog();
+    debug(`Stopping Compass application [${this.name}]`);
     await this.browser.deleteSession();
 
-    const compassLog = await getCompassLog(this.logPath ?? '');
-    const compassLogPath = path.join(LOG_PATH, `compass-log.${this.name}.log`);
-    debug(`Writing Compass application log to ${compassLogPath}`);
-    await fs.writeFile(compassLogPath, compassLog.raw);
-    this.logs = compassLog.structured;
+    this.logs = (await copyCompassLog()).structured;
   }
 }
 
@@ -677,20 +684,9 @@ async function startCompass(
             .join(', ')}`
     );
 
-    filteredProcesses.forEach((p) => {
-      try {
-        debug(`Killing process ${p.name} with PID ${p.pid}`);
-        if (process.platform === 'win32') {
-          crossSpawn.sync('taskkill', ['/PID', String(p.pid), '/F', '/T']);
-        } else {
-          process.kill(p.pid);
-        }
-      } catch (err) {
-        debug(`Failed to kill process ${p.name} with PID ${p.pid}`, {
-          error: (err as Error).stack,
-        });
-      }
-    });
+    for (const p of filteredProcesses) {
+      tryKillProcess(p.pid, p.name);
+    }
     throw err;
   }
 
@@ -704,6 +700,21 @@ async function startCompass(
   return compass;
 }
 
+function tryKillProcess(pid: number, name = '<unknown>'): void {
+  try {
+    debug(`Killing process ${name} with PID ${pid}`);
+    if (process.platform === 'win32') {
+      crossSpawn.sync('taskkill', ['/PID', String(pid), '/F', '/T']);
+    } else {
+      process.kill(pid);
+    }
+  } catch (err) {
+    debug(`Failed to kill process ${name} with PID ${pid}`, {
+      error: (err as Error).stack,
+    });
+  }
+}
+
 /**
  * @param {string} logPath The compass application log path
  * @returns {Promise<CompassLog>}
@@ -918,18 +929,18 @@ export async function cleanup(compass?: Compass): Promise<void> {
   }
 
   let timeoutId;
-  const timeoutPromise = new Promise<void>((resolve) => {
+  const timeoutPromise = new Promise<'timeout'>((resolve) => {
     timeoutId = setTimeout(() => {
-      console.error('It took too long to close compass');
-      resolve();
+      console.error(`It took too long to close compass [${compass.name}]`);
+      resolve('timeout');
     }, 30000);
   });
 
-  const closePromise = (async function close(): Promise<void> {
+  const closePromise = (async function close(): Promise<'closed'> {
     try {
       await compass.stop();
     } catch (err) {
-      debug('An error occurred while stopping compass:');
+      debug(`An error occurred while stopping compass: [${compass.name}]`);
       debug(err);
       try {
         // make sure the process can exit
@@ -939,10 +950,18 @@ export async function cleanup(compass?: Compass): Promise<void> {
       }
     }
     clearTimeout(timeoutId);
-    return;
+    return 'closed';
   })();
 
-  return Promise.race([timeoutPromise, closePromise]);
+  const result = await Promise.race([timeoutPromise, closePromise]);
+  if (result === 'timeout' && compass.mainProcessPid) {
+    try {
+      debug(`Trying to manually kill Compass [${compass.name}]`);
+      tryKillProcess(compass.mainProcessPid, compass.name);
+    } catch {
+      /* already logged ... */
+    }
+  }
 }
 
 function pathName(text: string) {

packages/compass-e2e-tests/helpers/compass.ts

@@ -81,6 +81,7 @@ export type CliOnlyPreferences = {
   version?: boolean;
   help?: boolean;
   showExampleConfig?: boolean;
+  trustedConnectionString?: boolean;
 };
 
 export type NonUserPreferences = {
@@ -747,6 +748,22 @@ const cliOnlyPreferencesProps: Required<{
     validator: z.boolean().optional(),
     type: 'boolean',
   },
+  /**
+   * Allows the automatic initiation of the connection establishment process
+   * when launching Compass from the command line to indicate that the connection string comes from a trusted source,
+   * even if the provided connection string contains disallowed connection options.
+   */
+  trustedConnectionString: {
+    ui: false,
+    cli: true,
+    global: false,
+    description: {
+      short: 'Always allow to Automatically Connect',
+      long: 'Allow automatic connection establishment when launching Compass, even if the provided connection string contains connection options that would not be accepted when coming from an untrusted source',
+    },
+    validator: z.boolean().default(false),
+    type: 'boolean',
+  },
 };
 
 const nonUserPreferences: Required<{

packages/compass-preferences-model/src/preferences-schema.ts

@@ -213,6 +213,8 @@
     "lodash": "^4.17.21",
     "make-fetch-happen": "^8.0.14",
     "marky": "^1.2.1",
+    "mongodb": "^6.2.0",
+    "mongodb-build-info": "^1.7.0",
     "mongodb-connection-string-url": "^2.6.0",
     "mongodb-data-service": "^22.17.4",
     "mongodb-instance-model": "^12.17.4",

packages/compass/package.json

@@ -4,6 +4,7 @@ import { promises as fsPromises } from 'fs';
 import { UUID } from 'bson';
 import { ipcRenderer } from 'hadron-ipc';
 import { ConnectionString } from 'mongodb-connection-string-url';
+import { getConnectionStringFromArgs } from '../main/auto-connect';
 import type { AutoConnectPreferences } from '../main/auto-connect';
 
 async function getWindowAutoConnectPreferences(): Promise<AutoConnectPreferences> {
@@ -60,7 +61,7 @@ export async function loadAutoConnectInfo(
       });
       let id: string | undefined;
       if (positionalArguments.length > 0) {
-        id = positionalArguments[0];
+        id = getConnectionStringFromArgs(positionalArguments);
       } else if (connections.length === 1) {
         id = connections[0].id;
       }
@@ -77,11 +78,13 @@ export async function loadAutoConnectInfo(
       }
       return applyUsernameAndPassword(connectionInfo, { username, password });
     } else {
+      const connectionString = getConnectionStringFromArgs(positionalArguments);
+      if (!connectionString) {
+        throw new Error('Could not find a connection string');
+      }
       return applyUsernameAndPassword(
         {
-          connectionOptions: {
-            connectionString: positionalArguments[0],
-          },
+          connectionOptions: { connectionString },
           id: new UUID().toString(),
         },
         { username, password }

packages/compass/src/app/auto-connect.ts

@@ -297,6 +297,10 @@ class CompassApplication {
     ipcMain?.handle('compass:appName', () => {
       return app.getName();
     });
+
+    ipcMain?.handle('compass:mainProcessPid', () => {
+      return process.pid;
+    });
   }
 
   private static async setupLogging(): Promise<void> {