Optimize children lookup O(N)→O(1), add path traversal prevention, expand test coverage

app/src/main/java/org/joefang/webdav/provider/WebDavCache.kt

@@ -30,8 +30,8 @@ class WebDavCache (private val context: Context, private val dao: CacheDao) {
         if (path.parent != null) {
             val parentFile = cache[path.parent]
             if (parentFile != null) {
-                // only return the cached metadata if the given path does not refer to a directory
-                val childFile = parentFile.children.find { f -> f.path == path }
+                // Use O(1) HashMap lookup instead of O(N) linear search
+                val childFile = parentFile.findChildByPath(path)
                 if (childFile != null && !childFile.isDirectory) {
                     return childFile
                 }

app/src/main/java/org/joefang/webdav/provider/WebDavClient.kt

@@ -161,7 +161,7 @@ class WebDavClient(
             val file = WebDavFile(desc)
             if (file.path != path.path) {
                 file.parent = root
-                root.children.add(file)
+                root.addChild(file)
             }
         }
 

app/src/main/java/org/joefang/webdav/provider/WebDavFile.kt

@@ -11,14 +11,44 @@ import java.text.SimpleDateFormat
 import java.util.Date
 import java.util.Locale
 
+/**
+ * Represents a file or directory on a WebDAV server.
+ * 
+ * This class maintains a tree structure of files where directories can have children.
+ * 
+ * ## Data Structure Choice: LinkedHashMap
+ * 
+ * Children are stored in a [LinkedHashMap] keyed by [Path], which provides:
+ * - **O(1) lookup** by path via [findChildByPath] or [containsChild]
+ * - **O(1) insertion** at the end (preserves insertion order)
+ * - **O(1) deletion** by key/path
+ * - **Ordered iteration** in insertion order (important for consistent UI display)
+ * 
+ * This is optimal for our access patterns which require:
+ * - Fast path-based lookups (cache validation, document resolution)
+ * - Adding children when listing directories
+ * - Removing children when files are deleted
+ * - Iterating over children for directory listings
+ * 
+ * We do NOT require indexed access (e.g., `children[5]`), so LinkedHashMap is ideal.
+ * See `docs/DATA_STRUCTURES.md` for detailed analysis.
+ * 
+ * @property path The immutable path of this file. Immutability ensures cache consistency.
+ */
 class WebDavFile(
-    var path: Path,
+    val path: Path,
     var isDirectory: Boolean = false,
     var contentType: String? = null,
     var isPending: Boolean = false
 ) {
     var parent: WebDavFile? = null
-    var children: MutableList<WebDavFile> = ArrayList()
+
+    /**
+     * Children stored in a LinkedHashMap for O(1) lookup, insertion, and deletion.
+     * Iteration preserves insertion order for consistent directory listings.
+     */
+    private val childrenMap: LinkedHashMap<Path, WebDavFile> = LinkedHashMap()
+
     val writable: Boolean = true
 
     var etag: String? = null
@@ -84,4 +114,106 @@ class WebDavFile(
 
         return "application/octet-stream"
     }
+
+    // ==================== Copy Method ====================
+
+    /**
+     * Creates a copy of this file with a new path.
+     * All properties are copied except children (which are not transferred during rename).
+     * 
+     * @param newPath The path for the new file
+     * @return A new WebDavFile with the same properties but a different path
+     */
+    fun copyWithNewPath(newPath: Path): WebDavFile {
+        return WebDavFile(newPath, isDirectory, contentType, isPending).also {
+            it.parent = parent
+            it.etag = etag
+            it.contentLength = contentLength
+            it.quotaUsedBytes = quotaUsedBytes
+            it.quotaAvailableBytes = quotaAvailableBytes
+            it.lastModified = lastModified
+        }
+    }
+
+    // ==================== Children Management API ====================
+    // All operations below are O(1) thanks to LinkedHashMap
+
+    /**
+     * Returns the number of children. O(1).
+     */
+    val childCount: Int
+        get() = childrenMap.size
+
+    /**
+     * Checks if this file has any children. O(1).
+     */
+    fun hasChildren(): Boolean = childrenMap.isNotEmpty()
+
+    /**
+     * Finds a child file by its path. O(1).
+     * @param childPath The path of the child to find
+     * @return The child file, or null if not found
+     */
+    fun findChildByPath(childPath: Path): WebDavFile? = childrenMap[childPath]
+
+    /**
+     * Checks if a child with the given path exists. O(1).
+     */
+    fun containsChild(childPath: Path): Boolean = childrenMap.containsKey(childPath)
+
+    /**
+     * Adds a child file. O(1).
+     * If a child with the same path already exists, it will be replaced.
+     * @param child The child file to add
+     */
+    fun addChild(child: WebDavFile) {
+        childrenMap[child.path] = child
+    }
+
+    /**
+     * Adds multiple children. O(n) where n is the number of children to add.
+     */
+    fun addChildren(children: Collection<WebDavFile>) {
+        children.forEach { childrenMap[it.path] = it }
+    }
+
+    /**
+     * Removes a child by reference. O(1).
+     * @return true if the child was removed, false if not found
+     */
+    fun removeChild(child: WebDavFile): Boolean = childrenMap.remove(child.path) != null
+
+    /**
+     * Removes a child by path. O(1).
+     * @return The removed child, or null if not found
+     */
+    fun removeChildByPath(childPath: Path): WebDavFile? = childrenMap.remove(childPath)
+
+    /**
+     * Removes all children. O(1).
+     */
+    fun clearChildren() {
+        childrenMap.clear()
+    }
+
+    /**
+     * Returns an iterator over the children in insertion order.
+     * Modifications during iteration are supported via the iterator's remove() method.
+     */
+    fun childrenIterator(): MutableIterator<WebDavFile> = childrenMap.values.iterator()
+
+    /**
+     * Returns all children as a collection view.
+     * 
+     * **Warning:** This is a view backed by the map. If children are modified while
+     * iterating (except via the iterator's remove method), a ConcurrentModificationException
+     * may be thrown. For safe iteration during modification, use [childrenSnapshot].
+     */
+    fun children(): Collection<WebDavFile> = childrenMap.values
+
+    /**
+     * Returns a snapshot (copy) of all children as a list.
+     * Use this when you need to iterate while potentially modifying the children.
+     */
+    fun childrenSnapshot(): List<WebDavFile> = childrenMap.values.toList()
 }

app/src/main/java/org/joefang/webdav/provider/WebDavProvider.kt

@@ -126,7 +126,7 @@ class WebDavProvider : DocumentsProvider() {
             cache.setFileMeta(account, parentFile)
 
             result.apply {
-                for (file in parentFile.children) {
+                for (file in parentFile.children()) {
                     if (!file.isPending) {
                         includeFile(this, account, file)
                     }
@@ -227,16 +227,16 @@ class WebDavProvider : DocumentsProvider() {
         val callback = WebDavWriteProxyCallback(clients.get(account), file,
             onSuccess = { newFile ->
                 file.parent?.let {
-                    it.children.remove(file)
-                    it.children.add(newFile)
+                    it.removeChild(file)
+                    it.addChild(newFile)
 
                     val notifyUri = buildDocumentUri(account, it)
                     mustGetContext().contentResolver.notifyChange(notifyUri, null, 0)
                 }
                 writeProxies.remove(documentId)
             },
             onFail = {
-                file.parent?.children?.remove(file)
+                file.parent?.removeChild(file)
                 writeProxies.remove(documentId)
             }
         )
@@ -268,7 +268,7 @@ class WebDavProvider : DocumentsProvider() {
             if (res.isSuccessful) {
                 val file = WebDavFile(path, true, contentType = mimeType)
                 file.parent = dir
-                dir.children.add(file)
+                dir.addChild(file)
                 cache.setFileMeta(account, file)
 
                 val notifyUri = buildDocumentUri(account, file.parent!!)
@@ -279,7 +279,7 @@ class WebDavProvider : DocumentsProvider() {
         } else {
             val file = WebDavFile(path, false, contentType = mimeType, isPending = true)
             file.parent = dir
-            dir.children.add(file)
+            dir.addChild(file)
 
             resDocumentId = buildDocumentId(account, file)
         }
@@ -302,7 +302,7 @@ class WebDavProvider : DocumentsProvider() {
 
         if (res.isSuccessful) {
             cache.removeFileMeta(account, file.path)
-            file.parent?.children?.remove(file)
+            file.parent?.removeChild(file)
 
             val notifyUri = buildDocumentUri(account, file.path.parent)
             mustGetContext().contentResolver.notifyChange(notifyUri, null, 0)
@@ -328,13 +328,19 @@ class WebDavProvider : DocumentsProvider() {
             clients.get(account).move(file.davPath, WebDavPath(newPath, file.isDirectory))
         }
         if (res.isSuccessful) {
-            file.path = newPath
+            // Create a copy with the new path (using copyWithNewPath to ensure all fields are copied)
+            val renamedFile = file.copyWithNewPath(newPath)
+
+            // Update parent's children: remove old, add new
+            file.parent?.removeChild(file)
+            file.parent?.addChild(renamedFile)
+
             if (file.isDirectory) {
                 cache.removeFileMeta(account, oldPath)
-                cache.setFileMeta(account, file)
+                cache.setFileMeta(account, renamedFile)
             }
 
-            val notifyUri = buildDocumentUri(account, file.path.parent)
+            val notifyUri = buildDocumentUri(account, newPath.parent)
             mustGetContext().contentResolver.notifyChange(notifyUri, null, 0)
 
             return buildDocumentId(account, newPath)
@@ -377,7 +383,8 @@ class WebDavProvider : DocumentsProvider() {
             val file = if (isRoot) {
                 resFile
             } else {
-                resFile.children.find { f -> f.path == doc.path }
+                // Use O(1) HashMap lookup instead of O(N) linear search
+                resFile.findChildByPath(doc.path)
             }
 
             if (file != null) {
@@ -474,19 +481,60 @@ class WebDavProvider : DocumentsProvider() {
     }
 
     companion object {
+        /**
+         * Parses a document ID into account ID and path components.
+         * 
+         * Document ID format: /{accountId}/{path...}
+         * Example: /1/documents/file.txt -> (1, /documents/file.txt)
+         * 
+         * Security: This method validates the input to prevent:
+         * - Path traversal attacks (../)
+         * - Invalid account IDs
+         * - Malformed document IDs
+         * 
+         * @throws IllegalArgumentException if the document ID is invalid
+         */
         fun parseDocumentId(documentId: String): Pair<Long, Path> {
+            // Validate basic structure
+            if (documentId.isEmpty() || !documentId.startsWith("/")) {
+                throw IllegalArgumentException("Invalid document ID: '$documentId' (must start with /)")
+            }
+
             val parts = documentId.split("/")
             if (parts.size < 3) {
                 throw IllegalArgumentException("Invalid document ID: '$documentId'")
             }
 
             val id = try {
-                parts[1].toLong()
+                val accountIdStr = parts[1]
+                // Validate account ID is a non-negative long (only digits allowed)
+                if (accountIdStr.isEmpty() || accountIdStr.any { !it.isDigit() }) {
+                    throw NumberFormatException("Account ID must be a non-negative integer")
+                }
+                accountIdStr.toLong().also {
+                    if (it < 0) throw NumberFormatException("Account ID must be non-negative")
+                }
             } catch (e: NumberFormatException) {
-                throw IllegalArgumentException("Invalid document ID: '$documentId' (Bad account ID: ${parts[1]}")
+                throw IllegalArgumentException("Invalid document ID: '$documentId' (Bad account ID: ${parts[1]})")
             }
 
-            val path = Paths.get(parts.drop(2).joinToString("/", prefix = "/"))
+            val pathStr = parts.drop(2).joinToString("/", prefix = "/")
+
+            // Security: Check for path traversal attempts
+            // We check for ".." and "." as standalone path segments, not as parts of filenames
+            // This allows legitimate files like ".config" or "file..backup" while blocking "../"
+            val pathSegments = pathStr.split("/").filter { it.isNotEmpty() }
+            if (pathSegments.any { it == ".." || it == "." }) {
+                throw IllegalArgumentException("Invalid document ID: '$documentId' (Path traversal detected)")
+            }
+
+            val path = Paths.get(pathStr).normalize()
+
+            // Additional security check: ensure normalized path doesn't escape root
+            if (!path.toString().startsWith("/")) {
+                throw IllegalArgumentException("Invalid document ID: '$documentId' (Path must be absolute)")
+            }
+
             return Pair(id, path)
         }