From 6bafdf5257ad6a95641fc7befedd510e311eee76 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Tue, 23 Jul 2024 15:01:12 +0000 Subject: [PATCH] refactor(http-client): inherit interceptors from parent instance --- packages/backend/src/config/app.js | 3 + .../backend/src/helpers/axios-with-proxy.js | 51 ++++++-- .../src/helpers/axios-with-proxy.test.js | 119 ++++++++++++++++++ .../backend/src/helpers/http-client/index.js | 41 +++--- 4 files changed, 181 insertions(+), 33 deletions(-) create mode 100644 packages/backend/src/helpers/axios-with-proxy.test.js diff --git a/packages/backend/src/config/app.js b/packages/backend/src/config/app.js index 72892bb2..fa291616 100644 --- a/packages/backend/src/config/app.js +++ b/packages/backend/src/config/app.js @@ -100,6 +100,9 @@ const appConfig = { additionalDrawerLinkIcon: process.env.ADDITIONAL_DRAWER_LINK_ICON, additionalDrawerLinkText: process.env.ADDITIONAL_DRAWER_LINK_TEXT, disableSeedUser: process.env.DISABLE_SEED_USER === 'true', + httpProxy: process.env.http_proxy, + httpsProxy: process.env.https_proxy, + noProxy: process.env.no_proxy, }; if (!appConfig.encryptionKey) { diff --git a/packages/backend/src/helpers/axios-with-proxy.js b/packages/backend/src/helpers/axios-with-proxy.js index 0dbad80a..2066354f 100644 --- a/packages/backend/src/helpers/axios-with-proxy.js +++ b/packages/backend/src/helpers/axios-with-proxy.js @@ -1,12 +1,13 @@ import axios from 'axios'; import { HttpsProxyAgent } from 'https-proxy-agent'; import { HttpProxyAgent } from 'http-proxy-agent'; +import appConfig from '../config/app.js'; const config = axios.defaults; -const httpProxyUrl = process.env.http_proxy; -const httpsProxyUrl = process.env.https_proxy; +const httpProxyUrl = appConfig.httpProxy; +const httpsProxyUrl = appConfig.httpsProxy; const supportsProxy = httpProxyUrl || httpsProxyUrl; -const noProxyEnv = process.env.no_proxy; +const noProxyEnv = appConfig.noProxy; const noProxyHosts = noProxyEnv ? noProxyEnv.split(',').map(host => host.trim()) : []; if (supportsProxy) { @@ -29,15 +30,43 @@ function shouldSkipProxy(hostname) { }); }; -axiosWithProxyInstance.interceptors.request.use(function skipProxyIfInNoProxy(requestConfig) { - const hostname = new URL(requestConfig.url).hostname; +/** + * 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; - } + if (supportsProxy && shouldSkipProxy(hostname)) { + requestConfig.httpAgent = undefined; + requestConfig.httpsAgent = undefined; + } - return requestConfig; -}); + 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; diff --git a/packages/backend/src/helpers/axios-with-proxy.test.js b/packages/backend/src/helpers/axios-with-proxy.test.js new file mode 100644 index 00000000..db2c7108 --- /dev/null +++ b/packages/backend/src/helpers/axios-with-proxy.test.js @@ -0,0 +1,119 @@ +import { beforeEach, describe, it, expect, vi } from 'vitest'; + +describe('Axios with proxy', () => { + beforeEach(() => { + vi.resetModules(); + }); + + it('should have two interceptors by default', async () => { + const axios = (await import('./axios-with-proxy.js')).default; + const requestInterceptors = axios.interceptors.request.handlers; + + expect(requestInterceptors.length).toBe(2); + }); + + it('should have default interceptors in a certain order', async () => { + const axios = (await import('./axios-with-proxy.js')).default; + + const requestInterceptors = axios.interceptors.request.handlers; + const firstRequestInterceptor = requestInterceptors[0]; + const secondRequestInterceptor = requestInterceptors[1]; + + expect(firstRequestInterceptor.fulfilled.name).toBe('skipProxyIfInNoProxy'); + expect(secondRequestInterceptor.fulfilled.name).toBe('removeBaseUrlForAbsoluteUrls'); + }); + + describe('skipProxyIfInNoProxy', () => { + let appConfig, axios; + beforeEach(async() => { + appConfig = (await import('../config/app.js')).default; + + vi.spyOn(appConfig, 'httpProxy', 'get').mockReturnValue('http://proxy.automatisch.io'); + vi.spyOn(appConfig, 'httpsProxy', 'get').mockReturnValue('http://proxy.automatisch.io'); + vi.spyOn(appConfig, 'noProxy', 'get').mockReturnValue('name.tld,automatisch.io'); + + axios = (await import('./axios-with-proxy.js')).default; + }); + + it('should skip proxy for hosts in no_proxy environment variable', async () => { + const skipProxyIfInNoProxy = axios.interceptors.request.handlers[0].fulfilled; + + const mockRequestConfig = { + ...axios.defaults, + baseURL: 'https://automatisch.io' + }; + + const interceptedRequestConfig = skipProxyIfInNoProxy(mockRequestConfig); + + expect(interceptedRequestConfig.httpAgent).toBeUndefined(); + expect(interceptedRequestConfig.httpsAgent).toBeUndefined(); + expect(interceptedRequestConfig.proxy).toBe(false); + }); + + it('should not skip proxy for hosts not in no_proxy environment variable', async () => { + const skipProxyIfInNoProxy = axios.interceptors.request.handlers[0].fulfilled; + + const mockRequestConfig = { + ...axios.defaults, + // beware the intentional typo! + baseURL: 'https://automatish.io' + }; + + const interceptedRequestConfig = skipProxyIfInNoProxy(mockRequestConfig); + + expect(interceptedRequestConfig.httpAgent).toBeDefined(); + expect(interceptedRequestConfig.httpsAgent).toBeDefined(); + expect(interceptedRequestConfig.proxy).toBe(false); + }); + }); + + describe('removeBaseUrlForAbsoluteUrls', () => { + let axios; + beforeEach(async() => { + axios = (await import('./axios-with-proxy.js')).default; + }); + + it('should trim the baseUrl from absolute urls', async () => { + const removeBaseUrlForAbsoluteUrls = axios.interceptors.request.handlers[1].fulfilled; + + const mockRequestConfig = { + ...axios.defaults, + url: 'https://automatisch.io/path' + }; + + const interceptedRequestConfig = removeBaseUrlForAbsoluteUrls(mockRequestConfig); + + expect(interceptedRequestConfig.baseURL).toBe('https://automatisch.io'); + expect(interceptedRequestConfig.url).toBe('/path'); + }); + + it('should not mutate separate baseURL and urls', async () => { + const removeBaseUrlForAbsoluteUrls = axios.interceptors.request.handlers[1].fulfilled; + + const mockRequestConfig = { + ...axios.defaults, + baseURL: 'https://automatisch.io', + url: '/path?query=1' + }; + + const interceptedRequestConfig = removeBaseUrlForAbsoluteUrls(mockRequestConfig); + + expect(interceptedRequestConfig.baseURL).toBe('https://automatisch.io'); + expect(interceptedRequestConfig.url).toBe('/path?query=1'); + }); + + it('should not strip querystring from url', async () => { + const removeBaseUrlForAbsoluteUrls = axios.interceptors.request.handlers[1].fulfilled; + + const mockRequestConfig = { + ...axios.defaults, + url: 'https://automatisch.io/path?query=1' + }; + + const interceptedRequestConfig = removeBaseUrlForAbsoluteUrls(mockRequestConfig); + + expect(interceptedRequestConfig.baseURL).toBe('https://automatisch.io'); + expect(interceptedRequestConfig.url).toBe('/path?query=1'); + }); + }); +}); diff --git a/packages/backend/src/helpers/http-client/index.js b/packages/backend/src/helpers/http-client/index.js index e32482df..d0d6e772 100644 --- a/packages/backend/src/helpers/http-client/index.js +++ b/packages/backend/src/helpers/http-client/index.js @@ -1,41 +1,38 @@ -import { URL } from 'node:url'; import HttpError from '../../errors/http.js'; import axios from '../axios-with-proxy.js'; -const removeBaseUrlForAbsoluteUrls = (requestConfig) => { - try { - const url = new URL(requestConfig.url); - requestConfig.baseURL = url.origin; - requestConfig.url = url.pathname + url.search; - - return requestConfig; - } catch { - return requestConfig; - } -}; +// 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 + } + ); + }); +} export default function createHttpClient({ $, baseURL, beforeRequest = [] }) { const instance = axios.create({ baseURL, }); + // 1. apply the beforeRequest functions from the app instance.interceptors.request.use((requestConfig) => { - const newRequestConfig = removeBaseUrlForAbsoluteUrls(requestConfig); - const result = beforeRequest.reduce((newConfig, beforeRequestFunc) => { return beforeRequestFunc($, newConfig); - }, newRequestConfig); + }, requestConfig); - /** - * axios seems to want InternalAxiosRequestConfig returned not AxioRequestConfig - * anymore even though requests do require AxiosRequestConfig. - * - * Since both interfaces are very similar (InternalAxiosRequestConfig - * extends AxiosRequestConfig), we can utilize an assertion below - **/ return result; }); + // 2. inherit the request inceptors from the parent instance + copyRequestInterceptors(axios, instance); + instance.interceptors.response.use( (response) => response, async (error) => {