Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

Add env var to suppress the CSP script nonce in development #636

Merged
merged 3 commits into from
Jan 28, 2022
Merged

Add env var to suppress the CSP script nonce in development #636

merged 3 commits into from
Jan 28, 2022

Conversation

guym4c
Copy link
Member

@guym4c guym4c commented Dec 20, 2021

Description

This PR adds an environment variable ONE_CSP_ALLOW_INLINE_SCRIPTS that suppresses the script nonce that One App prepends to the script-src CSP.

Motivation and Context

This will allow inline scripts to be executed in development - for example, to allow browser devtools to inject content scripts in some browsers that otherwise restrict this. (Firefox restricts this due to a long-standing browser bug.)

Other elements of the CSP are configurable through the developmentAdditions API, but the script nonce is not - this enables full configuration of the CSP.

How Has This Been Tested?

  • Added a test that expects the generated CSP header not to contain the script nonce.
  • Validated that this enables devtools to inject content scripts in Firefox 95.0.2.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update
  • Security update

Checklist:

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch.
  • This change requires cross browser checks.
  • Performance tests should be ran against the server prior to merging.
  • This change impacts caching for client browsers.
  • This change impacts HTTP headers.
  • This change adds additional environment variable requirements for One App users. (no requirements, just optional?)
  • I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

This PR enables the use of Firefox for development using OneApp and browser devtools such as those built for React and Redux. I don't think it's realistic to expect developers not to use these tools whilst developing applications using OneApp, and so this PR adds that browser as an option for OneApp developers.

@guym4c guym4c requested review from a team as code owners December 20, 2021 16:36
@CLAassistant
Copy link

CLAassistant commented Dec 20, 2021

CLA assistant check
All committers have signed the CLA.

@infoxicator
Copy link
Contributor

I don't think there is anything wrong with the code proposed, however, my only concern is the fact that inline-scripts are forbidden, by enabling them just for development to enable a development tool due to a browser bug could lead to confusion. For example if someone enables that envVar and start adding inline scripts they will only find out they are forbidden once they deploy to production.

We are working on a similar approach to disable the CSP in is entirety in development mode for certain use cases, that should also fix the issue with the script nonce for dev tools

@guym4c
Copy link
Member Author

guym4c commented Dec 21, 2021

For example if someone enables that envVar and start adding inline scripts they will only find out they are forbidden once they deploy to production.

Happy to suitably document those concerns on the env vars page

We are working on a similar approach to disable the CSP in is entirety in development mode for certain use cases

Oh, great! Is there a branch for this?

@code-forger
Copy link
Member

We are working on a similar approach to disable the CSP in is entirety in development mode for certain use cases

Oh, great! Is there a branch for this?

I dont think that change applies here, we are allowing an app to run in dev with no csp. If there is a CSP in your tenancy then its respected still.

@guym4c you mentioned that there is a developmentAdditions part of the api. I think I would prefer your change be an additional key in that configuration object. This is more apparent than an env var, would persist for a project, and keeps all csp related control in one place.

@guym4c
Copy link
Member Author

guym4c commented Jan 5, 2022

you mentioned that there is a developmentAdditions part of the api.

@code-forger Sorry, I was referring to the CSP generator we use internally to produce the policy - today it's then always joined to the script nonce by OneApp in the middleware in this PR.

The nonce once generated is added the response object (as that needs to be known by OneApp). Do you mean that the script nonce to be used be something that can be overriden (to be empty-string?) by the root module's app config?

@code-forger
Copy link
Member

@guym4c I guess it depends, do we want this fix to be 'part of' an application. When someone creates a new application do we want to allow them to add this 'fix' then keep it there in the root module.

Alternatively do we want every engineer who comes to work on the project to have to apply this 'fix' locally?

@infoxicator
Copy link
Contributor

@guym4c This PR to disable the CSP completely in development has just been merged #640 once we release a new version of One App you should be able to turn off the CSP and you shouldn't see that firefox issue anymore 👍

@guym4c
Copy link
Member Author

guym4c commented Jan 20, 2022

Should something be added to the docs explaining that we have decided for Firefox, use of the CSP and browser devtools are mutually exclusive?

@infoxicator
Copy link
Contributor

Should something be added to the docs explaining that we have decided for Firefox, use of the CSP and browser devtools are mutually exclusive?

I have been discussing with the team and it could be useful to just disable the nonce in development rather than disabling the entire CSP. It is a shame we have to do that just to support a browser but it is a valid use case.

@guym4c would you mind solving the conflicts so we can give it another review? thanks 👍

# Conflicts:
#	docs/api/server/Environment-Variables.md
@guym4c
Copy link
Member Author

guym4c commented Jan 25, 2022

@infoxicator Updated 👍

@10xLaCroixDrinker 10xLaCroixDrinker merged commit 0916756 into americanexpress:main Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants