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 <ronan@jouchet.fr>

Co-authored-by: Ronan Jouchet <ronan@jouchet.fr>

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;
-  }
-}

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<string | jest.DoneCallback>(['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<string | jest.DoneCallback>(['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<string | jest.DoneCallback>([
+    '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<string | jest.DoneCallback>(['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();
+        },
+      );
+    },
+  );
 });

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<Record<string, string[]>> {
+  // 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,