Repository: GoogleChrome/lighthouse (js)

Issue:
Third-party Google Assets don't follow their own audit rules, causing report errors
When I run lighthouse reports, my only errors and warnings come from assets delivered by the creator of lighthouse: Google.

Specifically, this warning/error shows up.

## Uses inefficient cache policy on static assets
*All* the items in this report come from 
  * `securepubads.g.doubleclick.net` (google property).
  * `googletagmanager.com` (google property)
  * `pagead2.googlesyndication.com` (google property)
  * `www.google-analytics.com` (google property)
  * `tpc.googlesyndication.com` (google property)

Why does Google serve assets that fail to follow their own recommendations as based on lighthouse?  If they recommend static assets be sent with proper caching headers, why doesn't google follow through with that? Some of their assets have a ttl of *0* or *15m*!

If they aren't going to follow their own recommendations, then it seems that lighthouse should exclude their own domains from this analysis, since it creates a bunch of false positives which I can do nothing about.

Pull request:
core(uses-long-cache-ttl): ignore private, must-validate, no-cache
Fixes #6140.

Patch:
diff --git a/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js b/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
index c00c19a47..2ce5f806e 100644
--- a/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
+++ b/lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js
@@ -6,7 +6,6 @@
 'use strict';
 
 const assert = require('assert');
-// @ts-ignore - typed where used.
 const parseCacheControl = require('parse-cache-control');
 const Audit = require('../audit');
 const NetworkRequest = require('../../lib/network-request');
@@ -105,22 +104,14 @@ class CacheHeaders extends Audit {
   }
 
   /**
-   * Computes the user-specified cache lifetime, 0 if explicit no-cache policy is in effect, and null if not
-   * user-specified. See https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
-   *
+   * Return max-age if defined, otherwise expires header if defined, and null if not.
    * @param {Map<string, string>} headers
-   * @param {{'no-cache'?: boolean,'no-store'?: boolean, 'max-age'?: number}} cacheControl Follows the potential settings of cache-control, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
+   * @param {ReturnType<typeof parseCacheControl>} cacheControl
    * @return {?number}
    */
   static computeCacheLifetimeInSeconds(headers, cacheControl) {
-    if (cacheControl) {
-      // Cache-Control takes precendence over expires
-      if (cacheControl['no-cache'] || cacheControl['no-store']) return 0;
-      const maxAge = cacheControl['max-age'];
-      if (maxAge !== undefined && Number.isFinite(maxAge)) return Math.max(maxAge, 0);
-    } else if ((headers.get('pragma') || '').includes('no-cache')) {
-      // The HTTP/1.0 Pragma header can disable caching if cache-control is not set, see https://tools.ietf.org/html/rfc7234#section-5.4
-      return 0;
+    if (cacheControl && cacheControl['max-age'] !== undefined) {
+      return cacheControl['max-age'];
     }
 
     const expiresHeaders = headers.get('expires');
@@ -128,7 +119,7 @@ class CacheHeaders extends Audit {
       const expires = new Date(expiresHeaders).getTime();
       // Invalid expires values MUST be treated as already expired
       if (!expires) return 0;
-      return Math.max(0, Math.ceil((expires - Date.now()) / 1000));
+      return Math.ceil((expires - Date.now()) / 1000);
     }
 
     return null;
@@ -169,6 +160,31 @@ class CacheHeaders extends Audit {
     );
   }
 
+  /**
+   * Returns true if headers suggest a record should not be cached for a long time.
+   * @param {Map<string, string>} headers
+   * @param {ReturnType<typeof parseCacheControl>} cacheControl
+   * @returns {boolean}
+   */
+  static shouldSkipRecord(headers, cacheControl) {
+    // The HTTP/1.0 Pragma header can disable caching if cache-control is not set, see https://tools.ietf.org/html/rfc7234#section-5.4
+    if (!cacheControl && (headers.get('pragma') || '').includes('no-cache')) {
+      return true;
+    }
+
+    // Ignore assets where policy implies they should not be cached long periods
+    if (cacheControl &&
+      (
+        cacheControl['must-validate'] ||
+        cacheControl['no-cache'] ||
+        cacheControl['no-store'] ||
+        cacheControl['private'])) {
+      return true;
+    }
+
+    return false;
+  }
+
   /**
    * @param {LH.Artifacts} artifacts
    * @param {LH.Audit.Context} context
@@ -197,15 +213,20 @@ class CacheHeaders extends Audit {
         }
 
         const cacheControl = parseCacheControl(headers.get('cache-control'));
-        let cacheLifetimeInSeconds = CacheHeaders.computeCacheLifetimeInSeconds(
-          headers,
-          cacheControl
-        );
+        if (this.shouldSkipRecord(headers, cacheControl)) {
+          continue;
+        }
 
-        // Ignore assets with an explicit no-cache policy
-        if (cacheLifetimeInSeconds === 0) continue;
+        // Ignore if cacheLifetimeInSeconds is a nonpositive number.
+        let cacheLifetimeInSeconds = CacheHeaders.computeCacheLifetimeInSeconds(
+          headers, cacheControl);
+        if (cacheLifetimeInSeconds !== null &&
+          (!Number.isFinite(cacheLifetimeInSeconds) || cacheLifetimeInSeconds <= 0)) {
+          continue;
+        }
         cacheLifetimeInSeconds = cacheLifetimeInSeconds || 0;
 
+        // Ignore assets whose cache lifetime is already high enough
         const cacheHitProbability = CacheHeaders.getCacheHitProbability(cacheLifetimeInSeconds);
         if (cacheHitProbability > IGNORE_THRESHOLD_IN_PERCENT) continue;
 
@@ -218,7 +239,10 @@ class CacheHeaders extends Audit {
 
         results.push({
           url,
-          cacheControl,
+          // Include cacheControl in results, but cast as any so table types
+          // are happy. cacheControl is not shown in the table so this is OK.
+          // TODO(bckenny): fix DetailsItem
+          cacheControl: /** @type {any} */ (cacheControl),
           cacheLifetimeMs: cacheLifetimeInSeconds * 1000,
           cacheHitProbability,
           totalBytes,
diff --git a/types/parse-cache-control/index.d.ts b/types/parse-cache-control/index.d.ts
new file mode 100644
index 000000000..e78a485b6
--- /dev/null
+++ b/types/parse-cache-control/index.d.ts
@@ -0,0 +1,19 @@
+/**
+ * @license Copyright 2019 Google Inc. All Rights Reserved.
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
+ */
+
+declare module 'parse-cache-control' {
+  // Follows the potential settings of cache-control, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
+  interface CacheHeaders {
+    'max-age'?: number;
+    'must-validate'?: boolean;
+    'no-cache'?: boolean;
+    'no-store'?: boolean;
+    'private'?: boolean;
+  }
+
+  function ParseCacheControl(headers?: string): CacheHeaders | null;
+  export = ParseCacheControl;
+}


Test patch:
diff --git a/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js b/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js
index 46afd745b..a4fa6ac43 100644
--- a/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js
+++ b/lighthouse-core/test/audits/byte-efficiency/uses-long-cache-ttl-test.js
@@ -74,6 +74,22 @@ describe('Cache headers audit', () => {
     });
   });
 
+  it('ignores nonpositive and nonfinite max-age headers', () => {
+    const infinityMaxAgeStringValue = '1'.repeat(400);
+    assert.equal(Number.parseInt(infinityMaxAgeStringValue), Infinity);
+    const networkRecords = [
+      networkRecord({headers: {'cache-control': 'max-age=' + infinityMaxAgeStringValue}}),
+      networkRecord({headers: {'cache-control': 'max-age=-' + infinityMaxAgeStringValue}}),
+      networkRecord({headers: {'cache-control': 'max-age=-100'}}),
+    ];
+
+    const context = {options, computedCache: new Map()};
+    return CacheHeadersAudit.audit(getArtifacts(networkRecords), context).then(result => {
+      const items = result.details.items;
+      assert.equal(items.length, 0);
+    });
+  });
+
   it('detects low value expires headers', () => {
     const expiresIn = seconds => new Date(Date.now() + seconds * 1000).toGMTString();
     const closeEnough = (actual, exp) => assert.ok(Math.abs(actual - exp) <= 1, 'invalid expires');
@@ -103,11 +119,10 @@ describe('Cache headers audit', () => {
 
     const networkRecords = [
       networkRecord({headers: {
-        'cache-control': 'must-revalidate,max-age=3600',
+        'cache-control': 'no-transform,max-age=3600',
         'expires': expiresIn(86400),
       }}),
       networkRecord({headers: {
-        'cache-control': 'private,must-revalidate',
         'expires': expiresIn(86400),
       }}),
     ];
@@ -173,6 +188,21 @@ describe('Cache headers audit', () => {
     });
   });
 
+  it('ignores assets where policy implies they should not be cached long periods', () => {
+    const networkRecords = [
+      networkRecord({headers: {'cache-control': 'must-validate'}}),
+      networkRecord({headers: {'cache-control': 'no-cache'}}),
+      networkRecord({headers: {'cache-control': 'private'}}),
+    ];
+
+    const context = {options, computedCache: new Map()};
+    return CacheHeadersAudit.audit(getArtifacts(networkRecords), context).then(result => {
+      const items = result.extendedInfo.value.results;
+      assert.equal(result.score, 1);
+      assert.equal(items.length, 0);
+    });
+  });
+
   it('ignores potentially uncacheable records', () => {
     const networkRecords = [
       networkRecord({statusCode: 500}),