From 7715eda3c5debe85f1cb903606bea4a516d839db Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Tue, 20 Nov 2018 16:16:25 -0800 Subject: [PATCH] core(service-worker): check that test page is in SW scope (#6609) --- lighthouse-core/audits/service-worker.js | 31 +++- .../audits/webapp-install-banner.js | 17 -- .../gather/gatherers/service-worker.js | 3 + .../test/audits/service-worker-test.js | 163 ++++++++++++++++-- .../test/audits/webapp-install-banner-test.js | 19 -- .../gather/gatherers/service-worker-test.js | 31 ++-- lighthouse-core/test/results/sample_v2.json | 5 +- proto/sample_v2_round_trip.json | 5 +- types/artifacts.d.ts | 2 +- 9 files changed, 194 insertions(+), 82 deletions(-) diff --git a/lighthouse-core/audits/service-worker.js b/lighthouse-core/audits/service-worker.js index 7204189076f8..a89c186f05e8 100644 --- a/lighthouse-core/audits/service-worker.js +++ b/lighthouse-core/audits/service-worker.js @@ -5,8 +5,8 @@ */ 'use strict'; -const URL = require('../lib/url-shim'); -const Audit = require('./audit'); +const URL = require('../lib/url-shim.js'); +const Audit = require('./audit.js'); class ServiceWorker extends Audit { /** @@ -29,17 +29,30 @@ class ServiceWorker extends Audit { * @return {LH.Audit.Product} */ static audit(artifacts) { - // Find active service worker for this URL. Match against + const {versions, registrations} = artifacts.ServiceWorker; + const pageUrl = new URL(artifacts.URL.finalUrl); + + // Find active service workers for this origin. Match against // artifacts.URL.finalUrl so audit accounts for any redirects. - const versions = artifacts.ServiceWorker.versions; - const url = artifacts.URL.finalUrl; + const matchingSWVersions = versions.filter(v => v.status === 'activated') + .filter(v => new URL(v.scriptURL).origin === pageUrl.origin); + + if (matchingSWVersions.length === 0) { + return {rawValue: false}; + } + + // Find the normalized scope URLs of possibly-controlling SWs. + const matchingScopeUrls = matchingSWVersions + .map(v => registrations.find(r => r.registrationId === v.registrationId)) + .filter(/** @return {r is LH.Crdp.ServiceWorker.ServiceWorkerRegistration} */ r => !!r) + .map(r => new URL(r.scopeURL).href); - const origin = new URL(url).origin; - const matchingSW = versions.filter(v => v.status === 'activated') - .find(v => new URL(v.scriptURL).origin === origin); + // Ensure page is included in a SW's scope. + // See https://w3c.github.io/ServiceWorker/v1/#scope-match-algorithm + const inScope = matchingScopeUrls.some(scopeUrl => pageUrl.href.startsWith(scopeUrl)); return { - rawValue: !!matchingSW, + rawValue: inScope, }; } } diff --git a/lighthouse-core/audits/webapp-install-banner.js b/lighthouse-core/audits/webapp-install-banner.js index d0cbc2b0fae9..bbe114413325 100644 --- a/lighthouse-core/audits/webapp-install-banner.js +++ b/lighthouse-core/audits/webapp-install-banner.js @@ -6,7 +6,6 @@ 'use strict'; const MultiCheckAudit = require('./multi-check-audit'); -const SWAudit = require('./service-worker'); const ManifestValues = require('../computed/manifest-values.js'); /** @@ -80,20 +79,6 @@ class WebappInstallBanner extends MultiCheckAudit { return failures; } - /** - * @param {LH.Artifacts} artifacts - * @return {Array} - */ - static assessServiceWorker(artifacts) { - const failures = []; - const hasServiceWorker = SWAudit.audit(artifacts).rawValue; - if (!hasServiceWorker) { - failures.push('Site does not register a service worker'); - } - - return failures; - } - /** * @param {LH.Artifacts} artifacts * @param {LH.Audit.Context} context @@ -102,12 +87,10 @@ class WebappInstallBanner extends MultiCheckAudit { static async audit_(artifacts, context) { const manifestValues = await ManifestValues.request(artifacts.Manifest, context); const manifestFailures = WebappInstallBanner.assessManifest(manifestValues); - const swFailures = WebappInstallBanner.assessServiceWorker(artifacts); return { failures: [ ...manifestFailures, - ...swFailures, ], manifestValues, }; diff --git a/lighthouse-core/gather/gatherers/service-worker.js b/lighthouse-core/gather/gatherers/service-worker.js index 0a9cad683005..6a27febb644a 100644 --- a/lighthouse-core/gather/gatherers/service-worker.js +++ b/lighthouse-core/gather/gatherers/service-worker.js @@ -14,8 +14,11 @@ class ServiceWorker extends Gatherer { */ async beforePass(passContext) { const {versions} = await passContext.driver.getServiceWorkerVersions(); + const {registrations} = await passContext.driver.getServiceWorkerRegistrations(); + return { versions, + registrations, }; } } diff --git a/lighthouse-core/test/audits/service-worker-test.js b/lighthouse-core/test/audits/service-worker-test.js index fa7adb2fab03..da0a8b243e54 100644 --- a/lighthouse-core/test/audits/service-worker-test.js +++ b/lighthouse-core/test/audits/service-worker-test.js @@ -5,39 +5,168 @@ */ 'use strict'; -const Audit = require('../../audits/service-worker.js'); +const ServiceWorker = require('../../audits/service-worker.js'); +const URL = require('../../lib/url-shim.js'); const assert = require('assert'); /* eslint-env jest */ +function getBaseDirectory(urlStr) { + const url = new URL(urlStr); + return url.origin + url.pathname.substring(0, url.pathname.lastIndexOf('/') + 1); +} + +/** + * Create a ServiceWorker artifact from an array of SW config opts. + * @param {Array<{scriptURL: string, status: string, scopeURL?: string}>} swOpts + * @param {string} finalUrl + */ +function createArtifacts(swOpts, finalUrl) { + const artifact = {versions: [], registrations: []}; + + swOpts.forEach((sw, index) => { + artifact.versions.push({ + registrationId: `${index}`, + status: sw.status, + scriptURL: sw.scriptURL, + }); + + const scopeURL = sw.scopeURL || getBaseDirectory(sw.scriptURL); + assert.ok(scopeURL.endsWith('/')); // required by SW spec. + + artifact.registrations.push({ + registrationId: `${index}`, + scopeURL, + }); + }); + + return { + ServiceWorker: artifact, + URL: {finalUrl}, + }; +} + describe('Offline: service worker audit', () => { it('passes when given a matching service worker version', () => { - const output = Audit.audit({ - ServiceWorker: { - versions: [{ - status: 'activated', - scriptURL: 'https://example.com/sw.js', - }], - }, - URL: {finalUrl: 'https://example.com'}, - }); + const finalUrl = 'https://example.com'; + const swOpts = [{ + status: 'activated', + scriptURL: 'https://example.com/sw.js', + }]; + const output = ServiceWorker.audit(createArtifacts(swOpts, finalUrl)); assert.equal(output.rawValue, true); }); + it('fails when matching service worker is not activated', () => { + const finalUrl = 'https://example.com'; + const swOpts = [{ + status: 'redundant', + scriptURL: 'https://example.com/sw.js', + }]; + + const output = ServiceWorker.audit(createArtifacts(swOpts, finalUrl)); + assert.equal(output.rawValue, false); + }); + it('discards service worker registrations for other origins', () => { - const versions = [{ + const finalUrl = 'https://example.com'; + const swOpts = [{ status: 'activated', scriptURL: 'https://other-example.com', }]; - const output = Audit.audit({ - ServiceWorker: { - versions, - }, - URL: {finalUrl: 'https://example.com'}, - }); + const output = ServiceWorker.audit(createArtifacts(swOpts, finalUrl)); + assert.equal(output.rawValue, false); + }); + + it('fails when URL is out of scope', () => { + const finalUrl = 'https://example.com/index.html'; + const swOpts = [{ + status: 'activated', + scriptURL: 'https://example.com/serviceworker/sw.js', + }]; + + const output = ServiceWorker.audit(createArtifacts(swOpts, finalUrl)); + assert.equal(output.rawValue, false); + }); + + it('fails when explicit scopeURL puts the URL out of scope', () => { + const finalUrl = 'https://example.com/index.html'; + const swOpts = [{ + status: 'activated', + scriptURL: 'https://example.com/sw.js', + scopeURL: 'https://example.com/serviceworker/', + }]; + + const output = ServiceWorker.audit(createArtifacts(swOpts, finalUrl)); + assert.equal(output.rawValue, false); + }); + + it('passes when outside default scope but explicit scopeURL puts it back in', () => { + const finalUrl = 'https://example.com/index.html'; + const swOpts = [{ + status: 'activated', + scriptURL: 'https://example.com/serviceworker/sw.js', + // can happen when 'Service-Worker-Allowed' header widens max scope. + scopeURL: 'https://example.com/', + }]; + + const output = ServiceWorker.audit(createArtifacts(swOpts, finalUrl)); + assert.equal(output.rawValue, true); + }); + + it('passes when multiple SWs control the scope', () => { + const finalUrl = 'https://example.com/project/index.html'; + const swOpts = [{ + status: 'activated', + scriptURL: 'https://example.com/sw.js', + }, { + status: 'activated', + scriptURL: 'https://example.com/project/sw.js', + }]; + + const output = ServiceWorker.audit(createArtifacts(swOpts, finalUrl)); + assert.equal(output.rawValue, true); + }); + + it('passes when multiple SWs control the origin but only one is in scope', () => { + const finalUrl = 'https://example.com/index.html'; + const swOpts = [{ + status: 'activated', + scriptURL: 'https://example.com/sw.js', + scopeURL: 'https://example.com/project/', + }, { + status: 'activated', + scriptURL: 'https://example.com/project/sw.js', + scopeURL: 'https://example.com/project/', + }, { + status: 'activated', + scriptURL: 'https://example.com/project/subproject/sw.js', + scopeURL: 'https://example.com/', + }]; + + const output = ServiceWorker.audit(createArtifacts(swOpts, finalUrl)); + assert.equal(output.rawValue, true); + }); + + it('fails when multiple SWs control the origin but are all out of scope', () => { + const finalUrl = 'https://example.com/index.html'; + const swOpts = [{ + status: 'activated', + scriptURL: 'https://example.com/sw.js', + scopeURL: 'https://example.com/project/', + }, { + status: 'activated', + scriptURL: 'https://example.com/project/sw.js', + scopeURL: 'https://example.com/project/', + }, { + status: 'activated', + scriptURL: 'https://example.com/project/subproject/sw.js', + scopeURL: 'https://example.com/project/', + }]; + const output = ServiceWorker.audit(createArtifacts(swOpts, finalUrl)); assert.equal(output.rawValue, false); }); }); diff --git a/lighthouse-core/test/audits/webapp-install-banner-test.js b/lighthouse-core/test/audits/webapp-install-banner-test.js index 9e9d4b2b6682..2835a33a17a5 100644 --- a/lighthouse-core/test/audits/webapp-install-banner-test.js +++ b/lighthouse-core/test/audits/webapp-install-banner-test.js @@ -19,12 +19,6 @@ function generateMockArtifacts(src = manifestSrc) { const clonedArtifacts = JSON.parse(JSON.stringify({ Manifest: exampleManifest, - ServiceWorker: { - versions: [{ - status: 'activated', - scriptURL: 'https://example.com/sw.js', - }], - }, URL: {finalUrl: 'https://example.com'}, })); return clonedArtifacts; @@ -146,17 +140,4 @@ describe('PWA: webapp install banner audit', () => { assert.strictEqual(failures.length, 1, failures); }); }); - - it('fails if page had no SW', () => { - const artifacts = generateMockArtifacts(); - artifacts.ServiceWorker.versions = []; - const context = generateMockAuditContext(); - - return WebappInstallBannerAudit.audit(artifacts, context).then(result => { - assert.strictEqual(result.rawValue, false); - assert.ok(result.explanation.includes('service worker'), result.explanation); - const failures = result.details.items[0].failures; - assert.strictEqual(failures.length, 1, failures); - }); - }); }); diff --git a/lighthouse-core/test/gather/gatherers/service-worker-test.js b/lighthouse-core/test/gather/gatherers/service-worker-test.js index 9d4c5916e573..e5fd4a3e83fa 100644 --- a/lighthouse-core/test/gather/gatherers/service-worker-test.js +++ b/lighthouse-core/test/gather/gatherers/service-worker-test.js @@ -7,32 +7,37 @@ /* eslint-env jest */ -const ServiceWorkerGather = require('../../../gather/gatherers/service-worker'); +const ServiceWorkerGather = require('../../../gather/gatherers/service-worker.js'); const assert = require('assert'); -let serviceWorkerGatherer; describe('service worker gatherer', () => { - // Reset the Gatherer before each test. - beforeEach(() => { - serviceWorkerGatherer = new ServiceWorkerGather(); - }); - - it('obtains the active service worker registration', () => { + it('obtains the active service worker registration', async () => { const url = 'https://example.com/'; const versions = [{ + registrationId: '123', status: 'activated', scriptURL: url, }]; + const registrations = [{ + registrationId: '123', + scopeUrl: url, + isDeleted: false, + }]; - return serviceWorkerGatherer.beforePass({ + const serviceWorkerGatherer = new ServiceWorkerGather(); + const artifact = await serviceWorkerGatherer.beforePass({ driver: { - getServiceWorkerVersions() { - return Promise.resolve({versions}); + async getServiceWorkerVersions() { + return {versions}; + }, + async getServiceWorkerRegistrations() { + return {registrations}; }, }, url, - }).then(artifact => { - assert.deepEqual(artifact.versions, versions); }); + + assert.deepEqual(artifact.versions, versions); + assert.deepEqual(artifact.registrations, registrations); }); }); diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json index 3c75700763db..c527d7ac5b13 100644 --- a/lighthouse-core/test/results/sample_v2.json +++ b/lighthouse-core/test/results/sample_v2.json @@ -467,13 +467,12 @@ "score": 0, "scoreDisplayMode": "binary", "rawValue": false, - "explanation": "Failures: No manifest was fetched,\nSite does not register a service worker.", + "explanation": "Failures: No manifest was fetched.", "details": { "items": [ { "failures": [ - "No manifest was fetched", - "Site does not register a service worker" + "No manifest was fetched" ], "isParseFailure": true, "parseFailureReason": "No manifest was fetched" diff --git a/proto/sample_v2_round_trip.json b/proto/sample_v2_round_trip.json index a3339bc094e5..9e8c3c34300d 100644 --- a/proto/sample_v2_round_trip.json +++ b/proto/sample_v2_round_trip.json @@ -2548,15 +2548,14 @@ "items": [ { "failures": [ - "No manifest was fetched", - "Site does not register a service worker" + "No manifest was fetched" ], "isParseFailure": true, "parseFailureReason": "No manifest was fetched" } ] }, - "explanation": "Failures: No manifest was fetched,\nSite does not register a service worker.", + "explanation": "Failures: No manifest was fetched.", "id": "webapp-install-banner", "score": 0.0, "scoreDisplayMode": "binary", diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index b8ee7ff34d36..a8dff54d290f 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -108,7 +108,7 @@ declare global { /** The content of all scripts loaded by the page, keyed by networkRecord requestId. */ Scripts: Record; /** Version information for all ServiceWorkers active after the first page load. */ - ServiceWorker: {versions: Crdp.ServiceWorker.ServiceWorkerVersion[]}; + ServiceWorker: {versions: Crdp.ServiceWorker.ServiceWorkerVersion[], registrations: Crdp.ServiceWorker.ServiceWorkerRegistration[]}; /** The status of an offline fetch of the page's start_url. -1 and a explanation if missing or there was an error. */ StartUrl: {statusCode: number, explanation?: string}; /** Information on