Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] MSAL exceptions should not have PII #3788

Closed
4 tasks
Tracked by #3375
AMaini503 opened this issue Nov 3, 2022 · 3 comments · Fixed by #3865
Closed
4 tasks
Tracked by #3375

[Bug] MSAL exceptions should not have PII #3788

AMaini503 opened this issue Nov 3, 2022 · 3 comments · Fixed by #3865
Assignees
Milestone

Comments

@AMaini503
Copy link

Which version of MSAL.NET are you using?
Assembly Microsoft.Identity.Client, Version=4.47.1.0,

Platform
WPF

What authentication flow has the issue?

  • Desktop
    • Interactive
    • Integrated Windows Authentication
    • Username Password
    • Device code flow (browserless)

Not sure. The app tries to acquire the token silently and AcquireTokenSilent throws an MsalUiRequiredException.

Other?
The issue is that the exception message accessed through System.Exception.Message.ToStirng() contains PII: the email ID of the user.
e.g. (Message: (Could not find a WAM account for the selected user [[email protected]]. Status: AccountNotFound Context: Account with id '(pii)' not found Tag: some-hex-number)).

Some information is scrubbed '(pii)' but not the email id of the user.

Is this a new or existing app?
b. The app is in production, I haven't upgraded MSAL, but started seeing this issue.

Repro

var builder = PublicClientApplicationBuilder.Create(s_clientId)
                    .WithBrokerPreview(true)
                    .WithAuthority(authority)
                    .WithDefaultRedirectUri()
                    .WithLogging(OnMsalLoggingInvoked, LogLevel.Warning, enablePiiLogging: false, enableDefaultPlatformLogging: false);

public class ExceptionLogging
{
        public static string GetExceptionMessage(Exception _ex)
        {
            return $"(Message: ({_ex.Message.ToString()}))";
        }
}


try
{
    AuthenticationResult result = await s_clientApp.AcquireTokenSilent(scope, account)
                        .WithCorrelationId(correlationId)
                        .ExecuteAsync();
    ....
}
catch (MsalUiRequiredException ex)
{
    Log(ExceptionLogging.GetExceptionMessage(ex));
}

Expected behavior
With enablePiiLogging set to false, email ID should be scrubbed in the exception message.

Actual behavior
Email ID is not scrubbed in the exception message.

@bgavrilMS bgavrilMS added this to the 4.49.0 milestone Nov 3, 2022
@bgavrilMS bgavrilMS changed the title [Bug] Is there a way to get a scrubbed exception message from MsalUiRequiredException object/ [Bug] MSAL exceptions should not have PII Nov 3, 2022
@bgavrilMS bgavrilMS added the WAM label Nov 4, 2022
@bgavrilMS
Copy link
Member

Fix is to remove PII from that particular message and to review all other exceptions comming from RuntimeBroker.cs

@AMaini503
Copy link
Author

@bgavrilMS Is your comment addressed to me or to the developers?

Are you saying the PII needs to be filtered by me manually or will this be addressed in MSAL.NET?

@bgavrilMS
Copy link
Member

@AMaini503 - comment is addressed to MSAL developers, not to you. We will fix this.

@gladjohn gladjohn self-assigned this Nov 23, 2022
@gladjohn gladjohn moved this from Triage to In Progress in MSAL Customer Trust / QM Nov 26, 2022
@bgavrilMS bgavrilMS self-assigned this Dec 13, 2022
Repository owner moved this from In Progress to Fixed in MSAL Customer Trust / QM Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants