Handle MsalRuntime Error Surfacing #8120
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances error handling for MSAL Runtime (native broker) errors by introducing a new NativeAuthError class and attaching it to wrapped MSAL.js errors. The key purpose is to preserve detailed broker error information (status code, tag) when native broker errors are converted to standard MSAL errors.
Key changes:
- Introduces
NativeAuthErrorclass inmsal-commonto preserve broker-specific error details (status code and tag) - Adds
msalNodeRuntimeErrorproperty toAuthErrorbase class to hold the original native error - Modifies error wrapping logic in
NativeBrokerPluginto attachNativeAuthErrorinstances to all wrapped errors
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/msal-common/src/exports-common.ts | Exports the new NativeAuthError class from msal-common package |
| lib/msal-common/src/error/NativeAuthError.ts | Defines new error class that extends AuthError with broker-specific properties (statusCode, tag) |
| lib/msal-common/src/error/AuthError.ts | Adds optional msalNodeRuntimeError property to store the original native broker error |
| extensions/msal-node-extensions/src/broker/NativeBrokerPlugin.ts | Refactors error wrapping to create NativeAuthError once and attach it to all wrapped errors via msalNodeRuntimeError property |
| extensions/msal-node-extensions/test/broker/NativeBrokerPlugin.spec.ts | Adds test to verify msalNodeRuntimeError is properly attached to wrapped errors |
| /** | ||
| * Default NativeAuthError from MsalNodeRuntime when broker is enabled | ||
| */ | ||
| msalNodeRuntimeError?: NativeAuthError; |
There was a problem hiding this comment.
Let's make sure this is general enough to encompass our browser scenarios as well
| msalNodeRuntimeError?: NativeAuthError; | |
| platformBrokerError?: PlatformBrokerError; |
| /** | ||
| * Error class for MSAL Runtime errors that preserves detailed broker information | ||
| */ | ||
| export class NativeAuthError extends AuthError { |
There was a problem hiding this comment.
Let's rename this to PlatformBrokerError as we've had a bit of a naming conflict with the CIAM team for Native Auth
| */ | ||
|
|
||
| export class StringUtils { | ||
| export class NativeBrokerStringUtils { |
There was a problem hiding this comment.
Let's include this function in the PlatformBrokerError file as a standalone function rather than a static function on a class.
| return new NativeAuthError( | ||
| wrappedError = nativeAuthError; | ||
| // Clone error to avoid circular reference | ||
| const clonedError = new NativeAuthError( |
There was a problem hiding this comment.
Rather than wrapping a broker error inside a broker error let's use a ClientAuthError here as the outer error and you can give it a code of either "unknown" or "broker_error"
| const tagString = NativeBrokerStringUtils.tagToString(errorTag); | ||
| const enhancedErrorContext = errorContext | ||
| ? `${errorContext} (Error Code: ${errorCode}, Tag: ${tagString})` | ||
| : `(Error Code: ${errorCode}, Tag: ${tagString})`; |
There was a problem hiding this comment.
Can we do this stuff in the PlatformBrokerError constructor instead? Then you don't need to export the Util function at all.
Aimed to solve the problem discussed here:
Solution 5 implementation