From 0fd215ffcaf63dd19aef7a9c2d1453931d87bdc5 Mon Sep 17 00:00:00 2001 From: Aitor Viana Date: Mon, 2 Dec 2024 21:09:50 +0000 Subject: [PATCH 1/3] Add localStorage exceptions to duckduckgo.com --- .../app/browser/WebViewDataManagerTest.kt | 19 ++++++- .../duckduckgo/app/browser/WebDataManager.kt | 53 +++++++++++-------- 2 files changed, 50 insertions(+), 22 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/WebViewDataManagerTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/WebViewDataManagerTest.kt index bc76b3476e36..898dead27cc2 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/WebViewDataManagerTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/WebViewDataManagerTest.kt @@ -18,7 +18,9 @@ package com.duckduckgo.app.browser import android.annotation.SuppressLint import android.content.Context +import android.webkit.ValueCallback import android.webkit.WebStorage +import android.webkit.WebStorage.Origin import android.webkit.WebView import androidx.test.platform.app.InstrumentationRegistry import com.duckduckgo.app.browser.httpauth.WebViewHttpAuthStore @@ -29,9 +31,14 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.test.runTest import kotlinx.coroutines.withContext import org.junit.Assert.assertTrue +import org.junit.Before import org.junit.Test +import org.mockito.kotlin.any +import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever @Suppress("RemoveExplicitTypeArguments") @SuppressLint("NoHardcodedCoroutineDispatcher") @@ -44,6 +51,15 @@ class WebViewDataManagerTest { private val mockWebViewHttpAuthStore: WebViewHttpAuthStore = mock() private val testee = WebViewDataManager(context, WebViewSessionInMemoryStorage(), mockCookieManager, mockFileDeleter, mockWebViewHttpAuthStore) + @Before + fun setup() { + doAnswer { invocation -> + val callback = invocation.arguments[0] as ValueCallback> + callback.onReceiveValue(emptyMap()) // Simulate callback invocation + null + }.whenever(mockStorage).getOrigins(any()) + } + @Test fun whenDataClearedThenWebViewHistoryCleared() = runTest { withContext(Dispatchers.Main) { @@ -76,7 +92,8 @@ class WebViewDataManagerTest { withContext(Dispatchers.Main) { val webView = TestWebView(context) testee.clearData(webView, mockStorage) - verify(mockStorage).deleteAllData() + // we call deleteOrigin() instead and we should make sure we don't call deleteAllData() + verify(mockStorage, never()).deleteAllData() } } diff --git a/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt b/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt index 0522ea462901..b3861342e93c 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt @@ -18,6 +18,7 @@ package com.duckduckgo.app.browser import android.content.Context import android.webkit.WebStorage +import android.webkit.WebStorage.Origin import android.webkit.WebView import com.duckduckgo.app.browser.httpauth.WebViewHttpAuthStore import com.duckduckgo.app.browser.session.WebViewSessionStorage @@ -25,6 +26,8 @@ import com.duckduckgo.app.global.file.FileDeleter import com.duckduckgo.cookies.api.DuckDuckGoCookieManager import java.io.File import javax.inject.Inject +import kotlin.coroutines.resume +import kotlin.coroutines.suspendCoroutine interface WebDataManager { suspend fun clearData( @@ -53,7 +56,7 @@ class WebViewDataManager @Inject constructor( clearFormData(webView) clearAuthentication(webView) clearExternalCookies() - clearWebViewDirectories(exclusions = WEBVIEW_FILES_EXCLUDED_FROM_DELETION) + clearWebViewDirectories() } private fun clearWebViewCache(webView: WebView) { @@ -64,8 +67,25 @@ class WebViewDataManager @Inject constructor( webView.clearHistory() } - private fun clearWebStorage(webStorage: WebStorage) { - webStorage.deleteAllData() + private suspend fun clearWebStorage(webStorage: WebStorage) { + suspendCoroutine { continuation -> + webStorage.getOrigins { origins -> + kotlin.runCatching { + for (origin in origins) { + val originString = (origin as Origin).origin + + // Check if this is the domain to exclude + if (!originString.endsWith(".duckduckgo.com")) { + // Delete all other origins + webStorage.deleteOrigin(originString) + } + } + continuation.resume(Unit) + }.onFailure { + continuation.resume(Unit) + } + } + } } private fun clearFormData(webView: WebView) { @@ -73,17 +93,19 @@ class WebViewDataManager @Inject constructor( } /** - * Deletes web view directory content. The Cookies file is kept as we clear cookies separately to avoid a crash and maintain ddg cookies. - * Cookies may appear in files: - * app_webview/Cookies - * app_webview/Default/Cookies + * Deletes web view directory content except the following directories + * app_webview/Cookies + * app_webview/Default/Cookies + * app_webview/Default/Local Storage + * + * the excluded directories above are to avoid clearing unnecessary cookies and because localStorage is cleared using clearWebStorage */ - private suspend fun clearWebViewDirectories(exclusions: List) { + private suspend fun clearWebViewDirectories() { val dataDir = context.applicationInfo.dataDir - fileDeleter.deleteContents(File(dataDir, WEBVIEW_DATA_DIRECTORY_NAME), exclusions) + fileDeleter.deleteContents(File(dataDir, "app_webview"), listOf("Default", "Cookies")) // We don't delete the Default dir as Cookies may be inside however we do clear any other content - fileDeleter.deleteContents(File(dataDir, WEBVIEW_DEFAULT_DIRECTORY_NAME), exclusions) + fileDeleter.deleteContents(File(dataDir, "app_webview/Default"), listOf("Cookies", "Local Storage")) } private suspend fun clearAuthentication(webView: WebView) { @@ -98,15 +120,4 @@ class WebViewDataManager @Inject constructor( override fun clearWebViewSessions() { webViewSessionStorage.deleteAllSessions() } - - companion object { - private const val WEBVIEW_DATA_DIRECTORY_NAME = "app_webview" - private const val WEBVIEW_DEFAULT_DIRECTORY_NAME = "app_webview/Default" - private const val DATABASES_DIRECTORY_NAME = "databases" - - private val WEBVIEW_FILES_EXCLUDED_FROM_DELETION = listOf( - "Default", - "Cookies", - ) - } } From 726016ba26ad782fe6295eb06ecb7057d37da3f9 Mon Sep 17 00:00:00 2001 From: Aitor Viana Date: Thu, 5 Dec 2024 12:16:23 +0000 Subject: [PATCH 2/3] address review comments --- .../main/java/com/duckduckgo/app/browser/WebDataManager.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt b/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt index b3861342e93c..0602e53b3a06 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt @@ -71,7 +71,7 @@ class WebViewDataManager @Inject constructor( suspendCoroutine { continuation -> webStorage.getOrigins { origins -> kotlin.runCatching { - for (origin in origins) { + for (origin in origins.values) { val originString = (origin as Origin).origin // Check if this is the domain to exclude @@ -82,6 +82,8 @@ class WebViewDataManager @Inject constructor( } continuation.resume(Unit) }.onFailure { + // fallback, if we crash we delete everything + webStorage.deleteAllData() continuation.resume(Unit) } } From af0cf12076bb9152ed3a879489fb9bc6e31c96ff Mon Sep 17 00:00:00 2001 From: Aitor Viana Date: Thu, 5 Dec 2024 12:34:37 +0000 Subject: [PATCH 3/3] add kill switch in case we needed --- .../app/browser/WebViewDataManagerTest.kt | 12 +++++++++++- .../com/duckduckgo/app/browser/WebDataManager.kt | 15 ++++++++++++++- .../duckduckgo/app/browser/di/BrowserModule.kt | 12 ------------ .../remoteconfig/AndroidBrowserConfigFeature.kt | 7 +++++++ 4 files changed, 32 insertions(+), 14 deletions(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/WebViewDataManagerTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/WebViewDataManagerTest.kt index 898dead27cc2..f3bd4bf4caac 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/WebViewDataManagerTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/WebViewDataManagerTest.kt @@ -26,7 +26,9 @@ import androidx.test.platform.app.InstrumentationRegistry import com.duckduckgo.app.browser.httpauth.WebViewHttpAuthStore import com.duckduckgo.app.browser.session.WebViewSessionInMemoryStorage import com.duckduckgo.app.global.file.FileDeleter +import com.duckduckgo.app.pixels.remoteconfig.AndroidBrowserConfigFeature import com.duckduckgo.cookies.api.DuckDuckGoCookieManager +import com.duckduckgo.feature.toggles.api.FakeFeatureToggleFactory import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.test.runTest import kotlinx.coroutines.withContext @@ -49,7 +51,15 @@ class WebViewDataManagerTest { private val context = InstrumentationRegistry.getInstrumentation().targetContext private val mockFileDeleter: FileDeleter = mock() private val mockWebViewHttpAuthStore: WebViewHttpAuthStore = mock() - private val testee = WebViewDataManager(context, WebViewSessionInMemoryStorage(), mockCookieManager, mockFileDeleter, mockWebViewHttpAuthStore) + private val feature = FakeFeatureToggleFactory.create(AndroidBrowserConfigFeature::class.java) + private val testee = WebViewDataManager( + context, + WebViewSessionInMemoryStorage(), + mockCookieManager, + mockFileDeleter, + mockWebViewHttpAuthStore, + feature, + ) @Before fun setup() { diff --git a/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt b/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt index 0602e53b3a06..da52c34e27d0 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/WebDataManager.kt @@ -23,11 +23,16 @@ import android.webkit.WebView import com.duckduckgo.app.browser.httpauth.WebViewHttpAuthStore import com.duckduckgo.app.browser.session.WebViewSessionStorage import com.duckduckgo.app.global.file.FileDeleter +import com.duckduckgo.app.pixels.remoteconfig.AndroidBrowserConfigFeature import com.duckduckgo.cookies.api.DuckDuckGoCookieManager +import com.duckduckgo.di.scopes.AppScope +import com.squareup.anvil.annotations.ContributesBinding +import dagger.SingleInstanceIn import java.io.File import javax.inject.Inject import kotlin.coroutines.resume import kotlin.coroutines.suspendCoroutine +import timber.log.Timber interface WebDataManager { suspend fun clearData( @@ -38,12 +43,15 @@ interface WebDataManager { fun clearWebViewSessions() } +@ContributesBinding(AppScope::class) +@SingleInstanceIn(AppScope::class) class WebViewDataManager @Inject constructor( private val context: Context, private val webViewSessionStorage: WebViewSessionStorage, private val cookieManager: DuckDuckGoCookieManager, private val fileDeleter: FileDeleter, private val webViewHttpAuthStore: WebViewHttpAuthStore, + private val androidBrowserConfigFeature: AndroidBrowserConfigFeature, ) : WebDataManager { override suspend fun clearData( @@ -77,6 +85,7 @@ class WebViewDataManager @Inject constructor( // Check if this is the domain to exclude if (!originString.endsWith(".duckduckgo.com")) { // Delete all other origins + Timber.d("aitor delete $originString / $origin") webStorage.deleteOrigin(originString) } } @@ -107,7 +116,11 @@ class WebViewDataManager @Inject constructor( fileDeleter.deleteContents(File(dataDir, "app_webview"), listOf("Default", "Cookies")) // We don't delete the Default dir as Cookies may be inside however we do clear any other content - fileDeleter.deleteContents(File(dataDir, "app_webview/Default"), listOf("Cookies", "Local Storage")) + if (androidBrowserConfigFeature.deleteLocalStorageKillSwitch().isEnabled()) { + fileDeleter.deleteContents(File(dataDir, "app_webview/Default"), listOf("Cookies")) + } else { + fileDeleter.deleteContents(File(dataDir, "app_webview/Default"), listOf("Cookies", "Local Storage")) + } } private suspend fun clearAuthentication(webView: WebView) { diff --git a/app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt b/app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt index 2d78f0857f1b..98876d97a533 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt @@ -74,7 +74,6 @@ import com.duckduckgo.app.trackerdetection.CloakedCnameDetector import com.duckduckgo.app.trackerdetection.TrackerDetector import com.duckduckgo.common.utils.DispatcherProvider import com.duckduckgo.cookies.api.CookieManagerProvider -import com.duckduckgo.cookies.api.DuckDuckGoCookieManager import com.duckduckgo.cookies.api.ThirdPartyCookieNames import com.duckduckgo.customtabs.api.CustomTabDetector import com.duckduckgo.di.scopes.AppScope @@ -159,17 +158,6 @@ class BrowserModule { @Provides fun webViewSessionStorage(): WebViewSessionStorage = WebViewSessionInMemoryStorage() - @SingleInstanceIn(AppScope::class) - @Provides - fun webDataManager( - context: Context, - webViewSessionStorage: WebViewSessionStorage, - cookieManager: DuckDuckGoCookieManager, - fileDeleter: FileDeleter, - webViewHttpAuthStore: WebViewHttpAuthStore, - ): WebDataManager = - WebViewDataManager(context, webViewSessionStorage, cookieManager, fileDeleter, webViewHttpAuthStore) - @Provides fun clipboardManager(context: Context): ClipboardManager { return context.getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager diff --git a/app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt b/app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt index cc6328142d2d..55375f6b24d4 100644 --- a/app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt +++ b/app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt @@ -83,4 +83,11 @@ interface AndroidBrowserConfigFeature { */ @Toggle.DefaultValue(false) fun featuresRequestHeader(): Toggle + + /** + * When enabled we should delete the app_webview/Default/Local Storage folder. If all goes well, we should not need to set this to `true` + * in remote config + */ + @Toggle.DefaultValue(false) + fun deleteLocalStorageKillSwitch(): Toggle }