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

nodeIntegrationを無効にする&sandbox=trueにする #1017

Closed
MT224244 opened this issue Nov 5, 2022 · 6 comments · Fixed by #1042 or #2074
Closed

nodeIntegrationを無効にする&sandbox=trueにする #1017

MT224244 opened this issue Nov 5, 2022 · 6 comments · Fixed by #1042 or #2074

Comments

@MT224244
Copy link
Contributor

MT224244 commented Nov 5, 2022

内容

現状、Electronの nodeIntegration が有効になっていますが、これはセキュリティ的に大変よろしくないので、無効にするべきです。
また、 #1002 で少し触れられている sandbox も有効にするべきだと思います。

Pros 良くなる点

セキュリティの強度が上がる

Cons 悪くなる点

renderer側のコードでNodeJSの機能が使用できなくなる

実現方法

renderer側のコードでNodeJSの機能が使われている箇所を、使用しないコードに置き換える。
sandbox も有効にする場合は、preloadでも使わないようにする。

その他

試しに nodeIntegration を無効にしてみたところ、特に問題なく動いていそうでした。
というより、fsモジュールは有効だろうが無効だろうが使えなかったので、もしかしたら既に勝手に無効になっているのかもしれません…。

EditorHome.vue 等でpathモジュールが使われていたりしますが、これはpolyfill的なのが差し込まれていそうです。
webpackの依存関係にある node-libs-browser の中身を見てみると、fsはnullでpathには path-browserify が使われているようです。
これは #1000 を実行する場合に少し問題になりそうです。
image

sandbox を有効にすると流石に module not found エラーがでました。

@Hiroshiba
Copy link
Member

issue作成ありがとうございます!
なるほどです、いつの間にか依存を取り除けてたんですね!!

依存を取り除けてるっぽいのであれば話は簡単で、nodeIntegration無効にとても賛成です。

sandbox を有効にすると流石に module not found エラーがでました。

確認なのですが、こちらはsandbox=true時にfsやpathをimportするとエラーが出る、という認識で合っていそうでしょうか。

@MT224244
Copy link
Contributor Author

MT224244 commented Nov 6, 2022

sandbox=true時にfsやpathをimportするとエラーが出る

そうです!
sandboxを有効にするとpreloadスクリプト内で一部を除いたNodeJSの機能が使えなくなるので、現在importされているfsとpathがエラーになります。

もう読んだことがあるとは思いますが、詳細はsandboxのドキュメントをご覧ください。

@Hiroshiba
Copy link
Member

Hiroshiba commented Nov 6, 2022

なるほどです!preload.ts見てみた感じ、fsとpathはまあまあ使われてますが、全部メイン側に移行しても良さそうだなと感じました!

@Hiroshiba
Copy link
Member

がマージされました・・・!

@MT224244
Copy link
Contributor Author

あ、sandboxがまだoffなのでこれは開いたままの方がいいかも…?

@Hiroshiba
Copy link
Member

タイトル変えてReopenしますか!

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