Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Updated content security policy to allow script and style from google analytics #2649

Merged
merged 6 commits into from
Apr 6, 2022
Merged

Updated content security policy to allow script and style from google analytics #2649

merged 6 commits into from
Apr 6, 2022

Conversation

RajeshPaul38
Copy link
Contributor

Signed-off-by: Rajesh Paul rajesh.paul@progress.com

Description

[Please describe what this change achieves]

Issues Resolved

[List any existing issues this PR resolves, or any Discourse or
StackOverflow discussions that are relevant]

Check List

@RajeshPaul38 RajeshPaul38 requested review from a team as code owners March 17, 2022 11:35
@netlify
Copy link

netlify bot commented Mar 17, 2022 •
edited
Loading

👷 Deploy Preview for chef-supermarket processing.

Name Link
🔨 Latest commit 058fccd
🔍 Latest deploy log https://app.netlify.com/sites/chef-supermarket/deploys/624c5b0599e2450008b2b458

@RajeshPaul38
Copy link
Contributor Author

Buildkite: https://buildkite.com/chef/chef-supermarket-main-omnibus-adhoc/builds/423

@RajeshPaul38
Copy link
Contributor Author

ad hoc build has passed

@RajeshPaul38 RajeshPaul38 changed the title WIP: updated content security policy to allow script and style from google analytics Updated content security policy to allow script and style from google analytics Mar 21, 2022
RajeshPaul38
RajeshPaul38 commented Mar 21, 2022
View reviewed changes
src/supermarket/app/views/application/_analytics.html.erb
ga('create', '<%= ENV["GOOGLE_ANALYTICS_ID"] %>', 'auto');
ga('require', 'linkid', 'linkid.js');
ga('require', 'displayfeatures');
ga('send', 'pageview');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This snippet doesn't seem to be working. Got the reference to the new script from this link: https://developers.google.com/analytics/devguides/collection/gtagjs

@msys-sgarg
Copy link
Contributor

@RajeshPaul38 Can we have screenshots with the PR to make sure that google analytics are working fine with this change ?

@RajeshPaul38
Copy link
Contributor Author

@RajeshPaul38 Can we have screenshots with the PR to make sure that google analytics are working fine with this change ?

Sure, let me attach

@RajeshPaul38
Copy link
Contributor Author

Here is how the data is getting reflected in google analytics. @msys-sgarg
Screenshot 2022-03-25 at 1 45 35 PM

antima-gupta
antima-gupta reviewed Mar 28, 2022
View reviewed changes
Copy link
Contributor

@antima-gupta antima-gupta left a comment

Choose a reason for hiding this comment

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

Looks good to me

@msys-sgarg
Copy link
Contributor

@RajeshPaul38 I hope we took a look into the script injection possibilities with this change ?

@RajeshPaul38 RajeshPaul38 force-pushed the rajeshpaul38/csp-allow-google-analytics/INFSUS-107 branch 2 times, most recently from d1b4c6e to de39c3f Compare March 29, 2022 18:08
@RajeshPaul38
Copy link
Contributor Author

@msys-sgarg thanks for pointing out the possible chances of XSS attack. I've tried to mitigate it as much as possible by disabling inline scripts and only keeping the inline css enabled. I've used a CSP mechanism called nonce which ensures only the inline scripts that is signed with a valid nonce, will be allowed by the CSP. This nonce is a hash that gets generated uniquely for every request. Hence the chances of XSS attack is zero as no one else will know what will be the nonce hash for a certain request. Hope this is a satisfactory resolution for the issues pointed out during the demo. If yes pls approve.

@RajeshPaul38
Copy link
Contributor Author

Build: https://buildkite.com/chef/chef-supermarket-main-omnibus-adhoc/builds/438

@RajeshPaul38
Copy link
Contributor Author

Checked my changes in an EC2 instance. See below screenshot:

Screenshot 2022-03-30 at 1 26 20 AM

@RajeshPaul38
Copy link
Contributor Author

Can we get this merged with priority @saghoshprogress as this is a customer bug?

@RajeshPaul38
Copy link
Contributor Author

Ready for merge @saghoshprogress

@RoyShravani
Copy link
Contributor

@RajeshPaul38 Deploy preview check failed. please take a look!

@RajeshPaul38 RajeshPaul38 force-pushed the rajeshpaul38/csp-allow-google-analytics/INFSUS-107 branch from a580fa5 to c42e169 Compare April 5, 2022 09:36
@RajeshPaul38 RajeshPaul38 requested a review from a team as a code owner April 5, 2022 13:06
@github-actions github-actions bot added the Documentation Gets work onto the docs board label Apr 5, 2022
@RajeshPaul38 RajeshPaul38 force-pushed the rajeshpaul38/csp-allow-google-analytics/INFSUS-107 branch from f24ae79 to c42e169 Compare April 5, 2022 14:12
@RajeshPaul38
Copy link
Contributor Author

I've tried debugging the issue with netlify build. But seems like an issue in the build process, not related to this PR. Pls approve.

@RajeshPaul38
updated content security policy to allow script and style from google…
921f7bf 
… analytics scripts

Signed-off-by: Rajesh Paul <rajesh.paul@progress.com>
@RajeshPaul38
fix linter errors
58bd17d 
Signed-off-by: Rajesh Paul <rajesh.paul@progress.com>
@RajeshPaul38
changed google analytics code snippet to enable tracking
ec1e364 
Signed-off-by: Rajesh Paul <rajesh.paul@progress.com>
@RajeshPaul38
corrected erb tag closing
dc93744 
Signed-off-by: Rajesh Paul <rajesh.paul@progress.com>
@RajeshPaul38
Removed :unsafe_inline from CSP and added nonce in CSP for allowing i…
e05afdf 
…nline scripts

Signed-off-by: Rajesh Paul <rajesh.paul@progress.com>
@RajeshPaul38
allow inline script for segment
058fccd 
Signed-off-by: Rajesh Paul <rajesh.paul@progress.com>
@RajeshPaul38 RajeshPaul38 force-pushed the rajeshpaul38/csp-allow-google-analytics/INFSUS-107 branch from c42e169 to 058fccd Compare April 5, 2022 15:06
@saghoshprogress saghoshprogress merged commit dcad237 into main Apr 6, 2022
@saghoshprogress saghoshprogress deleted the rajeshpaul38/csp-allow-google-analytics/INFSUS-107 branch April 6, 2022 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dheerajd-msys dheerajd-msys dheerajd-msys approved these changes

@antima-gupta antima-gupta Awaiting requested review from antima-gupta

Assignees
No one assigned
Labels
Documentation Gets work onto the docs board
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RajeshPaul38 @msys-sgarg @RoyShravani @dheerajd-msys @antima-gupta @saghoshprogress