throw error when folks try to use a bad selector

5 files changed, 45 insertions, 55 deletions

diff --git a/js/src/util.js b/js/src/util.js
index 653598ae2b..653b14a17f 100644
--- a/js/src/util.js
+++ b/js/src/util.js
@@ -79,14 +79,11 @@ const Util = (($) => {
       let selector = element.getAttribute('data-target')
 
       if (!selector || selector === '#') {
-        selector = (element.getAttribute('href') || '').trim()
+        const hrefAttr = element.getAttribute('href')
+        selector = hrefAttr && hrefAttr !== '#' ? hrefAttr.trim() : ''
       }
 
-      try {
-        return document.querySelector(selector) ? selector : null
-      } catch (err) {
-        return null
-      }
+      return selector && document.querySelector(selector) ? selector : null
     },
 
     getTransitionDurationFromElement(element) {
diff --git a/js/tests/unit/dropdown.js b/js/tests/unit/dropdown.js
index 81d35ff3a7..40489c5f2c 100644
--- a/js/tests/unit/dropdown.js
+++ b/js/tests/unit/dropdown.js
@@ -216,30 +216,6 @@ $(function () {
     $dropdown.trigger('click')
   })
 
-  QUnit.test('should test if element has a # before assuming it\'s a selector', function (assert) {
-    assert.expect(1)
-    var done = assert.async()
-    var dropdownHTML = '<div class="tabs">' +
-        '<div class="dropdown">' +
-        '<a href="/foo/" class="dropdown-toggle" data-toggle="dropdown">Dropdown</a>' +
-        '<div class="dropdown-menu">' +
-        '<a class="dropdown-item" href="#">Secondary link</a>' +
-        '<a class="dropdown-item" href="#">Something else here</a>' +
-        '<div class="divider"/>' +
-        '<a class="dropdown-item" href="#">Another link</a>' +
-        '</div>' +
-        '</div>' +
-        '</div>'
-    var $dropdown = $(dropdownHTML).find('[data-toggle="dropdown"]').bootstrapDropdown()
-    $dropdown
-      .parent('.dropdown')
-      .on('shown.bs.dropdown', function () {
-        assert.ok($dropdown.parent('.dropdown').hasClass('show'), '"show" class added on click')
-        done()
-      })
-    $dropdown.trigger('click')
-  })
-
   QUnit.test('should remove "show" class if body is clicked', function (assert) {
     assert.expect(2)
     var done = assert.async()
diff --git a/js/tests/unit/modal.js b/js/tests/unit/modal.js
index 53a2e04167..914366ae22 100644
--- a/js/tests/unit/modal.js
+++ b/js/tests/unit/modal.js
@@ -607,36 +607,40 @@ $(function () {
     assert.expect(1)
     var done = assert.async()
 
-    var $toggleBtn = $('<button data-toggle="modal" data-target="&lt;div id=&quot;modal-test&quot;&gt;&lt;div class=&quot;contents&quot;&lt;div&lt;div id=&quot;close&quot; data-dismiss=&quot;modal&quot;/&gt;&lt;/div&gt;&lt;/div&gt;"/>')
-      .appendTo('#qunit-fixture')
+    try {
+      var $toggleBtn = $('<button data-toggle="modal" data-target="&lt;div id=&quot;modal-test&quot;&gt;&lt;div class=&quot;contents&quot;&lt;div&lt;div id=&quot;close&quot; data-dismiss=&quot;modal&quot;/&gt;&lt;/div&gt;&lt;/div&gt;"/>')
+        .appendTo('#qunit-fixture')
 
-    $toggleBtn.trigger('click')
-    setTimeout(function () {
+      $toggleBtn.trigger('click')
+    } catch (e) {
       assert.strictEqual($('#modal-test').length, 0, 'target has not been parsed and added to the document')
       done()
-    }, 1)
+    }
   })
 
   QUnit.test('should not execute js from target', function (assert) {
     assert.expect(0)
     var done = assert.async()
 
-    // This toggle button contains XSS payload in its data-target
-    // Note: it uses the onerror handler of an img element to execute the js, because a simple script element does not work here
-    //       a script element works in manual tests though, so here it is likely blocked by the qunit framework
-    var $toggleBtn = $('<button data-toggle="modal" data-target="&lt;div&gt;&lt;image src=&quot;missing.png&quot; onerror=&quot;$(&apos;#qunit-fixture button.control&apos;).trigger(&apos;click&apos;)&quot;&gt;&lt;/div&gt;"/>')
-      .appendTo('#qunit-fixture')
-    // The XSS payload above does not have a closure over this function and cannot access the assert object directly
-    // However, it can send a click event to the following control button, which will then fail the assert
-    $('<button>')
-      .addClass('control')
-      .on('click', function () {
-        assert.notOk(true, 'XSS payload is not executed as js')
-      })
-      .appendTo('#qunit-fixture')
-
-    $toggleBtn.trigger('click')
-    setTimeout(done, 500)
+    try {
+      // This toggle button contains XSS payload in its data-target
+      // Note: it uses the onerror handler of an img element to execute the js, because a simple script element does not work here
+      //       a script element works in manual tests though, so here it is likely blocked by the qunit framework
+      var $toggleBtn = $('<button data-toggle="modal" data-target="&lt;div&gt;&lt;image src=&quot;missing.png&quot; onerror=&quot;$(&apos;#qunit-fixture button.control&apos;).trigger(&apos;click&apos;)&quot;&gt;&lt;/div&gt;"/>')
+        .appendTo('#qunit-fixture')
+      // The XSS payload above does not have a closure over this function and cannot access the assert object directly
+      // However, it can send a click event to the following control button, which will then fail the assert
+      $('<button>')
+        .addClass('control')
+        .on('click', function () {
+          assert.notOk(true, 'XSS payload is not executed as js')
+        })
+        .appendTo('#qunit-fixture')
+
+      $toggleBtn.trigger('click')
+    } catch (e) {
+      done()
+    }
   })
 
   QUnit.test('should not try to open a modal which is already visible', function (assert) {
diff --git a/js/tests/unit/tab.js b/js/tests/unit/tab.js
index c70e0d1c96..e28ca83a75 100644
--- a/js/tests/unit/tab.js
+++ b/js/tests/unit/tab.js
@@ -186,8 +186,8 @@ $(function () {
         '<ul class="drop nav">' +
         '  <li class="dropdown"><a data-toggle="dropdown" href="#">1</a>' +
         '    <ul class="dropdown-menu nav">' +
-        '      <li><a href="#1-1" data-toggle="tab">1-1</a></li>' +
-        '      <li><a href="#1-2" data-toggle="tab">1-2</a></li>' +
+        '      <li><a href="#a1-1" data-toggle="tab">1-1</a></li>' +
+        '      <li><a href="#a1-2" data-toggle="tab">1-2</a></li>' +
         '    </ul>' +
         '  </li>' +
         '</ul>'
@@ -198,10 +198,10 @@ $(function () {
       .end()
       .find('ul > li:last-child a')
       .on('show.bs.tab', function (e) {
-        assert.strictEqual(e.relatedTarget.hash, '#1-1', 'references correct element as relatedTarget')
+        assert.strictEqual(e.relatedTarget.hash, '#a1-1', 'references correct element as relatedTarget')
       })
       .on('shown.bs.tab', function (e) {
-        assert.strictEqual(e.relatedTarget.hash, '#1-1', 'references correct element as relatedTarget')
+        assert.strictEqual(e.relatedTarget.hash, '#a1-1', 'references correct element as relatedTarget')
         done()
       })
       .bootstrapTab('show')
diff --git a/js/tests/unit/util.js b/js/tests/unit/util.js
index 37327b8681..768afc8fe5 100644
--- a/js/tests/unit/util.js
+++ b/js/tests/unit/util.js
@@ -20,6 +20,19 @@ $(function () {
     assert.strictEqual(Util.getSelectorFromElement($el2[0]), null)
   })
 
+  QUnit.test('Util.getSelectorFromElement should throw error when there is a bad selector', function (assert) {
+    assert.expect(2)
+
+    var $el = $('<div data-target="#1"></div>').appendTo($('#qunit-fixture'))
+
+    try {
+      assert.ok(true, 'trying to use a bad selector')
+      Util.getSelectorFromElement($el[0])
+    } catch (e) {
+      assert.ok(e instanceof DOMException)
+    }
+  })
+
   QUnit.test('Util.typeCheckConfig should thrown an error when a bad config is passed', function (assert) {
     assert.expect(1)
     var namePlugin = 'collapse'