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] HTTP Telemetry sends invalid data through HTTP Headers, causing MSAL to crash #1881

Closed
ssahon opened this issue Jun 11, 2020 · 20 comments · Fixed by #2209
Closed

[Bug] HTTP Telemetry sends invalid data through HTTP Headers, causing MSAL to crash #1881

ssahon opened this issue Jun 11, 2020 · 20 comments · Fixed by #2209
Assignees
Milestone

Comments

@ssahon
Copy link

ssahon commented Jun 11, 2020

Which Version of MSAL are you using ?
Microsoft.Identity.Client 4.14.0

Platform
.net core 3.1 wpf

What authentication flow has the issue?

  • Desktop / Mobile
    Interactive

Is this a new or existing app?
c. This is a new app or experiment

Repro
I've updated microsoft sample sample to work with b2c in .net core wpf application.
there was no reset password implementation so I added this functionality. It basically checks error code during signin and runs reset policy.
If I run reset flow I have weird error at the end related to http header processing

the only workaround to clean up publicClientApplication and signIn again

image

@bgavrilMS
Copy link
Member

Looks like smth wrong with the headers. Anyway you can get hold of a network trace (Fiddler would be great) to inspect the headers?

@ssahon
Copy link
Author

ssahon commented Jun 11, 2020

here it is
b2c reset pass 2.txt

@neha-bhargava
Copy link
Contributor

@ssahon The fiddler trace does not seem to indicate the problem. Did you look at https://github.com/Azure-Samples/active-directory-b2c-dotnet-desktop/blob/133a352aed5ea9c30cf039ec88785c109c385b69/active-directory-b2c-wpf/MainWindow.xaml.cs#L43 where the sample is handling reset password. I was able to use the sample to reset password.

@trwalke
Copy link
Member

trwalke commented Sep 10, 2020

Please provide more info and reopen @ssahon

@akiratoda
Copy link

I have same issue.

  • Which Version of MSAL are you using ?
    Microsoft.Identity.Client 4.22.0
    Microsoft.Identity.Client.Extensions.Msal 2.16.5
    (I use ICustomWebUi)

  • Platform
    .net core 3.1 wpf

What authentication flow has the issue?

  • Desktop / Mobile
    Interactive

My code is almost same as sample code.

_pca = PublicClientApplicationBuilder.Create(B2CConstants.ClientID)
    .WithRedirectUri(B2CConstants.RedirectURI)
    .WithTenantId(B2CConstants.Tenant)
    .WithB2CAuthority(B2CConstants.AuthoritySignIn)
    .Build();
if (ex.ErrorCode.Contains("AADB2C90118"))
{
    try
    {
        AuthenticationResult authResult = await _pca.AcquireTokenInteractive(B2CConstants.Scopes) //Scopes = {"offline_access"}
            .WithPrompt(Prompt.NoPrompt)
            .WithB2CAuthority(B2CConstants.AuthorityResetPassword)
            .WithCustomWebUi(customWebUi)
            .ExecuteAsync();
        return;
    }
    catch (MsalException)
    {
        return;
    }
    catch (FormatException)
    {
        //Error occur
        return;
    }
}

Signin flow has no exception.
But ResetPassword flow gets FormatException with massage "New-line characters in header values must be followed by a white-space character." after we enter new passwaord and push [Continue] button.

After password was changed, we also gets FormatException with same message in Singin flow.
But I can signin using with new password after I relaunch application.
It means password change succeed although exception occured.

Thank you.

@bgavrilMS
Copy link
Member

@akiratoda - do you have the exception stack trace and a Fiddler or some network trace?

@akiratoda
Copy link

@bgavrilMS
Here it is.
stacktrace.txt
fiddler.txt

@bgavrilMS
Copy link
Member

