Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: nodeIntegrationInSubFrames for preload scripts #22582

Open
3 tasks done
Jelmerro opened this issue Mar 6, 2020 · 11 comments
Open
3 tasks done

Feature request: nodeIntegrationInSubFrames for preload scripts #22582

Jelmerro opened this issue Mar 6, 2020 · 11 comments

Comments

@Jelmerro
Copy link

Jelmerro commented Mar 6, 2020

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 a feature request that matches the one I want to file, without success.

Problem Description

I want the preload script of a webview to run in ALL subframes of that webview, such as iframes, so that I can detect anchor tags within these subframes. This is required to fix this issue for one of my projects.
Correct me if I am wrong, but it would seem that this was possibly before, but it was considered a bug: #18429
The purpose of the nodeIntegrationInSubFrames is now ill-defined, and does not do what it says it does regarding the preload script, at least according to my interpretation of the documentation:

nodeIntegrationInSubFrames Boolean (optional) - Experimental option for enabling Node.js support in sub-frames such as iframes and child windows.
All your preloads will load for every iframe, you can use process.isMainFrame to determine if you are in the main frame or not.

Proposed Solution

I would like restore this behavior for preload scripts, possibly with a separate setting to enable it. If there is any existing way to run preload scripts for iframes that are inside a webview, please enlighten me, because I have not found any other way to achieve this.

Alternatives Considered

I have tried to run my preload script for iframes inside webview, by enabling nodeIntegrationInSubFrames, but so far without success. After I discovered #18429, I decided it was time to open a feature request in order to resolve this confusing situation.

Additional Information

I would like to answer this question once and for all, because the documentation is now inconsistent with the actual behavior. It would seem that I am not the only one who is confused by this change:
#19260 (comment)

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Apr 21, 2020

