Merge pull request #10384 from nextcloud/backport/10369/stable-34.0

[stable-34.0] Check for file provider upload size mismatch

Author: nilsding

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Create.swift

@@ -202,14 +202,31 @@ public extension Item {
             """
         )
 
-        if let expectedSize = itemTemplate.documentSize??.int64Value, size != expectedSize {
-            logger.info(
-                """
-                Created item upload reported as successful, but there are differences between
-                the received file size (\(Int(size ?? -1)))
-                and the original file size (\(itemTemplate.documentSize??.int64Value ?? 0))
-                """
-            )
+        // Integrity check: the size the server reports it stored must match the bytes we
+        // uploaded. On a mismatch the transfer was torn/truncated (a dropped connection, or an app
+        // such as Adobe InDesign/Illustrator still writing the file). Refuse to record a clean
+        // creation: delete the partial remote object (best effort) so a retry starts clean, then
+        // return a *transient* error so the File Provider system re-drives the create instead of
+        // surfacing a broken file as synced.
+        //
+        // The error must be transient to get an automatic retry: per NSFileProviderReplicatedExtension,
+        // resolvable NSFileProviderError codes such as `.cannotSynchronize` make the system back off
+        // until the provider signals resolution, whereas "any other error … in NSCocoaErrorDomain" is
+        // retried. NOTE: a create retry may arrive with `.mayAlreadyExist`, which `create()` currently
+        // short-circuits — tracked as a separate follow-up.
+        let localAttributes = try? FileManager.default.attributesOfItem(atPath: localPath)
+
+        if let localSize = localAttributes?[.size] as? Int64, let uploadedSize = size, uploadedSize != localSize {
+            logger.error("Upload integrity check failed for created item: server stored \(uploadedSize) bytes but the local file is \(localSize) bytes. Removing the partial remote object and returning a transient error so the system retries.", [.name: itemTemplate.filename])
+            let (_, _, deleteError) = await remoteInterface.delete(remotePath: remotePath, account: account, options: .init(), taskHandler: { _ in })
+
+            if deleteError != .success {
+                logger.error("Could not remove partial remote object after integrity failure.", [.name: itemTemplate.filename, .error: deleteError])
+            }
+
+            return (nil, NSError(domain: NSCocoaErrorDomain, code: NSFileWriteUnknownError, userInfo: [
+                NSLocalizedDescriptionKey: "Upload integrity check failed: server stored \(uploadedSize) of \(localSize) bytes."
+            ]))
         }
 
         let contentType: String = if itemTemplate.contentType == .aliasFile {

shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Modify.swift

@@ -229,15 +229,25 @@ public extension Item {
             """
         )
 
+        // Integrity check: the size the server reports it stored must match the bytes we handed it.
+        // A mismatch means a torn/truncated transfer (a dropped connection, or an app such as Adobe
+        // InDesign/Illustrator still writing the file). Do NOT record it as a clean upload — return a
+        // *transient* error so the File Provider system re-drives the modification instead of
+        // committing a broken file. See F1 in the Adobe compatibility diagnosis.
+        //
+        // The error must be transient to get an automatic retry: per NSFileProviderReplicatedExtension,
+        // the resolvable NSFileProviderError codes (`.cannotSynchronize`, `.notAuthenticated`,
+        // `.excludedFromSync`, …) make the system back off until the provider calls
+        // `signalErrorResolved(_:)`. "Any other error … in NSCocoaErrorDomain" is treated as transient
+        // and retried. We leave the row in its `.uploading` state so the retry settles it to `.normal`.
         let contentAttributes = try? FileManager.default.attributesOfItem(atPath: newContents.path)
-        if let expectedSize = contentAttributes?[.size] as? Int64, size != expectedSize {
-            logger.info(
-                """
-                Item content modification upload reported as successful,
-                but there are differences between the received file size (\(size ?? -1))
-                and the original file size (\(documentSize?.int64Value ?? 0))
-                """
-            )
+
+        if let localSize = contentAttributes?[.size] as? Int64, let uploadedSize = size, uploadedSize != localSize {
+            logger.error("Upload integrity check failed for item: server stored \(uploadedSize) bytes but the local file is \(localSize) bytes. Returning a transient error so the system retries.", [.name: filename, .item: ocId])
+
+            return (nil, NSError(domain: NSCocoaErrorDomain, code: NSFileWriteUnknownError, userInfo: [
+                NSLocalizedDescriptionKey: "Upload integrity check failed: server stored \(uploadedSize) of \(localSize) bytes."
+            ]))
         }
 
         var newMetadata =

shell_integration/MacOSX/NextcloudFileProviderKit/Tests/Interface/MockRemoteInterface.swift

@@ -591,6 +591,14 @@ public class MockRemoteInterface: RemoteInterface, @unchecked Sendable {
     /// Use this to simulate server-side upload rejections (e.g. 404 path gone, 507 quota).
     public var uploadError: NKError?
 
+    /// When non-nil, the upload mock reports this as the stored size in its response, regardless
+    /// of the bytes actually written to the mock tree. Use this to simulate a torn/truncated
+    /// transfer (the server stores fewer/more bytes than the client sent) so the upload integrity
+    /// check in the production code can be exercised. The chunked upload path delegates to this
+    /// same `upload(...)`, so the override applies to both the single-shot and chunked paths.
+    /// `nil` echoes the real stored size.
+    public var uploadResponseSizeOverride: Int64?
+
     /// Handler to track enumerate calls
     public var enumerateCallHandler: ((String, EnumerateDepth, Bool, [String], Data?, Account, NKRequestOptions, @escaping (URLSessionTask) -> Void) -> Void)?
 
@@ -832,7 +840,7 @@ public class MockRemoteInterface: RemoteInterface, @unchecked Sendable {
             item.identifier,
             item.versionIdentifier,
             responseDate,
-            item.size,
+            uploadResponseSizeOverride ?? item.size,
             nil,
             .success
         )

shell_integration/MacOSX/NextcloudFileProviderKit/Tests/InterfaceTests/MockRemoteInterfaceTests.swift

@@ -503,7 +503,12 @@ final class MockRemoteInterfaceTests: XCTestCase {
         XCTAssertEqual(targetRootFile?.ocId, expectedRoot?.identifier)
         XCTAssertEqual(targetRootFile?.fileName, NextcloudKit.shared.nkCommonInstance.rootFileName) // NextcloudKit gives the root dir this name
         XCTAssertEqual(targetRootFile?.serverUrl, "https://mock.nc.com/remote.php/dav/files/testUserId") // NextcloudKit gives the root dir this url
-        XCTAssertEqual(targetRootFile?.date, expectedRoot?.creationDate)
+        // `toNKFile()` maps the item's `modificationDate` onto `NKFile.date`, so compare against
+        // that — not `creationDate`. `MockRemoteItem.init` defaults `creationDate` and
+        // `modificationDate` to two separate `Date()` calls, which are usually (but not always)
+        // identical; comparing `date` to `creationDate` therefore passed only when the clock did
+        // not tick between those two calls, and flaked on CI when it did.
+        XCTAssertEqual(targetRootFile?.date, expectedRoot?.modificationDate)
         XCTAssertEqual(targetRootFile?.etag, expectedRoot?.versionIdentifier)
 
         let resultChildren = await remoteInterface.enumerate(

shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemCreateTests.swift

@@ -124,6 +124,53 @@ final class ItemCreateTests: NextcloudFileProviderKitTestCase {
         XCTAssertTrue(createdItem.isUploaded)
     }
 
+    /// Upload integrity guard (F1): when the server reports it stored a different number of bytes
+    /// than the local file contains, `create` must NOT record the item as a clean upload. It
+    /// returns a *transient* error (so the File Provider system automatically retries the create)
+    /// and best-effort removes the partial remote object, instead of surfacing a torn file as synced.
+    func testCreateFileFailsOnUploadSizeMismatch() async throws {
+        let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem)
+        var fileItemMetadata = SendableItemMetadata(
+            ocId: "file-id", fileName: "file", account: Self.account
+        )
+        fileItemMetadata.classFile = NKTypeClassFile.document.rawValue
+
+        let tempUrl = FileManager.default.temporaryDirectory
+            .appendingPathComponent("integrity-mismatch-create")
+        try Data("Hello world".utf8).write(to: tempUrl)
+
+        // Simulate the server storing fewer bytes than we sent (a torn transfer).
+        remoteInterface.uploadResponseSizeOverride = 3
+
+        let fileItemTemplate = Item(
+            metadata: fileItemMetadata,
+            parentItemIdentifier: .rootContainer,
+            account: Self.account,
+            remoteInterface: remoteInterface,
+            dbManager: Self.dbManager
+        )
+        let (createdItemMaybe, error) = await Item.create(
+            basedOn: fileItemTemplate,
+            contents: tempUrl,
+            account: Self.account,
+            remoteInterface: remoteInterface,
+            progress: Progress(),
+            dbManager: Self.dbManager,
+            log: FileProviderLogMock()
+        )
+
+        XCTAssertNil(createdItemMaybe)
+
+        // The error must be *transient* (NSCocoaErrorDomain) so the system automatically retries
+        // the create rather than backing off until the provider signals resolution.
+        let nsError = try XCTUnwrap(error as NSError?)
+        XCTAssertEqual(nsError.domain, NSCocoaErrorDomain)
+        XCTAssertEqual(nsError.code, NSFileWriteUnknownError)
+
+        // Nothing must be recorded as a clean upload for this item.
+        XCTAssertNil(Self.dbManager.itemMetadata(ocId: "file-id"))
+    }
+
     func testCreateFile() async throws {
         let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem)
         var fileItemMetadata = SendableItemMetadata(

shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemModifyTests.swift

@@ -330,6 +330,66 @@ final class ItemModifyTests: NextcloudFileProviderKitTestCase {
         XCTAssertEqual(remoteItem.data, originalRemoteData)
     }
 
+    /// Upload integrity guard (F1): when the server reports it stored a different number of bytes
+    /// than the local file contains, the modify must NOT record the item as a clean upload. It
+    /// returns a *transient* error (so the File Provider system automatically retries the modify)
+    /// and leaves the row un-uploaded, instead of committing a truncated/torn file.
+    func testModifyFileFailsOnUploadSizeMismatch() async throws {
+        let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem)
+
+        var itemMetadata = remoteItem.toItemMetadata(account: Self.account)
+        itemMetadata.uploaded = true
+        itemMetadata.downloaded = true
+        Self.dbManager.addItemMetadata(itemMetadata)
+
+        let newContents = "Hello, New World!".data(using: .utf8)!
+        let newContentsUrl = FileManager.default.temporaryDirectory
+            .appendingPathComponent("integrity-mismatch-modify")
+        try newContents.write(to: newContentsUrl)
+
+        // Simulate the server storing fewer bytes than we sent (a torn transfer).
+        remoteInterface.uploadResponseSizeOverride = Int64(newContents.count - 1)
+
+        var targetItemMetadata = SendableItemMetadata(value: itemMetadata)
+        targetItemMetadata.size = Int64(newContents.count)
+
+        let item = Item(
+            metadata: itemMetadata,
+            parentItemIdentifier: .rootContainer,
+            account: Self.account,
+            remoteInterface: remoteInterface,
+            dbManager: Self.dbManager
+        )
+        let targetItem = Item(
+            metadata: targetItemMetadata,
+            parentItemIdentifier: .rootContainer,
+            account: Self.account,
+            remoteInterface: remoteInterface,
+            dbManager: Self.dbManager
+        )
+
+        let (modifiedItem, error) = await item.modify(
+            itemTarget: targetItem,
+            changedFields: [.contents],
+            contents: newContentsUrl,
+            dbManager: Self.dbManager
+        )
+
+        XCTAssertNil(modifiedItem)
+
+        // The error must be *transient* (NSCocoaErrorDomain, outside the resolvable
+        // NSFileProviderError set) so the system automatically retries the modify rather than
+        // backing off until the provider signals resolution.
+        let nsError = try XCTUnwrap(error as NSError?)
+        XCTAssertEqual(nsError.domain, NSCocoaErrorDomain)
+        XCTAssertEqual(nsError.code, NSFileWriteUnknownError)
+
+        // The item must not be recorded as a clean upload; it stays pending for the retry.
+        let dbItem = try XCTUnwrap(Self.dbManager.itemMetadata(ocId: itemMetadata.ocId))
+        XCTAssertFalse(dbItem.uploaded)
+        XCTAssertNotEqual(dbItem.status, Status.normal.rawValue)
+    }
+
     /// When the server returns 404 during an upload (the parent folder was renamed
     /// on another client while the file was open), the extension must:
     ///   - clear the stale lock token so the next attempt goes without an If: header