From be5e838d143bb369190984daed484b9d03498813 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Fri, 18 Jun 2021 15:05:56 +0300 Subject: [PATCH] fix(service-worker): correctly determine client ID on navigation requests 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 --- packages/service-worker/worker/src/driver.ts | 3 ++- packages/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 | 10 +++++++--- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts index 94c02baeee2f0c..f40e8313509577 100644 --- a/packages/service-worker/worker/src/driver.ts +++ b/packages/service-worker/worker/src/driver.ts @@ -609,7 +609,8 @@ 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; + // (For navigation requests, we care about the `resultingClientId`.) + 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 a608f597de69c1..71281d3679871a 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 fcaf4bd27dc5a0..5f191e7204a685 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 57e77055ee02d8..7b931569c62464 100644 --- a/packages/service-worker/worker/testing/scope.ts +++ b/packages/service-worker/worker/testing/scope.ts @@ -219,12 +219,15 @@ export class SwTestHarness extends Adapter implements ServiceWorkerGlobalScope, waitUntil(promise: Promise): void {} - handleFetch(req: Request, clientId: string|null = null): + 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 +376,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(); }