Fix CSP handling

[sulkaharo]

• Replaces the node-cache module with memory-cache, which works with CSP
• Fixes a memory leak with profile caching
• Allow embedded fonts with CSP
• Allow web socket connection to self

[jweismann]

@sulkaharo. Looks good. A couple of comments:

I guess connectSrc: [ "'self'" ] is overkill with your defaultSrc.

Moreover, may I suggest to add

app.use(bodyParser.json({ type: ['json', 'application/csp-report'] })) app.post('/report-violation', (req, res) => { if (req.body) { console.log('CSP Violation: ', req.body) } else { console.log('CSP Violation: No data received!') } res.status(204).end() })

prior to the CSP directive and then also add

upgradeInsecureRequests: true, reportUri: '/report-violation' }, reportOnly: true }));

to the CSP directive allowing us to catch issues in the log.

Finally, in the past I also had to add ``imgSrc: [ "'self'", 'data:'], to the directives too (for report, profile or admin section) but maybe this isn't necessary anymore.

Thanks for your efforts on this. Highly appreciated!

[jweismann]

@PieterGit That was extremely fast:). Thanks. It looks like reportOnly got inside the directive scope. It has to go outside, c.f. the }, after reportUri: '/report-violation'.

[PieterGit]

@jweismann Will fix that tommorow. It took a bit longer than I thought, and I don't want to test this overnight. The branch is available at https://github.com/PieterGit/cgm-remote-monitor/commits/201903_wip_fix_csp

I think this error is causing the /socket.io/?EIO=3&transport=polling&t=XXX 503 (Service Unavailable) on that branch now.

[PieterGit]

Pushed fixes to this branch/PR. Am I right that only the clock stuff requires scriptSrc: ["'self'", "'unsafe-inline'"],? That is the only inline unsafe Javascript I can find, with a search on: https://github.com/nightscout/cgm-remote-monitor/search?q=javascript&type=Code

[jweismann]

@PieterGit

As to unsafe-inline: There are issues in view/index.html with the onload construct, cf:
<link rel="stylesheet" type="text/css" href="css/jquery.tooltips.css" media="none" onload="if(media!='all')media='all'">

My suggestion was to have reportOnly: true while we test this option. This will flag all violations in the log but will still run them so pages will show as usual but log will reveal all violations and they can be handled one by one. Eventually, when pages have been tested one by one we should set reportOnly: false whereby we have bumped our header grade to A :).

Having said that, as long as we have unsafe-inline present we will get a warning about this since this is not recommended. Maybe this link could serve as inspiration for the refactoring of these constructs.

[sulkaharo]

@jweismann What's the rationale behind reportOnly: true? I got websocket errors without the connectSrc and thus added it, but I guess that might have been caused by something else.

[jweismann]

@sulkaharo The rationale is that we can turn CSP on and have all pages behave as usual with reportOnly: true, but besides working as usual all violations to our current specification of directives will be flagged in the log. Thus the violations become transparent and we have two options in dealing with each of the violations either by a refactoring that avoids the violations or if we are OK with it then make the directives less strict so that a given construction will not be flagged as a violation anymore. Once we cannot trigger more violations on our pages we have a set of directives that we can enforce by setting reportOnly: false.

I think that your websocket errors was caused by something else.

[sulkaharo]

Note replacing the node-cache module with memory-cache changed the profile calculations slightly, as it looks like the previous value cache implementation was buggy and didn't actually cache the values. This broke a few tests that assumed IOB is calculated at 8 digit precision at all times, so I changed the IOB math to use three digit precision for the presentation, which should be at least as precise as needed given the smallest available dosing in there market is 0.025 IU / dose. Regardless, this PR needs good testing to ensure IOB is still reported consistently.

[sulkaharo]

Re: websockets, we have to add a directive to allow the client to connect to the site itself, as not all browsers recognize a websocket connection correctly when only "self" is mapped. This is fixed in newer Chrome versions but Safari for example still reports websocket connections to self as a violation. https://outlandish.com/blog/configure-content-security-policy-with-websockets-and-express/ lists how to do this, but looks like setting the HOSTNAME variable for the env has other side effects so that can't be used for this without refactoring. @jweismann do you know if there's a way to have helmet set the csp policy upon request, so we could detect the host from the request? This would be the user friendly way for people who host in Heroku, Azure etc

