Add flighted validation in WebView intent handling, Fixes AB#3674205#3170
Open
cacosta33 wants to merge 11 commits into
Open
Add flighted validation in WebView intent handling, Fixes AB#3674205#3170cacosta33 wants to merge 11 commits into
cacosta33 wants to merge 11 commits into
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. |
…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. |
Restructure processIntentToInstallBrokerApp so the flight branch reads as an
explicit positive gate for reviewers:
if (ENABLE_BROKER_INSTALL_INTENT_VALIDATION) { NEW validation + span }
else { original dev try/catch, byte-for-byte unchanged }
Previously the method used a negated early-return
(if (!flag) { old; return; }) followed by the new code below, which moved the
original code and made the diff harder to review. The else branch now preserves
dev's original behavior verbatim (only re-indented), so flight-OFF behavior is
provably identical and all new, flighted logic/telemetry is isolated in the if
branch. No behavior change; small duplication of the original try/catch is
intentional for reviewer clarity.
AB#3674205
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cacosta33
changed the title
|
✅ Work item link check complete. Description contains link AB#3674205 to an Azure Boards work item. |
melissaahn
approved these changes
Jul 16, 2026
- Default unstubbed flights to their real default value in the WebView intent-validation test helper so tests don't silently depend on Mockito's boolean default as the WebViewClient evolves (Copilot review comment). - Reword the #3170 changelog entry to describe the security behavior (allow-list validation of broker-install intent targets) rather than only the telemetry span (melissaahn review comment). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
shahzaibj
reviewed
Jul 17, 2026
shahzaibj
reviewed
Jul 17, 2026
… span, intent hardening, selector tests - Build the broker-install gate substring from GOOGLE_PLAY_STORE_PACKAGE_NAME so the URL gate and the allow-list check can't drift (thread #2). - Remove the dedicated ProcessBrokerInstallIntent child span; emit is_broker_install_intent_blocked on the current WebView-processing span and drop the now-unused SpanName entry (thread #3). - Extract sanitizeAndValidateBrokerInstallIntent(): clear component/selector, allow-list the package, strip FLAG_GRANT_*_URI_PERMISSION and add CATEGORY_BROWSABLE (thread #4). - Add direct unit tests for the sanitizer covering selector clearing + block, non-allow-listed package block, and grant-flag stripping + CATEGORY_BROWSABLE (thread #1). - Update changelog and AttributeName doc; note broker-repo sync requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
shahzaibj
approved these changes
Jul 21, 2026
…Name sync note - Extract the flight-ON and flight-OFF bodies of processIntentToInstallBrokerApp into launchValidatedBrokerInstallIntent(...) and launchBrokerInstallIntentLegacy(...), leaving the original method as a thin flight dispatcher (shahzaibj nits). - Correct the is_broker_install_intent_blocked sync note: the broker repo is identity-authnz-teams/ad-accounts-for-android, and clarify the mirror is functionally required (broker exporter drops unresolved attribute names). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restore the original processIntentToInstallBrokerApp body verbatim for the flight-OFF path (gated by an early return) so the diff shows no changes to the old flow. The new validated behavior stays isolated in launchValidatedBrokerInstallIntent(...); drop the redundant legacy wrapper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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):
AB#3674205
Notes for reviewers
OneAuthSharedFunctionsor any 1P IPC surface.