From d0336ea5c2817da8df86a279c31c39fcf9ecd465 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 18 Jun 2020 17:15:47 -0700 Subject: [PATCH] fix(network): disallow intercepting redirects (#2617) WebKit and Firefox are only able to continue redirects. Firefox is faking it on the backend, so you can't even stall it. Instead, we just do not fire routes for redirects on all browsers, to avoid surprises. --- browsers.json | 2 +- src/chromium/crNetworkManager.ts | 9 +++++++- src/network.ts | 1 + src/webkit/wkInterceptableRequest.ts | 2 ++ src/webkit/wkPage.ts | 13 +++++++++-- test/interception.spec.js | 32 +++++++++++++++++----------- 6 files changed, 42 insertions(+), 17 deletions(-) diff --git a/browsers.json b/browsers.json index f642d15984..a8d9459624 100644 --- a/browsers.json +++ b/browsers.json @@ -6,7 +6,7 @@ }, { "name": "firefox", - "revision": "1112" + "revision": "1113" }, { "name": "webkit", diff --git a/src/chromium/crNetworkManager.ts b/src/chromium/crNetworkManager.ts index a815c10618..00b4c5634f 100644 --- a/src/chromium/crNetworkManager.ts +++ b/src/chromium/crNetworkManager.ts @@ -191,6 +191,13 @@ export class CRNetworkManager { this._client._sendMayFail('Fetch.continueRequest', { requestId: requestPausedEvent.requestId }); return; } + let allowInterception = this._userRequestInterceptionEnabled; + if (redirectedFrom) { + allowInterception = false; + // We do not support intercepting redirects. + if (requestPausedEvent) + this._client._sendMayFail('Fetch.continueRequest', { requestId: requestPausedEvent.requestId }); + } const isNavigationRequest = requestWillBeSentEvent.requestId === requestWillBeSentEvent.loaderId && requestWillBeSentEvent.type === 'Document'; const documentId = isNavigationRequest ? requestWillBeSentEvent.loaderId : undefined; if (isNavigationRequest) @@ -199,7 +206,7 @@ export class CRNetworkManager { client: this._client, frame, documentId, - allowInterception: this._userRequestInterceptionEnabled, + allowInterception, requestWillBeSentEvent, requestPausedEvent, redirectedFrom diff --git a/src/network.ts b/src/network.ts index ec92e0a9f4..26986c5a25 100644 --- a/src/network.ts +++ b/src/network.ts @@ -115,6 +115,7 @@ export class Request { constructor(routeDelegate: RouteDelegate | null, frame: frames.Frame, redirectedFrom: Request | null, documentId: string | undefined, url: string, resourceType: string, method: string, postData: string | null, headers: Headers) { assert(!url.startsWith('data:'), 'Data urls should not fire requests'); + assert(!(routeDelegate && redirectedFrom), 'Should not be able to intercept redirects'); this._routeDelegate = routeDelegate; this._frame = frame; this._redirectedFrom = redirectedFrom; diff --git a/src/webkit/wkInterceptableRequest.ts b/src/webkit/wkInterceptableRequest.ts index 37a727d6b4..ff8a7f58bf 100644 --- a/src/webkit/wkInterceptableRequest.ts +++ b/src/webkit/wkInterceptableRequest.ts @@ -44,10 +44,12 @@ export class WKInterceptableRequest implements network.RouteDelegate { readonly _requestId: string; _interceptedCallback: () => void = () => {}; private _interceptedPromise: Promise; + readonly _allowInterception: boolean; constructor(session: WKSession, allowInterception: boolean, frame: frames.Frame, event: Protocol.Network.requestWillBeSentPayload, redirectedFrom: network.Request | null, documentId: string | undefined) { this._session = session; this._requestId = event.requestId; + this._allowInterception = allowInterception; const resourceType = event.type ? event.type.toLowerCase() : (redirectedFrom ? redirectedFrom.resourceType() : 'other'); this.request = new network.Request(allowInterception ? this : null, frame, redirectedFrom, documentId, event.request.url, resourceType, event.request.method, event.request.postData || null, headersObject(event.request.headers)); diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index 16048e0409..e5331107cd 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -841,7 +841,9 @@ export class WKPage implements PageDelegate { const documentId = isNavigationRequest ? event.loaderId : undefined; if (isNavigationRequest) this._page._frameManager.frameUpdatedDocumentIdForNavigation(event.frameId, documentId!); - const request = new WKInterceptableRequest(session, this._page._needsRequestInterception(), frame, event, redirectedFrom, documentId); + // We do not support intercepting redirects. + const allowInterception = this._page._needsRequestInterception() && !redirectedFrom; + const request = new WKInterceptableRequest(session, allowInterception, frame, event, redirectedFrom, documentId); this._requestIdToRequest.set(event.requestId, request); this._page._frameManager.requestStarted(request.request); } @@ -856,8 +858,15 @@ export class WKPage implements PageDelegate { _onRequestIntercepted(event: Protocol.Network.requestInterceptedPayload) { const request = this._requestIdToRequest.get(event.requestId); - if (request) + if (!request) + return; + if (!request._allowInterception) { + // Intercepted, although we do not intend to allow interception. + // Just continue. + this._session.sendMayFail('Network.interceptWithRequest', { requestId: request._requestId }); + } else { request._interceptedCallback(); + } } _onResponseReceived(event: Protocol.Network.responseReceivedPayload) { diff --git a/test/interception.spec.js b/test/interception.spec.js index 65088a8c30..1bb2db49a2 100644 --- a/test/interception.spec.js +++ b/test/interception.spec.js @@ -216,21 +216,26 @@ describe('Page.route', function() { else expect(error.message).toContain('net::ERR_FAILED'); }); - it('should work with redirects', async({page, server}) => { - const requests = []; + it('should not work with redirects', async({page, server}) => { + const intercepted = []; await page.route('**/*', route => { route.continue(); - requests.push(route.request()); + intercepted.push(route.request()); }); server.setRedirect('/non-existing-page.html', '/non-existing-page-2.html'); server.setRedirect('/non-existing-page-2.html', '/non-existing-page-3.html'); server.setRedirect('/non-existing-page-3.html', '/non-existing-page-4.html'); server.setRedirect('/non-existing-page-4.html', '/empty.html'); + const response = await page.goto(server.PREFIX + '/non-existing-page.html'); expect(response.status()).toBe(200); expect(response.url()).toContain('empty.html'); - expect(requests.length).toBe(5); - expect(requests[2].resourceType()).toBe('document'); + + expect(intercepted.length).toBe(1); + expect(intercepted[0].resourceType()).toBe('document'); + expect(intercepted[0].isNavigationRequest()).toBe(true); + expect(intercepted[0].url()).toContain('/non-existing-page.html'); + const chain = []; for (let r = response.request(); r; r = r.redirectedFrom()) { chain.push(r); @@ -246,10 +251,10 @@ describe('Page.route', function() { expect(chain[i].redirectedTo()).toBe(i ? chain[i - 1] : null); }); it('should work with redirects for subresources', async({page, server}) => { - const requests = []; + const intercepted = []; await page.route('**/*', route => { route.continue(); - requests.push(route.request()); + intercepted.push(route.request()); }); server.setRedirect('/one-style.css', '/two-style.css'); server.setRedirect('/two-style.css', '/three-style.css'); @@ -259,14 +264,16 @@ describe('Page.route', function() { const response = await page.goto(server.PREFIX + '/one-style.html'); expect(response.status()).toBe(200); expect(response.url()).toContain('one-style.html'); - expect(requests.length).toBe(5); - expect(requests[0].resourceType()).toBe('document'); - let r = requests.find(r => r.url().includes('/four-style.css')); - for (const url of ['/four-style.css', '/three-style.css', '/two-style.css', '/one-style.css']) { + expect(intercepted.length).toBe(2); + expect(intercepted[0].resourceType()).toBe('document'); + expect(intercepted[0].url()).toContain('one-style.html'); + + let r = intercepted[1]; + for (const url of ['/one-style.css', '/two-style.css', '/three-style.css', '/four-style.css']) { expect(r.resourceType()).toBe('stylesheet'); expect(r.url()).toContain(url); - r = r.redirectedFrom(); + r = r.redirectedTo(); } expect(r).toBe(null); }); @@ -602,7 +609,6 @@ describe('Interception vs isNavigationRequest', () => { server.setRedirect('/rrredirect', '/frames/one-frame.html'); await page.goto(server.PREFIX + '/rrredirect'); expect(requests.get('rrredirect').isNavigationRequest()).toBe(true); - expect(requests.get('one-frame.html').isNavigationRequest()).toBe(true); expect(requests.get('frame.html').isNavigationRequest()).toBe(true); expect(requests.get('script.js').isNavigationRequest()).toBe(false); expect(requests.get('style.css').isNavigationRequest()).toBe(false);