Skip to content

Adds a CSP that is enabled by default

Dominic Couture requested to merge default-csp into master

What does this MR do?

This MR enables the Content-Security-Policy by default. Most of the policy is very generic and not strict at all. The idea is mostly for this to be a "better than nothing" iteration. The critical piece however is the addition of strict-dynamic to the script-src directive. This builds on a previous MR that added nonces to add <script> tags and should greatly reduce the impact of XSS in GitLab. As XSS is one of the vulnerabilities we see the most, this will lead to increased security and also $ savings on the bug bounty program by greatly reducing the impact of successful exploitation.

strict-dynamic allows loading JavaScript from <script> tags with a valid nonce attribute and then allows those validated scripts to do as they please. When a browser supports strict-dynamic, it ignores the other attributes present in the script-src directive.

Browsers that don't support strict-dynamic (i.e. Safari as far as GitLab-supported browsers are concerned) fall back on the other elements of the script-src directive which is why 'self' 'unsafe-inline' 'unsafe-eval' https://www.recaptcha.net https://apis.google.com" is also included in the policy. This comes from the documentation where we recommend those items to be added to the CSP.

Dev concerns

CSPs are often annoying for developers because it breaks a bunch of a stuff in the development environment. I spent some time testing around that to avoid such a situation and that's why in dev the webpack dev server is dynamically added to the CSP allowlist.

Breaking change concerns

  • Users that currently have custom CSP settings will override those new defaults and shouldn't see a difference when upgrading.
  • Users that current use the default settings will have the CSP enabled when they update

    • There is a small risk of something breaking on the more customized installs though I tried to prevent most of that.

      The good news is that should something break for a specific customer with custom settings, we don't need to ship a patch everything can be fixed with a configuration change. For this reason here I think that targeting %14.0 might be safer. WDYT maintainer? :)

Closes #30720

Related documentation MR omnibus-gitlab!5210 (merged)

Also related to this AppSec initiative https://gitlab.com/gitlab-com/gl-security/appsec/appsec-reviews/-/issues/44

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Dominic Couture

Merge request reports