From 9a6c6f870df75ca2a580128d8c429cf5071d6bad Mon Sep 17 00:00:00 2001 From: Adam Weeden Date: Wed, 9 Jun 2021 09:49:35 -0400 Subject: [PATCH] Selective css injection (#1222) * Don't inject CSS for unneeded responses * Remove some non-injectable methods * Actually check for CSS to inject + unit tests * Update app/src/helpers/windowHelpers.ts Co-authored-by: Ronan Jouchet Co-authored-by: Ronan Jouchet --- app/src/helpers/helpers.ts | 8 -- app/src/helpers/windowHelpers.test.ts | 118 ++++++++++++++++++++++++-- app/src/helpers/windowHelpers.ts | 76 ++++++++++------- 3 files changed, 155 insertions(+), 47 deletions(-) diff --git a/app/src/helpers/helpers.ts b/app/src/helpers/helpers.ts index c9a6233..9690109 100644 --- a/app/src/helpers/helpers.ts +++ b/app/src/helpers/helpers.ts @@ -179,11 +179,3 @@ export function removeUserAgentSpecifics( .replace(`Electron/${process.versions.electron} `, '') .replace(`${appName}/${appVersion} `, ' '); } - -export function shouldInjectCSS(): boolean { - try { - return fs.existsSync(INJECT_DIR); - } catch (e) { - return false; - } -} diff --git a/app/src/helpers/windowHelpers.test.ts b/app/src/helpers/windowHelpers.test.ts index f6f6676..ee12665 100644 --- a/app/src/helpers/windowHelpers.test.ts +++ b/app/src/helpers/windowHelpers.test.ts @@ -8,7 +8,7 @@ jest.mock('loglevel'); import { error } from 'loglevel'; jest.mock('./helpers'); -import { getCSSToInject, shouldInjectCSS } from './helpers'; +import { getCSSToInject } from './helpers'; jest.mock('./windowEvents'); import { clearAppData, createNewTab, injectCSS } from './windowHelpers'; @@ -99,7 +99,6 @@ describe('createNewTab', () => { describe('injectCSS', () => { const mockGetCSSToInject: jest.SpyInstance = getCSSToInject as jest.Mock; const mockLogError: jest.SpyInstance = error as jest.Mock; - const mockShouldInjectCSS: jest.SpyInstance = shouldInjectCSS as jest.Mock; const mockWebContentsInsertCSS: jest.SpyInstance = jest.spyOn( WebContents.prototype, 'insertCSS', @@ -111,25 +110,21 @@ describe('injectCSS', () => { beforeEach(() => { mockGetCSSToInject.mockReset().mockReturnValue(''); mockLogError.mockReset(); - mockShouldInjectCSS.mockReset().mockReturnValue(true); mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined); }); afterAll(() => { mockGetCSSToInject.mockRestore(); mockLogError.mockRestore(); - mockShouldInjectCSS.mockRestore(); mockWebContentsInsertCSS.mockRestore(); }); - test('will not inject if shouldInjectCSS is false', () => { - mockShouldInjectCSS.mockReturnValue(false); - + test('will not inject if getCSSToInject is empty', () => { const window = new BrowserWindow(); injectCSS(window); - expect(mockGetCSSToInject).not.toHaveBeenCalled(); + expect(mockGetCSSToInject).toHaveBeenCalled(); expect(mockWebContentsInsertCSS).not.toHaveBeenCalled(); }); @@ -176,7 +171,7 @@ describe('injectCSS', () => { (result: HeadersReceivedResponse) => { expect(mockWebContentsInsertCSS).toHaveBeenCalledWith(css); expect(mockLogError).toHaveBeenCalledWith( - 'webContents.insertCSS ERROR', + 'injectCSSIntoResponse ERROR', 'css insertion error', ); expect(result.cancel).toBe(false); @@ -185,4 +180,109 @@ describe('injectCSS', () => { }, ); }); + + test.each(['DELETE', 'OPTIONS'])( + 'will not inject for method %s', + (method: string, done: jest.DoneCallback) => { + mockGetCSSToInject.mockReturnValue(css); + const window = new BrowserWindow(); + + injectCSS(window); + + expect(mockGetCSSToInject).toHaveBeenCalled(); + + window.webContents.emit('did-navigate'); + // @ts-ignore this function doesn't exist in the actual electron version, but will in our mock + window.webContents.session.webRequest.send( + 'onHeadersReceived', + { responseHeaders, webContents: window.webContents, method }, + (result: HeadersReceivedResponse) => { + expect(mockWebContentsInsertCSS).not.toHaveBeenCalled(); + expect(result.cancel).toBe(false); + expect(result.responseHeaders).toBe(responseHeaders); + done(); + }, + ); + }, + ); + + test.each(['GET', 'PATCH', 'POST', 'PUT'])( + 'will inject for method %s', + (method: string, done: jest.DoneCallback) => { + mockGetCSSToInject.mockReturnValue(css); + const window = new BrowserWindow(); + + injectCSS(window); + + expect(mockGetCSSToInject).toHaveBeenCalled(); + + window.webContents.emit('did-navigate'); + // @ts-ignore this function doesn't exist in the actual electron version, but will in our mock + window.webContents.session.webRequest.send( + 'onHeadersReceived', + { responseHeaders, webContents: window.webContents, method }, + (result: HeadersReceivedResponse) => { + expect(mockWebContentsInsertCSS).toHaveBeenCalled(); + expect(result.cancel).toBe(false); + expect(result.responseHeaders).toBe(responseHeaders); + done(); + }, + ); + }, + ); + + test.each([ + 'image', + 'script', + 'stylesheet', + 'xhr', + ])( + 'will not inject for resource type %s', + (resourceType: string, done: jest.DoneCallback) => { + mockGetCSSToInject.mockReturnValue(css); + const window = new BrowserWindow(); + + injectCSS(window); + + expect(mockGetCSSToInject).toHaveBeenCalled(); + + window.webContents.emit('did-navigate'); + // @ts-ignore this function doesn't exist in the actual electron version, but will in our mock + window.webContents.session.webRequest.send( + 'onHeadersReceived', + { responseHeaders, webContents: window.webContents, resourceType }, + (result: HeadersReceivedResponse) => { + expect(mockWebContentsInsertCSS).not.toHaveBeenCalled(); + expect(result.cancel).toBe(false); + expect(result.responseHeaders).toBe(responseHeaders); + done(); + }, + ); + }, + ); + + test.each(['html', 'other'])( + 'will inject for resource type %s', + (resourceType: string, done: jest.DoneCallback) => { + mockGetCSSToInject.mockReturnValue(css); + const window = new BrowserWindow(); + + injectCSS(window); + + expect(mockGetCSSToInject).toHaveBeenCalled(); + + window.webContents.emit('did-navigate'); + // @ts-ignore this function doesn't exist in the actual electron version, but will in our mock + window.webContents.session.webRequest.send( + 'onHeadersReceived', + { responseHeaders, webContents: window.webContents, resourceType }, + (result: HeadersReceivedResponse) => { + expect(mockWebContentsInsertCSS).toHaveBeenCalled(); + expect(result.cancel).toBe(false); + expect(result.responseHeaders).toBe(responseHeaders); + done(); + }, + ); + }, + ); }); diff --git a/app/src/helpers/windowHelpers.ts b/app/src/helpers/windowHelpers.ts index 58766b4..cc42e24 100644 --- a/app/src/helpers/windowHelpers.ts +++ b/app/src/helpers/windowHelpers.ts @@ -10,12 +10,7 @@ import { import log from 'loglevel'; import path from 'path'; -import { - getCSSToInject, - isOSX, - nativeTabsSupported, - shouldInjectCSS, -} from './helpers'; +import { getCSSToInject, isOSX, nativeTabsSupported } from './helpers'; const ZOOM_INTERVAL = 0.1; @@ -198,12 +193,12 @@ export function hideWindow( } export function injectCSS(browserWindow: BrowserWindow): void { - if (!shouldInjectCSS()) { + const cssToInject = getCSSToInject(); + + if (!cssToInject) { return; } - const cssToInject = getCSSToInject(); - browserWindow.webContents.on('did-navigate', () => { log.debug( 'browserWindow.webContents.did-navigate', @@ -218,33 +213,54 @@ export function injectCSS(browserWindow: BrowserWindow): void { details: OnHeadersReceivedListenerDetails, callback: (headersReceivedResponse: HeadersReceivedResponse) => void, ) => { - log.debug( - 'browserWindow.webContents.session.webRequest.onHeadersReceived', - { details, callback }, - ); - if (details.webContents) { - details.webContents - .insertCSS(cssToInject) - .catch((err) => { - log.error('webContents.insertCSS ERROR', err); - }) - .finally(() => - callback({ - cancel: false, - responseHeaders: details.responseHeaders, - }), - ); - } else { - callback({ - cancel: false, - responseHeaders: details.responseHeaders, + injectCSSIntoResponse(details, cssToInject) + .then((responseHeaders) => { + callback({ + cancel: false, + responseHeaders, + }); + }) + .catch((err) => { + log.error('injectCSSIntoResponse ERROR', err); + callback({ + cancel: false, + responseHeaders: details.responseHeaders, + }); }); - } }, ); }); } +async function injectCSSIntoResponse( + details: OnHeadersReceivedListenerDetails, + cssToInject: string, +): Promise> { + // We go with a denylist rather than a whitelist (e.g. only GET/html) + // to avoid "whoops I didn't think this should have been CSS-injected" cases + const nonInjectableMethods = ['DELETE', 'OPTIONS']; + const nonInjectableResourceTypes = ['image', 'script', 'stylesheet', 'xhr']; + + if ( + nonInjectableMethods.includes(details.method) || + nonInjectableResourceTypes.includes(details.resourceType) || + !details.webContents + ) { + log.debug( + `Skipping CSS injection for:\n${details.url}\nwith method ${details.method} and resourceType ${details.resourceType} and content-type ${details.responseHeaders['content-type']}`, + ); + return details.responseHeaders; + } + + log.debug('browserWindow.webContents.session.webRequest.onHeadersReceived', { + details, + contentType: details.responseHeaders['content-type'], + }); + await details.webContents.insertCSS(cssToInject); + + return details.responseHeaders; +} + export function sendParamsOnDidFinishLoad( options, window: BrowserWindow,