Null reference exception at ServerRequestParameters.ts in populateQueryParams

[JasonPaape]

Library

• msal@1.2.2-beta.3

Framework

Found while using Angular 9 with @azure/msal-angular 1.0.0-beta.4 but that's irrelevant, the bug is in msal-core.

Description

There is a null reference bug at msal-core/src/ServerRequestParameters.ts in function 'populateQueryParams' at line 116:
// sanity check for developer passed extraQueryParameters
const eQParams: StringDict = request.extraQueryParameters;
Where 'request' can be null. There is a check above on line 88 'if (request)' but line 116 is outside of that.
This happens when the 'populateQueryParams' is called from msal-core/src/UserAgentApplication.ts
in function 'acquireTokenSilent' at line 644:
serverAuthenticationRequest.populateQueryParams(account, null, adalIdTokenObject);
Note the second param passed is null.
The code in ServerRequestParameters.ts line 116 should have a null check or be moved into the above 'if (request)' block.

Security

No

Regression

Unknown

Configuration

Irrelevant.

Reproduction steps

1. Call acquireTokenSilent with AuthenticationParameters containing values set to flow into this block on line 640 of UserAgentApplication.ts:
   // if user didn't pass login_hint/sid and adal's idtoken is present, extract the login_hint from the adalIdToken
   else if (!account && !StringUtils.isEmpty(adalIdToken)) {
2. See that null gets passed on line 644
3. And that in line 116 of ServerRequestParameters.ts a null exception occurs causing a runtime script exception visible in the console.

Expected behavior

Expect no script errors, and for aquireTokenSilent to function correctly.

Browsers

Not browser specific.

[Porges]

This is the commit which introduces the bug: bf86b89#diff-01b40b5f69b6021ac7031ce85ef0d452L120-L123

The request parameter should be marked as optional, I don’t know how this is even typechecking at the callsite which is passing null?

[jasonnutter]

@Porges It is not getting typechecked because strictNullChecks is disabled (which we are aware of and plan to fix, #1097).

I'll take a look at that PR, thanks!

[tnorling]

@JasonPaape This was included in msal@1.3.0 which was just released. Let us know if this is still an issue. Thanks!