From bc27d4aae4a07ae603637f4a0b1b53562ad9d6b7 Mon Sep 17 00:00:00 2001
From: George Kalpakas <kalpakas.g@gmail.com>
Date: Tue, 1 May 2018 01:55:32 +0300
Subject: [PATCH] fix(service-worker): correctly handle requests with empty
 `clientId` (#23625)

Requests from clients that are not assigned a client ID by the browser
will produce `fetch` events with `null` or empty (`''`) `clientId`s.

Previously, the ServiceWorker only handled `null` values correctly. Yet
empty strings are also valid (see for example [here][1] and [there][2]).
With this commit, the SW will interpret _all_ falsy `clientId` values
the same (i.e. "no client ID assigned") and handle them appropriately.

Related Chromium issue/discussion: [#832105][3]

[1]: https://github.com/w3c/ServiceWorker/blob/4cc72bd0f13359f16cc733d88bed4ea52ee063c0/docs/index.bs#L1392
[2]: https://w3c.github.io/ServiceWorker/#fetchevent-interface
[3]: https://bugs.chromium.org/p/chromium/issues/detail?id=832105

Fixes #23526

PR Close #23625
---
 packages/service-worker/worker/src/driver.ts  |  4 ++--
 .../service-worker/worker/test/happy_spec.ts  | 21 +++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts
index bc21c4564c..12475c7e4b 100644
--- a/packages/service-worker/worker/src/driver.ts
+++ b/packages/service-worker/worker/src/driver.ts
@@ -544,10 +544,10 @@ export class Driver implements Debuggable, UpdateSource {
    * Decide which version of the manifest to use for the event.
    */
   private async assignVersion(event: FetchEvent): Promise<AppVersion|null> {
-    // First, check whether the event has a client ID. If it does, the version may
+    // First, check whether the event has a (non empty) client ID. If it does, the version may
     // already be associated.
     const clientId = event.clientId;
-    if (clientId !== null) {
+    if (clientId) {
       // Check if there is an assigned client id.
       if (this.clientVersionMap.has(clientId)) {
         // There is an assignment for this client already.
diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts
index c68f2615b8..ad7ed0b519 100644
--- a/packages/service-worker/worker/test/happy_spec.ts
+++ b/packages/service-worker/worker/test/happy_spec.ts
@@ -307,6 +307,27 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate));
       expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo v2');
     });
 
+    async_it('handles empty client ID', async() => {
+      const navRequest = (url: string, clientId: string | null) =>
+          makeRequest(scope, url, clientId, {
+            headers: {Accept: 'text/plain, text/html, text/css'},
+            mode: 'navigate',
+          });
+
+      // Initialize the SW.
+      expect(await navRequest('/foo/file1', '')).toEqual('this is foo');
+      expect(await navRequest('/bar/file2', null)).toEqual('this is foo');
+      await driver.initialized;
+
+      // Update to a new version.
+      scope.updateServerState(serverUpdate);
+      expect(await driver.checkForUpdate()).toEqual(true);
+
+      // Correctly handle navigation requests, even if `clientId` is null/empty.
+      expect(await navRequest('/foo/file1', '')).toEqual('this is foo v2');
+      expect(await navRequest('/bar/file2', null)).toEqual('this is foo v2');
+    });
+
     async_it('checks for updates on restart', async() => {
       expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');
       await driver.initialized;