From df26209f7cf78f5e27c9d001eb24e9741344a8a7 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Tue, 29 Oct 2024 14:58:28 -0700 Subject: [PATCH] Collect additional telemetry for crypto/runtime errors (#7394) - Adds additional check for the subtle property on crypto - Fixes error stack logic to keep top 5 lines instead of bottom 5 - Collects common, known runtime errors and redacts when necessary --------- Co-authored-by: Konstantin Shabelko --- ...-a7e3c096-c445-4017-9b15-e9c0a67e1aa4.json | 7 + ...-e50cbab0-daa5-497d-af15-caeed7c34e6c.json | 7 + .../apiReview/msal-browser.api.md | 2 +- lib/msal-browser/src/crypto/BrowserCrypto.ts | 21 ++- lib/msal-browser/src/crypto/CryptoOps.ts | 2 +- .../src/error/BrowserAuthError.ts | 11 +- lib/msal-common/apiReview/msal-common.api.md | 12 +- .../performance/PerformanceClient.ts | 54 ++++-- .../test/telemetry/PerformanceClient.spec.ts | 174 +++++++++++++++++- 9 files changed, 256 insertions(+), 34 deletions(-) create mode 100644 change/@azure-msal-browser-a7e3c096-c445-4017-9b15-e9c0a67e1aa4.json create mode 100644 change/@azure-msal-common-e50cbab0-daa5-497d-af15-caeed7c34e6c.json diff --git a/change/@azure-msal-browser-a7e3c096-c445-4017-9b15-e9c0a67e1aa4.json b/change/@azure-msal-browser-a7e3c096-c445-4017-9b15-e9c0a67e1aa4.json new file mode 100644 index 0000000000..95e74f01d3 --- /dev/null +++ b/change/@azure-msal-browser-a7e3c096-c445-4017-9b15-e9c0a67e1aa4.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Capture runtime errors in telemetry for submeasures", + "packageName": "@azure/msal-browser", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/change/@azure-msal-common-e50cbab0-daa5-497d-af15-caeed7c34e6c.json b/change/@azure-msal-common-e50cbab0-daa5-497d-af15-caeed7c34e6c.json new file mode 100644 index 0000000000..03c6226c49 --- /dev/null +++ b/change/@azure-msal-common-e50cbab0-daa5-497d-af15-caeed7c34e6c.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Validate crypto.subtle is available during initialization", + "packageName": "@azure/msal-common", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/lib/msal-browser/apiReview/msal-browser.api.md b/lib/msal-browser/apiReview/msal-browser.api.md index 642325b17f..e96ae24bef 100644 --- a/lib/msal-browser/apiReview/msal-browser.api.md +++ b/lib/msal-browser/apiReview/msal-browser.api.md @@ -184,7 +184,7 @@ function blockReloadInHiddenIframes(): void; // // @public export class BrowserAuthError extends AuthError { - constructor(errorCode: string); + constructor(errorCode: string, subError?: string); } declare namespace BrowserAuthErrorCodes { diff --git a/lib/msal-browser/src/crypto/BrowserCrypto.ts b/lib/msal-browser/src/crypto/BrowserCrypto.ts index 8015f2b877..ef8faa7b33 100644 --- a/lib/msal-browser/src/crypto/BrowserCrypto.ts +++ b/lib/msal-browser/src/crypto/BrowserCrypto.ts @@ -9,7 +9,6 @@ import { } from "../error/BrowserAuthError.js"; import { IPerformanceClient, - Logger, PerformanceEvents, } from "@azure/msal-common/browser"; import { KEY_FORMAT_JWK } from "../utils/BrowserConstants.js"; @@ -36,6 +35,8 @@ const UUID_CHARS = "0123456789abcdef"; // Array to store UINT32 random value const UINT32_ARR = new Uint32Array(1); +const SUBTLE_SUBERROR = "crypto_subtle_undefined"; + const keygenAlgorithmOptions: RsaHashedKeyGenParams = { name: PKCS1_V15_KEYGEN_ALG, hash: S256_HASH_ALG, @@ -46,13 +47,21 @@ const keygenAlgorithmOptions: RsaHashedKeyGenParams = { /** * Check whether browser crypto is available. */ -export function validateCryptoAvailable(logger: Logger): void { - if ("crypto" in window) { - logger.verbose("BrowserCrypto: modern crypto interface available"); - } else { - logger.error("BrowserCrypto: crypto interface is unavailable"); +export function validateCryptoAvailable(): void { + if (!window) { + throw createBrowserAuthError( + BrowserAuthErrorCodes.nonBrowserEnvironment + ); + } + if (!window.crypto) { throw createBrowserAuthError(BrowserAuthErrorCodes.cryptoNonExistent); } + if (!window.crypto.subtle) { + throw createBrowserAuthError( + BrowserAuthErrorCodes.cryptoNonExistent, + SUBTLE_SUBERROR + ); + } } /** diff --git a/lib/msal-browser/src/crypto/CryptoOps.ts b/lib/msal-browser/src/crypto/CryptoOps.ts index 91d76f84c2..5b424ea61a 100644 --- a/lib/msal-browser/src/crypto/CryptoOps.ts +++ b/lib/msal-browser/src/crypto/CryptoOps.ts @@ -53,7 +53,7 @@ export class CryptoOps implements ICrypto { constructor(logger: Logger, performanceClient?: IPerformanceClient) { this.logger = logger; // Browser crypto needs to be validated first before any other classes can be set. - BrowserCrypto.validateCryptoAvailable(logger); + BrowserCrypto.validateCryptoAvailable(); this.cache = new AsyncMemoryStorage(this.logger); this.performanceClient = performanceClient; } diff --git a/lib/msal-browser/src/error/BrowserAuthError.ts b/lib/msal-browser/src/error/BrowserAuthError.ts index 568047c776..1cbc966270 100644 --- a/lib/msal-browser/src/error/BrowserAuthError.ts +++ b/lib/msal-browser/src/error/BrowserAuthError.ts @@ -351,14 +351,17 @@ export const BrowserAuthErrorMessage = { * Browser library error class thrown by the MSAL.js library for SPAs */ export class BrowserAuthError extends AuthError { - constructor(errorCode: string) { - super(errorCode, BrowserAuthErrorMessages[errorCode]); + constructor(errorCode: string, subError?: string) { + super(errorCode, BrowserAuthErrorMessages[errorCode], subError); Object.setPrototypeOf(this, BrowserAuthError.prototype); this.name = "BrowserAuthError"; } } -export function createBrowserAuthError(errorCode: string): BrowserAuthError { - return new BrowserAuthError(errorCode); +export function createBrowserAuthError( + errorCode: string, + subError?: string +): BrowserAuthError { + return new BrowserAuthError(errorCode, subError); } diff --git a/lib/msal-common/apiReview/msal-common.api.md b/lib/msal-common/apiReview/msal-common.api.md index 352f8a10de..a77d9304e3 100644 --- a/lib/msal-common/apiReview/msal-common.api.md +++ b/lib/msal-common/apiReview/msal-common.api.md @@ -4299,12 +4299,12 @@ const X_MS_LIB_CAPABILITY = "x-ms-lib-capability"; // src/response/ResponseHandler.ts:430:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen // src/response/ResponseHandler.ts:431:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen // src/response/ResponseHandler.ts:432:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen -// src/telemetry/performance/PerformanceClient.ts:886:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen -// src/telemetry/performance/PerformanceClient.ts:886:15 - (tsdoc-param-tag-with-invalid-type) The @param block should not include a JSDoc-style '{type}' -// src/telemetry/performance/PerformanceClient.ts:898:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen -// src/telemetry/performance/PerformanceClient.ts:898:27 - (tsdoc-param-tag-with-invalid-type) The @param block should not include a JSDoc-style '{type}' -// src/telemetry/performance/PerformanceClient.ts:899:24 - (tsdoc-escape-right-brace) The "}" character should be escaped using a backslash to avoid confusion with a TSDoc inline tag -// src/telemetry/performance/PerformanceClient.ts:899:17 - (tsdoc-malformed-inline-tag) Expecting a TSDoc tag starting with "{@" +// src/telemetry/performance/PerformanceClient.ts:916:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen +// src/telemetry/performance/PerformanceClient.ts:916:15 - (tsdoc-param-tag-with-invalid-type) The @param block should not include a JSDoc-style '{type}' +// src/telemetry/performance/PerformanceClient.ts:928:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen +// src/telemetry/performance/PerformanceClient.ts:928:27 - (tsdoc-param-tag-with-invalid-type) The @param block should not include a JSDoc-style '{type}' +// src/telemetry/performance/PerformanceClient.ts:929:24 - (tsdoc-escape-right-brace) The "}" character should be escaped using a backslash to avoid confusion with a TSDoc inline tag +// src/telemetry/performance/PerformanceClient.ts:929:17 - (tsdoc-malformed-inline-tag) Expecting a TSDoc tag starting with "{@" // src/telemetry/performance/PerformanceEvent.ts:565:21 - (tsdoc-escape-right-brace) The "}" character should be escaped using a backslash to avoid confusion with a TSDoc inline tag // src/telemetry/performance/PerformanceEvent.ts:565:14 - (tsdoc-malformed-inline-tag) Expecting a TSDoc tag starting with "{@" // src/telemetry/performance/PerformanceEvent.ts:565:8 - (tsdoc-undefined-tag) The TSDoc tag "@type" is not defined in this configuration diff --git a/lib/msal-common/src/telemetry/performance/PerformanceClient.ts b/lib/msal-common/src/telemetry/performance/PerformanceClient.ts index 936e33ab9b..9be26bb0c4 100644 --- a/lib/msal-common/src/telemetry/performance/PerformanceClient.ts +++ b/lib/msal-common/src/telemetry/performance/PerformanceClient.ts @@ -198,22 +198,39 @@ export function compactStack(stack: string, stackMaxSize: number): string[] { } const stackArr = stack.split("\n") || []; - if (stackArr.length < 2) { - return []; - } const res = []; - // Get top N stack lines - for ( - // Skip first line as it may contain PII data - let ix = Math.max(stackArr.length - stackMaxSize - 1, 1); - ix < stackArr.length; - ix++ + + // Check for a handful of known, common runtime errors and log them (with redaction where applicable). + const firstLine = stackArr[0]; + if ( + firstLine.startsWith("TypeError: Cannot read property") || + firstLine.startsWith("TypeError: Cannot read properties of") || + firstLine.startsWith("TypeError: Cannot set property") || + firstLine.startsWith("TypeError: Cannot set properties of") || + firstLine.endsWith("is not a function") ) { - const line = stackArr[ix]; + // These types of errors are not at risk of leaking PII. They will indicate unavailable APIs + res.push(compactStackLine(firstLine)); + } else if ( + firstLine.startsWith("SyntaxError") || + firstLine.startsWith("TypeError") + ) { + // Prevent unintentional leaking of arbitrary info by redacting contents between both single and double quotes + res.push( + compactStackLine( + // Example: SyntaxError: Unexpected token 'e', "test" is not valid JSON -> SyntaxError: Unexpected token , is not valid JSON + firstLine.replace(/['].*[']|["].*["]/g, "") + ) + ); + } + + // Get top N stack lines + for (let ix = 1; ix < stackArr.length; ix++) { if (res.length >= stackMaxSize) { break; } + const line = stackArr[ix]; res.push(compactStackLine(line)); } return res; @@ -630,14 +647,27 @@ export abstract class PerformanceClient implements IPerformanceClient { event.correlationId ); + if (error) { + addError(error, this.logger, rootEvent); + } + // Add sub-measurement attribute to root event. if (!isRoot) { rootEvent[event.name + "DurationMs"] = Math.floor(event.durationMs); return { ...rootEvent }; } - if (error) { - addError(error, this.logger, rootEvent); + if ( + isRoot && + !error && + (rootEvent.errorCode || rootEvent.subErrorCode) + ) { + this.logger.trace( + `PerformanceClient: Remove error and sub-error codes for root event ${event.name} as intermediate error was successfully handled`, + event.correlationId + ); + rootEvent.errorCode = undefined; + rootEvent.subErrorCode = undefined; } let finalEvent: PerformanceEvent = { ...rootEvent, ...event }; diff --git a/lib/msal-common/test/telemetry/PerformanceClient.spec.ts b/lib/msal-common/test/telemetry/PerformanceClient.spec.ts index 8d0208509f..6ba199fbdc 100644 --- a/lib/msal-common/test/telemetry/PerformanceClient.spec.ts +++ b/lib/msal-common/test/telemetry/PerformanceClient.spec.ts @@ -5,7 +5,6 @@ import { ApplicationTelemetry, - AuthError, IGuidGenerator, InteractionRequiredAuthError, IPerformanceClient, @@ -21,6 +20,7 @@ import { } from "../../src/telemetry/performance/PerformanceClient"; import * as PerformanceClient from "../../src/telemetry/performance/PerformanceClient"; import { PerformanceEventAbbreviations } from "../../src/telemetry/performance/PerformanceEvent"; +import { AuthError } from "../../src/error/AuthError.js"; const sampleClientId = "test-client-id"; const authority = "https://login.microsoftonline.com/common"; @@ -255,6 +255,102 @@ describe("PerformanceClient.spec.ts", () => { }); }); + it("captures runtime errors from submeasurements", (done) => { + const mockPerfClient = new MockPerformanceClient(); + const correlationId = "test-correlation-id"; + + const publicError = new AuthError( + "public_test_error", + "This error will be thrown to caller" + ); + const runtimeError = new TypeError("This error caused publicError"); + + mockPerfClient.addPerformanceCallback((events) => { + expect(events.length).toEqual(1); + const event = events[0]; + expect(event["errorCode"]).toBe(publicError.errorCode); + expect(event["errorName"]).toBe("TypeError"); + expect(event["errorStack"]).toEqual( + compactStack(runtimeError.stack as string, 5) + ); + done(); + }); + + // Start and end top-level measurement + const topLevelEvent = mockPerfClient.startMeasurement( + PerformanceEvents.AcquireTokenSilent, + correlationId + ); + + // Start and complete submeasurements + mockPerfClient + .startMeasurement( + PerformanceEvents.AcquireTokenSilentAsync, + correlationId + ) + .end({ success: false }, publicError); + mockPerfClient + .startMeasurement( + PerformanceEvents.SilentIframeClientAcquireToken, + correlationId + ) + .end({ success: false }, runtimeError); + + topLevelEvent.end( + { + success: false, + }, + publicError + ); + }); + + it("captures runtime errors from submeasurements and removes error code", (done) => { + const mockPerfClient = new MockPerformanceClient(); + const correlationId = "test-correlation-id"; + + const publicError = new AuthError( + "public_test_error", + "This error will be thrown to caller" + ); + const runtimeError = new TypeError("This error caused publicError"); + + mockPerfClient.addPerformanceCallback((events) => { + expect(events.length).toEqual(1); + const event = events[0]; + expect(event["errorCode"]).toBeUndefined(); + expect(event["subErrorCode"]).toBeUndefined(); + expect(event["errorName"]).toBe("TypeError"); + expect(event["errorStack"]).toEqual( + compactStack(runtimeError.stack as string, 5) + ); + done(); + }); + + // Start and end top-level measurement + const topLevelEvent = mockPerfClient.startMeasurement( + PerformanceEvents.AcquireTokenSilent, + correlationId + ); + + // Start and complete submeasurements + mockPerfClient + .startMeasurement( + PerformanceEvents.AcquireTokenSilentAsync, + correlationId + ) + .end({ success: false }, publicError); + mockPerfClient + .startMeasurement( + PerformanceEvents.SilentIframeClientAcquireToken, + correlationId + ) + .end({ success: false }, runtimeError); + + topLevelEvent.end({ + success: true, + }); + }); + it("discards incomplete submeasurements", (done) => { const mockPerfClient = new MockPerformanceClient(); const correlationId = "test-correlation-id"; @@ -605,9 +701,9 @@ describe("PerformanceClient.spec.ts", () => { const result1 = compactStack(error.stack!, 3); expect(result1.length).toEqual(3); expect(result1).toEqual([ - "at testFunction18 (testFile18.js:10:1)", - "at testFunction19 (testFile19.js:10:1)", - "at testFunction20 (testFile20.js:10:1)", + "at testFunction2 (testFile2.js:10:1)", + "at testFunction3 (testFile3.js:10:1)", + "at testFunction4 (testFile4.js:10:1)", ]); expect(compactStack(error.stack!, -2)).toEqual([]); @@ -620,6 +716,76 @@ describe("PerformanceClient.spec.ts", () => { ).toEqual(["at testFunction (testFile.js:10:1)"]); }); + it("Includes first line if it's a property read error", () => { + let error: Error; + try { + // @ts-ignore + error.test; // This will throw Cannot access property error + throw new Error("This is unexpected"); + } catch (e) { + error = e as Error; + } + + const result1 = compactStack(error.stack!, 3); + expect(result1.length).toEqual(3); + expect(result1[0]).toEqual( + "TypeError: Cannot read properties of undefined (reading 'test')" + ); + }); + + it("Includes first line if it's a property set error", () => { + let error: Error; + try { + // @ts-ignore + error.test = "test"; // This will throw Cannot access property error + throw new Error("This is unexpected"); + } catch (e) { + error = e as Error; + } + + const result1 = compactStack(error.stack!, 3); + expect(result1.length).toEqual(3); + expect(result1[0]).toEqual( + "TypeError: Cannot set properties of undefined (setting 'test')" + ); + }); + + it("Includes first line and redacts if it's a TypeError", () => { + let error = new TypeError("Unable to access 'aribtrary field'"); + + const result1 = compactStack(error.stack!, 1); + expect(result1.length).toEqual(1); + expect(result1[0]).toEqual( + "TypeError: Unable to access " + ); + + let error2 = new TypeError('Unable to access "aribtrary field"'); + + const result2 = compactStack(error2.stack!, 1); + expect(result2.length).toEqual(1); + expect(result2[0]).toEqual( + "TypeError: Unable to access " + ); + }); + + it("Includes first line and redacts if it's a SyntaxError", () => { + let error = new SyntaxError("Unable to access 'aribtrary field'"); + + const result1 = compactStack(error.stack!, 1); + expect(result1.length).toEqual(1); + expect(result1[0]).toEqual( + "SyntaxError: Unable to access " + ); + + let error2 = new SyntaxError('Unable to access "aribtrary field"'); + + const result2 = compactStack(error2.stack!, 1); + expect(result2.length).toEqual(1); + expect(result2[0]).toEqual( + "SyntaxError: Unable to access " + ); + }); + it("handles empty error stack", () => { expect(compactStack("", 3)).toEqual([]); });