-
Notifications
You must be signed in to change notification settings - Fork 46
Add AAD authority validation for DUNA flow, Fixes AB#3282191 #2740
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. |
|
✅ Work item link check complete. Description contains link AB#3282191 to an Azure Boards work item. |
|
❌ Work item link check failed. Description contains AB#3282191 but the Bot could not link it to an Azure Boards work item.Click here to learn more. |
…/webview/switchbrowser/SwitchBrowserUriHelper.kt Co-authored-by: Copilot <[email protected]>
shahzaibj
left a comment
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.
Not clear from the code if you are validating action_uri or the overall URI that we receive from eSTS. The ask is validate action_uri. Can you confirm the code is doing that?
...va/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt
Outdated
Show resolved
Hide resolved
...va/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt
Outdated
Show resolved
Hide resolved
shahzaibj
left a comment
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 think what you've done is fine, though I think it would've been cleaner had you just validated the action_uri as opposed to validating complete process/resume URI
…ment action URI validation
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 adds security validation for the DUNA (Device-based User-to-Native-App) flow by implementing Azure Active Directory (AAD) authority validation. The change prevents potential phishing attacks by validating that action URIs in switch browser operations come from trusted Microsoft authorities.
- Adds AAD authority validation to prevent malicious apps from exploiting the DUNA flow
- Updates test cases to use valid Microsoft authorities instead of test domains
- Implements proper mock cleanup in test teardown methods
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SwitchBrowserUriHelper.kt | Adds validateActionUri method to check action URIs against trusted AAD authorities |
| SwitchBrowserUriHelperTest.kt | Updates test to properly mock state validation and adds teardown cleanup |
| SwitchBrowserProtocolCoordinatorTest.kt | Updates test URLs to use Microsoft domains and adds validation tests for invalid authorities |
| SwitchBrowserRequestHandlerTest.kt | Updates test URLs from example.com to login.microsoft.com |
| changelog.txt | Documents the security enhancement |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...va/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt
Outdated
Show resolved
Hide resolved
...va/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt
Outdated
Show resolved
Hide resolved
...va/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt
Outdated
Show resolved
Hide resolved
…/webview/switchbrowser/SwitchBrowserUriHelper.kt Co-authored-by: Copilot <[email protected]>
tweaked it to validate the action URL when we get the raw data. Also reused the AAD logic to keep it simple. |
AB#3282191
Issue
In the current DUNA flow, both
switch_browserandswitch_browser_resumeactions can be triggered via a native app URL scheme, which introduces a potential security vulnerability, which can be exploited by a malicious app to render a phishing page that mimics the ESTS UI and captures user credentials.Resolution
To mitigate this risk, we validate that the “action_uri” belongs to Microsoft by ensuring its host is included in our list of trusted authorities. This can be done by checking the “host” dictionary returned during authority discovery in AzureActiveDirectory