I second this, albeit I'm not using webviews (BrowserView). The documentation for nodeIntegrationInSubFrames is confusing and possibly dangerous. I don't want or need nodeIntegration at all, which is best practice anyway. What I do want and absolutely need is the preload script to run in all iframes, because I need to monkey patch some globals (I'm essentially writing a web debugging tool in Electron that renders a website in a BrowserView). I think these two requirements are completely unrelated and it doesn't make sense that they share a single flag. I must assume that if I have nodeIntegration: false but nodeIntegrationInSubFrames: true that the iframes could require Node.js modules but the parent could not? Or is the setting inherited and in this case it would only cause preload to execute for the iframes? In which case the option would have a weird name, because preload scripts always have Node.js enabled. This is not clear at all and might impose a security risk if it does indeed enable nodeIntegration even though my intention was to only allow preload scripts. FWIW in a BrowserView nodeIntegrationInSubFrames does indeed enable preload script for all iframes, but I assume it also enabled nodeIntegration for them.

TLDR: We need an option to enable preload in all nested iframes/frames without also enabling Node.js inside these frames. As is now I would consider the current behavior a security issue and would not consider this a feature request.

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Apr 21, 2020

As is now I would consider the current behavior a security issue and would not consider this a feature request.

The PR that added support for nodeIntegrationInSubFrames wrapped the security warning in an if (process.isMainFrame) {

https://github.com/electron/electron/pull/16425/files#diff-2d7b7c752f838654945dc8366c5f20ab

which is still the case

// Warn about security issues
if (process.isMainFrame) {
const { securityWarnings } = require('@electron/internal/renderer/security-warnings');
securityWarnings(nodeIntegration);
}

This means there is currently no warning when using nodeIntegrationInSubFrames. Also nodeIntegrationInSubFrames implies nodeIntegration because nothing keeps someone from creating a same-origin iframe and executing Node.js in it, even if the root has nodeIntegration disabled.


I don't know if there is a technical limitation, but you can see that the PR was mixing these two concerns (nodeIntegration vs enabling preload).

  bool should_load_preload =
      is_main_frame || is_devtools || allow_node_in_sub_frames;

https://github.com/electron/electron/pull/16425/files#diff-2908f2d45ca07c2f0b1f2c8404928a34

A new option like preloadInSubFrames could decouple the two.

Edit: arguably adding an option preloadSubFrames would also be possible (and maybe more flexible). If set it would load the given file as preload. So you can have an entirely different file instead of using isMainFrame all over the place.

@kksharma1618
Copy link

+1
We need a way to enable preload script inside iframe without enabling nodeIntegration, just like currently its supported in main frame. nodeIntegrationInSubFrames works, but it looks dangerous.

@andreasdj
Copy link
Contributor

When looking at the code changes within the PR (#16425) in which support for enabling preload scripts within iframes were added I think that the issue might be an ill named switch.

It sounds like nodeIntegrationInSubFrames enables node integration, but in code it looks like it allows node in subframes for preload script support. The logic inside the renderer initialization file https://github.com/electron/electron/blob/cd00a36304d4fca4fff85d5dabb27b2f25e0e575/lib/renderer/init.js seems to respect the nodeIntegration switch for the main frame as well as for sub frames and not the nodeIntegrationInSubFrames switch.

If that is the case it’s reasonable that the security warning is only shown for the main frame since all frames use the same switch for enabling/disabling nodeIntegration but for clarity the switch should then be renamed to something else (like the suggested preloadInSubFrames).

I might have missed something so it would be good if someone with a deeper understanding could confirm this, like @MarshallOfSound

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Oct 6, 2020

@andreasdj It does, here's a fiddle https://gist.github.com/Prinzhorn/233b4ca1ddbfd8a80ad3aa86200a14b7

The good news is that nodeIntegration: false has priority over nodeIntegrationInSubFrames so that Node.js is disabled and we don't get a preload (which this feature request is about, both concerns are mixed).

However, if you both are enabled you get Node.js integration in every frame including other origins you don't control.

true + false (iframe doesn't render the stuff that requires Node.js integration)

Selection_669

true + true (the iframe has Node.js integration, I even tested it with index.html served from a server with a completely different origin)

Selection_670

So again, nodeIntegrationInSubFrames isn't simply named wrong, it does two unrelated things, one of which is a security issue. Nobody wants nodeIntegration in arbitrary origins they don't control. That's like an RCE invite.

Edit: I've updated the fiddle to use top.process and top.require. This just demonstrates that nodeIntegrationInSubFrames is implied by nodeIntegration for the same origin because you can just access the APIs via top or parent. https://gist.github.com/19633527249739f4e1e2c38d94667ca6

@andreasdj
Copy link
Contributor

andreasdj commented Oct 6, 2020

To be more specific I don't think the nodeIntegration switch take priority over nodeIntegrationInSubFrames.

I think the nodeIntegration switch alone controls node integration for all frames (main frame as well as sub frames). So I agree that it's not possible to enable node integration for the main frame and at the same time disable it for the sub frames since it's controlled with a single switch.

But it is possible to disable nodeIntegration for all frames and still enable preload script functionality for sub frames (with nodeIntegration disabled) using:

nodeIntegration: false,
nodeIntegrationInSubFrames: true,

This is where it starts to look really confusing and implies that we have enabled node integration in sub frames when we actually have not. That's the reason I think nodeIntegrationInSubFrames is named wrong and is mixing concerns as you have mentioned earlier, since it doesn't control nodeIntegration on it's own.

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Oct 6, 2020

I think the nodeIntegration switch alone controls node integration for all frames (main frame as well as sub frames). So I agree that it's not possible to enable node integration for the main frame and at the same time disable it for the sub frames since it's controlled with a single switch.

Look at my first screenshot. The iframe does not have Node.js integration. I was just demonstrating with the other fiddle that for same-origin frames it is identical from a security perspective because you can still access Node.js APIs via the parent frame.

But I think we all agree (and this conversation confirms) that the current situation is confusing, if not dangerous.

@Jelmerro
Copy link
Author

Jelmerro commented Oct 6, 2020

So for my project I was simply looking for a way to run any kind of code/preload on iframes that are INSIDE of webviews and I found the nodeIntegrationInSubFrames option. I couldn't get it to work in combination with webviews, because of this PR which for some reason considered the documented feature a bug. The docs explicitly mention that this is actually a feature, and still list it as one. Even worse, to even get all of this to work in the first place (if that inconsistent PR wasn't accepted) you would STILL need nodeIntegration enabled to use preloads inside of subframes, which to me, defeats the purpose of the preload script.

Although I wasn't too clear in my initial report, I completely agree with @Prinzhorn in that we need a different way to use preloads in subframes WITHOUT requiring nodeIntegration. A boolean toggle or script location field like preloadInSubFrames could be added, preferably in a consistent way for webviews, BrowserViews and BrowserWindows.

@quanglam2807
Copy link
Contributor

We also need this for our project. An option like preloadInSubFrames would be great.

@megalithic
Copy link

Bump!

@BrandonXLF
Copy link
Contributor

It seems that setting nodeIntegrationInSubFrames in the web preferences of both the WebView and the BrowserWindow enables the preload script for iframes within the webview. See https://gist.github.com/ad93987ddd6b1e40f94f11fa421578b6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants