diff --git a/packages/service-worker/worker/src/app-version.ts b/packages/service-worker/worker/src/app-version.ts index 8cc7e8634e753..1a6349d44bd23 100644 --- 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 index ec33c3b7a4287..c82a623596d26 100644 --- 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 index 771ff74efdab6..f83b2008d8126 100644 --- 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 index 435b3d98bb150..042ccab64f6df 100644 --- 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;