Fixes doyensec#58: Update checks based on latest webPreferences defaults

I went through https://www.electronjs.org/docs/breaking-changes and looked for all "Default changed" changes. These now respect the Electron version if known.
baltpeter · May 23, 2020 · a0e8f31 · a0e8f31
1 parent 85896b8
commit a0e8f31
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 14 deletions.
diff --git a/src/finder/checks/AtomicChecks/ContextIsolationJSCheck.js b/src/finder/checks/AtomicChecks/ContextIsolationJSCheck.js
@@ -1,3 +1,4 @@
+import { lt } from 'semver';
 import { sourceTypes } from '../../../parser/types';
 import { severity, confidence } from '../../attributes';
 
@@ -9,7 +10,7 @@ export default class ContextIsolationJSCheck {
     this.shortenedURL = "https://git.io/Jeu1p";
   }
 
-  match(astNode, astHelper, scope){
+  match(astNode, astHelper, scope, electron_version){
     if (astNode.type !== 'NewExpression') return null;
     if (astNode.callee.name !== 'BrowserWindow' && astNode.callee.name !== 'BrowserView') return null;
 
@@ -30,26 +31,25 @@ export default class ContextIsolationJSCheck {
         false,
         node => (node.key.value === 'contextIsolation' || node.key.name === 'contextIsolation'));
 
-      //At the time of writing this check, you always need contextIsolation (trust us!)  
-      //if (preload.length > 0) { 
+      // Prior to Electron 5.0, `contextIsolation` had to be explicitly set to `true`.
       if (contextIsolation.length > 0) {
         for (const node of contextIsolation) {
           // in practice if there are two keys with the same name, the value of the last one wins
           // but technically it is an invalid json
           // just to be on the safe side show a warning if any value is insecure
-          if(node.value.value !== true) {
+          if(node.value.value === false || (lt(electron_version, '5.0.0') && node.value.value !== true)) {
             location.push({ line: node.key.loc.start.line, column: node.key.loc.start.column, id: this.id, description: this.description, shortenedURL: this.shortenedURL, severity: severity.HIGH, confidence: confidence.FIRM, manualReview: false });
           }
         }
-      } else {
+      } else if (lt(electron_version, '5.0.0')) {
         location.push({ line: astNode.loc.start.line, column: astNode.loc.start.column, id: this.id, description: this.description, shortenedURL: this.shortenedURL, severity: severity.HIGH, confidence: confidence.FIRM, manualReview: false });
       }
-      
-    } else {
-      //No webpreferences
+
+    } else if (lt(electron_version, '5.0.0')) {
+      //No webpreferences and Electron < 5.0.0
       location.push({ line: astNode.loc.start.line, column: astNode.loc.start.column, id: this.id, description: this.description, shortenedURL: this.shortenedURL, severity: severity.HIGH, confidence: confidence.FIRM, manualReview: false });
     }
 
     return location;
   }
-}
+}
diff --git a/src/finder/checks/AtomicChecks/NodeIntegrationJSCheck.js b/src/finder/checks/AtomicChecks/NodeIntegrationJSCheck.js
@@ -1,3 +1,4 @@
+import { lt } from 'semver';
 import { sourceTypes } from '../../../parser/types';
 import { severity, confidence } from '../../attributes';
 
@@ -13,7 +14,7 @@ export default class NodeIntegrationJSCheck {
   //nodeIntegrationInWorker Boolean (optional) - Whether node integration is enabled in web workers. Default is false
   //nodeIntegrationInSubFrames Boolean (optional) - Whether node integration is enabled in in sub-frames such as iframes. Default is false
 
-  match(astNode, astHelper, scope){
+  match(astNode, astHelper, scope, electron_version){
     if (astNode.type !== 'NewExpression') return null;
     if (astNode.callee.name !== 'BrowserWindow' && astNode.callee.name !== 'BrowserView') return null;
 
@@ -37,7 +38,8 @@ export default class NodeIntegrationJSCheck {
         locations = locations.concat(loc);
     }
 
-    if (!nodeIntegrationFound) {
+    // Electron versions prior to 5.0.0 defaulted `nodeIntegration` to `true` if not set.
+    if (lt(electron_version, '5.0.0') && !nodeIntegrationFound) {
       locations.push({ line: astNode.loc.start.line, column: astNode.loc.start.column, id: this.id, description: this.description,  shortenedURL: this.shortenedURL, severity: severity.HIGH, confidence: confidence.FIRM, manualReview: false });
     }
 
@@ -61,7 +63,7 @@ export default class NodeIntegrationJSCheck {
         if ((node.key.value === "sandbox" || node.key.name === "sandbox") && isIdentifier) continue;
         if ((nodeIntegrationStrings.includes(node.key.value) || nodeIntegrationStrings.includes(node.key.name)) && !isIdentifier) continue;
       }
-        
+
       locations.push({
         line: node.key.loc.start.line,
         column: node.key.loc.start.column,

diff --git a/src/finder/checks/AtomicChecks/RemoteModuleJSCheck.js b/src/finder/checks/AtomicChecks/RemoteModuleJSCheck.js
@@ -1,3 +1,4 @@
+import { lt } from 'semver';
 import { sourceTypes } from '../../../parser/types';
 import { severity, confidence } from '../../attributes';
 
@@ -9,7 +10,7 @@ export default class RemoteModuleJSCheck {
     this.shortenedURL = "https://git.io/JvqrQ";
   }
 
-  match(astNode, astHelper, scope){
+  match(astNode, astHelper, scope, electron_version){
     if (astNode.type !== 'NewExpression') return null;
     if (astNode.callee.name !== 'BrowserWindow' && astNode.callee.name !== 'BrowserView') return null;
 
@@ -36,7 +37,7 @@ export default class RemoteModuleJSCheck {
 
     if (wasFound) {
       return loc;
-    } else { // default is module 'remote' enabled (assuming nodeIntegration:true), which is a misconfiguration
+    } else if(lt(electron_version, '9.0.0')) { // default prior to 9.0.0 was module 'remote' enabled (assuming nodeIntegration:true), which is a misconfiguration
       return [{ line: astNode.loc.start.line, column: astNode.loc.start.column, id: this.id, description: this.description, shortenedURL: this.shortenedURL, severity: severity.MEDIUM, confidence: confidence.TENTATIVE, manualReview: true }];
     }
   }