Skip to content

Algo 1/fix vulnerabilities#105

Merged
algo-1 merged 12 commits intomainfrom
algo-1/fix-vulnerabilities
Aug 15, 2024
Merged

Algo 1/fix vulnerabilities#105
algo-1 merged 12 commits intomainfrom
algo-1/fix-vulnerabilities

Conversation

@algo-1
Copy link
Copy Markdown
Contributor

@algo-1 algo-1 commented Aug 1, 2024

Purpose

Fix vulnerabilities in project dependencies

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ x] Other... Please describe: Update dependencies to fix vulnerabilities

How to Test

  • Get the code
git clone [repo-address]
cd [repo-name]
git checkout [branch-name]

For each sample, 
cd into the sample directory and follow the Readme instructions

Other Information

The bug fix is in ms-identity-ciam-javascript-tutorial/2-Authorization/0-call-api-vanillajs, the msal-browser version used in the index.html page does not fully support CIAM (Entra External ID) users. In this PR, msal.min.js is served from the backend and used instead similar to how it is done in ms-identity-ciam-javascript-tutorial/1-Authentication/0-sign-in-vanillajs.

@algo-1 algo-1 marked this pull request as ready for review August 7, 2024 15:13
// configuration parameters are located at authConfig.js
const myMSALObj = new msal.PublicClientApplication(msalConfig);

myMSALObj.initialize().then(() => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the correct copy/paste from the redirect changes to support popup?

Copy link
Copy Markdown
Contributor Author

@algo-1 algo-1 Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is, this is how it is also done in 1-Authorization/0-call-api-vanillajs/App/public/authPopup.js

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this isn't needed for popup flows. @sameerag are the samples in the MSAL.js repo all correct? If so we might need a pass at the azure samples as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested it now for popup without initializing and the error is:

authPopup.js:84  BrowserAuthError: uninitialized_public_client_application: You must call and await the initialize function before attempting to call any other MSAL API.  For more visit: aka.ms/msaljs/browser-errors
    at gi (msal-browser.min.js:69:17422)
    at Ea (msal-browser.min.js:69:22240)
    at Ra (msal-browser.min.js:69:22277)
    at js (msal-browser.min.js:69:125646)
    at Vs.acquireTokenPopup (msal-browser.min.js:69:132964)
    at Vs.loginPopup (msal-browser.min.js:69:145094)
    at tc.loginPopup (msal-browser.min.js:69:162444)
    at signIn (authPopup.js:81:15)
    at HTMLButtonElement.onclick ((index):23:92)

Copy link
Copy Markdown

@sameerag sameerag Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need initialize() for any 2.x usage. Which MSAL version is in use here?

Copy link
Copy Markdown

@sameerag sameerag Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a closer look, if you want to avoid initialize() call, please use createPublicClientApplication() instead. Docs here

And for the original q, for only popup we do not need handleRedirectPromise() at all. We add that for redirect APIs, on page load to check if this is a reload after an auth request. Our samples traditionally support all APIs, including popup and redirect in the same file, hence we probably have this as a template code in our browser samples.

If you are separating it, please get rid of any handleRedirectPromise() reference and recommend using createPublicClientApplication() so you can also avoid the initialize() call.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The versions of msal-browser are 3.16.0 and 3.17.0 for the 1-authorization & 2-Authorization vanilla js apps respectively.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a preference. My main goal is the security vulnerabilities so I aligned the implementation with what was there previously. Would you like me to avoid the initialize() call?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend using createPublicClientApplication() as a pattern, we are moving towards it and will deprecate new PublicClientApplication() potentially in 4.x.

And as for handleRedirectPromise() you will not need it if it is only popUp.

@algo-1 algo-1 requested a review from peterzenz August 12, 2024 08:55
@algo-1 algo-1 requested a review from sameerag August 14, 2024 19:52
@algo-1 algo-1 removed the request for review from peterzenz August 15, 2024 08:26
@algo-1 algo-1 merged commit 1d271cc into main Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants