From 9498da1038ffa854effa672fac3a898039853375 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Tue, 22 Jun 2021 16:09:39 +0300 Subject: [PATCH] fix(service-worker): correctly determine client ID on navigation requests (#42607) The ServiceWorker assigns an app-version to a each client to ensure that all subsequent requests for a client are served using the same app-version. The assignment is done based on the client ID. Previously, the ServiceWorker would only try to read the client's ID off of the `FetchEvent`'s `clientId` property. However, for navigation requests the new client's ID will be set on [resultingClientId][1], while `clientId` will either be empty or hold the ID of the client where the request initiated from. See also related discussions in w3c/ServiceWorker#870 and w3c/ServiceWorker#1266. In theory, this could lead to the navigation request (i.e. `index.html`) being served from a different app-version than the subsequent sub-resource requests (i.e. assets). In practice, the likelihood of this happening is probably very low though, since it would require the latest app-version to be updated between the initial navigation request and the first sub-resource request, which should happen very shortly after the navigation request. This commit ensures that the correct client ID is determined even for navigation requests by also taking the `resultingClientId` property into account. [1]: https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/resultingClientId PR Close #42607 --- packages/service-worker/worker/src/driver.ts | 5 ++++- .../service-worker/worker/src/service-worker.d.ts | 3 ++- packages/service-worker/worker/test/happy_spec.ts | 8 ++------ packages/service-worker/worker/testing/scope.ts | 11 +++++++---- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts index 94c02baeee..bcecf3b002 100644 --- a/packages/service-worker/worker/src/driver.ts +++ b/packages/service-worker/worker/src/driver.ts @@ -609,7 +609,10 @@ export class Driver implements Debuggable, UpdateSource { private async assignVersion(event: FetchEvent): Promise { // First, check whether the event has a (non empty) client ID. If it does, the version may // already be associated. - const clientId = event.clientId; + // + // NOTE: For navigation requests, we care about the `resultingClientId`. If it is undefined or + // the empty string (which is the case for sub-resource requests), we look at `clientId`. + const clientId = event.resultingClientId || event.clientId; if (clientId) { // Check if there is an assigned client id. if (this.clientVersionMap.has(clientId)) { diff --git a/packages/service-worker/worker/src/service-worker.d.ts b/packages/service-worker/worker/src/service-worker.d.ts index a608f597de..71281d3679 100644 --- a/packages/service-worker/worker/src/service-worker.d.ts +++ b/packages/service-worker/worker/src/service-worker.d.ts @@ -60,8 +60,9 @@ type WindowClientState = 'hidden'|'visible'|'prerender'|'unloaded'; // Fetch API interface FetchEvent extends ExtendableEvent { - clientId: string|null; request: Request; + clientId?: string; + resultingClientId?: string; respondWith(response: Promise|Response): Promise; } diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index fcaf4bd27d..5f191e7204 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -538,7 +538,6 @@ describe('Driver', () => { it('handles empty client ID', async () => { // Initialize the SW. expect(await makeNavigationRequest(scope, '/foo/file1', '')).toEqual('this is foo'); - expect(await makeNavigationRequest(scope, '/bar/file2', null)).toEqual('this is foo'); await driver.initialized; // Update to a new version. @@ -547,7 +546,6 @@ describe('Driver', () => { // Correctly handle navigation requests, even if `clientId` is null/empty. expect(await makeNavigationRequest(scope, '/foo/file1', '')).toEqual('this is foo v2'); - expect(await makeNavigationRequest(scope, '/bar/file2', null)).toEqual('this is foo v2'); }); it('checks for updates on restart', async () => { @@ -2008,8 +2006,7 @@ async function removeAssetFromCache( } async function makeRequest( - scope: SwTestHarness, url: string, clientId: string|null = 'default', - init?: Object): Promise { + scope: SwTestHarness, url: string, clientId = 'default', init?: Object): Promise { const [resPromise, done] = scope.handleFetch(new MockRequest(url, init), clientId); await done; const res = await resPromise; @@ -2020,8 +2017,7 @@ async function makeRequest( } function makeNavigationRequest( - scope: SwTestHarness, url: string, clientId?: string|null, - init: Object = {}): Promise { + scope: SwTestHarness, url: string, clientId?: string, init: Object = {}): Promise { return makeRequest(scope, url, clientId, { headers: { Accept: 'text/plain, text/html, text/css', diff --git a/packages/service-worker/worker/testing/scope.ts b/packages/service-worker/worker/testing/scope.ts index 57e77055ee..96eb23d5c9 100644 --- a/packages/service-worker/worker/testing/scope.ts +++ b/packages/service-worker/worker/testing/scope.ts @@ -219,12 +219,14 @@ export class SwTestHarness extends Adapter implements ServiceWorkerGlobalScope, waitUntil(promise: Promise): void {} - handleFetch(req: Request, clientId: string|null = null): - [Promise, Promise] { + handleFetch(req: Request, clientId = ''): [Promise, Promise] { if (!this.eventHandlers.has('fetch')) { throw new Error('No fetch handler registered'); } - const event = new MockFetchEvent(req, clientId); + + const isNavigation = req.mode === 'navigate'; + const event = isNavigation ? new MockFetchEvent(req, '', clientId) : + new MockFetchEvent(req, clientId, ''); this.eventHandlers.get('fetch')!.call(this, event); if (clientId) { @@ -373,7 +375,8 @@ class MockExtendableEvent extends OneTimeContext {} class MockFetchEvent extends MockExtendableEvent { response: Promise = Promise.resolve(undefined); - constructor(readonly request: Request, readonly clientId: string|null) { + constructor( + readonly request: Request, readonly clientId: string, readonly resultingClientId: string) { super(); }