From d9dc46e651d34f3df7f8607bbef415c8a2d6a88d Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Tue, 20 Mar 2018 15:10:59 +0200 Subject: [PATCH] fix(service-worker): ignore invalid `only-if-cached` requests (#22883) Under some circumstances (possibly related to opening Chrome DevTools), requests are made with `cache: 'only-if-cached'` and `mode: 'no-cors'`. These request will eventually fail, because `only-if-cached` is only allowed to be used with `mode: 'same-origin'`. This is likely a bug in Chrome DevTools. This commit avoids errors related to such requests by not handling them. Fixes #22362 PR Close #22883 --- packages/service-worker/worker/src/driver.ts | 35 ++++++++++++++++--- .../service-worker/worker/test/happy_spec.ts | 9 +++++ .../service-worker/worker/testing/fetch.ts | 5 ++- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts index 19fc0dded6..4ff6184bb1 100644 --- a/packages/service-worker/worker/src/driver.ts +++ b/packages/service-worker/worker/src/driver.ts @@ -94,6 +94,12 @@ export class Driver implements Debuggable, UpdateSource { */ private scheduledNavUpdateCheck: boolean = false; + /** + * Keep track of whether we have logged an invalid `only-if-cached` request. + * (See `.onFetch()` for details.) + */ + private loggedInvalidOnlyIfCachedRequest: boolean = false; + /** * A scheduler which manages a queue of tasks that need to be executed when the SW is * not doing any other work (not processing any other requests). @@ -156,12 +162,13 @@ export class Driver implements Debuggable, UpdateSource { * asynchronous execution that eventually resolves for respondWith() and waitUntil(). */ private onFetch(event: FetchEvent): void { + const req = event.request; + // The only thing that is served unconditionally is the debug page. - if (this.adapter.parseUrl(event.request.url, this.scope.registration.scope).path === - '/ngsw/state') { + if (this.adapter.parseUrl(req.url, this.scope.registration.scope).path === '/ngsw/state') { // Allow the debugger to handle the request, but don't affect SW state in any // other way. - event.respondWith(this.debugger.handleFetch(event.request)); + event.respondWith(this.debugger.handleFetch(req)); return; } @@ -177,6 +184,24 @@ export class Driver implements Debuggable, UpdateSource { return; } + // When opening DevTools in Chrome, a request is made for the current URL (and possibly related + // resources, e.g. scripts) with `cache: 'only-if-cached'` and `mode: 'no-cors'`. These request + // will eventually fail, because `only-if-cached` is only allowed to be used with + // `mode: 'same-origin'`. + // This is likely a bug in Chrome DevTools. Avoid handling such requests. + // (See also https://github.com/angular/angular/issues/22362.) + // TODO(gkalpak): Remove once no longer necessary (i.e. fixed in Chrome DevTools). + if ((req.cache as string) === 'only-if-cached' && req.mode !== 'same-origin') { + // Log the incident only the first time it happens, to avoid spamming the logs. + if (!this.loggedInvalidOnlyIfCachedRequest) { + this.loggedInvalidOnlyIfCachedRequest = true; + this.debugger.log( + `Ignoring invalid request: 'only-if-cached' can be set only with 'same-origin' mode`, + `Driver.fetch(${req.url}, cache: ${req.cache}, mode: ${req.mode})`); + } + return; + } + // Past this point, the SW commits to handling the request itself. This could still // fail (and result in `state` being set to `SAFE_MODE`), but even in that case the // SW will still deliver a response. @@ -611,8 +636,8 @@ export class Driver implements Debuggable, UpdateSource { * offline (detected as response status 504). */ private async fetchLatestManifest(ignoreOfflineError?: false): Promise; - private async fetchLatestManifest(ignoreOfflineError: true): Promise; - private async fetchLatestManifest(ignoreOfflineError = false): Promise { + private async fetchLatestManifest(ignoreOfflineError: true): Promise; + private async fetchLatestManifest(ignoreOfflineError = false): Promise { const res = await this.safeFetch(this.adapter.newRequest('ngsw.json?ngsw-cache-bust=' + Math.random())); if (!res.ok) { diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index f560394805..caacc9a839 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -708,6 +708,15 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate)); expect(driver.state).toEqual(DriverReadyState.EXISTING_CLIENTS_ONLY); }); + + async_it('ignores invalid `only-if-cached` requests ', async() => { + const requestFoo = (cache: RequestCache | 'only-if-cached', mode: RequestMode) => + makeRequest(scope, '/foo.txt', undefined, {cache, mode}); + + expect(await requestFoo('default', 'no-cors')).toBe('this is foo'); + expect(await requestFoo('only-if-cached', 'same-origin')).toBe('this is foo'); + expect(await requestFoo('only-if-cached', 'no-cors')).toBeNull(); + }); }); }); })(); diff --git a/packages/service-worker/worker/testing/fetch.ts b/packages/service-worker/worker/testing/fetch.ts index 5176e46801..5c4dde1196 100644 --- a/packages/service-worker/worker/testing/fetch.ts +++ b/packages/service-worker/worker/testing/fetch.ts @@ -110,6 +110,9 @@ export class MockRequest extends MockBody implements Request { Object.keys(headers).forEach(header => { this.headers.set(header, headers[header]); }); } } + if (init.cache !== undefined) { + this.cache = init.cache; + } if (init.mode !== undefined) { this.mode = init.mode; } @@ -170,4 +173,4 @@ export class MockResponse extends MockBody implements Response { return new MockResponse( this._body, {status: this.status, statusText: this.statusText, headers: this.headers}); } -} \ No newline at end of file +}