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
Set CSP header to prevent XSS #17798
Conversation
I think we should only be using |
👍 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? |
We could have a setting to choose between enabled, disabled, or report only mode, defaulting to disabled or report only. |
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 |
👍 @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. |
While report-uri seems to be deprecated, it is the only parameter that is actually supported by browsers: report-to is not supported by any non-chrome browser: https://caniuse.com/mdn-http_headers_report-to That said, |
Huh, I didn't notice that in my investigations. A couple of possible approaches:
Both trivial to implement, option 1 provides more possible configurations but I lean towards option 2 for simplicity, if everyone agrees? |
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) |
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
|
@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. |
@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. |
fyi I tested with in report mode (not report-only). |
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
@sgiehl it'd be great if you could give this a check over and if all ok we can merge as it won't have too much impact until we enable it. |
@justinvelluppillai Sure. Feel free to simply use the github feature to request for reviews in the future if you need one ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of minor comments. Btw. might also be good to merge in the latest changes from 4.x-dev
to see if tests are passing. Currently the UI tests are showing various failures, that actually look unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good now 👍
Description:
Fixes #17773 by setting a Content-Security-Policy which is fairly loose at this stage.
Review