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

Automatically inject preload scripts #359

Closed
wants to merge 19 commits into from

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Jul 29, 2021

With this PR, we automatically inject the correct preload scripts via the Electron session.setPreloads() API.

This has a number of advantages:

  • Lots of code removed and simplified
  • Users no longer have to supply appName in any process
  • Users no longer have to consider preload scripts unless they're bundling the main process
  • When bundling, no more issues with incorrect target for preload (init() crashes in preload script #351)
  • Renderer code no longer needs to reference Electron so no need to add electron to externals (Renderer fails on 'fs' requirement #355)
  • No longer need to 'ping' the main process to check the SDK has been enabled

Disadvantages

  • Requires a major version bump as it contains breaking changes
  • The session.setPreloads() API was added in Electron v2 so that would become our minimum supported version (previously v1.8)
    • Electron v1.8.8 reached EOL 3 years ago
  • preload scripts have to be available on disk which presents some issues if the main process is bundled. Currently preload scripts get transpiled to variables in code and are written to userData directory at runtime
  • If a user is already using the session.setPreloads() API, they will likely overwrite our preload scripts. I've added a link to the docs where we can help users diagnose and fix the issues they may encounter.

@timfish timfish requested review from jan-auer and HazAT and removed request for HazAT and jan-auer July 29, 2021 10:20
@timfish timfish changed the title Automatically inject preload to renderers Automatically inject preload scripts Jul 29, 2021
@timfish timfish marked this pull request as ready for review July 29, 2021 16:01
@timfish timfish requested review from jan-auer and HazAT July 29, 2021 16:01
Copy link
Contributor

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

From both Electrons perspective and Slacks this is unsafe. We don't want JS written to disk and arbitrarily loaded without any form of validation, this bypasses gatekeeper and any other kind of signature validation.

IMO no tool or framework should make architectural decisions like this for apps by default, this kind of thing (if it had happened without us noticing) could cause untold security issues for downstream apps that the Sentry SDK might not consider but we definitely do.

Users no longer have to supply appName in any process

You can solve this using additionalArguments in newly created webContents instead

No longer need to 'ping' the main process to check the SDK has been enabled

Can you provide more context on why this ping is needed? I might have alternatives to remove it

@timfish
Copy link
Collaborator Author

timfish commented Aug 3, 2021

IMO no tool or framework should make architectural decisions like this for apps by default, this kind of thing (if it had happened without us noticing) could cause untold security issues for downstream apps that the Sentry SDK might not consider but we definitely do.

I posted this PR to the Electron Discord security channel specifically because I doubted it was a sensible thing to do!

You can solve this using additionalArguments in newly created webContents instead

appName is only required in the renderer with Electron pre v9 because it's required when calling crashReporter.start().

I didn't know about additionalArguments and it looks like it's been there for years.

The problem with additionalArguments is that this SDK does not get involved in creating BrowserWindows, and there's currently no way to intercept their creation (electron/electron#13846).

It's probably simpler to just require passing appName in the renderer if you're using older versions of Electron 🤷‍♂️.

Can you provide more context on why this ping is needed? I might have alternatives to remove it

I think it is there mainly to improve developer experience. If the renderer cannot get an IPC response from the main process it logs a console.warn reminding devs that you also need to init the SDK in the main process if you want anything to work.

@timfish
Copy link
Collaborator Author

timfish commented Aug 6, 2021

Closed in favour of #361

@timfish timfish closed this Aug 6, 2021
@timfish timfish deleted the feat/preload branch August 24, 2021 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants