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

Add DOMPurify to address XSS issues; closes #420 #431

Merged

shakeelmohamed merged 4 commits into main from xss

Apr 21, 2024

Member

shakeelmohamed commented Apr 21, 2024

Closes #420


          Add DOMPurify to address XSS issues; closes #420

887e6bf

vercel bot commented Apr 21, 2024 •

edited

Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
zen-audio-player-github-io	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 21, 2024 5:48am

sourcery-ai bot reviewed

View reviewed changes

Contributor

sourcery-ai bot left a comment

Hey @shakeelmohamed - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 4 issues found
🟢 Security: all looks good
🟡 Testing: 5 issues found
🟢 Complexity: all looks good
🟢 Docstrings: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

bower_components/DOMPurify/src/utils.js

Comment on lines +1 to +7

+              const {
+                entries,
+                setPrototypeOf,
+                isFrozen,
+                getPrototypeOf,
+                getOwnPropertyDescriptor,
+              } = Object;

Contributor

sourcery-ai bot Apr 21, 2024

suggestion (code_refinement): Consider destructuring directly in the import statement for clarity.

This approach can make the imports cleaner and more declarative.

bower_components/DOMPurify/src/utils.js

+                getOwnPropertyDescriptor,
+              } = Object;
+              let { freeze, seal, create } = Object; // eslint-disable-line import/no-mutable-exports

Contributor

sourcery-ai bot Apr 21, 2024

suggestion (code_refinement): Avoid using 'let' for imports that do not change.

Using 'const' instead of 'let' for these imports would ensure they are not reassigned later, enhancing code reliability.

Suggested change

      
            let { freeze, seal, create } = Object; // eslint-disable-line import/no-mutable-exports
          
            const { freeze, seal, create } = Object;

bower_components/DOMPurify/src/utils.js

Comment on lines +12 to +16

+              if (!freeze) {
+                freeze = function (x) {
+                  return x;
+                };
+              }

Contributor

sourcery-ai bot Apr 21, 2024

suggestion (performance): Consider removing the polyfill for 'Object.freeze'.

Modern environments support 'Object.freeze'. Including polyfills for standard methods could lead to unnecessary code bloat.

Suggested change

      
            if (!freeze) {
          
              freeze = function (x) {
          
                return x;
          
              };
          
            }
          
            if (!freeze) {
          
              freeze = Object.freeze;
          
            }

bower_components/DOMPurify/dist/purify.es.mjs

+              }
+              var purify = createDOMPurify();
+              export { purify as default };

Contributor

sourcery-ai bot Apr 21, 2024

suggestion (code_refinement): Consider using explicit exports instead of a default export.

Using named exports can help with clearer import statements in consuming modules and potentially easier mocking in tests.

Suggested change

      
            export { purify as default };
          
            export { purify };

test/javascript_components.js Outdated Show resolved Hide resolved

bower_components/DOMPurify/dist/purify.es.mjs

+                      return unapply(desc.value);
+                    }
+                  }
+                  object = getPrototypeOf(object);

Contributor

sourcery-ai bot Apr 21, 2024

issue (code-quality): Don't reassign parameter - object (dont-reassign-parameters)

Explanation

Reassigning parameters can lead to unexpected behavior, especially when accessing the arguments object. It can also cause optimization issues, especially in V8.

From the Airbnb JavaScript Style Guide

bower_components/DOMPurify/dist/purify.es.mjs

Comment on lines +175 to +177

+                function fallbackValue() {
+                  return null;
+                }

Contributor

sourcery-ai bot Apr 21, 2024

issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)

Explanation

Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.

bower_components/DOMPurify/dist/purify.es.mjs

Comment on lines +220 to +232

+              var EXPRESSIONS = /*#__PURE__*/Object.freeze({
+                __proto__: null,
+                MUSTACHE_EXPR: MUSTACHE_EXPR,
+                ERB_EXPR: ERB_EXPR,
+                TMPLIT_EXPR: TMPLIT_EXPR,
+                DATA_ATTR: DATA_ATTR,
+                ARIA_ATTR: ARIA_ATTR,
+                IS_ALLOWED_URI: IS_ALLOWED_URI,
+                IS_SCRIPT_OR_DATA: IS_SCRIPT_OR_DATA,
+                ATTR_WHITESPACE: ATTR_WHITESPACE,
+                DOCTYPE_NAME: DOCTYPE_NAME,
+                CUSTOM_ELEMENT: CUSTOM_ELEMENT
+              });

Contributor

sourcery-ai bot Apr 21, 2024

issue (code-quality): Use const or let instead of var. (avoid-using-var)

Explanation

`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.

From the Airbnb JavaScript Style Guide

bower_components/DOMPurify/dist/purify.es.mjs

+                  document
+                } = window;
+                const originalDocument = document;
+                const currentScript = originalDocument.currentScript;

Contributor

sourcery-ai bot Apr 21, 2024

suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)

Suggested change

      
              const currentScript = originalDocument.currentScript;
          
              const {currentScript} = originalDocument;

Explanation

Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

bower_components/DOMPurify/dist/purify.es.mjs

Comment on lines +1085 to +1099

+                  if (ALLOW_DATA_ATTR && !FORBID_ATTR[lcName] && regExpTest(DATA_ATTR, lcName)) ; else if (ALLOW_ARIA_ATTR && regExpTest(ARIA_ATTR, lcName)) ; else if (!ALLOWED_ATTR[lcName] || FORBID_ATTR[lcName]) {
+                    if (
+                    // First condition does a very basic check if a) it's basically a valid custom element tagname AND
+                    // b) if the tagName passes whatever the user has configured for CUSTOM_ELEMENT_HANDLING.tagNameCheck
+                    // and c) if the attribute name passes whatever the user has configured for CUSTOM_ELEMENT_HANDLING.attributeNameCheck
+                    _isBasicCustomElement(lcTag) && (CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof RegExp && regExpTest(CUSTOM_ELEMENT_HANDLING.tagNameCheck, lcTag) || CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof Function && CUSTOM_ELEMENT_HANDLING.tagNameCheck(lcTag)) && (CUSTOM_ELEMENT_HANDLING.attributeNameCheck instanceof RegExp && regExpTest(CUSTOM_ELEMENT_HANDLING.attributeNameCheck, lcName) || CUSTOM_ELEMENT_HANDLING.attributeNameCheck instanceof Function && CUSTOM_ELEMENT_HANDLING.attributeNameCheck(lcName)) ||
+                    // Alternative, second condition checks if it's an `is`-attribute, AND
+                    // the value passes whatever the user has configured for CUSTOM_ELEMENT_HANDLING.tagNameCheck
+                    lcName === 'is' && CUSTOM_ELEMENT_HANDLING.allowCustomizedBuiltInElements && (CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof RegExp && regExpTest(CUSTOM_ELEMENT_HANDLING.tagNameCheck, value) || CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof Function && CUSTOM_ELEMENT_HANDLING.tagNameCheck(value))) ; else {
+                      return false;
+                    }
+                    /* Check value is safe. First, is attr inert? If so, is safe */
+                  } else if (URI_SAFE_ATTRIBUTES[lcName]) ; else if (regExpTest(IS_ALLOWED_URI$1, stringReplace(value, ATTR_WHITESPACE, ''))) ; else if ((lcName === 'src' || lcName === 'xlink:href' || lcName === 'href') && lcTag !== 'script' && stringIndexOf(value, 'data:') === 0 && DATA_URI_TAGS[lcTag]) ; else if (ALLOW_UNKNOWN_PROTOCOLS && !regExpTest(IS_SCRIPT_OR_DATA, stringReplace(value, ATTR_WHITESPACE, ''))) ; else if (value) {
+                    return false;
+                  } else ;

Contributor

sourcery-ai bot Apr 21, 2024

suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)

Suggested change

      
                if (ALLOW_DATA_ATTR && !FORBID_ATTR[lcName] && regExpTest(DATA_ATTR, lcName)) ; else if (ALLOW_ARIA_ATTR && regExpTest(ARIA_ATTR, lcName)) ; else if (!ALLOWED_ATTR[lcName] || FORBID_ATTR[lcName]) {
          
                  if (
          
                  // First condition does a very basic check if a) it's basically a valid custom element tagname AND
          
                  // b) if the tagName passes whatever the user has configured for CUSTOM_ELEMENT_HANDLING.tagNameCheck
          
                  // and c) if the attribute name passes whatever the user has configured for CUSTOM_ELEMENT_HANDLING.attributeNameCheck
          
                  _isBasicCustomElement(lcTag) && (CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof RegExp && regExpTest(CUSTOM_ELEMENT_HANDLING.tagNameCheck, lcTag) || CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof Function && CUSTOM_ELEMENT_HANDLING.tagNameCheck(lcTag)) && (CUSTOM_ELEMENT_HANDLING.attributeNameCheck instanceof RegExp && regExpTest(CUSTOM_ELEMENT_HANDLING.attributeNameCheck, lcName) || CUSTOM_ELEMENT_HANDLING.attributeNameCheck instanceof Function && CUSTOM_ELEMENT_HANDLING.attributeNameCheck(lcName)) ||
          
                  // Alternative, second condition checks if it's an `is`-attribute, AND
          
                  // the value passes whatever the user has configured for CUSTOM_ELEMENT_HANDLING.tagNameCheck
          
                  lcName === 'is' && CUSTOM_ELEMENT_HANDLING.allowCustomizedBuiltInElements && (CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof RegExp && regExpTest(CUSTOM_ELEMENT_HANDLING.tagNameCheck, value) || CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof Function && CUSTOM_ELEMENT_HANDLING.tagNameCheck(value))) ; else {
          
                    return false;
          
                  }
          
                  /* Check value is safe. First, is attr inert? If so, is safe */
          
                } else if (URI_SAFE_ATTRIBUTES[lcName]) ; else if (regExpTest(IS_ALLOWED_URI$1, stringReplace(value, ATTR_WHITESPACE, ''))) ; else if ((lcName === 'src' || lcName === 'xlink:href' || lcName === 'href') && lcTag !== 'script' && stringIndexOf(value, 'data:') === 0 && DATA_URI_TAGS[lcTag]) ; else if (ALLOW_UNKNOWN_PROTOCOLS && !regExpTest(IS_SCRIPT_OR_DATA, stringReplace(value, ATTR_WHITESPACE, ''))) ; else if (value) {
          
                  return false;
          
                } else ;
          
                if (ALLOW_DATA_ATTR && !FORBID_ATTR[lcName] && regExpTest(DATA_ATTR, lcName)) {
          
                  ;
          
                } else if (ALLOW_ARIA_ATTR && regExpTest(ARIA_ATTR, lcName)) ; else if (!ALLOWED_ATTR[lcName] || FORBID_ATTR[lcName]) {
          
                      if (
          
                      // First condition does a very basic check if a) it's basically a valid custom element tagname AND
          
                      // b) if the tagName passes whatever the user has configured for CUSTOM_ELEMENT_HANDLING.tagNameCheck
          
                      // and c) if the attribute name passes whatever the user has configured for CUSTOM_ELEMENT_HANDLING.attributeNameCheck
          
                      _isBasicCustomElement(lcTag) && (CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof RegExp && regExpTest(CUSTOM_ELEMENT_HANDLING.tagNameCheck, lcTag) || CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof Function && CUSTOM_ELEMENT_HANDLING.tagNameCheck(lcTag)) && (CUSTOM_ELEMENT_HANDLING.attributeNameCheck instanceof RegExp && regExpTest(CUSTOM_ELEMENT_HANDLING.attributeNameCheck, lcName) || CUSTOM_ELEMENT_HANDLING.attributeNameCheck instanceof Function && CUSTOM_ELEMENT_HANDLING.attributeNameCheck(lcName)) ||
          
                      // Alternative, second condition checks if it's an `is`-attribute, AND
          
                      // the value passes whatever the user has configured for CUSTOM_ELEMENT_HANDLING.tagNameCheck
          
                      lcName === 'is' && CUSTOM_ELEMENT_HANDLING.allowCustomizedBuiltInElements && (CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof RegExp && regExpTest(CUSTOM_ELEMENT_HANDLING.tagNameCheck, value) || CUSTOM_ELEMENT_HANDLING.tagNameCheck instanceof Function && CUSTOM_ELEMENT_HANDLING.tagNameCheck(value))) ; else {
          
                        return false;
          
                      }
          
                      /* Check value is safe. First, is attr inert? If so, is safe */
          
                    } else if (URI_SAFE_ATTRIBUTES[lcName]) ; else if (regExpTest(IS_ALLOWED_URI$1, stringReplace(value, ATTR_WHITESPACE, ''))) ; else if ((lcName === 'src' || lcName === 'xlink:href' || lcName === 'href') && lcTag !== 'script' && stringIndexOf(value, 'data:') === 0 && DATA_URI_TAGS[lcTag]) ; else if (ALLOW_UNKNOWN_PROTOCOLS && !regExpTest(IS_SCRIPT_OR_DATA, stringReplace(value, ATTR_WHITESPACE, ''))) ; else if (value) {
          
                      return false;
          
                    } else ;

Explanation

It is recommended to always use braces and create explicit statement blocks.

Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).


          Update test/javascript_components.js

289fee3

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

vercel bot deployed to Preview

April 21, 2024 05:42

View deployment

shakeelmohamed added 2 commits

April 20, 2024 22:45


          add sourcery config; pull travis CI config

a73c24c


          fix sourcery AI test problem

773b6f0

vercel bot deployed to Preview

April 21, 2024 05:48

View deployment

shakeelmohamed merged commit 8449dc0 into main

6 checks passed

shakeelmohamed deleted the xss branch

April 21, 2024 06:00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet