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: Configure Mozilla's Eslint Plugin eslint-plugin-no-unsanitized + DOMPurify.sanitize + escapeHtml #416

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
helix-importer-ui
helix-importer-ui
scripts/purify.min.js
33 changes: 33 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,23 @@
/*
Recommendation: Always try to refactor your code to simply avoid using unsafe browser APIs.
Use:
* Element.textContent
* Element.appendChild
* Document.createElement
* etc.
instead of:
* Element.innerHTML
* Element.outerHTML
* Element.insertAdjacentHTML
* Range.createContextualFragment
* etc.
https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html#guideline-3-use-documentcreateelement-elementsetattributevalue-elementappendchild-and-similar-to-build-dynamic-interfaces
*/
const ENCODE_SANITIZE_METHODS = [
'escapeHtml',
'DOMPurify.sanitize',
];

module.exports = {
root: true,
extends: 'airbnb-base',
Expand All @@ -10,9 +30,22 @@ module.exports = {
sourceType: 'module',
requireConfigFile: false,
},
globals: {
DOMPurify: 'readonly',
},
plugins: ['no-unsanitized'],
rules: {
'import/extensions': ['error', { js: 'always' }], // require js file extensions in imports
'linebreak-style': ['error', 'unix'], // enforce unix linebreaks
'no-param-reassign': [2, { props: false }], // allow modifying properties of param
// Flag usage of unsafe DOM APIs to mitigate XSS
'no-unsanitized/method': [
'error',
{ escape: { defaultDisable: true, methods: [...ENCODE_SANITIZE_METHODS] } },
],
'no-unsanitized/property': [
'error',
{ escape: { defaultDisable: true, methods: [...ENCODE_SANITIZE_METHODS] } },
],
},
};
5 changes: 3 additions & 2 deletions blocks/fragment/fragment.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Include content on a page as a fragment.
* https://www.aem.live/developer/block-collection/fragment
*/

import '../../scripts/purify.min.js';
import {
decorateMain,
} from '../../scripts/scripts.js';
Expand All @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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


// reset base path for media to fragment base
const resetAttributeBase = (tag, attr) => {
Expand Down
20 changes: 20 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"version": "1.3.0",
"description": "Starter project for Adobe Helix",
"scripts": {
"postinstall": "cp node_modules/dompurify/dist/purify.min.js scripts/purify.min.js",
"lint:js": "eslint .",
"lint:css": "stylelint blocks/**/*.css styles/*.css",
"lint": "npm run lint:js && npm run lint:css"
Expand All @@ -20,9 +21,11 @@
"homepage": "https://github.com/adobe/aem-boilerplate#readme",
"devDependencies": {
"@babel/eslint-parser": "7.25.1",
"dompurify": "3.1.7",
"eslint": "8.57.0",
"eslint-config-airbnb-base": "15.0.0",
"eslint-plugin-import": "2.29.1",
"eslint-plugin-no-unsanitized": "^4.1.0",
"stylelint": "16.8.2",
"stylelint-config-standard": "36.0.1"
}
Expand Down
3 changes: 2 additions & 1 deletion scripts/aem.js
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ function buildBlock(blockName, content) {
vals.forEach((val) => {
if (val) {
if (typeof val === 'string') {
colEl.innerHTML += val;
colEl.textContent += val;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

} else {
colEl.appendChild(val);
}
Expand All @@ -564,6 +564,7 @@ async function loadBlock(block) {
const decorationComplete = new Promise((resolve) => {
(async () => {
try {
// eslint-disable-next-line no-unsanitized/method
const mod = await import(
`${window.hlx.codeBasePath}/blocks/${blockName}/${blockName}.js`
);
Expand Down
3 changes: 3 additions & 0 deletions scripts/purify.min.js

Large diffs are not rendered by default.

21 changes: 21 additions & 0 deletions scripts/scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,27 @@ import {
sampleRUM,
} from './aem.js';

/**
* Minimal escaping of HTML special characters
*
* NOTICE: Mitigates a big class of XSS attacks, but not all of them.
* Always try to refactor your code to simply avoid using unsafe browser APIs.
*
* example where it would not help, because the attribute has a Javascript context:
* const userInput = 'alert(document.cookie)';
* container.innerHTML = `<img src="x" onerror="${escapeHtml(userInput)}"/>`;
* @param {string} unsafe
* @returns {string}
*/
export function escapeHtml(unsafe) {
return unsafe
.replaceAll('&', '&amp;')
.replaceAll('<', '&lt;')
.replaceAll('>', '&gt;')
.replaceAll('"', '&quot;')
.replaceAll('\'', '&#x27;');
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something for aem.js?

/**
* Builds hero block and prepends to main in a new section.
* @param {Element} main The container element
Expand Down