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

feat(runtime): add csp reporting url env var #81

Merged
merged 7 commits into from
Apr 8, 2020

Conversation

JAdshead
Copy link
Contributor

@JAdshead JAdshead commented Mar 26, 2020

Description

Adding additional env var allow the configuration of the csp-violations report url

Motivation and Context

Currently only a single environment variable is used to configuring reporting urls which has lead to misconfiguration.

How Has This Been Tested?

Unit & Integrations tests

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)

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.
  • I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

Allows configuration of csp-violation and error reporting urls

@JAdshead JAdshead requested a review from a team as a code owner March 26, 2020 21:06
@oneamexbot
Copy link
Contributor

oneamexbot commented Mar 26, 2020

📊 Bundle Size Report

file name size on disk gzip
app.js 112.6KB 31.4KB
runtime.js 15KB 5.3KB
vendors.js 128.1KB 37.9KB
app~vendors.js 405.9KB 106KB
legacy/app.js 119.4KB 33KB
legacy/runtime.js 15KB 5.3KB
legacy/vendors.js 163KB 44.8KB
legacy/app~vendors.js 412KB 107.6KB

Generated by 🚫 dangerJS against dad4fd2

@JamesSingleton
Copy link
Contributor

@JAdshead,

Any reason why there are changes in package-lock.json when there are none in the package.json?

JamesSingleton
JamesSingleton previously approved these changes Mar 30, 2020
Copy link
Contributor

@Francois-Esquire Francois-Esquire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update the route address for the middleware as well for reporting url?

https://github.com/americanexpress/one-app/blob/master/src/server/ssrServer.js#L69

Not sure if there is a reason for it or not. My only concern if I set ONE_CLIENT_CSP_REPORTING_URL to something different like /csp-violation instead, it would break with the hard coded route.. We also prob don't need that middleware if we have a complete url with https://my-csp-reporter.com/violation.

@JAdshead JAdshead closed this Mar 31, 2020
@JAdshead JAdshead reopened this Mar 31, 2020
@JAdshead
Copy link
Contributor Author

Do we need to update the route address for the middleware as well for reporting url?

https://github.com/americanexpress/one-app/blob/master/src/server/ssrServer.js#L69

Not sure if there is a reason for it or not. My only concern if I set ONE_CLIENT_CSP_REPORTING_URL to something different like /csp-violation instead, it would break with the hard coded route.. We also prob don't need that middleware if we have a complete url with https://my-csp-reporter.com/violation.

@Francois-Esquire one-app provides the reporting endpoints to allow users to optionally report back and log on the one-app server, there are cases where they will want to standup their own server for handling reports. setting ONE_CLIENT_CSP_REPORTING_URL and ONE_CLIENT_REPORTING_URL allows dependencies to consume those values and not be coupled to one-app's api.

docs/api/server/Environment-Variables.md Outdated Show resolved Hide resolved
docs/api/server/Environment-Variables.md Outdated Show resolved Hide resolved
@@ -421,11 +422,41 @@ ONE_CLIENT_REPORTING_URL=https://my-app-errors.com/client
**Default Value**
```bash
# if NODE_ENV=development
ONE_CLIENT_REPORTING_URL=/_
ONE_CLIENT_REPORTING_URL=/_/report/errors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're going to do this, may as well change the env var name altogether.

Suggested change
ONE_CLIENT_REPORTING_URL=/_/report/errors
ONE_CLIENT_ERROR_REPORTING_URL=/_/report/errors

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this also require a change to one-app-ducks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing the name would require a change which is why i left it, currently one-app-ducks is treating this as /_/report/errors - https://github.com/americanexpress/one-app-ducks/blob/master/src/errorReporting.js#L109

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

carry on 🖖

Co-Authored-By: Jimmy King <jimmy.king@aexp.com>
Copy link
Member

@10xLaCroixDrinker 10xLaCroixDrinker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be updated as well: https://github.com/americanexpress/one-app/blob/master/prod-sample/one-app/base.env#L2

Can you validate that this is covered everywhere?

@@ -1,5 +1,6 @@
HOLOCRON_MODULE_MAP_URL=https://sample-cdn.frank/module-map.json
ONE_CLIENT_REPORTING_URL=https://one-app:8443/_
ONE_CLIENT_REPORTING_URL=https://one-app:8443/_/report/errors
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JAdshead
Copy link
Contributor Author

JAdshead commented Apr 1, 2020

This should be updated as well: https://github.com/americanexpress/one-app/blob/master/prod-sample/one-app/base.env#L2

Can you validate that this is covered everywhere?

I have been through and checked other repos for use of ONE_CLIENT_REPORTING_URL/reportingUrl and only occurrences i could find were in the generator and one-app-ducks. The generator is the only place that requires an update

@JAdshead JAdshead requested a review from a team as a code owner April 2, 2020 21:20
@mtomcal
Copy link
Contributor

mtomcal commented Apr 7, 2020

Do we have documented anywhere how errors are POST-ed to the ONE_CLIENT_CSP_REPORTING_URL?

@JAdshead
Copy link
Contributor Author

JAdshead commented Apr 7, 2020

Do we have documented anywhere how errors are POST-ed to the ONE_CLIENT_CSP_REPORTING_URL?

This would be elaborated on in a creating CSP recipe that I don't think exists

@JAdshead JAdshead merged commit f6faa53 into master Apr 8, 2020
@JamesSingleton JamesSingleton deleted the feat/add-csp-violation-env-var branch September 7, 2020 16:21
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.

None yet

6 participants