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.

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

feat: meta based csp #228

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat: meta based csp #228

wants to merge 3 commits into from

Conversation

davidnuescheler
Copy link
Contributor

@davidnuescheler davidnuescheler commented Jun 15, 2023

setting a CSP that is only transported over the wire once and then cached on the client.
manages the CSP in an easy to read JSON file

https://main--helix-project-boilerplate--adobe.hlx.live/
vs.
https://meta-csp--helix-project-boilerplate--adobe.hlx.live/?rum=1

@aem-code-sync
Copy link

aem-code-sync bot commented Jun 15, 2023

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@aem-code-sync
Copy link

aem-code-sync bot commented Jun 15, 2023

Page Scores Audits Google
/ Lighthouse returned error: FAILED_DOCUMENT_REQUEST. Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Details: net::ERR_ACCESS_DENIED) PSI

@davidnuescheler davidnuescheler marked this pull request as draft June 15, 2023 22:58
@aem-code-sync
Copy link

aem-code-sync bot commented Jun 15, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link
Contributor

@trieloff trieloff left a comment

Choose a reason for hiding this comment

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

  • track violations in RUM
  • check for timing, violating resources might get loaded before CSP becomes effective

/**
* Loads everything that doesn't need to be delayed.
* @param {Element} doc The container element
*/
async function loadLazy(doc) {
await setCSP();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd check if this is effective in blocking eagerly loaded resources that violate the CSP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i am not sure that i am really that worried about the CSPs protecting block code...
to me this probably has to be in place before we execute untrusted code, eg. martech stack, so one could argue that this is in the wrong place altogether. so to a certain extent i think i am saying i don't even know when this should be loaded, so i am not really sure that an await really makes a difference here...

i am somewhat doubtful of the allowlist based approach altogether, so maybe a nonce based strict-dynamic approach would be the right thing here, but i definitely don't know enough about the repercussions of that vis-a-vis the somewhat questionable benefit of CSPs around XSS attacks on franklin sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... 99.34% of hosts with CSP use policies that offer no benefit against XSS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Revisiting this, I think this is right, but only the first step. The next step should be @chicharr's lightweight tag management controlling the CSP

scripts/scripts.js Show resolved Hide resolved
Co-authored-by: Lars Trieloff <lars@trieloff.net>
@aem-code-sync
Copy link

aem-code-sync bot commented Jun 19, 2023

Page Scores Audits Google
/?rum=1 PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Jun 19, 2023

Page Scores Audits Google
/?rum=1 PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@kptdobe
Copy link
Contributor

kptdobe commented Jun 21, 2023

For the record: frame-ancestors cannot be controlled via meta attribute (see https://www.w3.org/TR/CSP3/#directive-frame-ancestors), i.e. it won't work here.
And it makes sense (browser would have already started to load the iframe once the directive comes in).

@amol-anand
Copy link
Contributor

Used this approach across multiple projects. Works great. I would like for this to be default in boilerplate to encourage folks to use CSP.

If someone adds something to the martech stack via a tag manager (Launch/GTM) or some iframe in the content via the Embed block, then they have to explicitly allow that in the CSP in code and create a PR which would run PSI checks against the site to ensure the new plugin is not impacting page performance. This will force better practices and ensure that it is easy to still maintain performance while also adding additional libraries to martech.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants