vmware-tanzu-labs / educates-training-platform

A platform for hosting interactive workshop environments in Kubernetes, or on top of a local container runtime.
https://docs.educates.dev
Apache License 2.0
72 stars 18 forks source link

Update CSP directives targeting Google Analytics #378

Closed mocdaniel closed 4 months ago

mocdaniel commented 4 months ago

Change fixed www prefix for google-analytics.com and googletagmanager.com into wildcard.

This is related to this Slack thread, I opted for changing the www.googletagmanager.com directive in addition to the google-analytics.com directive that has already proven faulty, as I thought there might occur similar problems down the road for googletagmanager.com at some point.

I will build the training-portal image and test it, will report back if this resolves the issue.

Fixes #377

netlify[bot] commented 4 months ago

Deploy Preview for educates-docs ready!

Built without sensitive environment variables

Name Link
Latest commit 1531c5e6aa71174b26851d4ae92e397136e59a4f
Latest deploy log https://app.netlify.com/sites/educates-docs/deploys/664dfd0323dcd90008cd260d
Deploy Preview https://deploy-preview-378--educates-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jorgemoralespou commented 4 months ago

@mocdaniel Would you mind also adding a note to https://github.com/vmware-tanzu-labs/educates-training-platform/blob/develop/project-docs/release-notes/version-2.7.1.md so that your change is documented in next version's release notes? Once that's done I'll go ahead and merge the PR

mocdaniel commented 4 months ago

Will do. Right now I'm investigating some problems wrt. failing CSRF verification and whether that's got to do with my changes or some other changes on the develop branch that made it into the image I'm currently testing.

jorgemoralespou commented 4 months ago

@mocdaniel Then, let us know the final results of your testing, as we probably were going to blindly trust this change would do :-D

mocdaniel commented 4 months ago

Looks like it should work. The reason I get CSRF verification failures is due to commit 2552a4a as it introduces the following Django setting and semi-hardcodes it:

CSP_INCLUDE_NONCE_IN = ("script-src",)
CSP_FRAME_ANCESTORS = ("'self'",)

# this setting breaks CSRF verification for custom spec.portal.ingress.hostname
# in TrainingPortal configurations
CSRF_TRUSTED_ORIGINS = [f"{INGRESS_PROTOCOL}://{TRAINING_PORTAL}-ui.{INGRESS_DOMAIN}"]

FRAME_ANCESTORS = os.environ.get("FRAME_ANCESTORS", "")

However, this has nothing to do with this PR or the changes it introduces. I will open a separate issue describing newly discovered bug.

mocdaniel commented 4 months ago

I confirmed this PR works, I will add a note to the release docs and then it's good to go :)

jorgemoralespou commented 4 months ago

Thanks @mocdaniel