-
Notifications
You must be signed in to change notification settings - Fork 46
Updating handling of ssl error received in Android WebView's onReceivedSslError callback, Fixes AB#3268908 #2691
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
Conversation
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the handling of SSL errors in Android WebView to ensure that non-critical SSL errors (from sub-resources) do not disrupt the authentication flow. The key changes include introducing a new flight toggle and intent extra to control flow preservation on SSL errors, modifying the WebViewClient implementations to conditionally propagate SSL error responses, and updating tests to cover these new behaviors.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java | Added new flight constant for preserving WebView flow on SSL errors |
| common/src/test/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClientTest.java | Updated tests to verify legacy and new SSL error handling scenarios |
| common/src/test/java/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragmentTest.kt | Added tests to validate extracting and applying the new intent flag |
| common/src/main/java/com/microsoft/identity/common/internal/ui/webview/OAuth2WebViewClient.java | Introduced a flag and conditional error callback in SSL error handling |
| common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java | Updated constructors to support the new SSL error flow preservation flag |
| common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java | Enhanced state extraction to handle the new SSL error flag |
| common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java | Added an intent key constant for preserving the WebView flow on SSL error |
| changelog.txt | Recorded the minor update in the change log |
| final SslError error) { | ||
| // Developer does not have option to control this for now | ||
| super.onReceivedSslError(view, handler, error); | ||
| handler.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add broker telemetry to track if SSL error was seen in web view flow.
For OneAuth, this needs different wiring which I don't see we currently have. So, that would be separate change.
As we discussed, I will first rollout this for broker and keep disabled for OneAuth and MSAL Android. Once we have seen that there's no change in issues in broker. We will expand change to OneAuth. Meanwhile, I can work with OneAuth how can we track this in OneAuth telemetry. This may be non-trivial work.
| super.onReceivedSslError(view, handler, error); | ||
| handler.cancel(); | ||
|
|
||
| final String errMsg = "Received SSL Error during request. For more info see: " + SSL_HELP_URL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the OneAuth path it would be good to capture that an TLS error occurred and if the flow ended-up failing (like when the SSL error is for the top level domain not an iframe) we would be in OneAuth a UserCancelException + an error code indicating that there was a TLS error in the mix that we can log and monitor through MATS.
Please work with @nickbopp on the exact telemetry contract with OneAuth.
| mCompletionCallback.onChallengeResponseReceived( | ||
| RawAuthorizationResult.fromException( | ||
| new ClientException("Code:" + ERROR_FAILED_SSL_HANDSHAKE, error.toString()))); | ||
| if (!mShouldPreserveFlowOnSslError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I agree with your stand that going with a simpler solution where we don't need to track and match URLs and RU to identify the top level domain is preferable.
Let's just ensure we have telemetry to differentiate between user cancellations that will happen because of TLS + rest cases.
common/src/main/java/com/microsoft/identity/common/internal/ui/webview/OAuth2WebViewClient.java
Outdated
Show resolved
Hide resolved
|
So, I guess we are not doing WEB_VIEW_NEW_SSL_ERROR_HANDLER_ENABLED being passed from OneAuth for opt-in or opt-out in this PR? If we are not, then let's update the PR description with the chosen option? |
Updated |
| final SslError error) { | ||
| // Developer does not have option to control this for now | ||
| final String methodTag = TAG + ":onReceivedSslError"; | ||
| super.onReceivedSslError(view, handler, error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see 'cancel' being called anymore in the flight-off scenario. But it seems like tests are validating that it is. Is the job getting done super method invocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, super calls it hence removed.
Update:
After talking to @iulico-1 from OneAuth.
=====================
Fixes AB#3268908
Summary:
This PR updates the handling of SSL errors in the Android WebView used for authentication. The change is in response to an incident where the MSA sign-up flow failed due to an expired certificate on an MSA UX web page. The expired certificate was associated with a resource loaded by the page; however, the existing SSL error handling in Common’s WebView would cancel the loading of that resource and subsequently terminate the entire authentication flow. (See PBI for details.)
Solution
Option 1
We simply log and stop loading the resource that caused the error by calling
SssHandler.cancel()but not stop the flow by erroring out. This is a simple change. The only thing is the main URL (the url loading in the WebView) itself has SSL error then the flow would end up in blank page. This is as bad as finishing the flow but does not inform calling app of the error via exception.But if sub-resource has the error, then the flow continues.
Flighting
New SSL Error Handler:
Adds a new intent key (. The toggle would be added later if required. This simplifies current logic to only rely on Common/broker's flighting.WEB_VIEW_NEW_SSL_ERROR_HANDLER_ENABLED) inAuthenticationConstants.javato control whether the new SSL error handler logic is enabled. This can be used by OneAuth to opt-in or opt-out. If the key is not passed, then Common's default logic based on flighting will be used. If the key passed, that will be used (primarily meant for OneAuth/MSALCPP).false. So, OneAuth and MSAL would also use default flight value and continue using current behavior. For Broker, we would enable new flow with defaulttrue(https://github.com/AzureAD/ad-accounts-for-android/pull/3141), which can be controlled remotely to turn off if required.Option 2 (Not this PR, added for reference)
The objective of this update is to improve SSL error handling so that the flow is only cancelled if an SSL error occurs on the main frame resource (the primary page being loaded). If an SSL error affects a sub-resource, only the loading of that specific resource is cancelled, and the overall authentication flow continues. This approach helps prevent unnecessary failures caused by SSL errors on secondary or non-critical resources. (#2688)
The con of the approach is client needs to track the active url that's about and being loaded in WebView. The tracking adds cost of developing and maintaining to solution without significant benefit over option 1.