[core-rest-pipeline] Address issue with proxy port setting being ignored

sdk/core/core-rest-pipeline/CHANGELOG.md

@@ -8,6 +8,8 @@
 
 ### Bugs Fixed
 
+- Fixed an issue where `proxyPolicy` was ignoring a custom port setting. [PR #28974](https://github.com/Azure/azure-sdk-for-js/pull/28974)
+
 ### Other Changes
 
 ## 1.15.0 (2024-03-12)

sdk/core/core-rest-pipeline/review/core-rest-pipeline.api.md

@@ -160,7 +160,7 @@ export const formDataPolicyName = "formDataPolicy";
 // @public
 export type FormDataValue = string | Blob | File;
 
-// @public
+// @public @deprecated
 export function getDefaultProxySettings(proxyUrl?: string): ProxySettings | undefined;
 
 // @public
@@ -323,7 +323,7 @@ export interface PipelineRetryOptions {
 }
 
 // @public
-export function proxyPolicy(proxySettings?: ProxySettings | undefined, options?: {
+export function proxyPolicy(proxySettings?: ProxySettings, options?: {
     customNoProxyList?: string[];
 }): PipelinePolicy;
 

sdk/core/core-rest-pipeline/src/policies/proxyPolicy.ts

@@ -112,6 +112,7 @@ export function loadNoProxy(): string[] {
  * If no argument is given, it attempts to parse a proxy URL from the environment
  * variables `HTTPS_PROXY` or `HTTP_PROXY`.
  * @param proxyUrl - The url of the proxy to use. May contain authentication information.
+ * @deprecated - Internally this method is no longer necessary when setting proxy information.
  */
 export function getDefaultProxySettings(proxyUrl?: string): ProxySettings | undefined {
   if (!proxyUrl) {
@@ -131,7 +132,41 @@ export function getDefaultProxySettings(proxyUrl?: string): ProxySettings | unde
   };
 }
 
-function setProxyAgentOnRequest(request: PipelineRequest, cachedAgents: CachedAgents): void {
+/**
+ * This method attempts to parse a proxy URL from the environment
+ * variables `HTTPS_PROXY` or `HTTP_PROXY`.
+ */
+function getDefaultProxySettingsInternal(): URL | undefined {
+  const envProxy = loadEnvironmentProxyValue();
+  return envProxy ? new URL(envProxy) : undefined;
+}
+
+function getUrlFromProxySettings(settings: ProxySettings): URL {
+  let parsedProxyUrl: URL;
+  try {
+    parsedProxyUrl = new URL(settings.host);
+  } catch (_error) {
+    throw new Error(
+      `Expecting a valid host string in proxy settings, but found "${settings.host}".`,
+    );
+  }
+
+  parsedProxyUrl.port = String(settings.port);
+  if (settings.username) {
+    parsedProxyUrl.username = settings.username;
+  }
+  if (settings.password) {
+    parsedProxyUrl.password = settings.password;
+  }
+
+  return parsedProxyUrl;
+}
+
+function setProxyAgentOnRequest(
+  request: PipelineRequest,
+  cachedAgents: CachedAgents,
+  proxyUrl: URL,
+): void {
   // Custom Agent should take precedence so if one is present
   // we should skip to avoid overwriting it.
   if (request.agent) {
@@ -142,36 +177,24 @@ function setProxyAgentOnRequest(request: PipelineRequest, cachedAgents: CachedAg
 
   const isInsecure = url.protocol !== "https:";
 
-  const proxySettings = request.proxySettings;
-  if (proxySettings) {
-    let parsedProxyUrl: URL;
-    try {
-      parsedProxyUrl = new URL(proxySettings.host);
-    } catch (_error) {
-      throw new Error(
-        `Expecting a valid host string in proxy settings, but found "${proxySettings.host}".`,
-      );
-    }
-
-    if (request.tlsSettings) {
-      logger.warning(
-        "TLS settings are not supported in combination with custom Proxy, certificates provided to the client will be ignored.",
-      );
-    }
+  if (request.tlsSettings) {
+    logger.warning(
+      "TLS settings are not supported in combination with custom Proxy, certificates provided to the client will be ignored.",
+    );
+  }
 
-    const headers = request.headers.toJSON();
+  const headers = request.headers.toJSON();
 
-    if (isInsecure) {
-      if (!cachedAgents.httpProxyAgent) {
-        cachedAgents.httpProxyAgent = new HttpProxyAgent(parsedProxyUrl, { headers });
-      }
-      request.agent = cachedAgents.httpProxyAgent;
-    } else {
-      if (!cachedAgents.httpsProxyAgent) {
-        cachedAgents.httpsProxyAgent = new HttpsProxyAgent(parsedProxyUrl, { headers });
-      }
-      request.agent = cachedAgents.httpsProxyAgent;
+  if (isInsecure) {
+    if (!cachedAgents.httpProxyAgent) {
+      cachedAgents.httpProxyAgent = new HttpProxyAgent(proxyUrl, { headers });
+    }
+    request.agent = cachedAgents.httpProxyAgent;
+  } else {
+    if (!cachedAgents.httpsProxyAgent) {
+      cachedAgents.httpsProxyAgent = new HttpsProxyAgent(proxyUrl, { headers });
     }
+    request.agent = cachedAgents.httpsProxyAgent;
   }
 }
 
@@ -188,7 +211,7 @@ interface CachedAgents {
  * @param options - additional settings, for example, custom NO_PROXY patterns
  */
 export function proxyPolicy(
-  proxySettings = getDefaultProxySettings(),
+  proxySettings?: ProxySettings,
   options?: {
     /** a list of patterns to override those loaded from NO_PROXY environment variable. */
     customNoProxyList?: string[];
@@ -198,24 +221,31 @@ export function proxyPolicy(
     globalNoProxyList.push(...loadNoProxy());
   }
 
+  const defaultProxy = proxySettings
+    ? getUrlFromProxySettings(proxySettings)
+    : getDefaultProxySettingsInternal();
+
   const cachedAgents: CachedAgents = {};
 
   return {
     name: proxyPolicyName,
     async sendRequest(request: PipelineRequest, next: SendRequest): Promise<PipelineResponse> {
       if (
         !request.proxySettings &&
+        defaultProxy &&
         !isBypassed(
           request.url,
           options?.customNoProxyList ?? globalNoProxyList,
           options?.customNoProxyList ? undefined : globalBypassedMap,
         )
       ) {
-        request.proxySettings = proxySettings;
-      }
-
-      if (request.proxySettings) {
-        setProxyAgentOnRequest(request, cachedAgents);
+        setProxyAgentOnRequest(request, cachedAgents, defaultProxy);
+      } else if (request.proxySettings) {
+        setProxyAgentOnRequest(
+          request,
+          cachedAgents,
+          getUrlFromProxySettings(request.proxySettings),
+        );
       }
       return next(request);
     },

sdk/core/core-rest-pipeline/test/node/proxyPolicy.spec.ts

@@ -9,9 +9,26 @@ import {
   createPipelineRequest,
   getDefaultProxySettings,
   proxyPolicy,
+  Agent,
+  PipelineRequest,
 } from "../../src/index.js";
 import { globalNoProxyList, loadNoProxy } from "../../src/policies/proxyPolicy.js";
 
+interface ProxyAgent extends Agent {
+  proxy: URL;
+}
+
+function getProxyUrlForRequest(request: PipelineRequest): URL {
+  const proxyAgent = request.agent as ProxyAgent;
+  if (!proxyAgent) {
+    throw new Error(`Expected request for ${request.url} to have agent set`);
+  }
+  if (proxyAgent.proxy) {
+    return proxyAgent.proxy;
+  }
+  throw new Error(`Expected agent ${proxyAgent} to have a proxy property`);
+}
+
 describe("proxyPolicy (node)", function () {
   it("Sets proxy settings on the request", function () {
     const proxySettings: ProxySettings = {
@@ -29,7 +46,10 @@ describe("proxyPolicy (node)", function () {
     policy.sendRequest(request, next);
 
     assert.deepStrictEqual(next.mock.calls, [[request]], "next called with request");
-    assert.strictEqual(request.proxySettings, proxySettings);
+    assert.strictEqual(
+      getProxyUrlForRequest(request).toString(),
+      "https://proxy.example.com:8080/",
+    );
   });
 
   it("Doesn't override existing request proxy settings", function () {
@@ -54,7 +74,10 @@ describe("proxyPolicy (node)", function () {
     policy.sendRequest(request, next);
 
     assert.deepStrictEqual(next.mock.calls, [[request]], "next called with request");
-    assert.strictEqual(request.proxySettings, requestProxySettings);
+    assert.strictEqual(
+      getProxyUrlForRequest(request).toString(),
+      "https://proxy2.example.com:8080/",
+    );
   });
 
   it("Doesn't assign proxy settings to request when NO_PROXY contains a match of host name", function () {
@@ -79,36 +102,49 @@ describe("proxyPolicy (node)", function () {
       policy.sendRequest(request, next);
 
       assert.deepStrictEqual(next.mock.calls, [[request]], "next called with request");
-      assert.strictEqual(request.proxySettings, undefined);
+      assert.isUndefined(request.proxySettings);
+      assert.isUndefined(request.agent);
 
       request.url = "https://www.proxytest.com";
       policy.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, undefined);
+      assert.isUndefined(request.proxySettings);
+      assert.isUndefined(request.agent);
 
       request.url = "http://test.proxytest.com";
       policy.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, undefined);
+      assert.isUndefined(request.proxySettings);
+      assert.isUndefined(request.agent);
 
       request.url = "http://test.proxytest.com/path1";
       policy.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, undefined);
+      assert.isUndefined(request.proxySettings);
+      assert.isUndefined(request.agent);
 
       request.url = "http://test.proxytest.com/path2";
       policy.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, undefined);
+      assert.isUndefined(request.proxySettings);
+      assert.isUndefined(request.agent);
 
       request.url = "http://abcproxytest.com";
       policy.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, proxySettings);
+      assert.strictEqual(
+        getProxyUrlForRequest(request).toString(),
+        "https://proxy.example.com:8080/",
+      );
 
       request.proxySettings = undefined;
+      request.agent = undefined;
       request.url = "http://test.com";
       policy.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, undefined);
+      assert.isUndefined(request.proxySettings);
+      assert.isUndefined(request.agent);
 
       request.url = "http://www.test.com";
       policy.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, proxySettings);
+      assert.strictEqual(
+        getProxyUrlForRequest(request).toString(),
+        "https://proxy.example.com:8080/",
+      );
     } finally {
       process.env["NO_PROXY"] = saved;
       globalNoProxyList.splice(0, globalNoProxyList.length);
@@ -136,33 +172,51 @@ describe("proxyPolicy (node)", function () {
       });
       policy1.sendRequest(request, next);
       assert.deepStrictEqual(next.mock.calls, [[request]], "next called with request");
-      assert.strictEqual(request.proxySettings, proxySettings);
+      assert.strictEqual(
+        getProxyUrlForRequest(request).toString(),
+        "https://proxy.example.com:8080/",
+      );
 
       request.url = "https://test.com";
       request.proxySettings = undefined;
+      request.agent = undefined;
       policy1.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, undefined);
+      assert.isUndefined(request.proxySettings);
+      assert.isUndefined(request.agent);
 
       request.url = "http://another.com";
       request.proxySettings = undefined;
       policy1.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, proxySettings);
+      assert.strictEqual(
+        getProxyUrlForRequest(request).toString(),
+        "https://proxy.example.com:8080/",
+      );
 
       const policy2 = proxyPolicy(proxySettings, { customNoProxyList: ["proxytest.com"] });
       request.url = "http://test.com";
       request.proxySettings = undefined;
+      request.agent = undefined;
       policy2.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, proxySettings);
+      assert.strictEqual(
+        getProxyUrlForRequest(request).toString(),
+        "https://proxy.example.com:8080/",
+      );
 
       request.url = "http://proxytest.com";
       request.proxySettings = undefined;
+      request.agent = undefined;
       policy2.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, undefined);
+      assert.isUndefined(request.proxySettings);
+      assert.isUndefined(request.agent);
 
       request.url = "http://fourth.com";
       request.proxySettings = undefined;
+      request.agent = undefined;
       policy2.sendRequest(request, next);
-      assert.strictEqual(request.proxySettings, proxySettings);
+      assert.strictEqual(
+        getProxyUrlForRequest(request).toString(),
+        "https://proxy.example.com:8080/",
+      );
     } finally {
       process.env["NO_PROXY"] = saved;
       globalNoProxyList.splice(0, globalNoProxyList.length);

sdk/core/ts-http-runtime/review/ts-http-runtime.api.md

@@ -268,7 +268,7 @@ export function getClient(endpoint: string, options?: ClientOptions): Client;
 // @public
 export function getClient(endpoint: string, credentials?: TokenCredential | KeyCredential, options?: ClientOptions): Client;
 
-// @public
+// @public @deprecated
 export function getDefaultProxySettings(proxyUrl?: string): ProxySettings | undefined;
 
 // @public
@@ -578,7 +578,7 @@ export interface PipelineRetryOptions {
 }
 
 // @public
-export function proxyPolicy(proxySettings?: ProxySettings | undefined, options?: {
+export function proxyPolicy(proxySettings?: ProxySettings, options?: {
     customNoProxyList?: string[];
 }): PipelinePolicy;