From ba751f0bb4296fc0bf1b1fa59f1ae6f4fc04d2a9 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Tue, 18 Sep 2018 10:52:02 +0300 Subject: [PATCH] fix(service-worker): ignore passive mixed content requests Although [passive mixed content][1] requests (like images) only produce a warning without a ServiceWorker, fetching it via a ServiceWorker results in an error. See https://github.com/angular/angular/issues/23012#issuecomment-376430187 for more details. This commit makes the ServiceWorker ignore such requests and let them be handled by the browser directly to avoid breaking apps that would work without the ServiceWorker. [1]: https://developers.google.com/web/fundamentals/security/prevent-mixed-content/what-is-mixed-content#passive_mixed_content Fixes #23012 --- packages/service-worker/worker/src/driver.ts | 18 +++++++--- .../service-worker/worker/test/happy_spec.ts | 34 +++++++++++++++++-- .../service-worker/worker/testing/scope.ts | 2 +- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/packages/service-worker/worker/src/driver.ts b/packages/service-worker/worker/src/driver.ts index 1dc99259b36c2..e6dccd0f95174 100644 --- a/packages/service-worker/worker/src/driver.ts +++ b/packages/service-worker/worker/src/driver.ts @@ -162,11 +162,11 @@ export class Driver implements Debuggable, UpdateSource { */ private onFetch(event: FetchEvent): void { const req = event.request; + const parsedUrl = this.adapter.parseUrl(req.url, this.scope.registration.scope); // The only thing that is served unconditionally is the debug page. - 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. + if (parsedUrl.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(req)); return; } @@ -183,6 +183,16 @@ export class Driver implements Debuggable, UpdateSource { return; } + // Although "passive mixed content" (like images) only produces a warning without a + // ServiceWorker, fetching it via a ServiceWorker results in an error. Let such requests be + // handled by the browser, since handling with the ServiceWorker would fail anyway. + // See https://github.com/angular/angular/issues/23012#issuecomment-376430187 for more details. + if (parsedUrl.origin.startsWith('http:') && this.scope.registration.scope.startsWith('https:')) { + // Still, log the incident for debugging purposes. + this.debugger.log(`Ignoring passive mixed content request: Driver.fetch(${req.url})`); + 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 @@ -190,7 +200,7 @@ export class Driver implements Debuggable, UpdateSource { // 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') { + if (req.cache === '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; diff --git a/packages/service-worker/worker/test/happy_spec.ts b/packages/service-worker/worker/test/happy_spec.ts index 4ca3969b4a965..1092d5793ce05 100644 --- a/packages/service-worker/worker/test/happy_spec.ts +++ b/packages/service-worker/worker/test/happy_spec.ts @@ -185,8 +185,6 @@ const brokenServer = const server404 = new MockServerStateBuilder().withStaticFiles(dist).build(); -const scope = new SwTestHarnessBuilder().withServerState(server).build(); - const manifestHash = sha1(JSON.stringify(manifest)); const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate)); @@ -859,6 +857,38 @@ const manifestUpdateHash = sha1(JSON.stringify(manifestUpdate)); expect(await requestFoo('only-if-cached', 'same-origin')).toBe('this is foo'); expect(await requestFoo('only-if-cached', 'no-cors')).toBeNull(); }); + + async_it('ignores passive mixed content requests ', async() => { + const scopeFetchSpy = spyOn(scope, 'fetch').and.callThrough(); + const getRequestUrls = () => scopeFetchSpy.calls.allArgs().map(args => args[0].url); + + const httpScopeUrl = 'http://mock.origin.dev'; + const httpsScopeUrl = 'https://mock.origin.dev'; + const httpRequestUrl = 'http://other.origin.sh/unknown.png'; + const httpsRequestUrl = 'https://other.origin.sh/unknown.pnp'; + + // Registration scope: `http:` + (scope.registration.scope as string) = httpScopeUrl; + + await makeRequest(scope, httpRequestUrl); + await makeRequest(scope, httpsRequestUrl); + const requestUrls1 = getRequestUrls(); + + expect(requestUrls1).toContain(httpRequestUrl); + expect(requestUrls1).toContain(httpsRequestUrl); + + scopeFetchSpy.calls.reset(); + + // Registration scope: `https:` + (scope.registration.scope as string) = httpsScopeUrl; + + await makeRequest(scope, httpRequestUrl); + await makeRequest(scope, httpsRequestUrl); + const requestUrls2 = getRequestUrls(); + + expect(requestUrls2).not.toContain(httpRequestUrl); + expect(requestUrls2).toContain(httpsRequestUrl); + }); }); }); })(); diff --git a/packages/service-worker/worker/testing/scope.ts b/packages/service-worker/worker/testing/scope.ts index 3c648e382ad47..fb78468e31693 100644 --- a/packages/service-worker/worker/testing/scope.ts +++ b/packages/service-worker/worker/testing/scope.ts @@ -180,7 +180,7 @@ export class SwTestHarness implements ServiceWorkerGlobalScope, Adapter, Context return {origin: obj.origin, path: obj.pathname}; } else { const obj = require('url').parse(url); - return {origin: obj.origin, path: obj.pathname}; + return {origin: `${obj.protocol}//${obj.host}`, path: obj.pathname}; } }