-
Notifications
You must be signed in to change notification settings - Fork 87
feat(runtime): add csp reporting url env var #81
Conversation
📊 Bundle Size Report
|
Any reason why there are changes in |
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.
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. |
@@ -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 |
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.
If you're going to do this, may as well change the env var name altogether.
ONE_CLIENT_REPORTING_URL=/_/report/errors | |
ONE_CLIENT_ERROR_REPORTING_URL=/_/report/errors |
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.
Won't this also require a change to one-app-ducks?
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.
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
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.
carry on 🖖
Co-Authored-By: Jimmy King <jimmy.king@aexp.com>
1a70b5a
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.
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 |
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.
I have been through and checked other repos for use of |
Do we have documented anywhere how errors are POST-ed to the |
This would be elaborated on in a creating CSP recipe that I don't think exists |
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
Checklist:
What is the Impact to Developers Using One App?
Allows configuration of csp-violation and error reporting urls