Repository: codecov/uploader
Pull request #1084 (2023-06-19 18:15:55)

fix: use token param for upstream proxy auth
Hello from Lyft!

Unfortunately, [new feature for upstream proxy auth](https://github.com/codecov/uploader/pull/1069) is not work as expected (at least with our proxy), so I changed `ProxyAgent` init according to [undici docs](https://github.com/nodejs/undici/blob/main/docs/api/ProxyAgent.md#example---basic-proxy-request-with-authentication) (additional auth headers were replaced with `token` param).
Seems work fine in our environment.
Note: JS/TS is not my day-to-day language, so more idiomatic options are very welcomed (especially for `getAgentProperty` func with reflection hacks).

SUNx2YCH: PTAL @drazisil-codecov @thomasrockhu-codecov 
SUNx2YCH: @drazisil-codecov yeah, it's ready
I marked it as draft, because wasn't sure about code style and "idiomaticity"

Patch:
diff --git a/src/helpers/proxy.ts b/src/helpers/proxy.ts
index c184bac..356f0b3 100644
--- a/src/helpers/proxy.ts
+++ b/src/helpers/proxy.ts
@@ -1,18 +1,37 @@
 import { ProxyAgent } from 'undici';
 import { UploaderArgs, UploaderEnvs } from '../types.js';
-import { IncomingHttpHeaders } from 'undici/types/header.js';
+import { logError } from './logger'
 
-export function addProxyIfNeeded(envs: UploaderEnvs, args: UploaderArgs): ProxyAgent | undefined {
-  return args.upstream ? new ProxyAgent({ uri: args.upstream }) : undefined;
+export function getBasicAuthToken(username: string, password: string): string {
+  const authString = Buffer.from(`${username}:${password}`).toString('base64')
+  return `Basic ${authString}`
+}
+
+export function removeUrlAuth(url: string | URL): string {
+  const noAuthUrl = new URL(url)
+  noAuthUrl.username = ''
+  noAuthUrl.password = ''
+  return noAuthUrl.href
 }
 
-export function addProxyHeaders(envs: UploaderEnvs, initialHeaders: IncomingHttpHeaders): IncomingHttpHeaders {
-  let headers: IncomingHttpHeaders
-  if (envs['PROXY_BASIC_USER'] && envs['PROXY_BASIC_PASS']) {
-    const authString = Buffer.from(`${envs['PROXY_BASIC_USER']}:${envs['PROXY_BASIC_PASS']}`).toString("base64")
-    headers = { ...initialHeaders, Authorization: `Basic ${authString}` }
-  } else {
-    headers = initialHeaders
+export function addProxyIfNeeded(envs: UploaderEnvs, args: UploaderArgs): ProxyAgent | undefined {
+  if (!args.upstream) {
+    return undefined
   }
-  return headers
+
+  // https://github.com/nodejs/undici/blob/main/docs/api/ProxyAgent.md#example---basic-proxy-request-with-authentication
+  try {
+    const proxyUrl = new URL(args.upstream)
+    if (proxyUrl.username && proxyUrl.password) {
+      return new ProxyAgent({
+        uri: removeUrlAuth(proxyUrl),
+        token: getBasicAuthToken(proxyUrl.username, proxyUrl.password),
+      })
+    }
+    return new ProxyAgent({ uri: args.upstream })
+  } catch (err) {
+    logError(`Couldn't set upstream proxy: ${err}`)
+  }
+
+  return undefined
 }
diff --git a/src/helpers/web.ts b/src/helpers/web.ts
index b1397db..5ae14a5 100644
--- a/src/helpers/web.ts
+++ b/src/helpers/web.ts
@@ -16,8 +16,7 @@ import {
   UploaderInputs,
 } from '../types'
 import { info } from './logger'
-import { addProxyHeaders, addProxyIfNeeded } from './proxy'
-import { IncomingHttpHeaders } from 'undici/types/header.js'
+import { addProxyIfNeeded } from './proxy'
 
 /**
  *
@@ -175,13 +174,11 @@ export function generateRequestHeadersPOST(
     source,
   )}&token=${token}&${query}`, postURL)
 
-  const initialHeaders = {
+  const headers = {
     'X-Upload-Token': token,
     'X-Reduced-Redundancy': 'false',
   }
 
-  const headers: IncomingHttpHeaders = addProxyHeaders(envs, initialHeaders)
-
   return {
     agent: addProxyIfNeeded(envs, args),
     url: url,
@@ -202,12 +199,10 @@ export function generateRequestHeadersPUT(
   envs: UploaderEnvs,
   args: UploaderArgs,
 ): IRequestHeaders {
-  const initialHeaders = {
+  const headers = {
     'Content-Type': 'text/plain',
     'Content-Encoding': 'gzip',
   }
-  
-  const headers = addProxyHeaders(envs, initialHeaders)
 
   return {
     agent: addProxyIfNeeded(envs, args),


Test patch:
diff --git a/test/helpers/proxy.test.ts b/test/helpers/proxy.test.ts
new file mode 100644
index 0000000..5a3a536
--- /dev/null
+++ b/test/helpers/proxy.test.ts
@@ -0,0 +1,64 @@
+import { ProxyAgent } from 'undici'
+import { addProxyIfNeeded, getBasicAuthToken, removeUrlAuth } from '../../src/helpers/proxy'
+import { UploaderArgs } from '../../src/types'
+
+describe('Proxy - getBasicAuthToken', () => {
+    it('should make base64-encoded auth token from username and password', () => {
+        const token = getBasicAuthToken('alice', 'pa$$w0rd')
+        expect(token).toEqual('Basic YWxpY2U6cGEkJHcwcmQ=')
+    })
+})
+
+describe('Proxy - removeUrlAuth', () => {
+    it('should remove auth data from url', () => {
+        const proxyUrl = 'http://alice:pa$$w0rd@proxy.local:1234/'
+        expect(removeUrlAuth(proxyUrl)).toEqual('http://proxy.local:1234/')
+    })
+})
+
+// copied from undici sources, as not all of them are in module exports
+const kProxy = Symbol('proxy agent options')
+const kProxyHeaders = Symbol('proxy headers')
+const proxyAuthHeader = 'proxy-authorization'
+
+// helper to get ProxyAgent property indexed with given Symbol
+function getAgentProperty(agent: ProxyAgent, property: Symbol): any {
+    const originalSymbol = Reflect.ownKeys(agent).filter((item) => item.toString() === property.toString())?.[0]
+    return originalSymbol ? Reflect.get(agent, originalSymbol) : undefined
+}
+
+describe('Proxy - addProxyIfNeeded', () => {
+    it.each([
+        [''],
+        ['invalid'],
+    ])('should not return proxy if upstream argument is not set or invalid (%p)', (proxyUrl: string) => {
+        const args: UploaderArgs = {
+            flags: [],
+            slug: '',
+            upstream: proxyUrl,
+        }
+
+        const proxy = addProxyIfNeeded({}, args)
+        expect(proxy).toBeUndefined()
+    })
+
+    it.each([
+        ['http://proxy.local:1234/', 'http://proxy.local:1234/', undefined],
+        ['http://alice:pa$$w0rd@proxy.local:1234/', 'http://proxy.local:1234/', 'Basic YWxpY2U6cGEkJHcwcmQ='],
+    ])('should return proxy if upstream argument is set (%p)', (proxyUrl: string, expectedUri: string, expectedAuthHeader: any) => {
+        const args: UploaderArgs = {
+            flags: [],
+            slug: '',
+            upstream: proxyUrl,
+        }
+
+        const proxy = addProxyIfNeeded({}, args)
+        expect(proxy).toBeDefined()
+
+        const agentOptions = getAgentProperty(proxy as ProxyAgent, kProxy)
+        expect(agentOptions?.['uri']).toEqual(expectedUri)
+
+        const agentHeaders = getAgentProperty(proxy as ProxyAgent, kProxyHeaders)
+        expect(agentHeaders?.[proxyAuthHeader]).toEqual(expectedAuthHeader)
+    })
+})
diff --git a/test/proxy.test.ts b/test/proxy.test.ts
deleted file mode 100644
index 0edca79..0000000
--- a/test/proxy.test.ts
+++ /dev/null
@@ -1,62 +0,0 @@
-import { IncomingHttpHeaders } from "undici/types/header.js"
-import { addProxyHeaders } from "../src/helpers/proxy"
-import { UploaderEnvs } from "../src/types"
-
-describe('addProxyHeaders', () => {
-    it('should not return an Authorization header if PROXY_BASIC_USER and PROXY_BASIC_PASS are not set', () => {
-      // arrange
-      const envs: UploaderEnvs = {}
-      const headers: IncomingHttpHeaders = {}
-  
-      // act
-      const headersResult = addProxyHeaders(envs, headers)
-  
-      // assert
-      expect(headersResult['Authorization']).not.toBeDefined()
-    })
-  
-    it('should not return an Authorization header if only PROXY_BASIC_USER is set', () => {
-      // arrange
-      const envs: UploaderEnvs = {
-        PROXY_BASIC_USER: "testUser"
-      }
-      const headers: IncomingHttpHeaders = {}
-  
-      // act
-      const headersResult = addProxyHeaders(envs, headers)
-  
-      // assert
-      expect(headersResult['Authorization']).not.toBeDefined()
-    })
-  
-    it('should not return an Authorization header if only PROXY_BASIC_PASS is set', () => {
-      // arrange
-      const envs: UploaderEnvs = {
-        PROXY_BASIC_PASS: "testPass"
-      }
-      const headers: IncomingHttpHeaders = {}
-  
-      // act
-      const headersResult = addProxyHeaders(envs, headers)
-  
-      // assert
-      expect(headersResult['Authorization']).not.toBeDefined()
-    })
-  
-    it('should return an Authorization header if both PROXY_BASIC_PASS is set', () => {
-      // arrange
-      const envs: UploaderEnvs = {
-        PROXY_BASIC_USER: "testUser",
-        PROXY_BASIC_PASS: "testPass"
-      }
-      const headers: IncomingHttpHeaders = {}
-      const authString = `Basic ${Buffer.from(`${envs.PROXY_BASIC_USER}:${envs.PROXY_BASIC_PASS}`).toString("base64")}`
-  
-      // act
-      const headersResult = addProxyHeaders(envs, headers)
-  
-      // assert
-      expect(headersResult['Authorization']).toEqual(authString)
-    })
-  })
-  
\ No newline at end of file