gkalpak · Aug 25, 2022 · Aug 25, 2022 · Aug 25, 2022
diff --git a/packages/service-worker/worker/src/app-version.ts b/packages/service-worker/worker/src/app-version.ts
@@ -194,7 +194,7 @@ export class AppVersion implements UpdateSource {
       }
 
       // This was a navigation request. Re-enter `handleFetch` with a request for
-      // the URL.
+      // the index URL.
       return this.handleFetch(this.adapter.newRequest(this.indexUrl), event);
     }
 

diff --git a/packages/service-worker/worker/src/assets.ts b/packages/service-worker/worker/src/assets.ts
@@ -150,10 +150,8 @@ export abstract class AssetGroup {
         }
       }
 
-      // No already-cached response exists, so attempt a fetch/cache operation. The original request
-      // may specify things like credential inclusion, but for assets these are not honored in order
-      // to avoid issues with opaque responses. The SW requests the data itself.
-      const res = await this.fetchAndCacheOnce(this.adapter.newRequest(req.url));
+      // No already-cached response exists, so attempt a fetch/cache operation.
+      const res = await this.fetchAndCacheOnce(this.newRequestWithMetadata(req.url, req));
 
       // If this is successful, the response needs to be cloned as it might be used to respond to
       // multiple fetch operations at the same time.
@@ -360,7 +358,7 @@ export abstract class AssetGroup {
       }
 
       // Unwrap the redirect directly.
-      return this.fetchFromNetwork(this.adapter.newRequest(res.url), redirectLimit - 1);
+      return this.fetchFromNetwork(this.newRequestWithMetadata(res.url, req), redirectLimit - 1);
     }
 
     return res;
@@ -413,7 +411,7 @@ export abstract class AssetGroup {
         // data, or because the version on the server really doesn't match. A cache-busting
         // request will differentiate these two situations.
         // TODO: handle case where the URL has parameters already (unlikely for assets).
-        const cacheBustReq = this.adapter.newRequest(this.cacheBust(req.url));
+        const cacheBustReq = this.newRequestWithMetadata(this.cacheBust(req.url), req);
         response = await this.safeFetch(cacheBustReq);
 
         // If the response was successful, check the contents against the canonical hash.
@@ -476,6 +474,24 @@ export abstract class AssetGroup {
     return false;
   }
 
+  /**
+   * Create a new `Request` based on the specified URL and `RequestInit` options, preserving only
+   * metadata that are known to be safe.
+   *
+   * Currently, only headers are preserved.
+   *
+   * NOTE:
+   *   Things like credential inclusion are intentionally omitted to avoid issues with opaque
+   *   responses.
+   *
+   * TODO(gkalpak):
+   *   Investigate preserving more metadata. See, also, discussion on preserving `mode`:
+   *   https://github.com/angular/angular/issues/41931#issuecomment-1227601347
+   */
+  private newRequestWithMetadata(url: string, options: RequestInit): Request {
+    return this.adapter.newRequest(url, {headers: options.headers});
+  }
+
   /**
    * Construct a cache-busting URL for a given URL.
    */

diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts
@@ -32,8 +32,10 @@ const dist =
         .addFile('/qux.txt', 'this is qux')
         .addFile('/quux.txt', 'this is quux')
         .addFile('/quuux.txt', 'this is quuux')
+        .addFile('/redirect-target.txt', 'this was a redirect')
         .addFile('/lazy/unchanged1.txt', 'this is unchanged (1)')
         .addFile('/lazy/unchanged2.txt', 'this is unchanged (2)')
+        .addFile('/lazy/redirect-target.txt', 'this was a redirect too')
         .addUnhashedFile('/unhashed/a.txt', 'this is unhashed', {'Cache-Control': 'max-age=10'})
         .addUnhashedFile('/unhashed/b.txt', 'this is unhashed b', {'Cache-Control': 'no-cache'})
         .addUnhashedFile('/api/foo', 'this is api foo', {'Cache-Control': 'no-cache'})
@@ -48,6 +50,7 @@ const distUpdate =
         .addFile('/qux.txt', 'this is qux v2')
         .addFile('/quux.txt', 'this is quux v2')
         .addFile('/quuux.txt', 'this is quuux v2')
+        .addFile('/redirect-target.txt', 'this was a redirect')
         .addFile('/lazy/unchanged1.txt', 'this is unchanged (1)')
         .addFile('/lazy/unchanged2.txt', 'this is unchanged (2)')
         .addUnhashedFile('/unhashed/a.txt', 'this is unhashed v2', {'Cache-Control': 'max-age=10'})
@@ -159,6 +162,7 @@ const manifest: Manifest = {
       urls: [
         '/baz.txt',
         '/qux.txt',
+        '/lazy/redirected.txt',
       ],
       patterns: [],
       cacheQueryOptions: {ignoreVary: true},
@@ -265,23 +269,22 @@ const manifestUpdate: Manifest = {
   hashTable: tmpHashTableForFs(distUpdate),
 };
 
-const serverBuilderBase =
-    new MockServerStateBuilder()
-        .withStaticFiles(dist)
-        .withRedirect('/redirected.txt', '/redirect-target.txt', 'this was a redirect')
-        .withError('/error.txt');
+const serverBuilderBase = new MockServerStateBuilder()
+                              .withStaticFiles(dist)
+                              .withRedirect('/redirected.txt', '/redirect-target.txt')
+                              .withRedirect('/lazy/redirected.txt', '/lazy/redirect-target.txt')
+                              .withError('/error.txt');
 
 const server = serverBuilderBase.withManifest(manifest).build();
 
 const serverRollback =
     serverBuilderBase.withManifest({...manifest, timestamp: manifest.timestamp + 1}).build();
 
-const serverUpdate =
-    new MockServerStateBuilder()
-        .withStaticFiles(distUpdate)
-        .withManifest(manifestUpdate)
-        .withRedirect('/redirected.txt', '/redirect-target.txt', 'this was a redirect')
-        .build();
+const serverUpdate = new MockServerStateBuilder()
+                         .withStaticFiles(distUpdate)
+                         .withManifest(manifestUpdate)
+                         .withRedirect('/redirected.txt', '/redirect-target.txt')
+                         .build();
 
 const brokenServer =
     new MockServerStateBuilder().withStaticFiles(brokenFs).withManifest(brokenManifest).build();
@@ -1578,6 +1581,71 @@ describe('Driver', () => {
     });
   });
 
