fix: address CodeRabbit review feedback on #3370

jeanfbrito · jeanfbrito · commit 536bdcc0ce50 · 2026-06-23T10:49:44.000-03:00
Hardening fixes from CodeRabbit review (verified against code, false positives rejected): Security / stability: - videoCallWindow/ipc.ts: validate URL protocol (http/https) before the g.co external-open escape hatch (was reachable via ftp://g.co/...); tighten host match to exact g.co / *.g.co (was overmatching evilg.co); guard getRootWindow() rejection in restoreServerViewHandler so teardown can't produce an unhandled rejection (display-media restore still runs). - serverView/index.ts: gate the 'Permission request' debug log behind NODE_ENV==='development' (was logging payloads in production); wrap isProtocolAllowed() in try/catch so the openExternal permission callback always fires even on malformed URLs. - telephony/main.ts: handle shell.openExternal() promise rejection with .catch(); attach 'error' listeners to gnome/kcmshell spawns before unref() so a missing executable can't throw unhandled. - screenSharing/serverViewScreenSharing.ts: reset cached init state on provider init failure so later requests can retry instead of reusing a rejected promise until restart. - telephony/dialpad.ts: guard webContents.send against a destroyed handle. Data integrity: - PersistableValues.ts: preserve persisted telephonyGlobalShortcutConfig in the >=4.14.0 migration instead of resetting it to defaults on upgrade. UI / i18n: - TelephonyDiagnostics.tsx: catch rejected get-diagnostics IPC and set a controlled empty state instead of leaking an unhandled rejection. - i18n/ar, i18n/es: split reservedAccelerator into reservedByApp / reservedByOS to match the runtime keys (other locales fall back to en). QA tooling / installer: - validate-flows.mjs: enforce qase_id PRESENCE (key may be null per the flow contract) rather than truthiness, which would have rejected every existing flow; handle CRLF frontmatter delimiters. - export-qase-csv.mjs: normalize CRLF on read for cross-platform parsing. - package.json: declare yaml as a devDependency (used by export-qase-csv). - msiProjectCreated.js: fail fast on telephony RegWrite errors in the WriteTelephonyCapabilities custom action. Tests: - main.spec.ts: mock shell.openExternal as a resolved promise. - dialpad.spec.ts / deepLinks/main.spec.ts: add isDestroyed() to webContents mocks for the new destroyed-handle guard. - TelephonyGlobalShortcut.spec.tsx: add configurable:true so the process.platform restore in afterAll doesn't throw. Rejected: - Rename telephony/main.spec.ts -> main.main.spec.ts: false positive. jest testMatch already routes src/**/main.spec.ts to the main-process project; the rename would break discovery. Verified: tsc clean, lint clean, full suite 456 passed / 2 skipped / 0 failed, validate-flows passes all 14 telephony flows.
diff --git a/build/msiProjectCreated.js b/build/msiProjectCreated.js
@@ -210,7 +210,7 @@ exports.default = async function msiProjectCreated(projectFile) {
       Return="check">
       <![CDATA[
         On Error Resume Next
-        Dim shell, installDir, exePath
+        Dim shell, installDir, exePath, writeErr
 
         Set shell = CreateObject("WScript.Shell")
         installDir = Session.Property("CustomActionData")
@@ -224,6 +224,11 @@ exports.default = async function msiProjectCreated(projectFile) {
         exePath = Chr(34) & installDir & "Rocket.Chat.exe" & Chr(34) & " " & Chr(34) & "%1" & Chr(34)
 
         shell.RegWrite "HKLM\\SOFTWARE\\Classes\\RocketChat.tel\\", "URL:Rocket.Chat Telephony", "REG_SZ"
+        If Err.Number <> 0 Then
+          writeErr = Err.Description
+          Err.Clear
+          Err.Raise 1, "WriteTelephonyCapabilities", "Failed to write HKLM\\SOFTWARE\\Classes\\RocketChat.tel: " & writeErr
+        End If
         shell.RegWrite "HKLM\\SOFTWARE\\Classes\\RocketChat.tel\\URL Protocol", "", "REG_SZ"
         shell.RegWrite "HKLM\\SOFTWARE\\Classes\\RocketChat.tel\\DefaultIcon\\", installDir & "Rocket.Chat.exe,0", "REG_SZ"
         shell.RegWrite "HKLM\\SOFTWARE\\Classes\\RocketChat.tel\\shell\\open\\command\\", exePath, "REG_SZ"
@@ -239,6 +244,11 @@ exports.default = async function msiProjectCreated(projectFile) {
         shell.RegWrite "HKLM\\SOFTWARE\\Rocket.Chat\\Capabilities\\URLAssociations\\tel", "RocketChat.tel", "REG_SZ"
         shell.RegWrite "HKLM\\SOFTWARE\\Rocket.Chat\\Capabilities\\URLAssociations\\callto", "RocketChat.callto", "REG_SZ"
         shell.RegWrite "HKLM\\SOFTWARE\\RegisteredApplications\\Rocket.Chat", "Software\\Rocket.Chat\\Capabilities", "REG_SZ"
+        If Err.Number <> 0 Then
+          writeErr = Err.Description
+          Err.Clear
+          Err.Raise 1, "WriteTelephonyCapabilities", "Failed to write HKLM\\SOFTWARE\\RegisteredApplications\\Rocket.Chat: " & writeErr
+        End If
       ]]>
     </CustomAction>
 
diff --git a/package.json b/package.json
@@ -151,7 +151,8 @@
     "ts-jest": "~29.1.4",
     "ts-node": "~10.9.2",
     "typescript": "~5.7.3",
-    "xvfb-maybe": "~0.2.1"
+    "xvfb-maybe": "~0.2.1",
+    "yaml": "^1.10.2"
   },
   "optionalDependencies": {
     "fsevents": "2.3.3"
diff --git a/qa/scripts/export-qase-csv.mjs b/qa/scripts/export-qase-csv.mjs
@@ -78,7 +78,7 @@ const extractSection = (content, heading) => {
 };
 
 const parseFlow = (filePath) => {
-  const content = fs.readFileSync(filePath, 'utf8');
+  const content = fs.readFileSync(filePath, 'utf8').replace(/\r\n/g, '\n');
   const frontmatterMatch = content.match(/^---\n([\s\S]*?)\n---/);
 
   if (!frontmatterMatch) {
diff --git a/qa/scripts/validate-flows.mjs b/qa/scripts/validate-flows.mjs
@@ -52,7 +52,7 @@ const errors = [];
 
 const parseFlow = (filePath) => {
   const content = fs.readFileSync(filePath, 'utf8');
-  const frontmatterMatch = content.match(/^---\n([\s\S]*?)\n---/);
+  const frontmatterMatch = content.match(/^---\r?\n([\s\S]*?)\r?\n---/);
 
   if (!frontmatterMatch) {
     throw new Error('missing YAML frontmatter');
@@ -154,6 +154,10 @@ if (!fs.existsSync(flowsPath)) {
             errors.push(`${file}: frontmatter.qase.${field} is required`);
           }
         }
+        // qase_id must be present but may be null for not-yet-imported flows.
+        if (!('qase_id' in frontmatter.qase)) {
+          errors.push(`${file}: frontmatter.qase.qase_id is required`);
+        }
       }
 
       if (stepRows.length === 0) {
diff --git a/src/app/PersistableValues.ts b/src/app/PersistableValues.ts
@@ -221,8 +221,12 @@ export const migrations = {
       (before as Partial<PersistableValues_4_14_0>).telephonyPreferredServer ??
       null,
     telephonyGlobalShortcutConfig: {
-      enabled: false,
-      accelerator: null,
+      enabled:
+        (before as Partial<PersistableValues_4_14_0>)
+          .telephonyGlobalShortcutConfig?.enabled ?? false,
+      accelerator:
+        (before as Partial<PersistableValues_4_14_0>)
+          .telephonyGlobalShortcutConfig?.accelerator ?? null,
     },
   }),
   '>=4.15.0': (before: PersistableValues_4_14_0): PersistableValues_4_15_0 => ({
diff --git a/src/deepLinks/main.spec.ts b/src/deepLinks/main.spec.ts
@@ -190,6 +190,7 @@ describe('deepLinks/main.ts', () => {
   describe('performTelephonyCall', () => {
     const mockWebContents = {
       send: jest.fn(),
+      isDestroyed: jest.fn(() => false),
     };
 
     const mockLink: TelephonyLink = {
@@ -500,6 +501,7 @@ describe('deepLinks/main.ts', () => {
 
     const mockWebContents = {
       send: jest.fn(),
+      isDestroyed: jest.fn(() => false),
       loadURL: jest.fn(),
     };
 
@@ -655,6 +657,7 @@ describe('deepLinks/main.ts', () => {
 
     const mockWebContents = {
       send: jest.fn(),
+      isDestroyed: jest.fn(() => false),
       loadURL: jest.fn(),
     };
 
diff --git a/src/i18n/ar.i18n.json b/src/i18n/ar.i18n.json
@@ -32,7 +32,8 @@
         "save": "حفظ",
         "clear": "مسح",
         "registered": "تم تسجيل الاختصار",
-        "reservedAccelerator": "{{accelerator}} محجوز بواسطة Rocket.Chat أو نظام التشغيل لديك."
+        "reservedByApp": "{{accelerator}} مستخدم بالفعل بواسطة Rocket.Chat. اختر تركيبة أخرى.",
+        "reservedByOS": "{{accelerator}} محجوز بواسطة نظام التشغيل لديك. اختر تركيبة أخرى."
       }
     }
   },
diff --git a/src/i18n/es.i18n.json b/src/i18n/es.i18n.json
@@ -298,7 +298,8 @@
         "save": "Guardar",
         "clear": "Borrar",
         "registered": "Atajo registrado",
-        "reservedAccelerator": "{{accelerator}} está reservado por Rocket.Chat o por tu sistema operativo."
+        "reservedByApp": "{{accelerator}} ya está en uso por Rocket.Chat. Elige otra combinación.",
+        "reservedByOS": "{{accelerator}} está reservado por tu sistema operativo. Elige otra combinación."
       },
       "clearPermittedScreenCaptureServers": {
         "title": "Borrar permisos de captura de pantalla",
diff --git a/src/screenSharing/serverViewScreenSharing.ts b/src/screenSharing/serverViewScreenSharing.ts
@@ -55,7 +55,12 @@ const initializeProvider = (): Promise<void> => {
 
     providerReady = true;
     console.log(`Server view screen sharing: using ${provider.type} provider`);
-  })();
+  })().catch((error) => {
+    initPromise = null;
+    providerReady = false;
+    provider = null;
+    throw error;
+  });
 
   return initPromise;
 };
diff --git a/src/telephony/__tests__/dialpad.spec.ts b/src/telephony/__tests__/dialpad.spec.ts
@@ -46,7 +46,8 @@ const mockState = (state: {
 
 const stubWebContents = () => {
   const send = jest.fn();
-  getWebContentsByServerUrlMock.mockReturnValue({ send } as any);
+  const isDestroyed = jest.fn(() => false);
+  getWebContentsByServerUrlMock.mockReturnValue({ send, isDestroyed } as any);
   return { send };
 };
 
diff --git a/src/telephony/dialpad.ts b/src/telephony/dialpad.ts
@@ -125,6 +125,10 @@ export const openTelephonyDialpad = async (
       return;
     }
 
+    if (webContents.isDestroyed()) {
+      return;
+    }
+
     webContents.send('telephony/call-requested', {
       phoneNumber: link.phoneNumber,
       rawUri: link.rawUri,
diff --git a/src/telephony/main.spec.ts b/src/telephony/main.spec.ts
@@ -56,7 +56,7 @@ jest.mock('electron', () => {
       isSupported: jest.fn(() => true),
     }),
     shell: {
-      openExternal: jest.fn(),
+      openExternal: jest.fn().mockResolvedValue(undefined),
     },
   };
 });
diff --git a/src/telephony/main.ts b/src/telephony/main.ts
@@ -330,12 +330,10 @@ const buildWindowsDefaultAppsUri = (): string => {
 
 const openSystemDefaultAppsSettings = (): void => {
   if (process.platform === 'win32') {
-    try {
-      void shell.openExternal(buildWindowsDefaultAppsUri());
-    } catch (error) {
+    void shell.openExternal(buildWindowsDefaultAppsUri()).catch((error) => {
       logger.warn('Failed to open Windows default apps settings');
       logger.warn(error);
-    }
+    });
   } else if (process.platform === 'darwin') {
     // macOS: Launch Services already claimed tel: via app.setAsDefaultProtocolClient;
     // no System Settings pane exists for default tel handler.
@@ -350,21 +348,31 @@ const openSystemDefaultAppsSettings = (): void => {
         desktop.includes('UNITY') ||
         desktop.includes('CINNAMON')
       ) {
-        spawn('gnome-control-center', ['default-apps'], {
+        const child = spawn('gnome-control-center', ['default-apps'], {
           detached: true,
           stdio: 'ignore',
-        }).unref();
+        });
+        child.on('error', (error: NodeJS.ErrnoException) => {
+          logger.warn('Failed to open Linux default apps settings');
+          logger.warn(error);
+        });
+        child.unref();
       } else if (desktop.includes('KDE') || desktop.includes('PLASMA')) {
         const child = spawn('kcmshell5', ['componentchooser'], {
           detached: true,
           stdio: 'ignore',
         });
         child.on('error', (error: NodeJS.ErrnoException) => {
           if (error.code === 'ENOENT') {
-            spawn('kcmshell6', ['componentchooser'], {
+            const fallback = spawn('kcmshell6', ['componentchooser'], {
               detached: true,
               stdio: 'ignore',
-            }).unref();
+            });
+            fallback.on('error', (fallbackError: NodeJS.ErrnoException) => {
+              logger.warn('Failed to open Linux default apps settings');
+              logger.warn(fallbackError);
+            });
+            fallback.unref();
           }
         });
         child.unref();
diff --git a/src/ui/components/SettingsView/features/TelephonyDiagnostics.tsx b/src/ui/components/SettingsView/features/TelephonyDiagnostics.tsx
@@ -70,6 +70,8 @@ export const TelephonyDiagnostics = () => {
     try {
       const result = await invoke('telephony/get-diagnostics');
       setDiagnostics(result);
+    } catch {
+      setDiagnostics(null);
     } finally {
       setLoading(false);
     }
diff --git a/src/ui/components/SettingsView/features/TelephonyGlobalShortcut.spec.tsx b/src/ui/components/SettingsView/features/TelephonyGlobalShortcut.spec.tsx
@@ -79,11 +79,17 @@ const originalPlatform = process.platform;
 
 describe('TelephonyGlobalShortcut', () => {
   beforeAll(() => {
-    Object.defineProperty(process, 'platform', { value: 'linux' });
+    Object.defineProperty(process, 'platform', {
+      value: 'linux',
+      configurable: true,
+    });
   });
 
   afterAll(() => {
-    Object.defineProperty(process, 'platform', { value: originalPlatform });
+    Object.defineProperty(process, 'platform', {
+      value: originalPlatform,
+      configurable: true,
+    });
   });
 
   it('saves an accelerator captured from a key chord', () => {
diff --git a/src/ui/main/serverView/index.ts b/src/ui/main/serverView/index.ts
@@ -127,7 +127,9 @@ export const setupServerViewPermissionHandler = (
 ): void => {
   guestWebContents.session.setPermissionRequestHandler(
     async (_webContents, permission, callback, details) => {
-      console.log('Permission request', permission, details);
+      if (process.env.NODE_ENV === 'development') {
+        console.log('Permission request', permission, details);
+      }
       switch (permission) {
         case 'media': {
           const { mediaTypes = [] } = details as MediaAccessPermissionRequest;
@@ -149,15 +151,22 @@ export const setupServerViewPermissionHandler = (
           return;
 
         case 'openExternal': {
-          if (!(details as OpenExternalPermissionRequest).externalURL) {
+          const { externalURL } = details as OpenExternalPermissionRequest;
+          if (!externalURL) {
             callback(false);
             return;
           }
 
-          const allowed = await isProtocolAllowed(
-            (details as OpenExternalPermissionRequest).externalURL as string
-          );
-          callback(allowed);
+          try {
+            const allowed = await isProtocolAllowed(externalURL);
+            callback(allowed);
+          } catch (error) {
+            console.error(
+              'Failed to validate external protocol request:',
+              error
+            );
+            callback(false);
+          }
           return;
         }
 
diff --git a/src/videoCallWindow/ipc.ts b/src/videoCallWindow/ipc.ts
@@ -134,10 +134,20 @@ const restoreServerViewHandler = async (
 
   // The shared-session teardown reset the permission handler to deny-all,
   // which also kills permission prompts on the live main webview. Restore it.
-  const rootWindow = await getRootWindow();
-  // Re-check after the await: getRootWindow yields the event loop.
-  if (serverWc && !serverWc.isDestroyed() && rootWindow) {
-    setupServerViewPermissionHandler(serverWc, rootWindow);
+  // getRootWindow() can reject during teardown/before-quit (root window not
+  // initialized or already destroyed); only the permission-handler restore is
+  // skipped on failure — the display-media restore above always runs.
+  try {
+    const rootWindow = await getRootWindow();
+    // Re-check after the await: getRootWindow yields the event loop.
+    if (serverWc && !serverWc.isDestroyed() && rootWindow) {
+      setupServerViewPermissionHandler(serverWc, rootWindow);
+    }
+  } catch (error) {
+    console.warn(
+      'Video call window: could not restore server-view permission handler',
+      error
+    );
   }
 };
 
@@ -441,14 +451,26 @@ const openVideoCallWindow = async (
     validUrl.protocol
   );
 
-  if (validUrl.hostname.match(/(\.)?g\.co$/)) {
+  // Validate the protocol up front (fail closed). This must run BEFORE the
+  // g.co external-open special-case so a disallowed protocol (e.g. ftp://)
+  // can never reach openExternal.
+  if (!allowedProtocols.includes(validUrl.protocol)) {
+    throw new Error(
+      `Invalid video call URL protocol: ${validUrl.protocol}. Only http: and https: are allowed.`
+    );
+  }
+
+  // Exact host match (or a true subdomain of g.co) — avoids overmatching
+  // hostnames like `evilg.co` that a `(\.)?g\.co$` regex would accept.
+  if (validUrl.hostname === 'g.co' || validUrl.hostname.endsWith('.g.co')) {
     console.log(
       'Video call window: Google URL detected, opening externally instead of internal window'
     );
     openExternal(validUrl.toString());
     return;
   }
-  if (allowedProtocols.includes(validUrl.protocol)) {
+
+  {
     const mainWindow = await getRootWindow();
     const winBounds = await mainWindow.getNormalBounds();
 

Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,7 @@ const errors = [];`
`52`	`52`
`53`	`53`	`const parseFlow = (filePath) => {`
`54`	`54`	`const content = fs.readFileSync(filePath, 'utf8');`
`55`		`- const frontmatterMatch = content.match(/^---\n([\s\S]*?)\n---/);`
	`55`	`+ const frontmatterMatch = content.match(/^---\r?\n([\s\S]*?)\r?\n---/);`
`56`	`56`
`57`	`57`	`if (!frontmatterMatch) {`
`58`	`58`	`throw new Error('missing YAML frontmatter');`
`@@ -154,6 +154,10 @@ if (!fs.existsSync(flowsPath)) {`
`154`	`154`	errors.push(`${file}: frontmatter.qase.${field} is required`);
`155`	`155`	`}`
`156`	`156`	`}`
	`157`	`+ // qase_id must be present but may be null for not-yet-imported flows.`
	`158`	`+ if (!('qase_id' in frontmatter.qase)) {`
	`159`	+ errors.push(`${file}: frontmatter.qase.qase_id is required`);
	`160`	`+ }`
`157`	`161`	`}`
`158`	`162`
`159`	`163`	`if (stepRows.length === 0) {`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,8 @@`
`32`	`32`	`"save": "حفظ",`
`33`	`33`	`"clear": "مسح",`
`34`	`34`	`"registered": "تم تسجيل الاختصار",`
`35`		`- "reservedAccelerator": "{{accelerator}} محجوز بواسطة Rocket.Chat أو نظام التشغيل لديك."`
	`35`	`+ "reservedByApp": "{{accelerator}} مستخدم بالفعل بواسطة Rocket.Chat. اختر تركيبة أخرى.",`
	`36`	`+ "reservedByOS": "{{accelerator}} محجوز بواسطة نظام التشغيل لديك. اختر تركيبة أخرى."`
`36`	`37`	`}`
`37`	`38`	`}`
`38`	`39`	`},`