Thanks for the details @akiratoda (and very cool that you're using ICustomWebUi succesfully).

I can't figure out which header value is causing this problem. The code is here:

Would you be able to debug into MSAL and observe which header value is the culprit? We use SourceLink.GitHub so you should be able to debug into MSAL directly (e.g. disable "Just My Code" in VS) without having to clone the repo and build MSAL yourself.

@ssahon
Copy link
Author

ssahon commented Nov 17, 2020

the error happens in HttpManager while trying to CreateRequestMessage
it throws format exception on index 4 of headers collection (x-client-last-telemetry)
headers
codeWithError

@bgavrilMS bgavrilMS added bug and removed Investigate labels Nov 17, 2020
@bgavrilMS bgavrilMS changed the title [Bug] new-line characters in header values must be followed by a white-space character [Bug] HTTP Telemetry sends invalid data through HTTP Headers, causing MSAL to crash Nov 17, 2020
@bgavrilMS
Copy link
Member

bgavrilMS commented Nov 17, 2020

Very cool, that's a bug in MSAL (I updated the title), thanks.
We need to sanitize the error codes before sending them in telemetry headers.

@bgavrilMS bgavrilMS added the P1 label Nov 17, 2020
@bgavrilMS bgavrilMS added this to the 4.23.0 milestone Nov 17, 2020
@bgavrilMS bgavrilMS self-assigned this Nov 18, 2020
@bgavrilMS
Copy link
Member

Is this the "next gen" password reset policy which is in preview? That error code is wrong, error codes are short, like "access_denied" or "invalid_grant". I'll sanitize this string so that it can go in the header, but we need to talk to B2C team to fix this error code.

Since the flow is in preview, I expect them to change it. You should not depend on this string.

@ssahon
Copy link
Author

ssahon commented Nov 18, 2020

I do not think so, I used b2c custom policy with almost no modifications, I do not think that I can control those error messages.

@bgavrilMS
Copy link
Member

No, you can't control these messages. There's a new userflow for reset password which I think is has a few issues, see AzureAD/microsoft-identity-web#729 (comment)

@ssahon
Copy link
Author

ssahon commented Nov 18, 2020

no, I do not use this standard user flows. I use xml based custom b2c policies. https://docs.microsoft.com/en-us/azure/active-directory-b2c/custom-policy-overview

@bgavrilMS
Copy link
Member

I have a fix, but I will also talk to B2C team to see if we can get that error code changed. I'll update the thread either way, but the fix will anyway be shipped with MSAL 4.23, probably end of week next week.

@bgavrilMS
Copy link
Member

We're still trying to track down why that error code is a full blown sentence. @akiratoda @ssahon - would it be possible to capture the authorization URI ? It looks like this:

http[s]://<your_redirect_uri>/#error=access_denied&error_description=AADB2C90118%3a+The+user+has+forgotten+their+password.%0d%0aCorrelation+ID%3a+ac553bd9-2031-4a50-9b39-f638a7cc06de%0d%0aTimestamp%3a+2020-11-19+22%3a33%3a19Z%0d%0a

It is when the browser finishes the flow. The current stack trace does not have it.

@ssahon
Copy link
Author

ssahon commented Nov 20, 2020

from fiddler:
GET /oauth2/nativeclient?error=access_denied&error_description=AADB2C90118%3a+The+user+has+forgotten+their+password.%0d%0aCorrelation+ID%3a+fd6ec535-3877-46c0-8d5a-d6e3221e3e95%0d%0aTimestamp%3a+2020-11-20+14%3a25%3a22Z%0d%0a&state=04f1e84c-dff0-46ed-9acf-09f97bbfef0e98960226-a3c8-40a6-83e4-3794684494c8 HTTP/1.1

this url is used before the error.

in my case it shows UI to verify email, then UI to assign new password only after changing password I see the error

@bgavrilMS
Copy link
Member

Interesting. And the MsalException in your first screenshot shows the ErrorCode as being "The user has forggoten their password..."?

@ssahon
Copy link
Author

ssahon commented Nov 20, 2020

new fiddler traces. As you can see there additional requests after your url
reset_password - modified 2.zip
error happens when it tries to inject wrong value into header. Yes this is reset password flow (in my case custom policy) but it happens not immediately after user clicks the forgot password link, there are additional user interactions

@ssahon
Copy link
Author

ssahon commented Nov 24, 2020

I have a fix, but I will also talk to B2C team to see if we can get that error code changed. I'll update the thread either way, but the fix will anyway be shipped with MSAL 4.23, probably end of week next week.

ok it fixed http header issue but now I have error "no account or login hint was passed" after reset password while calling AcquireTokenSilent

I checked claims after signin and after reset password they look the same

should I create new bug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants