Fix #1038: Introduce Content Security Policies to app's pages

thamara / time-to-leave

Log work hours and get notified when it's time to leave the office and start to live.

http://timetoleave.app

GNU General Public License v3.0

450 stars 265 forks source link

Fix #1038: Introduce Content Security Policies to app's pages #1044

Closed tupaschoal closed 6 months ago

tupaschoal commented 7 months ago

Related issue

Closes #1038

Context / Background

App was giving out security warnings on the JS console when fired, up as shown here:

What change is being introduced by this PR?

As far as I understood from electron documentation, the Content Security Policy (CSP) when defined prevents a malicious user from modifying/injecting stuff into our script calls. For when we have inline scripts, we could either use unsafe-inline, which would defeat the purpose of the CSP, nonce or a hash. When setting the CSP to just self, it would complain and already give the hash of the inline scripts, so I just used those hashes to not restrict it instead. If we ever change those inline scripts we'll need to update the hash, but I think that is ok.

How will this be tested?

The warnings are gone, I don't really think this was a big problem for us, but at least

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (8c4e573) 79.37% compared to head (6b85894) 79.40%. Report is 11 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1044 +/- ## ========================================== + Coverage 79.37% 79.40% +0.03% ========================================== Files 21 21 Lines 1275 1277 +2 Branches 189 189 ========================================== + Hits 1012 1014 +2 Misses 263 263 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

araujoarthur0 commented 6 months ago

@tupaschoal looks like this hash is only needed because we have a short inline script to set the window.$ variable.

Buuut, I've just tested removing that part and it looks like we no longer need that. I'm not sure if it's because of the electron migration or something else, but taking the inline script out leads to the same behavior on the tool. Then we no longer need the hash on the CSP.

araujoarthur0 commented 6 months ago

@tupaschoal can you please do the same for the other windows?

tupaschoal commented 6 months ago

My bad @araujoarthur0, I somehow thought the issue applied only to the calendar. I've done it for them all now :)

araujoarthur0 commented 6 months ago

\changelog-update Message: Fix [#1044]: Introduce Content Security Policies to app's pages User: tupaschoal