Skip to content
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

Fix etag handling in WebDAV client #285

Merged
merged 2 commits into from
Nov 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ class UsedFileDaoTest {
credentials = FSCredentials.BasicCredentials(
url = "firstServerUrl",
username = "firstUsername",
password = "firstPassword"
password = "firstPassword",
isIgnoreSslValidation = false
),
type = FSType.INTERNAL_STORAGE,
isBrowsable = true
Expand All @@ -125,7 +126,8 @@ class UsedFileDaoTest {
credentials = FSCredentials.BasicCredentials(
url = "keyFileServerUrl",
username = "keyFilUsername",
password = "keyFilePassword"
password = "keyFilePassword",
isIgnoreSslValidation = true
),
type = FSType.INTERNAL_STORAGE,
isBrowsable = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ sealed class FSCredentials : Parcelable {
data class BasicCredentials(
val url: String,
val username: String,
val password: String
val password: String,
val isIgnoreSslValidation: Boolean
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security Consideration: SSL Validation Control

Adding the ability to disable SSL validation could lead to security vulnerabilities if not properly documented and controlled. Consider:

  1. Adding KDoc comments explaining the security implications
  2. Making this an opt-in feature with a default value of false
  3. Adding runtime warnings when SSL validation is disabled

Suggested improvement:

    data class BasicCredentials(
        val url: String,
        val username: String,
        val password: String,
-       val isIgnoreSslValidation: Boolean
+       /** 
+        * WARNING: Disabling SSL validation is dangerous and should only be used in controlled environments
+        * as it makes the connection vulnerable to man-in-the-middle attacks.
+        */
+       val isIgnoreSslValidation: Boolean = false
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
val isIgnoreSslValidation: Boolean
/**
* WARNING: Disabling SSL validation is dangerous and should only be used in controlled environments
* as it makes the connection vulnerable to man-in-the-middle attacks.
*/
val isIgnoreSslValidation: Boolean = false

Comment on lines +16 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

PR title should be updated to reflect both changes

The PR includes two distinct changes:

  1. Etag handling improvement: Adding a fallback to use modified time when etag is null
  2. SSL validation configuration: Adding the ability to ignore SSL certificate validation

The PR title should be updated to: "Fix etag handling and add SSL validation configuration in WebDAV client"

🔗 Analysis chain

Verify PR Title vs Implementation

The PR title mentions "Fix etag handling in WebDAV client" but the changes are focused on SSL validation. This discrepancy should be clarified:

  1. Is this change part of the etag handling fix?
  2. Should the PR title be updated to reflect the SSL validation changes?

Let's check for etag-related changes:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for etag-related changes in the codebase
rg "etag|ETag" --type kotlin

# Check if there are other files in this PR that handle etags
git diff --name-only HEAD~1 | xargs rg "etag|ETag" --type kotlin

Length of output: 605


Script:

#!/bin/bash
# Let's check the PR diff to understand the full scope of changes
git diff HEAD~1

# Also check the PR title and description
git log -1 --pretty=%B

Length of output: 33733


Script:

#!/bin/bash
# Let's check the PR title and commit message
git log -1 --format=%s

# Let's check for etag-related changes in WebDavClientV2.kt
git diff HEAD~1 app/src/main/kotlin/com/ivanovsky/passnotes/data/repository/file/webdav/WebDavClientV2.kt | grep -A 5 -B 5 "etag"

Length of output: 569

) : FSCredentials()

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ class FSAuthorityTypeConverter(
FSCredentials.BasicCredentials(
url = credsObj.optString(URL),
username = credsObj.optString(USERNAME),
password = credsObj.optString(PASSWORD)
password = credsObj.optString(PASSWORD),
isIgnoreSslValidation = credsObj.optBoolean(IS_IGNORE_SSL_VALIDATION)
)
}

Expand Down Expand Up @@ -125,6 +126,7 @@ class FSAuthorityTypeConverter(
creds.put(URL, credentials.url)
creds.put(USERNAME, credentials.username)
creds.put(PASSWORD, credentials.password)
creds.put(IS_IGNORE_SSL_VALIDATION, credentials.isIgnoreSslValidation)
}

is FSCredentials.GitCredentials -> {
Expand Down Expand Up @@ -162,6 +164,7 @@ class FSAuthorityTypeConverter(

private const val USERNAME = "username"
private const val PASSWORD = "password"
private const val IS_IGNORE_SSL_VALIDATION = "isIgnoreSslValidation"

private const val IS_SECRET_URL = "isSecretUrl"
private const val SALT = "salt"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ open class FileSystemResolver(
class WebdavFileSystemFactory : Factory {
override fun createProvider(fsAuthority: FSAuthority): FileSystemProvider {
val authenticator = WebdavAuthenticator(fsAuthority)
val client = RemoteApiClientAdapter(WebDavClientV2(authenticator, get()))
val client = RemoteApiClientAdapter(WebDavClientV2(authenticator))

return RemoteFileSystemProvider(
authenticator,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ class FakeFileSystemProvider(
credentials = FSCredentials.BasicCredentials(
url = "content://fakefs.com",
username = "user",
password = "abc123"
password = "abc123",
isIgnoreSslValidation = false
),
type = FSType.FAKE,
isBrowsable = true
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package com.ivanovsky.passnotes.data.repository.file.webdav

import android.annotation.SuppressLint
import com.ivanovsky.passnotes.BuildConfig
import java.security.SecureRandom
import java.security.cert.X509Certificate
import javax.net.ssl.SSLContext
import javax.net.ssl.X509TrustManager
import okhttp3.OkHttp
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the import and usage of OkHttp.

The class OkHttp does not exist in the OkHttp library, which may lead to a compilation error. In line 9, the import import okhttp3.OkHttp is incorrect, and in line 20, using OkHttp::class.java.simpleName will not function as intended.

Apply this diff to fix the issue:

-import okhttp3.OkHttp
+// Removed incorrect import of OkHttp

And update the logging statement:

-            Timber.tag(OkHttp::class.java.simpleName).d(it)
+            Timber.tag(OkHttpClient::class.java.simpleName).d(it)

Also applies to: 20-20

import okhttp3.OkHttpClient
import okhttp3.logging.HttpLoggingInterceptor
import timber.log.Timber

object HttpClientFactory {

fun createHttpClient(type: HttpClientType): OkHttpClient {
val builder = OkHttpClient.Builder()

val interceptor = HttpLoggingInterceptor {
Timber.tag(OkHttp::class.java.simpleName).d(it)
}.apply {
setLevel(HttpLoggingInterceptor.Level.BASIC)
}

builder.addInterceptor(interceptor)

if (BuildConfig.DEBUG && type == HttpClientType.UNSECURE) {
Timber.w("--------------------------------------------")
Timber.w("-- --")
Timber.w("-- --")
Timber.w("-- SSL Certificate validation is disabled --")
Timber.w("-- --")
Timber.w("-- --")
Timber.w("--------------------------------------------")

val unsecuredTrustManager = createUnsecuredTrustManager()
val sslContext = SSLContext.getInstance("TLS")
sslContext.init(null, arrayOf(unsecuredTrustManager), SecureRandom())

builder.sslSocketFactory(sslContext.socketFactory, unsecuredTrustManager)
builder.hostnameVerifier { _, _ -> true }
}

return builder.build()
}

@SuppressLint("CustomX509TrustManager")
private fun createUnsecuredTrustManager(): X509TrustManager {
return object : X509TrustManager {
@SuppressLint("TrustAllX509TrustManager")
override fun checkClientTrusted(
chain: Array<out X509Certificate>?,
authType: String?
) {
}

Comment on lines +51 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Provide justification for suppressing lint warnings and empty method bodies.

The methods checkClientTrusted and checkServerTrusted have empty bodies, and lint warnings are suppressed using @SuppressLint("TrustAllX509TrustManager"). While this is necessary for creating an unsecured trust manager, suppressing these warnings without explanation can be risky.

Include comments within the methods or before the suppression annotations explaining the reasoning behind suppressing lint warnings and leaving the methods empty. This will aid future maintainers in understanding the intent and prevent accidental misuse.

Also applies to: 58-63

🧰 Tools
🪛 detekt (1.23.7)

[warning] 54-55: This empty block of code can be removed.

(detekt.empty-blocks.EmptyFunctionBlock)

@SuppressLint("TrustAllX509TrustManager")
override fun checkServerTrusted(
chain: Array<out X509Certificate>?,
authType: String?
) {
}

override fun getAcceptedIssuers(): Array<X509Certificate> {
return arrayOf()
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.ivanovsky.passnotes.data.repository.file.webdav

enum class HttpClientType {
SECURE,
UNSECURE
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,13 @@ import java.io.File
import java.io.FileOutputStream
import java.io.IOException
import java.util.concurrent.atomic.AtomicBoolean
import okhttp3.OkHttpClient
import timber.log.Timber

class WebDavClientV2(
private val authenticator: WebdavAuthenticator,
httpClient: OkHttpClient
private val authenticator: WebdavAuthenticator
) : RemoteApiClientV2 {

private val webDavClient = WebDavNetworkLayer(httpClient).apply {
private val webDavClient = WebDavNetworkLayer().apply {
val creds = authenticator.getFsAuthority().credentials
if (creds != null) {
setCredentials(creds as FSCredentials.BasicCredentials)
Expand Down Expand Up @@ -330,7 +328,11 @@ class WebDavClientV2(
path = path,
serverModified = this.modified,
clientModified = this.modified,
revision = this.etag
revision = if (this.etag != null) {
this.etag
} else {
this.modified.time.toString()
}
)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,27 +1,33 @@
package com.ivanovsky.passnotes.data.repository.file.webdav

import com.ivanovsky.passnotes.BuildConfig
import com.ivanovsky.passnotes.data.entity.FSCredentials
import com.ivanovsky.passnotes.data.entity.OperationError
import com.ivanovsky.passnotes.data.entity.OperationResult
import com.thegrizzlylabs.sardineandroid.impl.OkHttpSardine
import com.thegrizzlylabs.sardineandroid.impl.SardineException
import java.io.IOException
import okhttp3.OkHttpClient
import java.util.concurrent.ConcurrentHashMap
import timber.log.Timber

class WebDavNetworkLayer(
httpClient: OkHttpClient
) {
class WebDavNetworkLayer {

private val webDavClient = OkHttpSardine(httpClient)
private val clients: MutableMap<HttpClientType, OkHttpSardine> = ConcurrentHashMap()

@Volatile
private var webDavClient: OkHttpSardine? = null

fun setCredentials(credentials: FSCredentials.BasicCredentials) {
webDavClient.setCredentials(credentials.username, credentials.password)
setupClient(isIgnoreSslValidation = credentials.isIgnoreSslValidation)
webDavClient?.setCredentials(credentials.username, credentials.password)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential null webDavClient after setup

After calling setupClient, webDavClient might still be null if client creation fails. Ensure that webDavClient is properly initialized before setting credentials.

Apply this diff to check for null and handle the error appropriately:

 fun setCredentials(credentials: FSCredentials.BasicCredentials) {
     setupClient(isIgnoreSslValidation = credentials.isIgnoreSslValidation)
-    webDavClient?.setCredentials(credentials.username, credentials.password)
+    val client = webDavClient
+    if (client != null) {
+        client.setCredentials(credentials.username, credentials.password)
+    } else {
+        // Handle the error, e.g., log or throw an exception
+        Timber.e("Failed to initialize WebDav client")
+    }
 }

Committable suggestion skipped: line range outside the PR's diff.

}

fun <T> execute(call: (webDavClient: OkHttpSardine) -> T): OperationResult<T> {
val client = webDavClient
requireNotNull(client)
Comment on lines +26 to +27
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle uninitialized webDavClient in execute method

Currently, if webDavClient is null, requireNotNull(client) will throw an IllegalArgumentException. Consider returning an OperationResult with an appropriate error message to handle this case more gracefully.

Apply this diff to return an error when webDavClient is not initialized:

 fun <T> execute(call: (webDavClient: OkHttpSardine) -> T): OperationResult<T> {
     val client = webDavClient
-    requireNotNull(client)
+    if (client == null) {
+        return OperationResult.error(OperationError.newGenericError("Client is not initialized"))
+    }

     try {
         return OperationResult.success(call.invoke(client))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
val client = webDavClient
requireNotNull(client)
val client = webDavClient
if (client == null) {
return OperationResult.error(OperationError.newGenericError("Client is not initialized"))
}


try {
return OperationResult.success(call.invoke(webDavClient))
return OperationResult.success(call.invoke(client))
} catch (exception: SardineException) {
Timber.d(exception)
return when (exception.statusCode) {
Expand All @@ -38,6 +44,22 @@ class WebDavNetworkLayer(
}
}

private fun setupClient(
isIgnoreSslValidation: Boolean
) {
val clientType = if (isIgnoreSslValidation && BuildConfig.DEBUG) {
HttpClientType.UNSECURE
} else {
HttpClientType.SECURE
}
Comment on lines +50 to +54
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Reconsider DEBUG flag usage for SSL validation

The current implementation only allows SSL validation to be ignored in debug builds. This could make it difficult to test or troubleshoot SSL issues in production environments.

Consider making this configurable independent of the build type:

-    val clientType = if (isIgnoreSslValidation && BuildConfig.DEBUG) {
+    val clientType = if (isIgnoreSslValidation) {
         HttpClientType.UNSECURE
     } else {
         HttpClientType.SECURE
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
val clientType = if (isIgnoreSslValidation && BuildConfig.DEBUG) {
HttpClientType.UNSECURE
} else {
HttpClientType.SECURE
}
val clientType = if (isIgnoreSslValidation) {
HttpClientType.UNSECURE
} else {
HttpClientType.SECURE
}


webDavClient = clients[clientType]
?: OkHttpSardine(HttpClientFactory.createHttpClient(clientType))
.apply {
clients[clientType] = this
}
Comment on lines +56 to +60
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Implement client cache management

The current implementation caches clients indefinitely without any cleanup mechanism, which could lead to memory leaks if multiple clients are created over time.

Consider:

  1. Adding a maximum cache size
  2. Implementing a cleanup mechanism
  3. Adding client disposal

Here's a suggested implementation:

+    private const val MAX_CLIENTS = 2
+
     private fun setupClient(
         isIgnoreSslValidation: Boolean
     ) {
         val clientType = if (isIgnoreSslValidation) {
             HttpClientType.UNSECURE
         } else {
             HttpClientType.SECURE
         }
 
+        if (clients.size >= MAX_CLIENTS) {
+            val unusedClients = clients.entries
+                .filter { it.value != webDavClient }
+                .map { it.key }
+            unusedClients.forEach { clients.remove(it) }
+        }
+
         webDavClient = clients[clientType]
             ?: OkHttpSardine(HttpClientFactory.createHttpClient(clientType))
                 .apply {
                     clients[clientType] = this
                 }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
webDavClient = clients[clientType]
?: OkHttpSardine(HttpClientFactory.createHttpClient(clientType))
.apply {
clients[clientType] = this
}
private const val MAX_CLIENTS = 2
private fun setupClient(
isIgnoreSslValidation: Boolean
) {
val clientType = if (isIgnoreSslValidation) {
HttpClientType.UNSECURE
} else {
HttpClientType.SECURE
}
if (clients.size >= MAX_CLIENTS) {
val unusedClients = clients.entries
.filter { it.value != webDavClient }
.map { it.key }
unusedClients.forEach { clients.remove(it) }
}
webDavClient = clients[clientType]
?: OkHttpSardine(HttpClientFactory.createHttpClient(clientType))
.apply {
clients[clientType] = this
}
}

}

companion object {
private const val HTTP_UNAUTHORIZED = 401
private const val HTTP_NOT_FOUND = 404
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import com.ivanovsky.passnotes.data.repository.file.OnConflictStrategy
import com.ivanovsky.passnotes.data.repository.keepass.FileKeepassKey
import com.ivanovsky.passnotes.data.repository.keepass.PasswordKeepassKey
import java.lang.Exception
import java.security.SecureRandom
import timber.log.Timber

fun EncryptedDatabaseKey.toCredentials(
Expand Down Expand Up @@ -42,7 +43,10 @@ fun EncryptedDatabaseKey.toCredentials(

val credentials = if (password == null) {
Credentials.from(
EncryptedValue.fromBinary(bytes)
EncryptedValue.fromBinary(
bytes = bytes,
random = SecureRandom()
)
Comment on lines +46 to +49
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider reusing SecureRandom instance

Creating a new SecureRandom instance for each encryption operation could lead to predictable seeds if called in rapid succession on some platforms. Consider reusing a single instance.

Here's a suggested implementation:

+ private val secureRandom = SecureRandom()
+
 fun EncryptedDatabaseKey.toCredentials(
     fileSystemResolver: FileSystemResolver
 ): OperationResult<Credentials> {
     // ...
                     EncryptedValue.fromBinary(
                         bytes = bytes,
-                        random = SecureRandom()
+                        random = secureRandom
                     )
     // ...
 }

Committable suggestion skipped: line range outside the PR's diff.

)
} else {
Credentials.from(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ class GetTestCredentialsUseCase(
FSCredentials.BasicCredentials(
url = data.webdavUrl,
username = data.webdavUsername,
password = data.webdavPassword
password = data.webdavPassword,
isIgnoreSslValidation = false
)
} else {
null
Expand Down Expand Up @@ -56,7 +57,8 @@ class GetTestCredentialsUseCase(
FSCredentials.BasicCredentials(
url = data.fakeFsUrl,
username = data.fakeFsUsername,
password = data.fakeFsPassword
password = data.fakeFsPassword,
isIgnoreSslValidation = false
)
} else {
null
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.ivanovsky.passnotes.injection.modules

import android.annotation.SuppressLint
import com.ivanovsky.passnotes.BuildConfig
import com.ivanovsky.passnotes.data.ObserverBus
import com.ivanovsky.passnotes.data.crypto.DataCipherProvider
import com.ivanovsky.passnotes.data.crypto.DataCipherProviderImpl
Expand All @@ -26,15 +24,7 @@ import com.ivanovsky.passnotes.domain.ResourceProvider
import com.ivanovsky.passnotes.domain.interactor.ErrorInteractor
import com.ivanovsky.passnotes.domain.interactor.SelectionHolder
import com.ivanovsky.passnotes.presentation.core.ThemeProvider
import java.security.SecureRandom
import java.security.cert.X509Certificate
import javax.net.ssl.SSLContext
import javax.net.ssl.X509TrustManager
import okhttp3.OkHttp
import okhttp3.OkHttpClient
import okhttp3.logging.HttpLoggingInterceptor
import org.koin.dsl.module
import timber.log.Timber

object CoreModule {

Expand All @@ -57,9 +47,6 @@ object CoreModule {
single { FileHelper(get(), get()) }
single { SAFHelper(get()) }

// Network
single { provideOkHttp(get()) }

// Database
single { AppDatabase.buildDatabase(get(), get()) }
single { provideRemoteFileRepository(get()) }
Expand All @@ -73,46 +60,6 @@ object CoreModule {
}
}

private fun provideOkHttp(settings: Settings): OkHttpClient {
val builder = OkHttpClient.Builder()

val interceptor = HttpLoggingInterceptor {
Timber.tag(OkHttp::class.java.simpleName).d(it)
}.apply {
setLevel(HttpLoggingInterceptor.Level.BASIC)
}

builder.addInterceptor(interceptor)

if (BuildConfig.DEBUG && !settings.isSslCertificateValidationEnabled) {
Timber.w("SSL Certificate validation is disabled")
val unsecuredTrustManager = createUnsecuredTrustManager()
val sslContext = SSLContext.getInstance("TLS")
sslContext.init(null, arrayOf(unsecuredTrustManager), SecureRandom())

builder.sslSocketFactory(sslContext.socketFactory, unsecuredTrustManager)
builder.hostnameVerifier { _, _ -> true }
}

return builder.build()
}

private fun createUnsecuredTrustManager(): X509TrustManager {
return object : X509TrustManager {
@SuppressLint("TrustAllX509TrustManager")
override fun checkClientTrusted(chain: Array<out X509Certificate>?, authType: String?) {
}

@SuppressLint("TrustAllX509TrustManager")
override fun checkServerTrusted(chain: Array<out X509Certificate>?, authType: String?) {
}

override fun getAcceptedIssuers(): Array<X509Certificate> {
return arrayOf()
}
}
}

private fun provideRemoteFileRepository(database: AppDatabase) =
RemoteFileRepository(database.remoteFileDao)

Expand Down
Loading