+  describe('request metadata', () => {
+    it('passes headers through to the server', async () => {
+      // Request a lazy-cached asset (so that it is fetched from the network) and provide headers.
+      const reqInit = {
+        headers: {SomeHeader: 'SomeValue'},
+      };
+      expect(await makeRequest(scope, '/baz.txt', undefined, reqInit)).toBe('this is baz');
+
+      // Verify that the headers were passed through to the network.
+      const [bazReq] = server.getRequestsFor('/baz.txt');
+      expect(bazReq.headers.get('SomeHeader')).toBe('SomeValue');
+    });
+
+    it('does not pass non-allowed metadata through to the server', async () => {
+      // Request a lazy-cached asset (so that it is fetched from the network) and provide some
+      // metadata.
+      const reqInit = {
+        credentials: 'include',
+        mode: 'same-origin',
+        unknownOption: 'UNKNOWN',
+      };
+      expect(await makeRequest(scope, '/baz.txt', undefined, reqInit)).toBe('this is baz');
+
+      // Verify that the metadata were not passed through to the network.
+      const [bazReq] = server.getRequestsFor('/baz.txt');
+      expect(bazReq.credentials).toBe('same-origin');  // The default value.
+      expect(bazReq.mode).toBe('cors');                // The default value.
+      expect((bazReq as any).unknownOption).toBeUndefined();
+    });
+
+    describe('for redirect requests', () => {
+      it('passes headers through to the server', async () => {
+        // Request a redirected, lazy-cached asset (so that it is fetched from the network) and
+        // provide headers.
+        const reqInit = {
+          headers: {SomeHeader: 'SomeValue'},
+        };
+        expect(await makeRequest(scope, '/lazy/redirected.txt', undefined, reqInit))
+            .toBe('this was a redirect too');
+
+        // Verify that the headers were passed through to the network.
+        const [redirectReq] = server.getRequestsFor('/lazy/redirect-target.txt');
+        expect(redirectReq.headers.get('SomeHeader')).toBe('SomeValue');
+      });
+
+      it('does not pass non-allowed metadata through to the server', async () => {
+        // Request a redirected, lazy-cached asset (so that it is fetched from the network) and
+        // provide some metadata.
+        const reqInit = {
+          credentials: 'include',
+          mode: 'same-origin',
+          unknownOption: 'UNKNOWN',
+        };
+        expect(await makeRequest(scope, '/lazy/redirected.txt', undefined, reqInit))
+            .toBe('this was a redirect too');
+
+        // Verify that the metadata were not passed through to the network.
+        const [redirectReq] = server.getRequestsFor('/lazy/redirect-target.txt');
+        expect(redirectReq.credentials).toBe('same-origin');  // The default value.
+        expect(redirectReq.mode).toBe('cors');                // The default value.
+        expect((redirectReq as any).unknownOption).toBeUndefined();
+      });
+    });
+  });
+
   describe('unhashed requests', () => {
     beforeEach(async () => {
       expect(await makeRequest(scope, '/foo.txt')).toEqual('this is foo');

diff --git a/packages/service-worker/worker/testing/fetch.ts b/packages/service-worker/worker/testing/fetch.ts
@@ -100,7 +100,7 @@ export class MockRequest extends MockBody implements Request {
   readonly isHistoryNavigation: boolean = false;
   readonly isReloadNavigation: boolean = false;
   readonly cache: RequestCache = 'default';
-  readonly credentials: RequestCredentials = 'omit';
+  readonly credentials: RequestCredentials = 'same-origin';
   readonly destination: RequestDestination = 'document';
   readonly headers: Headers = new MockHeaders();
   readonly integrity: string = '';

diff --git a/packages/service-worker/worker/testing/mock.ts b/packages/service-worker/worker/testing/mock.ts
@@ -104,9 +104,8 @@ export class MockServerStateBuilder {
     return this;
   }
 
-  withRedirect(from: string, to: string, toContents: string): MockServerStateBuilder {
-    this.resources.set(from, new MockResponse(toContents, {redirected: true, url: to}));
-    this.resources.set(to, new MockResponse(toContents));
+  withRedirect(from: string, to: string): MockServerStateBuilder {
+    this.resources.set(from, new MockResponse('', {redirected: true, url: to}));
     return this;
   }
 
@@ -180,6 +179,10 @@ export class MockServerState {
     this.resolve = null;
   }
 
+  getRequestsFor(url: string): Request[] {
+    return this.requests.filter(req => req.url.split('?')[0] === url);
+  }
+
   assertSawRequestFor(url: string): void {
     if (!this.sawRequestFor(url)) {
       throw new Error(`Expected request for ${url}, got none.`);
@@ -193,7 +196,7 @@ export class MockServerState {
   }
 
   sawRequestFor(url: string): boolean {
-    const matching = this.requests.filter(req => req.url.split('?')[0] === url);
+    const matching = this.getRequestsFor(url);
     if (matching.length > 0) {
       this.requests = this.requests.filter(req => req !== matching[0]);
       return true;