Address Bar Spoofing Test Cases + Remediation (#3882)

.github/workflows/end-to-end.yml

@@ -74,6 +74,17 @@ jobs:
           workspace: .maestro
           include-tags: privacyTest
 
+      - name: Security Tests
+        if: always()
+        uses: mobile-dev-inc/action-maestro-cloud@v1.4.1
+        with:
+          api-key: ${{ secrets.MOBILE_DEV_API_KEY }}
+          name: ${{ github.sha }}
+          app-file: apk/release.apk
+          android-api-level: 30
+          workspace: .maestro
+          include-tags: securityTest
+
       - name: Release Tests
         if: always()
         uses: mobile-dev-inc/action-maestro-cloud@v1.4.1

.maestro/security_tests/1_-_AddressBarSpoof,_basicauth.yaml

@@ -0,0 +1,41 @@
+appId: com.duckduckgo.mobile.android
+tags:
+    - securityTest
+---
+- launchApp:
+    clearState: true
+- runFlow: ../shared/onboarding.yaml
+- doubleTapOn:
+    id: "omnibarTextInput"
+- pressKey: Backspace
+# Test 1 - using \u2028 character
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-basicauth-2028.html"
+- pressKey: Enter
+- tapOn: "Got It"
+- tapOn: "run"
+- assertVisible: "Example Domain"
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText.indexOf("https://www.google.com") != 0}
+- tapOn:
+    id: "omnibarTextInput"
+# Test 2 - using \u2029 character
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-basicauth-2029.html"
+- pressKey: Enter
+- tapOn: "run"
+- assertVisible: "Example Domain"
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText.indexOf("https://www.google.com") != 0}
+- tapOn:
+    id: "omnibarTextInput"
+# Test 3 - using repeated " " space character
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-basicauth-whitespace.html"
+- pressKey: Enter
+- tapOn: "run"
+- assertVisible: "Example Domain"
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText.indexOf("https://www.google.com") != 0}
+- tapOn:
+    id: "omnibarTextInput"

.maestro/security_tests/2_-_AddressBarSpoof,_aboutblank.yaml

@@ -0,0 +1,21 @@
+appId: com.duckduckgo.mobile.android
+tags:
+    - securityTest
+---
+- launchApp:
+    clearState: true
+- runFlow: ../shared/onboarding.yaml
+- doubleTapOn:
+    id: "omnibarTextInput"
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-about-blank-rewrite.html"
+- pressKey: Enter
+- tapOn: "Got It"
+- tapOn: "Start"
+# This test is expected to load "about:blank" then duckduckgo.com, not remain on the current page with spoofed content.
+- extendedWaitUntil:
+    notVisible: "Not DDG."  # Spoofed content not visible
+    timeout: 10000
+- tapOn: "Phew!"
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText == "about:blank" || maestro.copiedText == "https://duckduckgo.com/"}

.maestro/security_tests/3_-_AddressBarSpoof,_appschemes.yaml

@@ -0,0 +1,30 @@
+appId: com.duckduckgo.mobile.android
+tags:
+    - securityTest
+---
+- launchApp:
+    clearState: true
+- runFlow: ../shared/onboarding.yaml
+# Test 1
+- doubleTapOn:
+    id: "omnibarTextInput"
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-application-scheme.html"
+- pressKey: Enter
+- tapOn: "Got It"
+- tapOn: "Start"
+- tapOn: "Phew!"
+# This test should reject trying to load
+# This test is expected to load duckduckgo.com, not remain on the current page with spoofed content.
+- assertVisible: "Privacy, simplified." # DuckDuckGo home page
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText == "https://duckduckgo.com/"} # DuckDuckGo home page
+- tapOn:
+    id: "omnibarTextInput"
+# Test 2
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-unsupported-scheme.html"
+- pressKey: Enter
+- tapOn: "Start"
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText == "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-unsupported-scheme.html"}

.maestro/security_tests/4_-_AddressBarSpoof,_b64_html.yaml

@@ -0,0 +1,20 @@
+appId: com.duckduckgo.mobile.android
+tags:
+    - securityTest
+---
+- launchApp:
+    clearState: true
+- runFlow: ../shared/onboarding.yaml
+# Test 1
+- doubleTapOn:
+    id: "omnibarTextInput"
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-open-b64-html.html"
+- pressKey: Enter
+- tapOn: "Got It"
+- tapOn: "Start"
+# This test is expected to open a new tab with empty origin ("") and then prompt to open the link in another app
+- assertVisible: "Open in another app"
+- tapOn: "Cancel"
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText.indexOf("duckduckgo.com") == -1}

