doyensec / electronegativity

Electronegativity is a tool to identify misconfigurations and security anti-patterns in Electron applications.
Apache License 2.0
972 stars 66 forks source link

How does one ignore LIMIT_NAVIGATION_GLOBAL_CHECK? #85

Closed reZach closed 3 years ago

reZach commented 3 years ago

Describe the bug All of my new-window and will-navigate events are secure and do not need to be notified as errors in the report that's generated by electronegativity. How do I disable LIMIT_NAVIGATION_GLOBAL_CHECK from spitting out an error? I tried /* eng-disable LIMIT_NAVIGATION_GLOBAL_CHECK */ at the top of my file, but the error still persists.

To Reproduce This sample main.js file can suffice to explain my attempts at disabling .

/* eng-disable LIMIT_NAVIGATION_GLOBAL_CHECK */
const {
    app,
    BrowserWindow,
  } = require("electron");
  const path = require("path");
  const isDev = process.env.NODE_ENV === "development";
  const port = 40992; // Hardcoded; needs to match webpack.development.js and package.json
  const selfHost = `http://localhost:${port}`;

  // Keep a global reference of the window object, if you don't, the window will
  // be closed automatically when the JavaScript object is garbage collected.
  let win;

  async function createWindow() {

    // Create the browser window.
    win = new BrowserWindow({
      width: 800,
      height: 600,
      title: "Application is currently initializing...",
      webPreferences: {
        devTools: isDev,
        nodeIntegration: false,
        nodeIntegrationInWorker: false,
        nodeIntegrationInSubFrames: false,
        contextIsolation: true,
        enableRemoteModule: false,
        additionalArguments: [`storePath:${app.getPath("userData")}`],
        preload: path.join(__dirname, "preload.js"), /* eng-disable PRELOAD_JS_CHECK */
        disableBlinkFeatures: "Auxclick"
      }
    });

    // Load app
    win.loadURL(selfHost);

    // Emitted when the window is closed.
    win.on("closed", () => {
      // Dereference the window object, usually you would store windows
      // in an array if your app supports multi windows, this is the time
      // when you should delete the corresponding element.
      win = null;
    });
  }

  // This method will be called when Electron has finished
  // initialization and is ready to create browser windows.
  // Some APIs can only be used after this event occurs.
  app.on("ready", createWindow);

  // Quit when all windows are closed.
  app.on("window-all-closed", () => {
    // On macOS it is common for applications and their menu bar
    // to stay active until the user quits explicitly with Cmd + Q
    if (process.platform !== "darwin") {
      app.quit();
    }
  });

  app.on("activate", () => {
    // On macOS it's common to re-create a window in the app when the
    // dock icon is clicked and there are no other windows open.
    if (win === null) {
      createWindow();
    }
  });

  // https://electronjs.org/docs/tutorial/security#12-disable-or-limit-navigation
  app.on("web-contents-created", (event, contents) => {
    contents.on("will-navigate", (contentsEvent, navigationUrl) => { /* eng-disable LIMIT_NAVIGATION_JS_CHECK  */
      const parsedUrl = new URL(navigationUrl);
      const validOrigins = [selfHost];

      // Log and prevent the app from navigating to a new page if that page's origin is not whitelisted
      if (!validOrigins.includes(parsedUrl.origin)) {
        console.error(
          `The application tried to redirect to the following address: '${parsedUrl}'. This origin is not whitelisted and the attempt to navigate was blocked.`
        );

        contentsEvent.preventDefault();
        return;
      }
    });

    // https://electronjs.org/docs/tutorial/security#13-disable-or-limit-creation-of-new-windows
    contents.on("new-window", (contentsEvent, navigationUrl) => { /* eng-disable LIMIT_NAVIGATION_JS_CHECK */
      const parsedUrl = new URL(navigationUrl);
      const validOrigins = [];

      // Log and prevent opening up a new window
      if (!validOrigins.includes(parsedUrl.origin)) {
        console.error(
          `The application tried to open a new window at the following address: '${navigationUrl}'. This attempt was blocked.`
        );

        contentsEvent.preventDefault();
        return;
      }
    });
  });

Expected behavior I expect the LIMIT_NAVIGATION_GLOBAL_CHECK error not to be logged in the report.

Stacktraces

│ LIMIT_NAVIGATION_GLOBAL_CHECK           │ N/A                                                                         │ 
0:0      │ Missing navigation limits using .on new-window   │
│ HIGH | CERTAIN                          │                                                                             │ 
         │ and will-navigate events                         │
│                                         │                                                                             │ 
         │ https://git.io/JeuMs                             │

Platform (please complete the following information):

Additional context I don't like putting the comment on the first line, I'd like to have the freedom to put it anywhere in my file.

phosphore commented 3 years ago

Hello @reZach and thanks for contributing,

Global Checks can't be disabled using code annotations, since they work on the first pass of Atomic Checks (i.e. making decisions over these).

The LIMIT_NAVIGATION_GLOBAL_CHECK checks if any LIMIT_NAVIGATION_JS_CHECK was found. If it's there, the corresponding global finding is not issued, but if it's missing it does.

    contents.on("new-window", (contentsEvent, navigationUrl) => { /* eng-disable LIMIT_NAVIGATION_JS_CHECK */

On the other hand, the LIMIT_NAVIGATION_JS_CHECK will still be left to remind you to manually review the on callback for bypasses. If you are confident it's safe, you can use the -l flag to disable Global and Atomic Checks selectively (-l LimitNavigationGlobalCheck).

I'll take care of clarifying this in the docs.

reZach commented 3 years ago

Hello @reZach and thanks for contributing,

Global Checks can't be disabled using code annotations, since they work on the first pass of Atomic Checks (i.e. making decisions over these).

The LIMIT_NAVIGATION_GLOBAL_CHECK checks if any LIMIT_NAVIGATION_JS_CHECK was found. If it's there, the corresponding global finding is not issued, but if it's missing it does.

    contents.on("new-window", (contentsEvent, navigationUrl) => { /* eng-disable LIMIT_NAVIGATION_JS_CHECK */

On the other hand, the LIMIT_NAVIGATION_JS_CHECK will still be left to remind you to manually review the on callback for bypasses. If you are confident it's safe, you can use the -l flag to disable Global and Atomic Checks selectively (-l LimitNavigationGlobalCheck).

I'll take care of clarifying this in the docs.

Thank you, I am able to run the command like so to ignore the check.

npx electronegativity -i ./ -x LimitNavigationGlobalCheck