@justinvelluppillai opened this Pull Request on July 20th 2021 Contributor

Description:

Fixes #17773 by setting a Content-Security-Policy which is fairly loose at this stage.

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@Findus23 commented on July 21st 2021 Member

I think we should only be using Content-Security-Policy-Report-Only for at least one or two releases as I'm sure there are a lot of people with weird setups where the CSP will break in fascinating ways.
I still think it is worth creating a secure one for Matomo as it will make Matomo a lot more secure, but also provide an option to fall back to Report-Only or disable it, for setups where using one is not possible.

@tsteur commented on July 21st 2021 Member

👍 be good indeed to have such an option to disable it, or using report-only. And could indeed start with report-only. Maybe in dev mode we would always from beginning use the real one so it's easier to make things compatible? might not make sense though just some thoughts. I suppose this would give some people more time to adjust their setup if needed. They might not notice these reports but at least they might notice it in the changelog maybe?

@justinvelluppillai commented on July 22nd 2021 Contributor

We could have a setting to choose between enabled, disabled, or report only mode, defaulting to disabled or report only.

@justinvelluppillai commented on July 26th 2021 Contributor

I've added a couple of settings, one to enable/disable and one to specify a report-uri. CSP is disabled by default for now, specifying a report-uri will switch it to Content-Security-Policy-Report-Only

@tsteur commented on July 26th 2021 Member

👍 @justinvelluppillai it seems report-uri is deprecated see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/report-uri ? Could maybe remove this feature and then have only one setting for the three different modes (disabled, report, enabled)?

Or seems it might be replaced by https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/report-to ? I don't know how commonly used it is. If it's not commonly used that often someone could maybe add this feature as a plugin.

@Findus23 commented on July 26th 2021 Member

While report-uri seems to be deprecated, it is the only parameter that is actually supported by browsers:
https://caniuse.com/mdn-http_headers_csp_content-security-policy_report-uri

report-to is not supported by any non-chrome browser: https://caniuse.com/mdn-http_headers_report-to

That said, Content-Security-Policy-Report-Only makes also sense without a report-uri (especially during development) as it will log violations in the console.

@justinvelluppillai commented on July 26th 2021 Contributor

Huh, I didn't notice that in my investigations. A couple of possible approaches:

  1. Specify both report-uri and report-to as described here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/report-uri
  2. Join @Findus23 and @tsteur's solutions: have three modes only and exclude the report-uri/report-to fields and let it log to console.

Both trivial to implement, option 1 provides more possible configurations but I lean towards option 2 for simplicity, if everyone agrees?

@Findus23 commented on July 27th 2021 Member

I think we could either add two settings (one switching between on, report-only and off and one specifying the report-uri (or being empty)) or use three settings (like e.g. in https://github.com/Chocobozzz/PeerTube/blob/develop/config/default.yaml#L151-L154)

@tsteur commented on July 27th 2021 Member

I suppose using two settings could work and let us extend it better in the future if we were to decide to add more features (which isn't currently needed).

So something like you mention

csp_enabled = 0/1
csp_report_only = 0/1 . # Only does something if csp is enabled
@tsteur commented on July 27th 2021 Member

@justinvelluppillai it seems like the Page Overlay feature doesn't work yet when the URL is from a different page? I just tested it and it won't load the site.

@justinvelluppillai commented on July 29th 2021 Contributor

@tsteur with the default settings we've chosen (report-only) it should still work as normal but report some issues. I can modify the Page Overlay feature to use a different (or no) CSP. I will take a look at getting the Page Overlay feature running locally then see what's best.

@tsteur commented on July 29th 2021 Member

fyi I tested with in report mode (not report-only).

Powered by GitHub Issue Mirror