Skip to content
This repository was archived by the owner on Apr 1, 2020. It is now read-only.

Security - Add Content Security Policy #2288

Merged
merged 8 commits into from
Jun 23, 2018

Conversation

bryphe
Copy link
Member

@bryphe bryphe commented Jun 6, 2018

Thanks @CrossR for catching this! There's a CSP error that the latest verison of Electron highlights. We should be setting a content security policy for Electron, to help reduce the risk of malicious script execution.

This change adds a CSP for index.html, and provides an alternate entry point for the webpack-dev-server build (npm run start), since that needs eval.

There's still one problem that needs to be fixed:
image

With the new CSP, we can't load configuration files - they get blocked. We should move to the same strategy we use to load plugins - creating a new vmcontext to run them, so that they are isolated.

@CrossR
Copy link
Member

CrossR commented Jun 19, 2018

Ideally, will we want this in the next release?

@bryphe
Copy link
Member Author

bryphe commented Jun 19, 2018

Ideally! But unfortunately it's cutting it a bit close (and it's easy for this to regress some corner cases, especially around configuration), so I may not have time to complete it by Wednesday.

It's not a 'new' issue per-se, it's been around for a while - Electron is just better about giving us a heads up, so I don't think it necessarily needs to block this release. But we definitely should fix it!

@codecov
Copy link

codecov bot commented Jun 21, 2018

Codecov Report

Merging #2288 into master will decrease coverage by 0.03%.
The diff coverage is 9.09%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2288      +/-   ##
=========================================
- Coverage   37.33%   37.3%   -0.04%     
=========================================
  Files         298     298              
  Lines       12355   12369      +14     
  Branches     1634    1635       +1     
=========================================
+ Hits         4613    4614       +1     
- Misses       7493    7506      +13     
  Partials      249     249
Impacted Files Coverage Δ
...ervices/Configuration/FileConfigurationProvider.ts 22.89% <9.09%> (-1.11%) ⬇️
browser/src/UI/Shell/ShellView.tsx 51.85% <0%> (-14.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e1f322...51a43b7. Read the comment docs.

@bryphe bryphe merged commit 7b15dce into master Jun 23, 2018
@bryphe bryphe deleted the bryphe/security/add-content-security-policy branch June 23, 2018 22:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants