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

feat: add webFrame.securityOrigin #35163

Conversation

samuelmaddock
Copy link
Member

Description of Change

Adds securityOrigin property to renderer's webFrame module.

Security origin tells a WebFrame which scripting context group it belongs to. A collection of same-site frames created using <iframe>, open(sameSiteUrl), or open('about:blank') will have the same security origin.

A use case for this feature is to determine whether a preload script in an about:blank popup should run if its security origin is the same as the secure host WebContents.

cc @MarshallOfSound @nornagon

Checklist

Release Notes

Notes: Added webFrame.securityOrigin to determine the security origin of the frame.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 1, 2022
@samuelmaddock samuelmaddock added the semver/minor backwards-compatible functionality label Aug 1, 2022
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems kind of minimally useful on the renderer side, as it's not really trustable. Does GetLastCommittedOrigin on the browser side fulfill a similar purpose? https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/render_frame_host.h;l=518;drc=fa230cce5395b280afd13ae362eabd0a680fc72a

@samuelmaddock
Copy link
Member Author

This seems kind of minimally useful on the renderer side, as it's not really trustable. Does GetLastCommittedOrigin on the browser side fulfill a similar purpose? https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/render_frame_host.h;l=518;drc=fa230cce5395b280afd13ae362eabd0a680fc72a

The use case for this feature is intended primarily for preload scripts to gate any code intended for specific sites.

// preload.js
const { webFrame } = require('electron');

if (webFrame.securityOrigin === 'https://example.app.domain/') {
  // Run my app-specific code
}

In the future, it'd be better if we could restrict preload scripts from running at all on certain origins. Much more work would be necessary for that change though.

More research is needed for determining browser-side validation of IPC which will come in a future PR.

@nornagon
Copy link
Member

nornagon commented Aug 5, 2022

Hm, I don't think that's really a super secure way to limit stuff in preload. It's better than what we have now, but it's not watertight as the renderer can theoretically be compromised. This really needs to be done browser-side by limiting preloads on a per-process / per-SiteInstance basis.

I'm 👎 on this API since it seems like the only uses would be incorrect-ish ones.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 25, 2022
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this superseded by #35438?

@samuelmaddock
Copy link
Member Author

is this superseded by #35438?

I think we still need a way to be able to guard preload code from running in certain contexts.

Preload scripts are currently run in all renderer contexts (main world by default). If we'd like to run preload code only in certain contexts—like those matching a certain security origin—we can only do so by guarding code within the preload itself.

example:

if (location.origin === 'https://electronjs.org') {
  // run the code specific for this origin
}

Probably better would be to have some filtering at the point a preload script is registered, but that will require more significant refactoring.

@nornagon
Copy link
Member

Yeah... I'm not sure I feel this is a good way to guard preload code. Your example used location.origin, is that already good enough to do this sort of guarding, at least somewhat? If we're going to introduce a new API intended to be used to selectively run preload code, I don't think this is it.

@nornagon
Copy link
Member

Throwing API DECLINED for the bot

@samuelmaddock
Copy link
Member Author

Closing since we no longer have an immediate need for this. Ultimately we should try to solve this with filtering from the main process if needed later.

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

Successfully merging this pull request may close these issues.

None yet

3 participants