core(seo): improve validation for invalid canonical URLs

core/audits/seo/canonical.js

@@ -5,7 +5,6 @@
  */
 
 import {Audit} from '../audit.js';
-import UrlUtils from '../../lib/url-utils.js';
 import {MainResource} from '../../computed/main-resource.js';
 import * as i18n from '../../lib/i18n/i18n.js';
 
@@ -92,10 +91,26 @@ class Canonical extends Audit {
 
         // Links that had an hrefRaw but didn't have a valid href were invalid, flag them
         if (!link.href) invalidCanonicalLink = link;
-        // Links that had a valid href but didn't have a valid hrefRaw must have been relatively resolved, flag them
-        else if (!UrlUtils.isValid(link.hrefRaw)) relativeCanonicallink = link;
-        // Otherwise, it was a valid canonical URL
-        else uniqueCanonicalURLs.add(link.href);
+
+        else if (link.hrefRaw) {
+          try {
+            // First test: Is Link syntactically valid or invalid?
+            new URL(link.hrefRaw, 'https://example.com');
+            try {
+              // Second test: Is Link valid absolute or valid relative?
+              new URL(link.hrefRaw);
+              // Link is a valid and absolute URL.
+              uniqueCanonicalURLs.add(link.href);
+            } catch (e) {
+              // The Second test FAILED. Link must be valid relative.
+              relativeCanonicallink = link;
+            }
+          } catch (e) {
+            // The First test FAILED. Link is invalid.
+            invalidCanonicalLink = link;
+          }
+        }
+
       } else if (link.rel === 'alternate') {
         if (link.href && link.hreflang) hreflangURLs.add(link.href);
       }
@@ -154,7 +169,7 @@ class Canonical extends Audit {
    * @return {LH.Audit.Product|undefined}
    */
   static findCommonCanonicalURLMistakes(canonicalURLData, canonicalURL, baseURL) {
-    const {hreflangURLs} = canonicalURLData;
+    const { hreflangURLs } = canonicalURLData;
 
     // cross-language or cross-country canonicals are a common issue
     if (
@@ -189,7 +204,7 @@ class Canonical extends Audit {
   static async audit(artifacts, context) {
     const devtoolsLog = artifacts.DevtoolsLog;
 
-    const mainResource = await MainResource.request({devtoolsLog, URL: artifacts.URL}, context);
+    const mainResource = await MainResource.request({ devtoolsLog, URL: artifacts.URL }, context);
     const baseURL = new URL(mainResource.url);
     const canonicalURLData = Canonical.collectCanonicalURLs(artifacts.LinkElements);
 

core/test/audits/seo/canonical-test.js

@@ -63,25 +63,46 @@ describe('SEO: Document has valid canonical link', () => {
     });
   });
 
-  it('fails when canonical url is invalid', () => {
-    const mainDocumentUrl = 'http://www.example.com';
-    const mainResource = {url: mainDocumentUrl};
-    const devtoolsLog = networkRecordsToDevtoolsLog([mainResource]);
-    const artifacts = {
-      DevtoolsLog: devtoolsLog,
-      URL: {mainDocumentUrl},
-      LinkElements: [
-        link({rel: 'canonical', source: 'head', href: null, hrefRaw: 'https:// example.com'}),
-      ],
-    };
+it('fails for a URL with a space in the host (simulating a permissive parser)', () => {
+  const mainDocumentUrl = 'http://www.example.com';
+  const mainResource = {url: mainDocumentUrl};
+  const devtoolsLog = networkRecordsToDevtoolsLog([mainResource]);
+  const artifacts = {
+    DevtoolsLog: devtoolsLog,
+    URL: {mainDocumentUrl},
+    LinkElements: [
+      link({rel: 'canonical', source: 'head', href: 'https://%20example.com', hrefRaw: 'https:// example.com'}),
+    ],
+  };
 
-    const context = {computedCache: new Map()};
-    return CanonicalAudit.audit(artifacts, context).then(auditResult => {
-      const {score, explanation} = auditResult;
-      assert.equal(score, 0);
-      expect(explanation).toBeDisplayString('Invalid URL (https:// example.com)');
-    });
+  const context = {computedCache: new Map()};
+  return CanonicalAudit.audit(artifacts, context).then(auditResult => {
+    const {score, explanation} = auditResult;
+    assert.equal(score, 0);
+    expect(explanation).toBeDisplayString('Invalid URL (https:// example.com)');
   });
+});
+
+it('fails for a URL invalid, the parser returns null', () => {
+  const mainDocumentUrl = 'http://www.example.com';
+  const mainResource = {url: mainDocumentUrl};
+  const devtoolsLog = networkRecordsToDevtoolsLog([mainResource]);
+  const artifacts = {
+    DevtoolsLog: devtoolsLog,
+    URL: {mainDocumentUrl},
+    LinkElements: [
+      link({rel: 'canonical', source: 'head', href: null, hrefRaw: 'I am not a URL'}),
+    ],
+  };
+
+  const context = {computedCache: new Map()};
+  return CanonicalAudit.audit(artifacts, context).then(auditResult => {
+    const {score, explanation} = auditResult;
+    assert.equal(score, 0);
+    expect(explanation).toBeDisplayString('Invalid URL (I am not a URL)');
+  });
+});
+
 
   it('fails when canonical url is relative', () => {
     const mainDocumentUrl = 'https://example.com/de/';