Merge pull request #1992 from automatisch/fix-proxy-configuration

fix(axios): update order of interceptors

packages/backend/src/helpers/axios-with-proxy.js

@@ -3,70 +3,99 @@ import { HttpsProxyAgent } from 'https-proxy-agent';
 import { HttpProxyAgent } from 'http-proxy-agent';
 import appConfig from '../config/app.js';
 
-const config = axios.defaults;
-const httpProxyUrl = appConfig.httpProxy;
-const httpsProxyUrl = appConfig.httpsProxy;
-const supportsProxy = httpProxyUrl || httpsProxyUrl;
-const noProxyEnv = appConfig.noProxy;
-const noProxyHosts = noProxyEnv ? noProxyEnv.split(',').map(host => host.trim()) : [];
+export function createInstance(customConfig = {}, { requestInterceptor, responseErrorInterceptor } = {}) {
+  const config = {
+    ...axios.defaults,
+    ...customConfig
+  };
+  const httpProxyUrl = appConfig.httpProxy;
+  const httpsProxyUrl = appConfig.httpsProxy;
+  const supportsProxy = httpProxyUrl || httpsProxyUrl;
+  const noProxyEnv = appConfig.noProxy;
+  const noProxyHosts = noProxyEnv ? noProxyEnv.split(',').map(host => host.trim()) : [];
 
-if (supportsProxy) {
-  if (httpProxyUrl) {
-    config.httpAgent = new HttpProxyAgent(httpProxyUrl);
+  if (supportsProxy) {
+    if (httpProxyUrl) {
+      config.httpAgent = new HttpProxyAgent(httpProxyUrl);
+    }
+
+    if (httpsProxyUrl) {
+      config.httpsAgent = new HttpsProxyAgent(httpsProxyUrl);
+    }
+
+    config.proxy = false;
   }
 
-  if (httpsProxyUrl) {
-    config.httpsAgent = new HttpsProxyAgent(httpsProxyUrl);
+  const instance = axios.create(config);
+
+  function shouldSkipProxy(hostname) {
+    return noProxyHosts.some(noProxyHost => {
+      return hostname.endsWith(noProxyHost) || hostname === noProxyHost;
+    });
+  };
+
+  /**
+   * The interceptors are executed in the reverse order they are added.
+   */
+  instance.interceptors.request.use(
+    function skipProxyIfInNoProxy(requestConfig) {
+      const hostname = new URL(requestConfig.baseURL).hostname;
+
+      if (supportsProxy && shouldSkipProxy(hostname)) {
+        requestConfig.httpAgent = undefined;
+        requestConfig.httpsAgent = undefined;
+      }
+
+      return requestConfig;
+    },
+    (error) => Promise.reject(error)
+  );
+
+  // not always we have custom request interceptors
+  if (requestInterceptor) {
+    instance.interceptors.request.use(
+      function customInterceptor(requestConfig) {
+        const result = requestInterceptor.reduce((newConfig, requestInterceptor) => {
+          return requestInterceptor(newConfig);
+        }, requestConfig);
+
+        return result;
+      }
+    );
   }
 
-  config.proxy = false;
+  instance.interceptors.request.use(
+    function removeBaseUrlForAbsoluteUrls(requestConfig) {
+      /**
+       * If the URL is an absolute URL, we remove its origin out of the URL
+       * and set it as baseURL. This lets us streamlines the requests made by Automatisch
+       * and requests made by app integrations.
+       */
+      try {
+        const url = new URL(requestConfig.url);
+        requestConfig.baseURL = url.origin;
+        requestConfig.url = url.pathname + url.search;
+
+        return requestConfig;
+      } catch (err) {
+        return requestConfig;
+      }
+    },
+    (error) => Promise.reject(error)
+  );
+
+
+  // not always we have custom response error interceptor
+  if (responseErrorInterceptor) {
+    instance.interceptors.response.use(
+      (response) => response,
+      responseErrorInterceptor
+    );
+  }
+
+  return instance;
 }
 
-const axiosWithProxyInstance = axios.create(config);
+const defaultInstance = createInstance();
 
-function shouldSkipProxy(hostname) {
-  return noProxyHosts.some(noProxyHost => {
-    return hostname.endsWith(noProxyHost) || hostname === noProxyHost;
-  });
-};
-
-/**
- * The interceptors are executed in the reverse order they are added.
- */
-axiosWithProxyInstance.interceptors.request.use(
-  function skipProxyIfInNoProxy(requestConfig) {
-    const hostname = new URL(requestConfig.baseURL).hostname;
-
-    if (supportsProxy && shouldSkipProxy(hostname)) {
-      requestConfig.httpAgent = undefined;
-      requestConfig.httpsAgent = undefined;
-    }
-
-    return requestConfig;
-  },
-  undefined,
-  { synchronous: true }
-);
-
-axiosWithProxyInstance.interceptors.request.use(
-  function removeBaseUrlForAbsoluteUrls(requestConfig) {
-    /**
-     * If the URL is an absolute URL, we remove its origin out of the URL
-     * and set it as baseURL. This lets us streamlines the requests made by Automatisch
-     * and requests made by app integrations.
-     */
-    try {
-      const url = new URL(requestConfig.url);
-      requestConfig.baseURL = url.origin;
-      requestConfig.url = url.pathname + url.search;
-
-      return requestConfig;
-    } catch {
-      return requestConfig;
-    }
-  },
-  undefined,
-  { synchronous: true}
-);
-
-export default axiosWithProxyInstance;
+export default defaultInstance;

packages/backend/src/helpers/axios-with-proxy.test.js

@@ -1,6 +1,6 @@
 import { beforeEach, describe, it, expect, vi } from 'vitest';
 
-describe('Axios with proxy', () => {
+describe('Custom default axios with proxy', () => {
   beforeEach(() => {
     vi.resetModules();
   });
@@ -23,7 +23,13 @@ describe('Axios with proxy', () => {
     expect(secondRequestInterceptor.fulfilled.name).toBe('removeBaseUrlForAbsoluteUrls');
   });
 
-  describe('skipProxyIfInNoProxy', () => {
+  it('should throw with invalid url (consisting of path alone)', async () => {
+    const axios = (await import('./axios-with-proxy.js')).default;
+
+    await expect(() => axios('/just-a-path')).rejects.toThrowError('Invalid URL');
+  });
+
+  describe('with skipProxyIfInNoProxy interceptor', () => {
     let appConfig, axios;
     beforeEach(async() => {
       appConfig = (await import('../config/app.js')).default;
@@ -67,7 +73,7 @@ describe('Axios with proxy', () => {
     });
   });
 
-  describe('removeBaseUrlForAbsoluteUrls', () => {
+  describe('with removeBaseUrlForAbsoluteUrls interceptor', () => {
     let axios;
     beforeEach(async() => {
       axios = (await import('./axios-with-proxy.js')).default;
@@ -116,4 +122,48 @@ describe('Axios with proxy', () => {
       expect(interceptedRequestConfig.url).toBe('/path?query=1');
     });
   });
+
+  describe('with extra requestInterceptors', () => {
+    it('should apply extra request interceptors in the middle', async () => {
+      const { createInstance } = await import('./axios-with-proxy.js');
+
+      const interceptor = (config) => {
+        config.test = true;
+        return config;
+      }
+
+      const instance = createInstance({}, {
+        requestInterceptor: [
+          interceptor
+        ]
+      });
+      const requestInterceptors = instance.interceptors.request.handlers;
+      const customInterceptor = requestInterceptors[1].fulfilled;
+
+      expect(requestInterceptors.length).toBe(3);
+      expect(customInterceptor({})).toStrictEqual({ test: true });
+    });
+
+    it('should work with a custom interceptor setting a baseURL and a request to path', async () => {
+      const { createInstance } = await import('./axios-with-proxy.js');
+
+      const interceptor = (config) => {
+        config.baseURL = 'http://localhost';
+        return config;
+      }
+
+      const instance = createInstance({}, {
+        requestInterceptor: [
+          interceptor
+        ]
+      });
+
+      try {
+        await instance.get('/just-a-path');
+      } catch (error) {
+        expect(error.config.baseURL).toBe('http://localhost');
+        expect(error.config.url).toBe('/just-a-path');
+      }
+    })
+  });
 });

packages/backend/src/helpers/http-client/index.js

@@ -1,65 +1,43 @@
 import HttpError from '../../errors/http.js';
-import axios from '../axios-with-proxy.js';
-
-// Mutates the `toInstance` by copying the request interceptors from `fromInstance`
-const copyRequestInterceptors = (fromInstance, toInstance) => {
-  // Copy request interceptors
-  fromInstance.interceptors.request.forEach(interceptor => {
-    toInstance.interceptors.request.use(
-      interceptor.fulfilled,
-      interceptor.rejected,
-      {
-        synchronous: interceptor.synchronous,
-        runWhen: interceptor.runWhen
-      }
-    );
-  });
-}
+import { createInstance } from '../axios-with-proxy.js';
 
 export default function createHttpClient({ $, baseURL, beforeRequest = [] }) {
-  const instance = axios.create({
-    baseURL,
-  });
+  async function interceptResponseError(error) {
+    const { config, response } = error;
+    // Do not destructure `status` from `error.response` because it might not exist
+    const status = response?.status;
 
-  // 1. apply the beforeRequest functions from the app
-  instance.interceptors.request.use((requestConfig) => {
-    const result = beforeRequest.reduce((newConfig, beforeRequestFunc) => {
-      return beforeRequestFunc($, newConfig);
-    }, requestConfig);
+    if (
+      // TODO: provide a `shouldRefreshToken` function in the app
+      (status === 401 || status === 403) &&
+      $.app.auth &&
+      $.app.auth.refreshToken &&
+      !$.app.auth.isRefreshTokenRequested
+    ) {
+      $.app.auth.isRefreshTokenRequested = true;
+      await $.app.auth.refreshToken($);
 
-    return result;
-  });
+      // retry the previous request before the expired token error
+      const newResponse = await instance.request(config);
+      $.app.auth.isRefreshTokenRequested = false;
 
-  // 2. inherit the request inceptors from the parent instance
-  copyRequestInterceptors(axios, instance);
-
-  instance.interceptors.response.use(
-    (response) => response,
-    async (error) => {
-      const { config, response } = error;
-      // Do not destructure `status` from `error.response` because it might not exist
-      const status = response?.status;
-
-      if (
-        // TODO: provide a `shouldRefreshToken` function in the app
-        (status === 401 || status === 403) &&
-        $.app.auth &&
-        $.app.auth.refreshToken &&
-        !$.app.auth.isRefreshTokenRequested
-      ) {
-        $.app.auth.isRefreshTokenRequested = true;
-        await $.app.auth.refreshToken($);
-
-        // retry the previous request before the expired token error
-        const newResponse = await instance.request(config);
-        $.app.auth.isRefreshTokenRequested = false;
-
-        return newResponse;
-      }
-
-      throw new HttpError(error);
+      return newResponse;
     }
-  );
+
+    throw new HttpError(error);
+  };
+
+  const instance = createInstance(
+    {
+      baseURL,
+    },
+    {
+      requestInterceptor: beforeRequest.map((originalBeforeRequest) => {
+        return (requestConfig) => originalBeforeRequest($, requestConfig);
+      }),
+      responseErrorInterceptor: interceptResponseError,
+    }
+  )
 
   return instance;
 }