From 0fbbb2cbbf3069c91cf4cdaf6e14c36ce8482cb0 Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Wed, 15 Oct 2025 12:44:31 -0400 Subject: [PATCH 1/5] Added logging to HttpClient --- lib/msal-node/src/network/HttpClient.ts | 725 ++++++++++-------- lib/msal-node/test/network/HttpClient.spec.ts | 2 +- 2 files changed, 413 insertions(+), 314 deletions(-) diff --git a/lib/msal-node/src/network/HttpClient.ts b/lib/msal-node/src/network/HttpClient.ts index 7c023219d4..bd0a2b5393 100644 --- a/lib/msal-node/src/network/HttpClient.ts +++ b/lib/msal-node/src/network/HttpClient.ts @@ -8,9 +8,12 @@ import { NetworkRequestOptions, NetworkResponse, HttpStatus, + Logger, + LoggerOptions, } from "@azure/msal-common/node"; import { HttpMethod, Constants, ProxyStatus } from "../utils/Constants.js"; import { NetworkUtils } from "../utils/NetworkUtils.js"; +import { name, version } from "../packageMetadata.js"; import http from "http"; import https from "https"; @@ -20,13 +23,19 @@ import https from "https"; export class HttpClient implements INetworkModule { private proxyUrl: string; private customAgentOptions: http.AgentOptions | https.AgentOptions; + private logger: Logger; + private isPiiEnabled: boolean; constructor( proxyUrl?: string, - customAgentOptions?: http.AgentOptions | https.AgentOptions + customAgentOptions?: http.AgentOptions | https.AgentOptions, + loggerOptions?: LoggerOptions ) { this.proxyUrl = proxyUrl || ""; this.customAgentOptions = customAgentOptions || {}; + + this.logger = new Logger(loggerOptions || {}, name, version); + this.isPiiEnabled = this.logger.isPiiLoggingEnabled(); } /** @@ -34,26 +43,23 @@ export class HttpClient implements INetworkModule { * @param url * @param options */ - async sendGetRequestAsync( + public async sendGetRequestAsync( url: string, options?: NetworkRequestOptions, timeout?: number ): Promise> { if (this.proxyUrl) { - return networkRequestViaProxy( - url, - this.proxyUrl, + return this.networkRequestViaProxy( HttpMethod.GET, + url, options, - this.customAgentOptions as http.AgentOptions, timeout ); } else { - return networkRequestViaHttps( - url, + return this.networkRequestViaHttps( HttpMethod.GET, + url, options, - this.customAgentOptions as https.AgentOptions, timeout ); } @@ -64,362 +70,455 @@ export class HttpClient implements INetworkModule { * @param url * @param options */ - async sendPostRequestAsync( + public async sendPostRequestAsync( url: string, options?: NetworkRequestOptions ): Promise> { if (this.proxyUrl) { - return networkRequestViaProxy( - url, - this.proxyUrl, - HttpMethod.POST, - options, - this.customAgentOptions as http.AgentOptions - ); + return this.networkRequestViaProxy(HttpMethod.POST, url, options); } else { - return networkRequestViaHttps( - url, - HttpMethod.POST, - options, - this.customAgentOptions as https.AgentOptions - ); + return this.networkRequestViaHttps(HttpMethod.POST, url, options); } } -} -const networkRequestViaProxy = ( - destinationUrlString: string, - proxyUrlString: string, - httpMethod: string, - options?: NetworkRequestOptions, - agentOptions?: http.AgentOptions, - timeout?: number -): Promise> => { - const destinationUrl = new URL(destinationUrlString); - const proxyUrl = new URL(proxyUrlString); - - // "method: connect" must be used to establish a connection to the proxy - const headers = options?.headers || ({} as Record); - const tunnelRequestOptions: https.RequestOptions = { - host: proxyUrl.hostname, - port: proxyUrl.port, - method: "CONNECT", - path: destinationUrl.hostname, - headers: headers, - }; - - if (agentOptions && Object.keys(agentOptions).length) { - tunnelRequestOptions.agent = new http.Agent(agentOptions); - } + private networkRequestViaProxy = ( + httpMethod: string, + destinationUrlString: string, + options?: NetworkRequestOptions, + timeout?: number + ): Promise> => { + const destinationUrl = new URL(destinationUrlString); + const proxyUrl = new URL(this.proxyUrl); + + // "method: connect" must be used to establish a connection to the proxy + const headers = options?.headers || ({} as Record); + const tunnelRequestOptions: https.RequestOptions = { + host: proxyUrl.hostname, + port: proxyUrl.port, + method: "CONNECT", + path: destinationUrl.hostname, + headers: headers, + }; - // compose a request string for the socket - let postRequestStringContent: string = ""; - if (httpMethod === HttpMethod.POST) { - const body = options?.body || ""; - postRequestStringContent = - "Content-Type: application/x-www-form-urlencoded\r\n" + - `Content-Length: ${body.length}\r\n` + - `\r\n${body}`; - } else { - // optional timeout is only for get requests (regionDiscovery, for example) - if (timeout) { - tunnelRequestOptions.timeout = timeout; - } - } - const outgoingRequestString = - `${httpMethod.toUpperCase()} ${destinationUrl.href} HTTP/1.1\r\n` + - `Host: ${destinationUrl.host}\r\n` + - "Connection: close\r\n" + - postRequestStringContent + - "\r\n"; - - return new Promise>((resolve, reject) => { - const request = http.request(tunnelRequestOptions); - - if (timeout) { - request.on("timeout", () => { - request.destroy(); - reject(new Error("Request time out")); - }); + if ( + this.customAgentOptions && + Object.keys(this.customAgentOptions).length + ) { + tunnelRequestOptions.agent = new http.Agent( + this.customAgentOptions + ); } - request.end(); + // compose a request string for the socket + let postRequestStringContent: string = ""; + if (httpMethod === HttpMethod.POST) { + const body = options?.body || ""; + postRequestStringContent = + "Content-Type: application/x-www-form-urlencoded\r\n" + + `Content-Length: ${body.length}\r\n` + + `\r\n${body}`; + } else { + // optional timeout is only for get requests (regionDiscovery, for example) + if (timeout) { + tunnelRequestOptions.timeout = timeout; + } + } + const outgoingRequestString = + `${httpMethod.toUpperCase()} ${destinationUrl.href} HTTP/1.1\r\n` + + `Host: ${destinationUrl.host}\r\n` + + "Connection: close\r\n" + + postRequestStringContent + + "\r\n"; + + return new Promise>((resolve, reject) => { + const request = http.request(tunnelRequestOptions); + + if (timeout) { + request.on("timeout", () => { + this.logUrlWithPiiAwareness( + `Request timeout after ${timeout}ms for URL`, + destinationUrlString + ); - // establish connection to the proxy - request.on("connect", (response, socket) => { - const proxyStatusCode = - response?.statusCode || ProxyStatus.SERVER_ERROR; - if ( - proxyStatusCode < ProxyStatus.SUCCESS_RANGE_START || - proxyStatusCode > ProxyStatus.SUCCESS_RANGE_END - ) { - request.destroy(); - socket.destroy(); - reject( - new Error( - `Error connecting to proxy. Http status code: ${ - response.statusCode - }. Http status message: ${ - response?.statusMessage || "Unknown" - }` - ) - ); + request.destroy(); + reject(new Error(`Request time out after ${timeout}ms`)); + }); } - // make a request over an HTTP tunnel - socket.write(outgoingRequestString); + request.end(); - const data: Buffer[] = []; - socket.on("data", (chunk) => { - data.push(chunk); - }); + // establish connection to the proxy + request.on("connect", (response, socket) => { + const proxyStatusCode = + response?.statusCode || ProxyStatus.SERVER_ERROR; + if ( + proxyStatusCode < ProxyStatus.SUCCESS_RANGE_START || + proxyStatusCode > ProxyStatus.SUCCESS_RANGE_END + ) { + request.destroy(); + socket.destroy(); + reject( + new Error( + `Error connecting to proxy. Http status code: ${ + response.statusCode + }. Http status message: ${ + response?.statusMessage || "Unknown" + }` + ) + ); + } - socket.on("end", () => { - // combine all received buffer streams into one buffer, and then into a string - const dataString = Buffer.concat([...data]).toString(); + // make a request over an HTTP tunnel + socket.write(outgoingRequestString); - // separate each line into it's own entry in an arry - const dataStringArray = dataString.split("\r\n"); - // the first entry will contain the statusCode and statusMessage - const httpStatusCode = parseInt( - dataStringArray[0].split(" ")[1] - ); - // remove "HTTP/1.1" and the status code to get the status message - const statusMessage = dataStringArray[0] - .split(" ") - .slice(2) - .join(" "); - // the last entry will contain the body - const body = dataStringArray[dataStringArray.length - 1]; - - // everything in between the first and last entries are the headers - const headersArray = dataStringArray.slice( - 1, - dataStringArray.length - 2 - ); + const data: Buffer[] = []; + socket.on("data", (chunk) => { + data.push(chunk); + }); - // build an object out of all the headers - const entries = new Map(); - headersArray.forEach((header) => { - /** - * the header might look like "Content-Length: 1531", but that is just a string - * it needs to be converted to a key/value pair - * split the string at the first instance of ":" - * there may be more than one ":" if the value of the header is supposed to be a JSON object - */ - const headerKeyValue = header.split(new RegExp(/:\s(.*)/s)); - const headerKey = headerKeyValue[0]; - let headerValue = headerKeyValue[1]; - - // check if the value of the header is supposed to be a JSON object - try { - const object = JSON.parse(headerValue); - - // if it is, then convert it from a string to a JSON object - if (object && typeof object === "object") { - headerValue = object; + socket.on("end", () => { + // combine all received buffer streams into one buffer, and then into a string + const dataString = Buffer.concat([...data]).toString(); + + // separate each line into it's own entry in an arry + const dataStringArray = dataString.split("\r\n"); + // the first entry will contain the statusCode and statusMessage + const httpStatusCode = parseInt( + dataStringArray[0].split(" ")[1] + ); + // remove "HTTP/1.1" and the status code to get the status message + const statusMessage = dataStringArray[0] + .split(" ") + .slice(2) + .join(" "); + // the last entry will contain the body + const body = dataStringArray[dataStringArray.length - 1]; + + // everything in between the first and last entries are the headers + const headersArray = dataStringArray.slice( + 1, + dataStringArray.length - 2 + ); + + // build an object out of all the headers + const entries = new Map(); + headersArray.forEach((header) => { + /** + * the header might look like "Content-Length: 1531", but that is just a string + * it needs to be converted to a key/value pair + * split the string at the first instance of ":" + * there may be more than one ":" if the value of the header is supposed to be a JSON object + */ + const headerKeyValue = header.split( + new RegExp(/:\s(.*)/s) + ); + const headerKey = headerKeyValue[0]; + let headerValue = headerKeyValue[1]; + + // check if the value of the header is supposed to be a JSON object + try { + const object = JSON.parse(headerValue); + + // if it is, then convert it from a string to a JSON object + if (object && typeof object === "object") { + headerValue = object; + } + } catch (e) { + // otherwise, leave it as a string } - } catch (e) { - // otherwise, leave it as a string - } - entries.set(headerKey, headerValue); - }); - const headers = Object.fromEntries(entries); - - const parsedHeaders = headers as Record; - const networkResponse = NetworkUtils.getNetworkResponse( - parsedHeaders, - parseBody( - httpStatusCode, - statusMessage, + entries.set(headerKey, headerValue); + }); + const headers = Object.fromEntries(entries); + + const parsedHeaders = headers as Record; + const networkResponse = NetworkUtils.getNetworkResponse( parsedHeaders, - body - ) as T, - httpStatusCode - ); + this.parseBody( + httpStatusCode, + statusMessage, + parsedHeaders, + body + ) as T, + httpStatusCode + ); + + if ( + this.shouldDestroyRequest( + httpStatusCode, + networkResponse + ) + ) { + request.destroy(); + } + resolve(networkResponse); + }); - if ( - (httpStatusCode < HttpStatus.SUCCESS_RANGE_START || - httpStatusCode > HttpStatus.SUCCESS_RANGE_END) && - // do not destroy the request for the device code flow - networkResponse.body["error"] !== - Constants.AUTHORIZATION_PENDING - ) { + socket.on("error", (chunk) => { request.destroy(); - } - resolve(networkResponse); + socket.destroy(); + reject(new Error(chunk.toString())); + }); }); - socket.on("error", (chunk) => { + request.on("error", (chunk) => { + this.logger.error( + `[MSAL-Network] Proxy request error: ${chunk.toString()}`, + "" + ); + this.logUrlWithPiiAwareness( + "Destination URL", + destinationUrlString + ); + this.logUrlWithPiiAwareness("Proxy URL", this.proxyUrl); + this.logger.error(`[MSAL-Network] Method: ${httpMethod}`, ""); + this.logHeadersWithPiiAwareness(headers); + request.destroy(); - socket.destroy(); reject(new Error(chunk.toString())); }); }); - - request.on("error", (chunk) => { - request.destroy(); - reject(new Error(chunk.toString())); - }); - }); -}; - -const networkRequestViaHttps = ( - urlString: string, - httpMethod: string, - options?: NetworkRequestOptions, - agentOptions?: https.AgentOptions, - timeout?: number -): Promise> => { - const isPostRequest = httpMethod === HttpMethod.POST; - const body: string = options?.body || ""; - - const url = new URL(urlString); - const headers = options?.headers || ({} as Record); - const customOptions: https.RequestOptions = { - method: httpMethod, - headers: headers, - ...NetworkUtils.urlToHttpOptions(url), }; - if (agentOptions && Object.keys(agentOptions).length) { - customOptions.agent = new https.Agent(agentOptions); - } - - if (isPostRequest) { - // needed for post request to work - customOptions.headers = { - ...customOptions.headers, - "Content-Length": body.length, + private networkRequestViaHttps = ( + httpMethod: string, + urlString: string, + options?: NetworkRequestOptions, + timeout?: number + ): Promise> => { + const isPostRequest = httpMethod === HttpMethod.POST; + const body: string = options?.body || ""; + + const url = new URL(urlString); + const headers = options?.headers || ({} as Record); + const customOptions: https.RequestOptions = { + method: httpMethod, + headers: headers, + ...NetworkUtils.urlToHttpOptions(url), }; - } else { - // optional timeout is only for get requests (regionDiscovery, for example) - if (timeout) { - customOptions.timeout = timeout; - } - } - return new Promise>((resolve, reject) => { - let request: http.ClientRequest; - // managed identity sources use http instead of https - if (customOptions.protocol === "http:") { - request = http.request(customOptions); - } else { - request = https.request(customOptions); + if ( + this.customAgentOptions && + Object.keys(this.customAgentOptions).length + ) { + customOptions.agent = new https.Agent(this.customAgentOptions); } if (isPostRequest) { - request.write(body); + // needed for post request to work + customOptions.headers = { + ...customOptions.headers, + "Content-Length": body.length, + }; + } else { + // optional timeout is only for get requests (regionDiscovery, for example) + if (timeout) { + customOptions.timeout = timeout; + } } - if (timeout) { - request.on("timeout", () => { - request.destroy(); - reject(new Error("Request time out")); - }); - } + return new Promise>((resolve, reject) => { + let request: http.ClientRequest; + // managed identity sources use http instead of https + if (customOptions.protocol === "http:") { + request = http.request(customOptions); + } else { + request = https.request(customOptions); + } - request.end(); + if (isPostRequest) { + request.write(body); + } - request.on("response", (response) => { - const headers = response.headers; - const statusCode = response.statusCode as number; - const statusMessage = response.statusMessage; + if (timeout) { + request.on("timeout", () => { + this.logUrlWithPiiAwareness( + `HTTPS request timeout after ${timeout}ms for URL`, + urlString + ); - const data: Buffer[] = []; - response.on("data", (chunk) => { - data.push(chunk); - }); + request.destroy(); + reject(new Error(`Request time out after ${timeout}ms`)); + }); + } + + request.end(); + + request.on("response", (response) => { + const headers = response.headers; + const statusCode = response.statusCode as number; + const statusMessage = response.statusMessage; + + const data: Buffer[] = []; + response.on("data", (chunk) => { + data.push(chunk); + }); - response.on("end", () => { - // combine all received buffer streams into one buffer, and then into a string - const body = Buffer.concat([...data]).toString(); + response.on("end", () => { + // combine all received buffer streams into one buffer, and then into a string + const body = Buffer.concat([...data]).toString(); - const parsedHeaders = headers as Record; - const networkResponse = NetworkUtils.getNetworkResponse( - parsedHeaders, - parseBody( - statusCode, - statusMessage, + const parsedHeaders = headers as Record; + const networkResponse = NetworkUtils.getNetworkResponse( parsedHeaders, - body - ) as T, - statusCode + this.parseBody( + statusCode, + statusMessage, + parsedHeaders, + body + ) as T, + statusCode + ); + + if ( + this.shouldDestroyRequest(statusCode, networkResponse) + ) { + request.destroy(); + } + resolve(networkResponse); + }); + }); + + request.on("error", (chunk) => { + this.logger.error( + `[MSAL-Network] HTTPS request error: ${chunk.toString()}`, + "" ); + this.logUrlWithPiiAwareness("URL", urlString); + this.logger.error(`[MSAL-Network] Method: ${httpMethod}`, ""); + this.logHeadersWithPiiAwareness(headers); - if ( - (statusCode < HttpStatus.SUCCESS_RANGE_START || - statusCode > HttpStatus.SUCCESS_RANGE_END) && - // do not destroy the request for the device code flow - networkResponse.body["error"] !== - Constants.AUTHORIZATION_PENDING - ) { - request.destroy(); - } - resolve(networkResponse); + request.destroy(); + reject(new Error(chunk.toString())); }); }); + }; - request.on("error", (chunk) => { - request.destroy(); - reject(new Error(chunk.toString())); - }); - }); -}; - -/** - * Check if extra parsing is needed on the repsonse from the server - * @param statusCode {number} the status code of the response from the server - * @param statusMessage {string | undefined} the status message of the response from the server - * @param headers {Record} the headers of the response from the server - * @param body {string} the body from the response of the server - * @returns {Object} JSON parsed body or error object - */ -const parseBody = ( - statusCode: number, - statusMessage: string | undefined, - headers: Record, - body: string -) => { - /* - * Informational responses (100 – 199) - * Successful responses (200 – 299) - * Redirection messages (300 – 399) - * Client error responses (400 – 499) - * Server error responses (500 – 599) + /** + * Check if extra parsing is needed on the repsonse from the server + * @param statusCode {number} the status code of the response from the server + * @param statusMessage {string | undefined} the status message of the response from the server + * @param headers {Record} the headers of the response from the server + * @param body {string} the body from the response of the server + * @returns {Object} JSON parsed body or error object */ + private parseBody = ( + statusCode: number, + statusMessage: string | undefined, + headers: Record, + body: string + ) => { + /* + * Informational responses (100 – 199) + * Successful responses (200 – 299) + * Redirection messages (300 – 399) + * Client error responses (400 – 499) + * Server error responses (500 – 599) + */ + + let parsedBody; + try { + parsedBody = JSON.parse(body); + } catch (error) { + let errorType; + let errorDescriptionHelper; + if ( + statusCode >= HttpStatus.CLIENT_ERROR_RANGE_START && + statusCode <= HttpStatus.CLIENT_ERROR_RANGE_END + ) { + errorType = "client_error"; + errorDescriptionHelper = "A client"; + } else if ( + statusCode >= HttpStatus.SERVER_ERROR_RANGE_START && + statusCode <= HttpStatus.SERVER_ERROR_RANGE_END + ) { + errorType = "server_error"; + errorDescriptionHelper = "A server"; + } else { + errorType = "unknown_error"; + errorDescriptionHelper = "An unknown"; + } - let parsedBody; - try { - parsedBody = JSON.parse(body); - } catch (error) { - let errorType; - let errorDescriptionHelper; - if ( - statusCode >= HttpStatus.CLIENT_ERROR_RANGE_START && - statusCode <= HttpStatus.CLIENT_ERROR_RANGE_END - ) { - errorType = "client_error"; - errorDescriptionHelper = "A client"; - } else if ( - statusCode >= HttpStatus.SERVER_ERROR_RANGE_START && - statusCode <= HttpStatus.SERVER_ERROR_RANGE_END - ) { - errorType = "server_error"; - errorDescriptionHelper = "A server"; + parsedBody = { + error: errorType, + error_description: `${errorDescriptionHelper} error occured.\nHttp status code: ${statusCode}\nHttp status message: ${ + statusMessage || "Unknown" + }\nHeaders: ${JSON.stringify(headers)}`, + }; + } + + return parsedBody; + }; + + /** + * Helper function to log URLs with PII-aware sanitization using pre-bound method + * @param urlString {string} the URL to log + * @param label {string} the label for the log message + */ + private logUrlWithPiiAwareness = ( + label: string, + urlString: string + ): void => { + if (this.isPiiEnabled) { + this.logger.errorPii(`[MSAL-Network] ${label}: ${urlString}`, ""); } else { - errorType = "unknown_error"; - errorDescriptionHelper = "An unknown"; + let urlHelper; + try { + const url = new URL(urlString); + urlHelper = `${url.protocol}//${url.host}${url.pathname}`; + } catch { + urlHelper = urlString.split("?")[0] || "unknown"; + } + + this.logger.error( + `[MSAL-Network] ${label}: ${urlHelper} [Enable PII logging to see additional details]`, + "" + ); } + }; - parsedBody = { - error: errorType, - error_description: `${errorDescriptionHelper} error occured.\nHttp status code: ${statusCode}\nHttp status message: ${ - statusMessage || "Unknown" - }\nHeaders: ${JSON.stringify(headers)}`, - }; - } + /** + * Helper function to log headers with PII awareness using pre-bound method and standard logger + * @param headers {Record} the headers to log + */ + private logHeadersWithPiiAwareness = ( + headers: Record + ): void => { + if (this.isPiiEnabled) { + this.logger.errorPii( + `[MSAL-Network] Headers: ${JSON.stringify(headers)}`, + "" + ); + } else { + this.logger.error( + `[MSAL-Network] Headers: [REDACTED - Enable PII logging to see headers]`, + "" + ); + } + }; - return parsedBody; -}; + /** + * Helper function to determine if a request should be destroyed based on status code and response body. + * Checks if the response is an error and not part of the device code flow (authorization_pending). + * @param statusCode {number} the status code of the response + * @param networkResponse {NetworkResponse} the network response object + * @returns {boolean} true if the request should be destroyed, false otherwise + */ + private shouldDestroyRequest = ( + statusCode: number, + networkResponse: NetworkResponse + ): boolean => { + return ( + (statusCode < HttpStatus.SUCCESS_RANGE_START || + statusCode > HttpStatus.SUCCESS_RANGE_END) && + // do not destroy the request for the device code flow + !( + networkResponse.body && + typeof networkResponse.body === "object" && + "error" in networkResponse.body && + (networkResponse.body as { error: string }).error === + Constants.AUTHORIZATION_PENDING + ) + ); + }; +} diff --git a/lib/msal-node/test/network/HttpClient.spec.ts b/lib/msal-node/test/network/HttpClient.spec.ts index 537ca68222..ced7133f7c 100644 --- a/lib/msal-node/test/network/HttpClient.spec.ts +++ b/lib/msal-node/test/network/HttpClient.spec.ts @@ -376,7 +376,7 @@ describe("HttpClient", () => { describe("Timeout Error (Post Requests Only)", () => { const timeoutInMilliseconds: number = 100; - const error: Error = new Error("Request time out"); + const error: Error = new Error("Request time out after 100ms"); test("Via Https", async () => { (https.request as jest.Mock).mockImplementationOnce( From 2ed5861d8c470d93517ffd52b570def00197cc8c Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Wed, 15 Oct 2025 12:45:03 -0400 Subject: [PATCH 2/5] Change files --- ...ure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@azure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json diff --git a/change/@azure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json b/change/@azure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json new file mode 100644 index 0000000000..6806c89ee4 --- /dev/null +++ b/change/@azure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Added logging to HttpClient (#8073)", + "packageName": "@azure/msal-node", + "email": "rginsburg@microsoft.com", + "dependentChangeType": "patch" +} From e7c8d7451eed2d828ed0f2b705b805e827e66aae Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Wed, 15 Oct 2025 12:47:10 -0400 Subject: [PATCH 3/5] beachball --- .../@azure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change/@azure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json b/change/@azure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json index 6806c89ee4..ec417eab96 100644 --- a/change/@azure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json +++ b/change/@azure-msal-node-edb8d743-61e2-4f42-a970-92e01f619a49.json @@ -1,6 +1,6 @@ { "type": "patch", - "comment": "Added logging to HttpClient (#8073)", + "comment": "Added logging to HttpClient (#8101)", "packageName": "@azure/msal-node", "email": "rginsburg@microsoft.com", "dependentChangeType": "patch" From 243d135110ac02b4c4c5cd86db009f6a842e5b29 Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Fri, 24 Oct 2025 13:13:22 -0400 Subject: [PATCH 4/5] Implemented co-pilot feedback --- lib/msal-node/src/network/HttpClient.ts | 4 ++-- lib/msal-node/test/network/HttpClient.spec.ts | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/msal-node/src/network/HttpClient.ts b/lib/msal-node/src/network/HttpClient.ts index bd0a2b5393..a7292f518c 100644 --- a/lib/msal-node/src/network/HttpClient.ts +++ b/lib/msal-node/src/network/HttpClient.ts @@ -452,8 +452,8 @@ export class HttpClient implements INetworkModule { /** * Helper function to log URLs with PII-aware sanitization using pre-bound method - * @param urlString {string} the URL to log * @param label {string} the label for the log message + * @param urlString {string} the URL to log */ private logUrlWithPiiAwareness = ( label: string, @@ -462,7 +462,7 @@ export class HttpClient implements INetworkModule { if (this.isPiiEnabled) { this.logger.errorPii(`[MSAL-Network] ${label}: ${urlString}`, ""); } else { - let urlHelper; + let urlHelper: string; try { const url = new URL(urlString); urlHelper = `${url.protocol}//${url.host}${url.pathname}`; diff --git a/lib/msal-node/test/network/HttpClient.spec.ts b/lib/msal-node/test/network/HttpClient.spec.ts index ced7133f7c..b0d522729a 100644 --- a/lib/msal-node/test/network/HttpClient.spec.ts +++ b/lib/msal-node/test/network/HttpClient.spec.ts @@ -376,7 +376,9 @@ describe("HttpClient", () => { describe("Timeout Error (Post Requests Only)", () => { const timeoutInMilliseconds: number = 100; - const error: Error = new Error("Request time out after 100ms"); + const error: Error = new Error( + `Request time out after ${timeoutInMilliseconds}ms` + ); test("Via Https", async () => { (https.request as jest.Mock).mockImplementationOnce( From 32d4f73207695ff86e64b1dcb3fc73a623bb3bae Mon Sep 17 00:00:00 2001 From: Robbie Ginsburg Date: Fri, 24 Oct 2025 16:41:54 -0400 Subject: [PATCH 5/5] Implemented GitHub Feedback --- lib/msal-node/src/network/HttpClient.ts | 44 +++++++++---------------- 1 file changed, 15 insertions(+), 29 deletions(-) diff --git a/lib/msal-node/src/network/HttpClient.ts b/lib/msal-node/src/network/HttpClient.ts index a7292f518c..57bb3b5ee7 100644 --- a/lib/msal-node/src/network/HttpClient.ts +++ b/lib/msal-node/src/network/HttpClient.ts @@ -263,7 +263,7 @@ export class HttpClient implements INetworkModule { request.on("error", (chunk) => { this.logger.error( - `[MSAL-Network] Proxy request error: ${chunk.toString()}`, + `HttpClient - Proxy request error: ${chunk.toString()}`, "" ); this.logUrlWithPiiAwareness( @@ -271,8 +271,11 @@ export class HttpClient implements INetworkModule { destinationUrlString ); this.logUrlWithPiiAwareness("Proxy URL", this.proxyUrl); - this.logger.error(`[MSAL-Network] Method: ${httpMethod}`, ""); - this.logHeadersWithPiiAwareness(headers); + this.logger.error(`HttpClient - Method: ${httpMethod}`, ""); + this.logger.errorPii( + `HttpClient - Headers: ${JSON.stringify(headers)}`, + "" + ); request.destroy(); reject(new Error(chunk.toString())); @@ -381,12 +384,15 @@ export class HttpClient implements INetworkModule { request.on("error", (chunk) => { this.logger.error( - `[MSAL-Network] HTTPS request error: ${chunk.toString()}`, + `HttpClient - HTTPS request error: ${chunk.toString()}`, "" ); this.logUrlWithPiiAwareness("URL", urlString); - this.logger.error(`[MSAL-Network] Method: ${httpMethod}`, ""); - this.logHeadersWithPiiAwareness(headers); + this.logger.error(`HttpClient - Method: ${httpMethod}`, ""); + this.logger.errorPii( + `HttpClient - Headers: ${JSON.stringify(headers)}`, + "" + ); request.destroy(); reject(new Error(chunk.toString())); @@ -451,7 +457,7 @@ export class HttpClient implements INetworkModule { }; /** - * Helper function to log URLs with PII-aware sanitization using pre-bound method + * Helper function to log a formatted message containing URLs, with PII-aware sanitization * @param label {string} the label for the log message * @param urlString {string} the URL to log */ @@ -460,7 +466,7 @@ export class HttpClient implements INetworkModule { urlString: string ): void => { if (this.isPiiEnabled) { - this.logger.errorPii(`[MSAL-Network] ${label}: ${urlString}`, ""); + this.logger.errorPii(`HttpClient - ${label}: ${urlString}`, ""); } else { let urlHelper: string; try { @@ -471,27 +477,7 @@ export class HttpClient implements INetworkModule { } this.logger.error( - `[MSAL-Network] ${label}: ${urlHelper} [Enable PII logging to see additional details]`, - "" - ); - } - }; - - /** - * Helper function to log headers with PII awareness using pre-bound method and standard logger - * @param headers {Record} the headers to log - */ - private logHeadersWithPiiAwareness = ( - headers: Record - ): void => { - if (this.isPiiEnabled) { - this.logger.errorPii( - `[MSAL-Network] Headers: ${JSON.stringify(headers)}`, - "" - ); - } else { - this.logger.error( - `[MSAL-Network] Headers: [REDACTED - Enable PII logging to see headers]`, + `HttpClient - ${label}: ${urlHelper} [Enable PII logging to see additional details]`, "" ); }