The handler of "setWindowOpenHandler" not fires on Electron 12.0.0

[PurpleHorrorRus]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for an issue that matches the one I want to file, without success.

Issue Details

• Electron Version:
  • 12.0.0
• Operating System:
  • Windows 10 20H2

Expected Behavior

Handler fires and no new window opens

Actual Behavior

Handler fails and a new window opens

To Reproduce

1. Write this code:

window.webContents.setWindowOpenHandler(() => ({ action: "deny" }));

2. Debug or run the application.
3. Press Alt + LMB (or Scroll) on any link in app.

[idanwork]

Hi,
This issue happens on macOS 10.15.6 as well.

Looks like the setWindowOpenHandler doesnt handle <a target="_blank"... elements and only handles window.open(...).

I feel like I've missed something in the documentation about it https://www.electronjs.org/docs/api/window-open
But I cant see reference to cases when the app is not sandboxed and has an anchor element.
BTW the webContents.on("new-window"...) works fine in v12 and previous electron version but it's deprecated.

If an example is required:
https://gist.github.com/idanwork/ecbd1a902250d4a5d2d58d76b069849f

Example output - first click on the anchor, then on the button

[PurpleHorrorRus]

Hi. Yes, I knew about it and used it before Electron 12.0.0. I read the documentation and noticed that a new handler is recommended, but since it doesn't work, I decided to create an issue.

[andreasdj]

I would expect that the given code in the reproduction step:
window.webContents.setWindowOpenHandler(() => ({ action: "deny" }));

also would prevent a new window from being opened if having navigateOnDragDrop enabled and dragging/dropping a url onto the window. But the handler is not called and a new window is indeed opened.

It feels like the same underlying issue so I wont create a new issue for it at this point.

[WildXBird]

12.0.2 still has this problem

[CoryDAsana]

Just stumbled upon this issue on my end. We previously relied on "new-window" as an opportunity to apply an allowlist that guaranteed only trusted content was loaded in a BrowserWindow. Paired with the same check on "will-navigate" and "will-redirect", we had confidence we were covering all possible page navigation.

Seeing "new-window" deprecated in favor of setWindowOpenHandler, I transitioned our code to the new standard, but am now seeing our allowlist isn't covering (ex:) shift-clicks on an anchor tag.

Given the deprecated standard and the new standard have different coverage over a potentially security-critical surface area,

• Could we call this out more explicitly in the documentation, rather than maintaining the misleading proximity of "two ways windows are created" and "how setWindowOpenHandler works" and suggesting setWindowOpenHandler as a new-window replacement?
• Which API should I be leveraging to impose security restrictions on which URLs could be opening in a new window via (ex:) shift+click?

[phosphore]

As a temporary fix before #28498, for anyone looking to prevent navigations to untrusted origins it should still possible to use the web-contents-created event emitted when a new webContents is created and attach then your new-window, will-navigate, and setWindowOpenHandler handlers:

app.on('web-contents-created', (createEvent, contents) => {
  
  contents.on('new-window', newEvent => {
    console.log("Blocked by 'new-window'")
    newEvent.preventDefault();
  });
  
  contents.on('will-navigate', newEvent => {
    console.log("Blocked by 'will-navigate'")
    newEvent.preventDefault()
  });
  
  contents.setWindowOpenHandler(({ url }) => {
    if (url.startsWith("https://doyensec.com/")) {
      setImmediate(() => {
        shell.openExternal(url);
        });
      return { action: 'allow' }
    } else {
      console.log("Blocked by 'setWindowOpenHandler'")
      return { action: 'deny' }
    }
  })
  
});

Link to the Fiddle gist used for testing. This works from 12.0.0 to the latest 12.0.2 as for now.

== 2022 Edit
For a better defense in depth, it's possible to add will-attach-webview to the list of blocked events.

[Delagen]

@codebytere Why this issue closed? I don't see this commit neither in release branch of 12.0.4 nor in changelog.

[VerteDinde]

@Delagen This is now being backported to 12 - it should be in the next 12 release, thanks for the catch! 👍

[ikkisoft]

Just tested on Electron V13 and will-navigate is not protected with the default implementation of setWindowOpenHandler. #27967 (comment) mitigation is still recommended. Stay safe out there!