Add flighted validation in WebView intent handling#3170
Open
cacosta33 wants to merge 4 commits into
Open
Conversation
Introduces a flight-gated validation step in the AAD WebView client's intent handling path. Default on; the flight provides a rollback path. Work item: https://dev.azure.com/IdentityDivision/Engineering/_workitems/edit/3674205 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a flight-gated hardening step to the AAD WebView client’s broker-install intent:// handling so that (when enabled) only Google Play Store intents are launched, while preserving legacy behavior when the flight is disabled.
Changes:
- Added a new ECS-controlled flight
ENABLE_BROKER_INSTALL_INTENT_VALIDATION(default OFF). - In the
intent://broker-install handling path (flight ON only): clear explicit component/selector and block non-Play-Store target packages before launching. - Expanded unit test coverage for allow-list enforcement, component clearing, and legacy behavior; fixed a previously non-executing test by adding
@Test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java | Adds the new flight definition (default OFF) to gate the new validation logic. |
| common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java | Implements the flighted post-parse validation (clear component/selector + allow-list com.android.vending) before startActivity. |
| common/src/test/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClientTest.java | Adds/updates intent-path tests to validate allow-list behavior, component clearing, and rollback behavior when flight is OFF. |
Comment on lines
+595
to
602
| private void setBrokerInstallIntentValidationFlight(final boolean enabled) { | ||
| final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class); | ||
| when(mockFlightsProvider.isFlightEnabled(CommonFlight.ENABLE_BROKER_INSTALL_INTENT_VALIDATION)) | ||
| .thenReturn(enabled); | ||
| final MockCommonFlightsManager mockCommonFlightsManager = new MockCommonFlightsManager(); | ||
| mockCommonFlightsManager.setMockCommonFlightsProvider(mockFlightsProvider); | ||
| CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockCommonFlightsManager); | ||
| } |
…idation Emits an OpenTelemetry span only when ENABLE_BROKER_INSTALL_INTENT_VALIDATION is on so the fix outcome (launched / blocked / error) can be confirmed in telemetry. Zero behavior change while the flight is off; all span usage is null-guarded. Adds one attribute (is_broker_install_intent_blocked) and a changelog entry. AB#3674205 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restructure processIntentToInstallBrokerApp so the flight-off path is the original method (unchanged), and all validation + telemetry lives inside the flight-on branch with a single generic catch(Throwable). This removes the scattered null-guards and keeps zero behavior/telemetry impact when the flight is off. AB#3674205 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve conflicts in changelog.txt, AzureActiveDirectoryWebViewClient.java, and AzureActiveDirectoryWebViewClientTest.java. Keep the flight-off path byte-for-byte identical to dev, preserve dev's onboarding telemetry hook (recordOnboardingStep before the flight check), and retain the flight-gated broker-install intent validation + span from PR #3170. AB#3674205 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a flight-gated validation step to the AAD WebView client's broker-install
intent://handling path. When the flight is enabled, the parsed intent's component/selector are cleared and its target package must be the Google Play Store before the activity is launched. When the flight is disabled, behavior is byte-for-byte identical to today (kill-switch / rollback).Work item: https://dev.azure.com/IdentityDivision/Engineering/_workitems/edit/3674205
Feature flag & rollout
EnableBrokerInstallIntentValidation(CommonFlight), default OFF.commonships to broker + MSAL + downstream consumers, so this is intended to ramp progressively (e.g. 1% then 10% then 100%) while watching install-flow success telemetry.Changes
CommonFlight.java— newENABLE_BROKER_INSTALL_INTENT_VALIDATIONflight (defaultfalse).AzureActiveDirectoryWebViewClient.java— inside the flighted branch only: clear any explicit component/selector on the parsed intent and require the resolved package to be on the allow-list (com.android.vending) beforestartActivity.@Test(so it never ran).Validation
:common:testLocalDebugUnitTestfor the affected client passed (BUILD SUCCESSFUL), all tests pass (4 intent-path tests confirmed executing).Do not close the linked work item until confirmed
A couple of points cannot be verified from this library's code alone and may affect priority — please confirm before resolving the linked item (the code hardening here is safe to merge/ramp regardless):
Notes for reviewers
OneAuthSharedFunctionsor any 1P IPC surface.