From 761a3f4faaf3c79d75222ae220f833561b60d680 Mon Sep 17 00:00:00 2001 From: Iva Horn Date: Tue, 14 Jul 2026 16:19:04 +0200 Subject: [PATCH 1/2] fix(file-provider): Check for upload size mismatch Signed-off-by: Iva Horn --- .../Item/Item+Create.swift | 33 +++++++--- .../Item/Item+Modify.swift | 26 +++++--- .../Tests/Interface/MockRemoteInterface.swift | 10 +++- .../ItemCreateTests.swift | 47 +++++++++++++++ .../ItemModifyTests.swift | 60 +++++++++++++++++++ 5 files changed, 159 insertions(+), 17 deletions(-) diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Create.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Create.swift index f2b7053f81082..5af0c6459d0f6 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Create.swift +++ b/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 { diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Modify.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Modify.swift index a868f9a97214a..258066d8aa89c 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Modify.swift +++ b/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 = diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/Interface/MockRemoteInterface.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/Interface/MockRemoteInterface.swift index 387a7f4178ee3..a77ebd55bc4a2 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/Interface/MockRemoteInterface.swift +++ b/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 ) diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemCreateTests.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemCreateTests.swift index fbaeb2d8f5eca..d2aef551219ec 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemCreateTests.swift +++ b/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( diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemModifyTests.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemModifyTests.swift index 213c183383282..70b5cc555591f 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/ItemModifyTests.swift +++ b/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 From facba71fa9b48d0989dbd61a94d5d8d54a32f23b Mon Sep 17 00:00:00 2001 From: Iva Horn Date: Wed, 15 Jul 2026 16:11:40 +0200 Subject: [PATCH 2/2] fix(file-provider): Fix flaky testEnumerate root date assertion MockRemoteInterfaceTests.testEnumerate compared the enumerated root file's date against the item's creationDate, but toNKFile() maps modificationDate onto NKFile.date. MockRemoteItem defaults creationDate and modificationDate to two separate Date() calls that are only sometimes identical, so the assertion flaked on CI when the clock ticked between them. Compare against modificationDate, the field the value is actually derived from. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Iva Horn --- .../Tests/InterfaceTests/MockRemoteInterfaceTests.swift | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/InterfaceTests/MockRemoteInterfaceTests.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/InterfaceTests/MockRemoteInterfaceTests.swift index 1001a0a8b753d..d1beb46228a74 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/InterfaceTests/MockRemoteInterfaceTests.swift +++ b/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(