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] OPTIONS preflight check is beaking the connection #803

Open
sgadot opened this issue Mar 27, 2024 · 5 comments
Open

[Bug] OPTIONS preflight check is beaking the connection #803

sgadot opened this issue Mar 27, 2024 · 5 comments
Labels
Bug Something isn't working, needs an investigation and a fix public-client For questions/issues related to public client apps Requires more info More information is needed, from either the person who opened the issue or another team

Comments

@sgadot
Copy link

sgadot commented Mar 27, 2024

Library version used

1.14.3

Java version

--

Scenario

Other - please specify

Is this a new or an existing app?

The app is in production, I haven't upgraded MSAL, but started seeing this issue

Issue description and reproduction steps

The context :

Third party app using MSAL4J for database Auth is trying to connect to a database using Microsoft JDBC driver.
This has been reproduced using :

  • Knime Analytics platform
  • DBEAVER Comminity edition

Login and navigation has been tested with

  • Brave Version 1.64.109 Chromium: 123.0.6312.58 (Build officiel) (64 bits)
  • Microsoft EDGE Version 123.0.2420.53 (Version officielle) (64 bits)

The flow is as follow

  1. App tries to connect
  2. Opens browser and connects to https://login.microsoftonline.com/common/oauth2/v2.0/authorize?scope=openid+profile+offline_access+https%3A%2F%2Fdatabase.windows.net%2Fuser_impersonation&response_type=code&redirect_uri=http%3A%2F%2Flocalhost%3A51355%2F
  3. User logs in
  4. The browser then does a preflight check to localhost:
    image
    image
  5. the library fails with error:
    ERROR Microsoft Authentication 4:2 com.microsoft.aad.msal4j.MsalClientException: No Authorization code was returned from the server
  6. the POST to localhost fails because socket has been closed

Relevant code snippets

No response

Expected behavior

OPTION Preflight check from the browser should be ignored and it should wait for the real POST payload

Identity provider

Microsoft Entra ID (Work and School accounts and Personal Microsoft accounts)

Regression

No response

Solution and workarounds

Things are OK if the browser does not perform preflight check

@sgadot sgadot added needs attention Automatically used when an issue is created through an issue template untriaged Automatically used when an issue is created through an issue template labels Mar 27, 2024
@Avery-Dunn Avery-Dunn added Bug Something isn't working, needs an investigation and a fix Requires more info More information is needed, from either the person who opened the issue or another team and removed needs attention Automatically used when an issue is created through an issue template untriaged Automatically used when an issue is created through an issue template labels Mar 27, 2024
@Avery-Dunn
Copy link
Collaborator

Avery-Dunn commented Mar 27, 2024

Hello @sgadot : By 'the app is in production', does that mean it had been working for some time and the issue started happening recently? Did it start happening after updating Chrome/Brave/Edge?

We've had some other customers report issues in the latest Chrome due to some overly cautious checks that Chrome considers a bug (#801). However, unlike that issue this preflight check might be an intended change that we'll have to handle in MSAL.

@bgavrilMS bgavrilMS added the public-client For questions/issues related to public client apps label Mar 27, 2024
@localden
Copy link

Tested this on the latest Chrome and cannot repro either:

image

That being said, I did reproduce the issue by halting the app execution before the request is done, and issuing an OPTIONS call against the localhost endpoint (thanks netstat -ab for helping identify the port and process it's running on), which results in a failure. Likely something we need to address (only accept GET with the code)

@sgadot
Copy link
Author

sgadot commented Mar 28, 2024

Hello @sgadot : By 'the app is in production', does that mean it had been working for some time and the issue started happening recently? Did it start happening after updating Chrome/Brave/Edge?

Hi @Avery-Dunn ,

Yes that's it. It was working for some time and suddently stopped working. I havent seen any particular update on my browser, but since it's updating automatically in the background, that does not mean much...

We've had some other customers report issues in the latest Chrome due to some overly cautious checks that Chrome considers a bug (#801). However, unlike that issue this preflight check might be an intended change that we'll have to handle in MSAL.

This looks indeed very similar to #801 and would explain why the issue does not always happen on every computer in my office.

=> I would recommend doing what @localden suggested, even if the problem is not purely a MSAL4J issue, but might rather be an issue with Chrome.

Thanks a lot for your time guys, much appreciated !

@daniel768
Copy link

Hello, we had the same Problem with Microsoft Edge 123. The Version 122 works perfectly. We've done a QuickFix in AuthorizationResponseHandler.handle(HttpExchange httpExchange) and added a check to skip the handling if the request is not "get" or "post" to ignore the new "options" request on / by Edge 123.
String requestMethod = httpExchange.getRequestMethod(); LOG.info("Received request on / path " + requestMethod + " method."); if (!(requestMethod.equalsIgnoreCase("GET") || requestMethod.equalsIgnoreCase("POST"))) { httpExchange.sendResponseHeaders(200, 0); return; }

@localden
Copy link

Thanks folks - we're looking into the issue! We might need to be a bit more explicit about handling HTTP verbs.

@daniel768 @sgadot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working, needs an investigation and a fix public-client For questions/issues related to public client apps Requires more info More information is needed, from either the person who opened the issue or another team
Projects
None yet
Development

No branches or pull requests

5 participants