fraserxu / electron-pdf

📄 A command line tool to generate PDF from URL, HTML or Markdown files.
MIT License
1.24k stars 136 forks source link

Enable context isolation #290

Closed kilburn closed 3 years ago

kilburn commented 3 years ago

Electron-pdf was already using a single ipcApi object to expose all functionality required in the web page context. However, the ipcApi object itself was referenced directly, without going through the contextBridge API.

Here we ensure that context isolation is enabled and share the ipcApi object with the website ("main world") context. Note that having context isolation enabled is now the default, so this should be the safer option moving forward.

Fixes #289

kilburn commented 3 years ago

I have tested this with out application and it works fine.

However, I can see this becoming an issue for whoever is using the trustRemoteContent option. Despite that, on the electron side they seem determined to remove that option so I am hesitant of doing more gymnastics to maintain an option that makes electron-pdf less secure and that will have to be dropped later anyway.

codecounselor commented 3 years ago

However, I can see this becoming an issue for whoever is using the trustRemoteContent option. Despite that, on the electron side they seem determined to remove that option so I am hesitant of doing more gymnastics to maintain an option that makes electron-pdf less secure and that will have to be dropped later anyway.

I think that's fine, if people are setting that option they'll just have to reconcile that when the upgrade. I only added it to keep backwards compatibility when I started setting nodeIntegration to false.

https://github.com/fraserxu/electron-pdf/commit/86a02917270bd3e5bf4363a0c0646ec5b1975089

codecounselor commented 3 years ago

Can you just bump the version to 10.0.2 in package.json and then I'll merge and release this to NPM?

codecounselor commented 3 years ago

Also, it's been a while since a release was done, so If you care to get on the latest version of electron feel free to bump that and the version to 12.0.0. If you do that, please also add a new line item in the Versioning section in the Readme.

image

codecounselor commented 3 years ago

I just updated master with v15 and this change exists there, so this can be closed.