Verify that attributes don't have namespaces

Follows the discussion in jsdom#2546. This includes linting rules to prevent getAttribute(), setAttribute(), hasAttribute(), removeAttribute(), and toggleAttribute() from being used, in favor of their NS-alternatives. This also bumps the webidl2js minimum version, since that release fixed the same bug in [Reflect]-implemented properties.
miggs125 · Apr 20, 2019 · 865ad59 · 865ad59
1 parent 243811f
commit 865ad59
Show file tree

Hide file tree

Showing 53 changed files with 242 additions and 214 deletions.
diff --git a/.eslintrc.json b/.eslintrc.json
@@ -290,5 +290,21 @@
       { "ancestor": "NodeImpl", "hook": "_childTextContentChangeSteps" },
       { "ancestor": "ElementImpl", "hook": "_attrModified" }
     ]
-  }
+  },
+
+  // Rules limited to specific locations
+  "overrides": [
+    {
+      "files": ["lib/**"],
+      "rules": {
+        "no-restricted-properties": ["error",
+          { "property": "getAttribute", "message": "Use 'getAttributeNS' with null as the namespace to access attributes within jsdom" },
+          { "property": "setAttribute", "message": "Use 'setAttributeNS' with null as the namespace to access attributes within jsdom" },
+          { "property": "hasAttribute", "message": "Use 'hasAttributeNS' with null as the namespace to access attributes within jsdom" },
+          { "property": "removeAttribute", "message": "Use 'removeAttributeNS' with null as the namespace to access attributes within jsdom" },
+          { "property": "toggleAttribute", "message": "Use 'setAttributeNS' and 'removeAttributeNS' with null as the namespace to access attributes within jsdom" }
+        ]
+      }
+    }
+  ]
 }
diff --git a/lib/jsdom/browser/Window.js b/lib/jsdom/browser/Window.js
@@ -332,10 +332,10 @@ function Window(options) {
       impl.text = text;
     }
     if (value !== undefined) {
-      impl.setAttribute("value", value);
+      impl.setAttributeNS(null, "value", value);
     }
     if (defaultSelected) {
-      impl.setAttribute("selected", "");
+      impl.setAttributeNS(null, "selected", "");
     }
     impl._selectedness = selected;
 
@@ -359,10 +359,10 @@ function Window(options) {
     const impl = idlUtils.implForWrapper(img);
 
     if (arguments.length > 0) {
-      impl.setAttribute("width", String(arguments[0]));
+      impl.setAttributeNS(null, "width", String(arguments[0]));
     }
     if (arguments.length > 1) {
-      impl.setAttribute("height", String(arguments[1]));
+      impl.setAttributeNS(null, "height", String(arguments[1]));
     }
 
     return img;
@@ -383,10 +383,10 @@ function Window(options) {
   function Audio(src) {
     const audio = window._document.createElement("audio");
     const impl = idlUtils.implForWrapper(audio);
-    impl.setAttribute("preload", "auto");
+    impl.setAttributeNS(null, "preload", "auto");
 
     if (src !== undefined) {
-      impl.setAttribute("src", String(src));
+      impl.setAttributeNS(null, "src", String(src));
     }
 
     return audio;

diff --git a/lib/jsdom/browser/resources/per-document-resource-loader.js b/lib/jsdom/browser/resources/per-document-resource-loader.js
@@ -66,9 +66,9 @@ module.exports = class PerDocumentResourceLoader {
       }
     };
 
-    if (element.localName === "script" && element.hasAttribute("async")) {
+    if (element.localName === "script" && element.hasAttributeNS(null, "async")) {
       this._asyncQueue.push(request, onLoadWrapped, onErrorWrapped, this._queue.getLastScript());
-    } else if (element.localName === "script" && element.hasAttribute("defer")) {
+    } else if (element.localName === "script" && element.hasAttributeNS(null, "defer")) {
       this._deferQueue.push(request, onLoadWrapped, onErrorWrapped, false, element);
     } else {
       this._queue.push(request, onLoadWrapped, onErrorWrapped, false, element);

diff --git a/lib/jsdom/living/attributes.js b/lib/jsdom/living/attributes.js
@@ -8,6 +8,8 @@ const { queueAttributeMutationRecord } = require("./helpers/mutation-observers")
 // The following three are for https://dom.spec.whatwg.org/#concept-element-attribute-has. We don't just have a
 // predicate tester since removing that kind of flexibility gives us the potential for better future optimizations.
 
+/* eslint-disable no-restricted-properties */
+
 exports.hasAttribute = function (element, A) {
   return element._attributeList.includes(A);
 };

diff --git a/lib/jsdom/living/attributes/NamedNodeMap-impl.js b/lib/jsdom/living/attributes/NamedNodeMap-impl.js
@@ -46,9 +46,11 @@ exports.implementation = class NamedNodeMapImpl {
     return attributes.getAttributeByNameNS(this._element, namespace, localName);
   }
   setNamedItem(attr) {
+    // eslint-disable-next-line no-restricted-properties
     return attributes.setAttribute(this._element, attr);
   }
   setNamedItemNS(attr) {
+    // eslint-disable-next-line no-restricted-properties
     return attributes.setAttribute(this._element, attr);
   }
   removeNamedItem(qualifiedName) {

diff --git a/lib/jsdom/living/helpers/document-base-url.js b/lib/jsdom/living/helpers/document-base-url.js
@@ -46,7 +46,7 @@ function frozenBaseURL(baseElement, fallbackBaseURL) {
   // https://html.spec.whatwg.org/multipage/semantics.html#frozen-base-url
   // The spec is eager (setting the frozen base URL when things change); we are lazy (getting it when we need to)
 
-  const baseHrefAttribute = baseElement.getAttribute("href");
+  const baseHrefAttribute = baseElement.getAttributeNS(null, "href");
   const result = whatwgURL.parseURL(baseHrefAttribute, { baseURL: fallbackBaseURL });
   return result === null ? fallbackBaseURL : result;
 }
diff --git a/lib/jsdom/living/helpers/focusing.js b/lib/jsdom/living/helpers/focusing.js
@@ -22,7 +22,7 @@ exports.isFocusableAreaElement = elImpl => {
     return true;
   }
 
-  if (!Number.isNaN(parseInt(elImpl.getAttribute("tabindex")))) {
+  if (!Number.isNaN(parseInt(elImpl.getAttributeNS(null, "tabindex")))) {
     return true;
   }
 
@@ -31,7 +31,7 @@ exports.isFocusableAreaElement = elImpl => {
       return true;
     }
 
-    if (elImpl._localName === "a" && elImpl.hasAttribute("href")) {
+    if (elImpl._localName === "a" && elImpl.hasAttributeNS(null, "href")) {
       return true;
     }
 

diff --git a/lib/jsdom/living/helpers/form-controls.js b/lib/jsdom/living/helpers/form-controls.js
@@ -29,14 +29,14 @@ const submittableLocalNames = new Set(["button", "input", "keygen", "object", "s
 exports.isDisabled = formControl => {
   if (formControl.localName === "button" || formControl.localName === "input" || formControl.localName === "select" ||
       formControl.localName === "textarea") {
-    if (formControl.hasAttribute("disabled")) {
+    if (formControl.hasAttributeNS(null, "disabled")) {
       return true;
     }
   }
 
   let e = formControl.parentNode;
   while (e) {
-    if (e.localName === "fieldset" && e.hasAttribute("disabled")) {
+    if (e.localName === "fieldset" && e.hasAttributeNS(null, "disabled")) {
       const firstLegendElementChild = firstChildWithLocalName(e, "legend");
       if (!firstLegendElementChild || !firstLegendElementChild.contains(formControl)) {
         return true;
@@ -159,7 +159,7 @@ exports.sanitizeValueByType = (input, val) => {
     case "email":
       // https://html.spec.whatwg.org/multipage/forms.html#e-mail-state-(type=email):value-sanitization-algorithm
       // https://html.spec.whatwg.org/multipage/forms.html#e-mail-state-(type=email):value-sanitization-algorithm-2
-      if (input.hasAttribute("multiple")) {
+      if (input.hasAttributeNS(null, "multiple")) {
         val = val.split(",").map(token => stripLeadingAndTrailingASCIIWhitespace(token)).join(",");
       } else {
         val = stripNewlines(val);
@@ -227,7 +227,7 @@ exports.sanitizeValueByType = (input, val) => {
 // https://github.com/whatwg/html/issues/4050 for some discussion.
 
 exports.formOwner = formControl => {
-  const formAttr = formControl.getAttribute("form");
+  const formAttr = formControl.getAttributeNS(null, "form");
   if (formAttr === "") {
     return null;
   }
@@ -239,7 +239,7 @@ exports.formOwner = formControl => {
   let firstElementWithId;
   for (const descendant of domSymbolTree.treeIterator(root)) {
     if (descendant.nodeType === NODE_TYPE.ELEMENT_NODE &&
-      descendant.getAttribute("id") === formAttr) {
+      descendant.getAttributeNS(null, "id") === formAttr) {
       firstElementWithId = descendant;
       break;
     }

diff --git a/lib/jsdom/living/helpers/stylesheets.js b/lib/jsdom/living/helpers/stylesheets.js
@@ -60,8 +60,8 @@ function fetchStylesheetInternal(elementImpl, urlString, parsedURL) {
   let defaultEncoding = document._encoding;
   const resourceLoader = document._resourceLoader;
 
-  if (elementImpl.localName === "link" && elementImpl.hasAttribute("charset")) {
-    defaultEncoding = whatwgEncoding.labelToName(elementImpl.getAttribute("charset"));
+  if (elementImpl.localName === "link" && elementImpl.hasAttributeNS(null, "charset")) {
+    defaultEncoding = whatwgEncoding.labelToName(elementImpl.getAttributeNS(null, "charset"));
   }
 
   function onStylesheetLoad(data) {

diff --git a/lib/jsdom/living/named-properties-window.js b/lib/jsdom/living/named-properties-window.js
@@ -38,9 +38,9 @@ function namedPropertyResolver(window, name, values) {
         continue;
       }
 
-      if (node.getAttribute("id") === name) {
+      if (node.getAttributeNS(null, "id") === name) {
         results.push(node);
-      } else if (node.getAttribute("name") === name && isNamedPropertyElement(node)) {
+      } else if (node.getAttributeNS(null, "name") === name && isNamedPropertyElement(node)) {
         results.push(node);
       }
     }
@@ -61,7 +61,7 @@ function namedPropertyResolver(window, name, values) {
     const node = objects[i];
 
     if ("contentWindow" in node && !hasOwnProp.call(node, "contentWindow") &&
-       node.getAttribute("name") === name) {
+       node.getAttributeNS(null, "name") === name) {
       return node.contentWindow;
     }
   }
@@ -93,11 +93,11 @@ exports.elementAttributeModified = function (element, name, value, oldValue) {
 
     // (tracker will be null if the document has no Window)
     if (tracker) {
-      if (name === "id" && (!useName || element.getAttribute("name") !== oldValue)) {
+      if (name === "id" && (!useName || element.getAttributeNS(null, "name") !== oldValue)) {
         tracker.untrack(oldValue, element);
       }
 
-      if (name === "name" && element.getAttribute("id") !== oldValue) {
+      if (name === "name" && element.getAttributeNS(null, "id") !== oldValue) {
         tracker.untrack(oldValue, element);
       }
 
@@ -116,10 +116,10 @@ exports.nodeAttachedToDocument = function (node) {
     return;
   }
 
-  tracker.track(node.getAttribute("id"), node);
+  tracker.track(node.getAttributeNS(null, "id"), node);
 
   if (isNamedPropertyElement(node)) {
-    tracker.track(node.getAttribute("name"), node);
+    tracker.track(node.getAttributeNS(null, "name"), node);
   }
 };
 
@@ -133,9 +133,9 @@ exports.nodeDetachedFromDocument = function (node) {
     return;
   }
 
-  tracker.untrack(node.getAttribute("id"), node);
+  tracker.untrack(node.getAttributeNS(null, "id"), node);
 
   if (isNamedPropertyElement(node)) {
-    tracker.untrack(node.getAttribute("name"), node);
+    tracker.untrack(node.getAttributeNS(null, "name"), node);
   }
 };
diff --git a/lib/jsdom/living/nodes/Document-impl.js b/lib/jsdom/living/nodes/Document-impl.js
@@ -420,7 +420,7 @@ class DocumentImpl extends NodeImpl {
       element: this,
       query: () => domSymbolTree.treeToArray(this, {
         filter: node => (node._localName === "a" || node._localName === "area") &&
-                        node.hasAttribute("href") &&
+                        node.hasAttributeNS(null, "href") &&
                         node._namespaceURI === HTML_NS
       })
     });
@@ -436,7 +436,7 @@ class DocumentImpl extends NodeImpl {
       element: this,
       query: () => domSymbolTree.treeToArray(this, {
         filter: node => node._localName === "a" &&
-                        node.hasAttribute("name") &&
+                        node.hasAttributeNS(null, "name") &&
                         node._namespaceURI === HTML_NS
       })
     });
@@ -531,7 +531,7 @@ class DocumentImpl extends NodeImpl {
     return NodeList.createImpl([], {
       element: this,
       query: () => domSymbolTree.treeToArray(this, {
-        filter: node => node.getAttribute && node.getAttribute("name") === elementName
+        filter: node => node.getAttributeNS && node.getAttributeNS(null, "name") === elementName
       })
     });
   }

diff --git a/lib/jsdom/living/nodes/Element-impl.js b/lib/jsdom/living/nodes/Element-impl.js
@@ -80,7 +80,7 @@ class ElementImpl extends NodeImpl {
   _attach() {
     namedPropertiesWindow.nodeAttachedToDocument(this);
 
-    const id = this.getAttribute("id");
+    const id = this.getAttributeNS(null, "id");
     if (id) {
       attachId(id, this, this._ownerDocument);
     }
@@ -93,7 +93,7 @@ class ElementImpl extends NodeImpl {
 
     namedPropertiesWindow.nodeDetachedFromDocument(this);
 
-    const id = this.getAttribute("id");
+    const id = this.getAttributeNS(null, "id");
     if (id) {
       detachId(id, this, this._ownerDocument);
     }
@@ -306,18 +306,22 @@ class ElementImpl extends NodeImpl {
   }
 
   setAttributeNode(attr) {
+    // eslint-disable-next-line no-restricted-properties
     return attributes.setAttribute(this, attr);
   }
 
   setAttributeNodeNS(attr) {
+    // eslint-disable-next-line no-restricted-properties
     return attributes.setAttribute(this, attr);
   }
 
   removeAttributeNode(attr) {
+    // eslint-disable-next-line no-restricted-properties
     if (!attributes.hasAttribute(this, attr)) {
       throw new DOMException("Tried to remove an attribute that was not present", "NotFoundError");
     }
 
+    // eslint-disable-next-line no-restricted-properties
     attributes.removeAttribute(this, attr);
 
     return attr;

diff --git a/lib/jsdom/living/nodes/ElementCSSInlineStyle-impl.js b/lib/jsdom/living/nodes/ElementCSSInlineStyle-impl.js
@@ -6,7 +6,7 @@ class ElementCSSInlineStyle {
     this._style = new cssstyle.CSSStyleDeclaration(newCssText => {
       if (!this._settingCssText) {
         this._settingCssText = true;
-        this.setAttribute("style", newCssText);
+        this.setAttributeNS(null, "style", newCssText);
         this._settingCssText = false;
       }
     });

diff --git a/lib/jsdom/living/nodes/GlobalEventHandlers-impl.js b/lib/jsdom/living/nodes/GlobalEventHandlers-impl.js
@@ -80,7 +80,7 @@ class GlobalEventHandlersImpl {
       return;
     }
 
-    const val = this.getAttribute(propName);
+    const val = this.getAttributeNS(null, propName);
     const handler = val === null ? null : { body: val };
     this._setEventHandlerFor(event, handler);
   }

diff --git a/lib/jsdom/living/nodes/HTMLAndSVGElementShared-impl.js b/lib/jsdom/living/nodes/HTMLAndSVGElementShared-impl.js
@@ -16,14 +16,14 @@ class HTMLAndSVGElementSharedImpl {
 
   // TODO this should be [Reflect]able if we added default value support to webidl2js's [Reflect]
   get tabIndex() {
-    if (!this.hasAttribute("tabindex")) {
+    if (!this.hasAttributeNS(null, "tabindex")) {
       return focusing.isFocusableAreaElement(this) ? 0 : -1;
     }
-    return conversions.long(this.getAttribute("tabindex"));
+    return conversions.long(this.getAttributeNS(null, "tabindex"));
   }
 
   set tabIndex(value) {
-    this.setAttribute("tabindex", String(value));
+    this.setAttributeNS(null, "tabindex", String(value));
   }
 
   focus() {

diff --git a/lib/jsdom/living/nodes/HTMLBaseElement-impl.js b/lib/jsdom/living/nodes/HTMLBaseElement-impl.js
@@ -7,7 +7,7 @@ class HTMLBaseElementImpl extends HTMLElementImpl {
   get href() {
     const document = this._ownerDocument;
 
-    const url = this.hasAttribute("href") ? this.getAttribute("href") : "";
+    const url = this.hasAttributeNS(null, "href") ? this.getAttributeNS(null, "href") : "";
     const parsed = whatwgURL.parseURL(url, { baseURL: fallbackBaseURL(document) });
 
     if (parsed === null) {
@@ -18,7 +18,7 @@ class HTMLBaseElementImpl extends HTMLElementImpl {
   }
 
   set href(value) {
-    this.setAttribute("href", value);
+    this.setAttributeNS(null, "href", value);
   }
 }
 

diff --git a/lib/jsdom/living/nodes/HTMLButtonElement-impl.js b/lib/jsdom/living/nodes/HTMLButtonElement-impl.js
@@ -29,7 +29,7 @@ class HTMLButtonElementImpl extends HTMLElementImpl {
   }
 
   _getValue() {
-    const valueAttr = this.getAttribute("value");
+    const valueAttr = this.getAttributeNS(null, "value");
     return valueAttr === null ? "" : valueAttr;
   }
 
@@ -42,7 +42,7 @@ class HTMLButtonElementImpl extends HTMLElementImpl {
   }
 
   get type() {
-    const typeAttr = (this.getAttribute("type") || "").toLowerCase();
+    const typeAttr = (this.getAttributeNS(null, "type") || "").toLowerCase();
     switch (typeAttr) {
       case "submit":
       case "reset":
@@ -59,10 +59,10 @@ class HTMLButtonElementImpl extends HTMLElementImpl {
       case "submit":
       case "reset":
       case "button":
-        this.setAttribute("type", v);
+        this.setAttributeNS(null, "type", v);
         break;
       default:
-        this.setAttribute("type", "submit");
+        this.setAttributeNS(null, "type", "submit");
         break;
     }
   }