.maestro/security_tests/5_-_AddressBarSpoof,_downloadpath.yaml

@@ -0,0 +1,33 @@
+appId: com.duckduckgo.mobile.android
+tags:
+    - securityTest
+---
+- launchApp:
+    clearState: true
+- runFlow: ../shared/onboarding.yaml
+# Test 1
+- doubleTapOn:
+    id: "omnibarTextInput"
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-js-download-url.html"
+- pressKey: Enter
+- tapOn: "Got It"
+- tapOn: "Start"
+# Download Acceptance Flow:
+- extendedWaitUntil:
+    visible: "Save to Downloads"
+    timeout: 10000
+- tapOn: "Save to Downloads"
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText == "Search or type URL"} # Downloads should occur in empty origin.
+- pressKey: Back
+# Download Cancel Flow:
+- tapOn: "Start"
+- extendedWaitUntil:
+    visible: "Cancel"  
+    timeout: 10000
+- tapOn: "Cancel"
+# Should redirect back to the last page.
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText == "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-js-download-url.html"} 

.maestro/security_tests/6_-_AddressBarSpoof,_formaction.yaml

@@ -0,0 +1,18 @@
+appId: com.duckduckgo.mobile.android
+tags:
+    - securityTest
+---
+- launchApp:
+    clearState: true
+- runFlow: ../shared/onboarding.yaml
+# Test 1
+- doubleTapOn:
+    id: "omnibarTextInput"
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-form-action.html"
+- pressKey: Enter
+- tapOn: "Got It"
+- tapOn: "run"
+# Should do nothing - the navigation should be prevented.
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText == "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-form-action.html"}

.maestro/security_tests/7_-_AddressBarSpoof,_pagerewrite.yaml

@@ -0,0 +1,19 @@
+appId: com.duckduckgo.mobile.android
+tags:
+    - securityTest
+---
+- launchApp:
+    clearState: true
+- runFlow: ../shared/onboarding.yaml
+# Test 1
+- doubleTapOn:
+    id: "omnibarTextInput"
+- inputText: "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-js-page-rewrite.html"
+- pressKey: Enter
+- tapOn: "Got It"
+- tapOn: "Start"
+# Now check the address bar hasn't been updated too early resulting in spoofed content
+- copyTextFrom:
+    id: "omnibarTextInput"
+- assertTrue: ${maestro.copiedText == "https://privacy-test-pages.site/security/address-bar-spoofing/spoof-js-page-rewrite.html"}
+- assertNotVisible: "DDG." 

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt

@@ -4384,6 +4384,109 @@ class BrowserTabViewModelTest {
         }
     }
 
