fix(service-worker): include headers in requests for assets

gkalpak · gkalpak · commit 281e3b3545a6 · 2022-08-25T21:25:06.000+03:00
Previously, when requesting non-cached asset resources from the network, the ServiceWorker would strip off all request metadata (including headers). This was done in order to avoid issues with opaque responses, but it turned out to be overly aggressive, breaking/worsening legit usecases (such as requesting compressed data). This commit fixes this by preserving the headers of such requests. For reference, Workbox passes the original request as is. (See for example the [NetworkFirst][1] strategy). > **Note** > Data requests (i.e. requests for URLs that belong to a data-group) are not affected by this. They already use the original resource as is. [1]: https://github.com/GoogleChrome/workbox/blob/95f97a207fd51efb3f8a653f6e3e58224183a778/packages/workbox-strategies/src/NetworkFirst.ts#L90 Fixes #24227
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
@@ -35,6 +35,7 @@ const dist =
         .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'})
@@ -161,6 +162,7 @@ const manifest: Manifest = {
       urls: [
         '/baz.txt',
         '/qux.txt',
+        '/lazy/redirected.txt',
       ],
       patterns: [],
       cacheQueryOptions: {ignoreVary: true},
@@ -270,6 +272,7 @@ const manifestUpdate: Manifest = {
 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();
@@ -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/mock.ts b/packages/service-worker/worker/testing/mock.ts
@@ -179,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.`);
@@ -192,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;

Original file line number	Diff line number	Diff line change
`@@ -194,7 +194,7 @@ export class AppVersion implements UpdateSource {`
`194`	`194`	`}`
`195`	`195`
`196`	`196`	// This was a navigation request. Re-enter `handleFetch` with a request for
`197`		`- // the URL.`
	`197`	`+ // the index URL.`
`198`	`198`	`return this.handleFetch(this.adapter.newRequest(this.indexUrl), event);`
`199`	`199`	`}`
`200`	`200`