[jweismann]

@sulkaharo alas, not that I'm aware of.

[jweismann]

BTW, should we add the last two headers just for the sake of completeness say something like:

app.use(helmet.referrerPolicy({ policy: 'no-referrer' }))

app.use(helmet.featurePolicy({ features: { payment: ["'none'"], } }))

[PieterGit]

@jpweisman Just added the referrerPolicy and featurePolicy when CSP is enabled.
I think we should release 0.11.2 with SECURE_CSP set to false and then enable it by default for 0.12.

@sulkaharo I had strange caching issue when upgrading from this branch with CSP enabled to dev (0.11 without this PR and so not working CSP ). The browser kept thinking CSP was enabled, even after restarting the dynos. So we need to check if enabling/disabling CSP works as expected and perhaps even roll back to see if there is no CSP caching issue.

@jpweisman @sulkaharo. Can you please confirm this branch is good to merge to dev and include it in 0.11.2?

[jweismann]

@PieterGit, @sulkaharo Thanks for all your efforts here. I will deploy it during the weekend and see if something looks wrong but here are some initial comments from a quick browsing of the code:

1. I wonder if we should confine the optional redirection to heroku deploys only, i.e. something like:

if (!insecureUseHttp) { app.use((req, res, next) => { if(req.hostname.indexOf('herokuapp.com') > -1) { console.info('Redirecting http traffic to https because INSECURE_USE_HTTP=', insecureUseHttp); if (req.header('x-forwarded-proto') !== 'https') ....

2. I wonder if the name REDIRECT_HTTP_2_HTTPS is easier to understand than INSECURE_USE_HTTP

3. I guess that we should set reportOnly: false before we release so that the rules are indeed enforced if the flag is set to true. While we test (like now) I agree that setting it to true is the right approach.

4. Finally, maybe the HEADER block should be controlled by the active protocol (HTTPS) rather than the flag 'INSECURE_USE_HTTP'.

Hope this is useful input, will get back when I have had time to deploy it.

[sulkaharo]

I think the idea behind having a variable called INSECURE_USE_HTTP and having that disable HTTPS is simply due to the fact that we need to default to a settings that are secure as most people won't configure their sites to almost any degree, so if we default to HTTP, for most users it's effectively the same as not having HTTPS redirects at all and explicitly naming the variable as INSECURE is possibly good way to make Joe Regular to understand they shouldn't touch this (at cost of confusion for experts).

Re: CSP - I realized implementing CSP by default might actually break a lot of scenarios for users, where I've seen all kinds of frame based implementations to see multiple users at the same time and I'm wondering what the impact will be for apps that act as Nightscout containers. And Tesla dashboards etc. CSP is great for protecting multitenant sites from attacks, but in case of Nightscout where each instance is uniquely hosted on per user basis, the attack vectors CSP aims to protect sites against don't really exist with Nightscout. So if we do implement CSP, this needs to be documented extremely clearly for people who have no idea how web security works and it might be better to actually default to CSP being turned off.

[jweismann]

@sulkaharo Regarding CSP, I didn't mean to turn it on by default (i.e. SECURE_CSP should default to false) but I mean that if its indeed turned on (SECURE_CSP explicitly set to true by the user) then we shouldn't just report violations in the console log but actually enforce it. Sorry if this was unclear.

Your point regarding the many variables that most users probably don't touch is indeed a good point.

[PieterGit]

@sulkaharo @jweismann Added SECURE_CSP_REPORT_ONLY (default false). So, my view for the defaults:

0.11.2: SECURE_CSP=false and SECURE_CSP_REPORT_ONLY=false
0.12.0-dev: SECURE_CSP=true and SECURE_CSP_REPORT_ONLY=true (not enforcing, testing)
0.12.0: SECURE_CSP=true and SECURE_CSP_REPORT_ONLY=false
Please let me know if you agree, this can be merged to dev

Let's discuss the future of INSECURE_USE_HTTP at #4483 . I think it's possible to let it work out of the box for all users, AND require secure connections by default.

[jweismann]

@PieterGit your flag settings sounds very reasonable to me. THANKS!

[jweismann]

@PieterGit Forgot to mention that I tried to deploy this branch too (on heroku) and did not stumble on any obstacles. Looks good to me.

[PieterGit]

I also am running this branch without any problems. Merging to dev