From ee35e223a7916d7d4ffa9f87f199793e1059cf0a Mon Sep 17 00:00:00 2001 From: Maximilian Koeller Date: Wed, 29 Apr 2020 16:59:15 +0200 Subject: [PATCH] feat(service-worker): use `ignoreVary: true` when retrieving responses from cache (#34663) The Angular ServiceWorker always uses a copy of the request without headers for caching assets (in order to avoid issues with opaque responses). Therefore, it was previously not possible to retrieve resources from the cache if the response contained [Vary](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Vary) headers. In addition to that, `Vary` headers do not work in all browsers (or work differently) and may not work as intended with ServiceWorker caches. See [this article](https://www.smashingmagazine.com/2017/11/understanding-vary-header) and the linked resources for more info. This commit avoids the aforementioned issues by making sure the Angular ServiceWorker always sets the `ignoreVary` option passed to [Cache#match()](https://developer.mozilla.org/en-US/docs/Web/API/Cache/match) to `true`. This allows the ServiceWorker to correctly retrieve cached responses with `Vary` headers, which was previously not possible. Fixes #36638 BREAKING CHANGE: Previously, [Vary](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Vary) headers would be taken into account when retrieving resources from the cache, completely preventing the retrieval of cached assets (due to ServiceWorker implementation details) and leading to unpredictable behavior due to inconsistent/buggy implementations in different browsers. Now, `Vary` headers are ignored when retrieving resources from the ServiceWorker caches, which can result in resources being retrieved even when their headers are different. If your application needs to differentiate its responses based on request headers, please make sure the Angular ServiceWorker is [configured](https://angular.io/guide/service-worker-config) to avoid caching the affected resources. PR Close #34663 --- .../service-worker/config/src/generator.ts | 7 +++-- .../config/test/generator_spec.ts | 8 ++--- .../service-worker/test/integration_spec.ts | 2 ++ .../service-worker/worker/test/data_spec.ts | 29 +++++++++++++++++-- .../service-worker/worker/test/happy_spec.ts | 11 +++++++ .../worker/test/prefetch_spec.ts | 10 ++++++- .../service-worker/worker/testing/mock.ts | 1 + 7 files changed, 59 insertions(+), 9 deletions(-) diff --git a/packages/service-worker/config/src/generator.ts b/packages/service-worker/config/src/generator.ts index ab5ce58fe0..2e87c45181 100644 --- a/packages/service-worker/config/src/generator.ts +++ b/packages/service-worker/config/src/generator.ts @@ -153,6 +153,9 @@ function withOrderedKeys(unorderedObj: T): T { } function buildCacheQueryOptions(inOptions?: Pick): - CacheQueryOptions|undefined { - return inOptions; + CacheQueryOptions { + return { + ignoreVary: true, + ...inOptions, + }; } diff --git a/packages/service-worker/config/test/generator_spec.ts b/packages/service-worker/config/test/generator_spec.ts index 7960863869..b52e279e8d 100644 --- a/packages/service-worker/config/test/generator_spec.ts +++ b/packages/service-worker/config/test/generator_spec.ts @@ -92,7 +92,7 @@ describe('Generator', () => { '\\/some\\/url\\?with\\+escaped\\+chars', '\\/test\\/relative\\/[^/]*\\.txt', ], - cacheQueryOptions: undefined, + cacheQueryOptions: {ignoreVary: true} }], dataGroups: [{ name: 'other', @@ -106,7 +106,7 @@ describe('Generator', () => { maxAge: 259200000, timeoutMs: 60000, version: 1, - cacheQueryOptions: undefined, + cacheQueryOptions: {ignoreVary: true} }], navigationUrls: [ {positive: true, regex: '^\\/included\\/absolute\\/.*$'}, @@ -229,7 +229,7 @@ describe('Generator', () => { '/main.js', ], patterns: [], - cacheQueryOptions: {ignoreSearch: true} + cacheQueryOptions: {ignoreSearch: true, ignoreVary: true} }], dataGroups: [{ name: 'other', @@ -241,7 +241,7 @@ describe('Generator', () => { maxAge: 259200000, timeoutMs: 60000, version: 1, - cacheQueryOptions: {ignoreSearch: false} + cacheQueryOptions: {ignoreSearch: false, ignoreVary: true} }], navigationUrls: [ {positive: true, regex: '^\\/.*$'}, diff --git a/packages/service-worker/test/integration_spec.ts b/packages/service-worker/test/integration_spec.ts index 5d104e2ab4..7bdf320b27 100644 --- a/packages/service-worker/test/integration_spec.ts +++ b/packages/service-worker/test/integration_spec.ts @@ -44,6 +44,7 @@ const manifest: Manifest = { updateMode: 'prefetch', urls: ['/only.txt'], patterns: [], + cacheQueryOptions: {ignoreVary: true}, }], navigationUrls: [], hashTable: tmpHashTableForFs(dist), @@ -60,6 +61,7 @@ const manifestUpdate: Manifest = { updateMode: 'prefetch', urls: ['/only.txt'], patterns: [], + cacheQueryOptions: {ignoreVary: true}, }], navigationUrls: [], hashTable: tmpHashTableForFs(distUpdate), diff --git a/packages/service-worker/worker/test/data_spec.ts b/packages/service-worker/worker/test/data_spec.ts index 68c1050b8a..ce57da9f05 100644 --- a/packages/service-worker/worker/test/data_spec.ts +++ b/packages/service-worker/worker/test/data_spec.ts @@ -56,6 +56,7 @@ const manifest: Manifest = { '/bar.txt', ], patterns: [], + cacheQueryOptions: {ignoreVary: true}, }, ], dataGroups: [ @@ -67,7 +68,7 @@ const manifest: Manifest = { timeoutMs: 1000, maxAge: 5000, version: 1, - cacheQueryOptions: {ignoreSearch: true}, + cacheQueryOptions: {ignoreVary: true, ignoreSearch: true}, }, { name: 'testRefresh', @@ -78,6 +79,7 @@ const manifest: Manifest = { refreshAheadMs: 1000, maxAge: 5000, version: 1, + cacheQueryOptions: {ignoreVary: true}, }, { name: 'testFresh', @@ -87,6 +89,7 @@ const manifest: Manifest = { timeoutMs: 1000, maxAge: 5000, version: 1, + cacheQueryOptions: {ignoreVary: true}, }, ], navigationUrls: [], @@ -201,7 +204,8 @@ describe('data cache', () => { await makeRequest(scope, '/api/a'); // the second one will be loaded from the cache await makeRequest(scope, '/api/a'); - expect(matchSpy).toHaveBeenCalledWith(new MockRequest('/api/a'), {ignoreSearch: true}); + expect(matchSpy).toHaveBeenCalledWith( + new MockRequest('/api/a'), {ignoreVary: true, ignoreSearch: true}); }); it('still matches if search differs but ignoreSearch is enabled', async () => { @@ -312,6 +316,27 @@ describe('data cache', () => { expect(await res2).toBe(''); }); + + it('CacheQueryOptions are passed through when falling back to cache', async () => { + const matchSpy = spyOn(MockCache.prototype, 'match').and.callThrough(); + await makeRequest(scope, '/fresh/data'); + server.clearRequests(); + scope.updateServerState(serverUpdate); + serverUpdate.pause(); + const [res, done] = makePendingRequest(scope, '/fresh/data'); + + await serverUpdate.nextRequest; + + // Since the network request doesn't return within the timeout of 1,000ms, + // this should return cached data. + scope.advance(2000); + await res; + expect(matchSpy).toHaveBeenCalledWith(new MockRequest('/fresh/data'), {ignoreVary: true}); + + // Unpausing allows the worker to continue with caching. + serverUpdate.unpause(); + await done; + }); }); }); })(); diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index 320240c555..8803f2a7bb 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -70,6 +70,7 @@ const brokenManifest: Manifest = { '/foo.txt', ], patterns: [], + cacheQueryOptions: {ignoreVary: true}, }], dataGroups: [], navigationUrls: processNavigationUrls(''), @@ -89,6 +90,7 @@ const brokenLazyManifest: Manifest = { '/foo.txt', ], patterns: [], + cacheQueryOptions: {ignoreVary: true}, }, { name: 'lazy-assets', @@ -98,6 +100,7 @@ const brokenLazyManifest: Manifest = { '/bar.txt', ], patterns: [], + cacheQueryOptions: {ignoreVary: true}, }, ], dataGroups: [], @@ -143,6 +146,7 @@ const manifest: Manifest = { patterns: [ '/unhashed/.*', ], + cacheQueryOptions: {ignoreVary: true}, }, { name: 'other', @@ -153,6 +157,7 @@ const manifest: Manifest = { '/qux.txt', ], patterns: [], + cacheQueryOptions: {ignoreVary: true}, }, { name: 'lazy_prefetch', @@ -165,6 +170,7 @@ const manifest: Manifest = { '/lazy/unchanged2.txt', ], patterns: [], + cacheQueryOptions: {ignoreVary: true}, } ], dataGroups: [ @@ -177,6 +183,7 @@ const manifest: Manifest = { patterns: [ '/api/.*', ], + cacheQueryOptions: {ignoreVary: true}, }, { name: 'api-static', @@ -187,6 +194,7 @@ const manifest: Manifest = { patterns: [ '/api-static/.*', ], + cacheQueryOptions: {ignoreVary: true}, }, ], navigationUrls: processNavigationUrls(''), @@ -213,6 +221,7 @@ const manifestUpdate: Manifest = { patterns: [ '/unhashed/.*', ], + cacheQueryOptions: {ignoreVary: true}, }, { name: 'other', @@ -223,6 +232,7 @@ const manifestUpdate: Manifest = { '/qux.txt', ], patterns: [], + cacheQueryOptions: {ignoreVary: true}, }, { name: 'lazy_prefetch', @@ -235,6 +245,7 @@ const manifestUpdate: Manifest = { '/lazy/unchanged2.txt', ], patterns: [], + cacheQueryOptions: {ignoreVary: true}, } ], navigationUrls: processNavigationUrls( diff --git a/packages/service-worker/worker/test/prefetch_spec.ts b/packages/service-worker/worker/test/prefetch_spec.ts index d4508e0afa..c42b6c2b04 100644 --- a/packages/service-worker/worker/test/prefetch_spec.ts +++ b/packages/service-worker/worker/test/prefetch_spec.ts @@ -9,6 +9,8 @@ import {PrefetchAssetGroup} from '../src/assets'; import {CacheDatabase} from '../src/db-cache'; import {IdleScheduler} from '../src/idle'; +import {MockCache} from '../testing/cache'; +import {MockRequest} from '../testing/fetch'; import {MockFileSystemBuilder, MockServerStateBuilder, tmpHashTable, tmpManifestSingleAssetGroup} from '../testing/mock'; import {SwTestHarness, SwTestHarnessBuilder} from '../testing/scope'; @@ -19,7 +21,7 @@ if (!SwTestHarness.envIsSupported()) { } const dist = new MockFileSystemBuilder() - .addFile('/foo.txt', 'this is foo') + .addFile('/foo.txt', 'this is foo', {Vary: 'Accept'}) .addFile('/bar.txt', 'this is bar') .build(); @@ -84,6 +86,12 @@ describe('prefetch assets', () => { const err = await errorFrom(group.initializeFully()); expect(err.message).toContain('Hash mismatch'); }); + it('CacheQueryOptions are passed through', async () => { + await group.initializeFully(); + const matchSpy = spyOn(MockCache.prototype, 'match').and.callThrough(); + await group.handleFetch(scope.newRequest('/foo.txt'), scope); + expect(matchSpy).toHaveBeenCalledWith(new MockRequest('/foo.txt'), {ignoreVary: true}); + }); }); })(); diff --git a/packages/service-worker/worker/testing/mock.ts b/packages/service-worker/worker/testing/mock.ts index 1369263a2e..e5eb92616b 100644 --- a/packages/service-worker/worker/testing/mock.ts +++ b/packages/service-worker/worker/testing/mock.ts @@ -225,6 +225,7 @@ export function tmpManifestSingleAssetGroup(fs: MockFileSystem): Manifest { updateMode: 'prefetch', urls: files, patterns: [], + cacheQueryOptions: {ignoreVary: true} }, ], navigationUrls: [],