feat: Configure Mozilla's Eslint Plugin eslint-plugin-no-unsanitized + DOMPurify.sanitize + escapeHtml #416

andreituicu · 2024-10-16T15:45:34Z

Building on top of #415

Test urls:

…+ DOMPurify

…+ escapeHTML

fkakatie · 2024-10-16T16:12:07Z

results of aem-psi-check would be necessary here.

andreituicu · 2024-10-16T16:21:10Z

@fkakatie completely agree. It looks like there's a problem with my fork, because when I access https://xss-lint-dompurify--helix-project-boilerplate--andreituicu.aem.live the head doesn't have any scripts/css... Could I possibly get write permissions to the boilerplate? That way I can create the branches directly in the project and recreate the PRs including PSI values.

rofe · 2024-10-18T08:37:25Z

blocks/fragment/fragment.js

@@ -22,7 +22,8 @@ export async function loadFragment(path) {
    const resp = await fetch(`${path}.plain.html`);
    if (resp.ok) {
      const main = document.createElement('main');
-      main.innerHTML = await resp.text();
+
+      main.innerHTML = DOMPurify.sanitize(await resp.text());


Note that the original fragment block source is in the block-collection and should probably be fixed there first, then updated here.

head.html

rofe · 2024-10-18T08:39:50Z

scripts/aem.js

@@ -537,7 +537,7 @@ function buildBlock(blockName, content) {
      vals.forEach((val) => {
        if (val) {
          if (typeof val === 'string') {
-            colEl.innerHTML += val;
+            colEl.textContent += val;


Note that the aem.js source is here and the build result gets copied to the boilerplate.

rofe · 2024-10-18T08:40:47Z

scripts/scripts.js

+    .replaceAll('"', '&quot;')
+    .replaceAll('\'', '&#x27;');
+}
+


Maybe something for aem.js?

andreituicu · 2024-10-21T15:16:41Z

@rofe I agree with your suggestions of moving different changes into aem-lib and aem-block-collection.
I created this PR mostly as a consolidated view to collect feedback and discuss whether this is a direction we'd like to go in, or not.

I'm not 100% convinced it is simple enough for developers (which I might be wrong) when they should escape, when they should sanitize and when are both needed.

Using any of them will make the linter happy, but depending on context they aren't both secure, or said differently, each offers protection in specific cases.

I'm not sure if it would be a better approach to just keeping it simple by saying that any use of unsafe API is unsafe.
The problem then becomes creating big structures with just document.createElement, document.appendChild and document.setAttribute and fragments always need to disable the linter rules, like: https://github.com/adobe/aem-boilerplate/pull/415/files#diff-7fdbc35e8d9d1b49261e0470a1900621a5eb2508a2818a9ee18e2e60514fae4eR26