+    @Test
+    fun whenBasicAuthCredentialsInUrlThenStrippedSafely() {
+        val testUrls = listOf(
+            // Valid basic auth URLs
+            "https://user:pass@example.com",
+            "http://user:pass@example.com",
+            "ftp://user:pass@example.com",
+            "https://user@example.com",
+            "https://user:pass@sub.example.com",
+            "https://user:pass@sub.sub.example.com",
+            "https://user:pass@sub.example.com/path",
+            "https://user:pass@sub.example.com/path?param=value",
+            "https://user:pass@sub.example.com/path#fragment",
+            "https://user:pass@sub.example.com/path?param=value#fragment",
+            "https://user:pass@sub.example.com:8080",
+            "https://user:pass@sub.example.com:8080/path",
+            "https://user:pass@sub.example.com:8080/path?param=value",
+            "https://user:pass@sub.example.com:8080/path#fragment",
+            "https://user:pass@sub.example.com:8080/path?param=value#fragment",
+            "https://user:pass@192.0.2.0",
+            "https://user:pass@[2001:db8::1]",
+            "https://user:pass@[2001:db8::1]/path",
+            "https://user:pass@[2001:db8::1]/path?param=value",
+            "https://user:pass@[2001:db8::1]/path#fragment",
+            "https://user:pass@[2001:db8::1]/path?param=value#fragment",
+            "https://user:pass@[2001:db8::1]:8080",
+            "https://user:pass@[2001:db8::1]:8080/path",
+            "https://user:pass@[2001:db8::1]:8080/path?param=value",
+            "https://user:pass@[2001:db8::1]:8080/path#fragment",
+            "https://user:pass@[2001:db8::1]:8080/path?param=value#fragment",
+        )
+
+        val expectedUrls = listOf(
+            "https://example.com",
+            "http://example.com",
+            "ftp://example.com",
+            "https://example.com",
+            "https://sub.example.com",
+            "https://sub.sub.example.com",
+            "https://sub.example.com/path",
+            "https://sub.example.com/path?param=value",
+            "https://sub.example.com/path#fragment",
+            "https://sub.example.com/path?param=value#fragment",
+            "https://sub.example.com:8080",
+            "https://sub.example.com:8080/path",
+            "https://sub.example.com:8080/path?param=value",
+            "https://sub.example.com:8080/path#fragment",
+            "https://sub.example.com:8080/path?param=value#fragment",
+            "https://192.0.2.0",
+            "https://[2001:db8::1]",
+            "https://[2001:db8::1]/path",
+            "https://[2001:db8::1]/path?param=value",
+            "https://[2001:db8::1]/path#fragment",
+            "https://[2001:db8::1]/path?param=value#fragment",
+            "https://[2001:db8::1]:8080",
+            "https://[2001:db8::1]:8080/path",
+            "https://[2001:db8::1]:8080/path?param=value",
+            "https://[2001:db8::1]:8080/path#fragment",
+            "https://[2001:db8::1]:8080/path?param=value#fragment",
+        )
+
+        for (i in testUrls.indices) {
+            val actual = testee.stripBasicAuthFromUrl(testUrls[i])
+            assertEquals(expectedUrls[i], actual)
+        }
+    }
+
+    @Test
+    fun whenNoBasicAuthProvidedThenDoNotAffectAddressBar() {
+        val testUrls = listOf(
+            // No basic auth, should not be affected
+            "https://example.com/@?param=value",
+            "https://example.com/@path/to/resource?param=value",
+            "https://example.com#@fragment",
+            "https://example.com/path/to/@resource#fragment",
+            "https://example.com?param=%E2%82%AC",
+            "https://example.com/@notbasicAuth?q=none#f",
+            "https://example.com:8080/foobar/",
+            "https://sub.domain.example.com/foobar/",
+            "https://sub.domain.example.com:8080/?q=none#f",
+            // IP address/port combinations
+            "https://192.0.2.0",
+            "https://192.0.2.0:1337",
+            "https://[2001:db8::1]",
+            "https://[2001:db8::1]/path?param=value#fragment",
+            "https://[2001:db8::1]:8080",
+            "https://[2001:db8::1]:8080/path",
+            "https://[2001:db8::1]:8080/path?param=value",
+            // invalid URLs, should do nothing
+            "https://user:pass%40example.com/%40urlencoded@symbol",
+            "user:pass@https://example.com",
+            "not a valid URI",
+            "982.000.564.11:65666",
+            "http://example.com/index[/].html",
+            "http://example.com/</a/path>",
+        )
+
+        for (i in testUrls.indices) {
+            val actual = testee.stripBasicAuthFromUrl(testUrls[i])
+            assertEquals(testUrls[i], actual)
+        }
+    }
+
     @Test
     fun whenOnPermissionsQueryThenSendCommand() = runTest {
         val url = "someUrl"

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

@@ -149,6 +149,8 @@ import dagger.Lazy
 import io.reactivex.android.schedulers.AndroidSchedulers
 import io.reactivex.disposables.Disposable
 import io.reactivex.schedulers.Schedulers
+import java.net.URI
+import java.net.URISyntaxException
 import java.util.*
 import java.util.concurrent.TimeUnit
 import java.util.concurrent.atomic.AtomicBoolean
@@ -1479,13 +1481,32 @@ class BrowserTabViewModel @Inject constructor(
         viewModelScope.launch { updateBookmarkAndFavoriteState(url) }
     }
 
+    @VisibleForTesting
+    fun stripBasicAuthFromUrl(url: String): String {
+        try {
+            val uri = URI(url)
+            val userInfo = uri.userInfo
+
+            if (userInfo != null) {
+                val queryStr = uri.rawQuery?.let { "?$it" } ?: ""
+                val uriFragment = uri.fragment?.let { "#$it" } ?: ""
+                val portStr = if (uri.port != -1) ":${uri.port}" else ""
+                return "${uri.scheme}://${uri.host}$portStr${uri.path}$queryStr$uriFragment"
+            }
+        } catch (e: URISyntaxException) {
+            Timber.e(e, "Failed to parse url for auth stripping")
+            return url
+        }
+        return url
+    }
+
     private fun omnibarTextForUrl(url: String?): String {
         if (url == null) return ""
 
         return if (duckDuckGoUrlDetector.isDuckDuckGoQueryUrl(url)) {
             duckDuckGoUrlDetector.extractQuery(url) ?: url
         } else {
-            url
+            stripBasicAuthFromUrl(url)
         }
     }