core(crawlable-anchors): ignore event listeners, validate url (#13105)

GoogleChrome · Oct 12, 2021 · 19c5b71 · 19c5b71
1 parent f38f293
commit 19c5b71
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 51 deletions.
diff --git a/lighthouse-cli/test/fixtures/seo/seo-tester.html b/lighthouse-cli/test/fixtures/seo/seo-tester.html
@@ -72,13 +72,13 @@ <h6>2</h6>
   <!-- PASS(crawlable-anchors): Link with a relative path -->
   <a href="/pass">Some link</a>
 
-  <!-- PASS(crawlable-anchors): Link with a click event listener -->
+  <!-- FAIL(crawlable-anchors): Link with a click event listener -->
   <a class="some-link">Some link</a>
   <script>
     document.querySelector('.some-link').addEventListener('click', () => {});
   </script>
 
-  <!-- PASS(crawlable-anchors): Link with an onclick which won't match the audit regex -->
+  <!-- FAIL(crawlable-anchors): Link with an onclick which won't match the audit regex -->
   <a onclick="window.Location = '';">Some link</a>
 </body>
 </html>
diff --git a/lighthouse-cli/test/smokehouse/test-definitions/seo/expectations.js b/lighthouse-cli/test/smokehouse/test-definitions/seo/expectations.js
@@ -235,7 +235,12 @@ const passing = {
         },
       },
       'crawlable-anchors': {
-        score: 1,
+        score: 0,
+        details: {
+          items: {
+            length: 2,
+          },
+        },
       },
       'link-text': {
         score: 1,

diff --git a/lighthouse-core/audits/seo/crawlable-anchors.js b/lighthouse-core/audits/seo/crawlable-anchors.js
@@ -31,23 +31,20 @@ class CrawlableAnchors extends Audit {
       title: str_(UIStrings.title),
       failureTitle: str_(UIStrings.failureTitle),
       description: str_(UIStrings.description),
-      requiredArtifacts: ['AnchorElements'],
+      requiredArtifacts: ['AnchorElements', 'URL'],
     };
   }
 
   /**
    * @param {LH.Artifacts} artifacts
    * @return {LH.Audit.Product}
    */
-  static audit({AnchorElements: anchorElements}) {
+  static audit({AnchorElements: anchorElements, URL: url}) {
     const failingAnchors = anchorElements.filter(({
       rawHref,
-      listeners = [],
-      onclick = '',
       name = '',
       role = '',
     }) => {
-      onclick = onclick.replace( /\s/g, '');
       rawHref = rawHref.replace( /\s/g, '');
       name = name.trim();
       role = role.trim();
@@ -56,19 +53,20 @@ class CrawlableAnchors extends Audit {
       // Ignore mailto links even if they use one of the failing patterns. See https://github.com/GoogleChrome/lighthouse/issues/11443#issuecomment-694898412
       if (rawHref.startsWith('mailto:')) return;
 
-      const windowLocationRegExp = /window\.location=/;
-      const windowOpenRegExp = /window\.open\(/;
       const javaScriptVoidRegExp = /javascript:void(\(|)0(\)|)/;
 
       if (rawHref.startsWith('file:')) return true;
-      if (windowLocationRegExp.test(onclick)) return true;
-      if (windowOpenRegExp.test(onclick)) return true;
-
-      const hasClickHandler = listeners.some(({type}) => type === 'click');
-      if (hasClickHandler || name.length > 0) return;
+      if (name.length > 0) return;
 
       if (rawHref === '') return true;
       if (javaScriptVoidRegExp.test(rawHref)) return true;
+
+      // checking if rawHref is a valid
+      try {
+        new URL(rawHref, url.finalUrl);
+      } catch (e) {
+        return true;
+      }
     });
 
     /** @type {LH.Audit.Details.Table['headings']} */

diff --git a/lighthouse-core/test/audits/seo/crawlable-anchors-test.js b/lighthouse-core/test/audits/seo/crawlable-anchors-test.js
@@ -33,6 +33,9 @@ function runAudit({
       role,
       node,
     }],
+    URL: {
+      finalUrl: 'http://example.com',
+    },
   });
 
   return score;
@@ -54,6 +57,8 @@ describe('SEO: Crawlable anchors audit', () => {
     assert.equal(runAudit({
       rawHref: '?query=string',
     }), 1, 'relative link which specifies a query string');
+
+    assert.equal(runAudit({rawHref: 'ftp://'}), 0, 'invalid FTP links fails');
   });
 
   it('allows anchors which use a name attribute', () => {
@@ -71,27 +76,22 @@ describe('SEO: Crawlable anchors audit', () => {
   });
 
   it('handles anchor elements which use event listeners', () => {
-    const auditResultClickPresent = runAudit({
-      listeners: [{type: 'click'}],
+    const auditResultMixtureOfListeners = runAudit({
+      rawHref: '/validPath',
+      listeners: [{type: 'nope'}, {type: 'another'}, {type: 'click'}],
     });
-    assert.equal(auditResultClickPresent, 1, 'presence of a click handler is a pass');
+    assert.equal(auditResultMixtureOfListeners, 1, 'valid href with any event listener is a pass');
 
-    const auditResultJavaScriptURI = runAudit({
+    const auditResultWithInvalidHref = runAudit({
       rawHref: 'javascript:void(0)',
-      listeners: [{type: 'click'}],
+      listeners: [{type: 'nope'}, {type: 'another'}, {type: 'click'}],
     });
-    const assertionMessage = 'hyperlink with a `javascript:` URI and a click handler';
-    assert.equal(auditResultJavaScriptURI, 1, assertionMessage);
+    assert.equal(auditResultWithInvalidHref, 0, 'invalid href with any event listener is a faile');
 
-    const auditResultNonClickListener = runAudit({
-      listeners: [{type: 'nope'}],
+    const auditResultNoListener = runAudit({
+      rawHref: '/validPath',
     });
-    assert.equal(auditResultNonClickListener, 0, 'no click event is a fail');
-
-    const auditResultMixtureOfListeners = runAudit({
-      listeners: [{type: 'nope'}, {type: 'another'}, {type: 'click'}],
-    });
-    assert.equal(auditResultMixtureOfListeners, 1, 'at least one click listener is a pass');
+    assert.equal(auditResultNoListener, 1, 'valid href with no event listener is a pass');
   });
 
   it('disallows uncrawlable anchors', () => {
@@ -119,44 +119,25 @@ describe('SEO: Crawlable anchors audit', () => {
       const auditResult = runAudit({rawHref: javaScriptVoidVariation});
       assert.equal(auditResult, 0, `'${javaScriptVoidVariation}' should fail the audit`);
     }
-
-    const expectedAuditPasses = [
-      'javascript:void',
-      'javascript:void()',
-      'javascript:0',
-    ];
-
-    for (const javaScriptVoidVariation of expectedAuditPasses) {
-      const auditResult = runAudit({rawHref: javaScriptVoidVariation});
-      assert.equal(auditResult, 1, `'${javaScriptVoidVariation}' should pass the audit`);
-    }
   });
 
   it('handles window.location and window.open assignments in an onclick attribute', () => {
-    const expectedAuditFailures = [
+    const expectedAuditPasses = [
       'window.location=',
       'window.location =',
       'window.open()',
       `window.open('')`,
       'window.open(`http://example.com`)',
       'window.open ( )',
       `window.open('foo', 'name', 'resizable)`,
-    ];
-
-    for (const onclickVariation of expectedAuditFailures) {
-      const auditResult = runAudit({onclick: onclickVariation});
-      assert.equal(auditResult, 0, `'${onclickVariation}' should fail the audit`);
-    }
-
-    const expectedAuditPasses = [
       'windowAlocation',
       'window.location.href',
       'window.Location =',
       'windowLopen()',
     ];
 
     for (const onclickVariation of expectedAuditPasses) {
-      const auditResult = runAudit({onclick: onclickVariation});
+      const auditResult = runAudit({rawHref: '/validPath', onclick: onclickVariation});
       assert.equal(auditResult, 1, `'${onclickVariation}' should pass the audit`);
     }
   });