From 9d885434a5e693b431118cf50d2de312ee2f78d2 Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Mon, 31 Aug 2026 18:16:06 +0000 Subject: [PATCH 1/2] fix(macos): guard against empty ocId items crashing FileProvider Fix for #10701. Empty ocId metadata rows became File Provider items with an empty identifier, aborting the framework with __FILEPROVIDER_BAD_ITEM_MISSING_IDENTIFIER__ on both the 405 collision path and change enumeration, in a self sustaining crash loop. Reject empty ocId on write, return nil when resolving an empty identifier, and skip empty ocId rows during enumeration. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres --- .../Database/FilesDatabaseManager.swift | 7 ++ .../NextcloudFileProviderKit/Item/Item.swift | 7 ++ .../Metadata/SendableItemMetadata+Array.swift | 7 ++ .../MissingIdentifierCrashGuardTests.swift | 94 +++++++++++++++++++ 4 files changed, 115 insertions(+) create mode 100644 shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MissingIdentifierCrashGuardTests.swift diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index f462fcafb27fd..36c50c0f35398 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -538,6 +538,13 @@ public final class FilesDatabaseManager: Sendable { } public func addItemMetadata(_ metadata: SendableItemMetadata) { + // An empty ocId would persist a row keyed by "" and later become a File Provider item with + // an empty identifier, crashing the framework. Refuse it at the source. See #10701. + guard !metadata.ocId.isEmpty else { + logger.error("Refusing to add item metadata with empty ocId.", [.name: metadata.fileName, .url: metadata.serverUrl]) + return + } + let database = ncDatabase() do { diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item.swift index db14716fd6085..e0e38503f38fd 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item.swift @@ -483,6 +483,13 @@ public final class Item: NSObject, NSFileProviderItem, Sendable { public static func storedItem(identifier: NSFileProviderItemIdentifier, account: Account, remoteInterface: RemoteInterface, dbManager: FilesDatabaseManager, log: any FileProviderLogging) async -> Item? { // resolve the given identifier to a record in the model + // An empty identifier can only come from a corrupt empty-ocId row. Resolving it would vend + // an item with an empty identifier and crash the framework, e.g. via the 405 collision + // path. See #10701. + guard !identifier.rawValue.isEmpty else { + return nil + } + let remoteSupportsTrash = await remoteInterface.supportsTrash(account: account) guard identifier != .rootContainer else { diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Metadata/SendableItemMetadata+Array.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Metadata/SendableItemMetadata+Array.swift index b39be8f6b99eb..2a824098be0e8 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Metadata/SendableItemMetadata+Array.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Metadata/SendableItemMetadata+Array.swift @@ -13,6 +13,13 @@ extension [SendableItemMetadata] { let allFilters = await Item.getContextMenuItemTypeFilters(account: account, remoteInterface: remoteInterface) return try await concurrentChunkedCompactMap { (itemMetadata: SendableItemMetadata) -> Item? in + // A corrupt empty-ocId row would become an item with an empty identifier and crash the + // framework when reported to didUpdate/didEnumerate. Skip it. See #10701. + guard !itemMetadata.ocId.isEmpty else { + logger.error("Skipping metadata with empty ocId in enumeration.", [.name: itemMetadata.fileName, .url: itemMetadata.serverUrl]) + return nil + } + guard !itemMetadata.e2eEncrypted else { logger.info("Skipping encrypted metadata in enumeration.", [.item: itemMetadata.ocId, .name: itemMetadata.fileName]) return nil diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MissingIdentifierCrashGuardTests.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MissingIdentifierCrashGuardTests.swift new file mode 100644 index 0000000000000..ad4b04773ced4 --- /dev/null +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MissingIdentifierCrashGuardTests.swift @@ -0,0 +1,94 @@ +// SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +// SPDX-License-Identifier: LGPL-3.0-or-later + +@preconcurrency import FileProvider +@testable import NextcloudFileProviderKit +import NextcloudFileProviderKitMocks +import NextcloudKit +import RealmSwift +import TestInterface +import XCTest + +/// Guards against nextcloud/desktop#10701: metadata rows with an empty ocId become File Provider +/// items with an empty identifier, which crashes the framework with +/// `__FILEPROVIDER_BAD_ITEM_MISSING_IDENTIFIER__` on both create and change enumeration. +final class MissingIdentifierCrashGuardTests: NextcloudFileProviderKitTestCase { + static let account = Account( + user: "testUser", id: "testUserId", serverUrl: "https://mock.nc.com", password: "abcd" + ) + + var rootItem: MockRemoteItem! + var dbManager: FilesDatabaseManager! + + override func setUp() { + super.setUp() + Realm.Configuration.defaultConfiguration.inMemoryIdentifier = name + rootItem = MockRemoteItem.rootItem(account: Self.account) + dbManager = FilesDatabaseManager( + account: Self.account, + databaseDirectory: makeDatabaseDirectory(), + fileProviderDomainIdentifier: NSFileProviderDomainIdentifier("test"), + log: FileProviderLogMock() + ) + } + + /// Inserts a metadata row straight into Realm, bypassing `addItemMetadata`, to simulate a + /// database already poisoned by a previous corrupt write. + private func insertRaw(_ metadata: SendableItemMetadata) { + let database = dbManager.ncDatabase() + try! database.write { + database.add(RealmItemMetadata(value: metadata), update: .all) + } + } + + // Layer 1: the write side must never persist a row keyed by an empty ocId. + func testAddItemMetadataRejectsEmptyOcId() { + var metadata = SendableItemMetadata(ocId: "", fileName: "folder", account: Self.account) + metadata.directory = true + + dbManager.addItemMetadata(metadata) + + XCTAssertNil(dbManager.itemMetadata(ocId: "")) + } + + // Layer 2: resolving an empty identifier must never return an item, even if a poisoned row + // matches. This protects the 405 collision path that feeds the framework a colliding item. + func testStoredItemReturnsNilForEmptyIdentifier() async { + var poison = SendableItemMetadata(ocId: "", fileName: "folder", account: Self.account) + poison.directory = true + insertRaw(poison) + + let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem) + let item = await Item.storedItem( + identifier: .init(""), + account: Self.account, + remoteInterface: remoteInterface, + dbManager: dbManager, + log: FileProviderLogMock() + ) + + XCTAssertNil(item) + } + + // Layer 3: enumeration must skip empty-ocId rows instead of vending an item with an empty + // identifier to `didUpdate` / `didEnumerate`. + func testEnumerationSkipsEmptyOcIdMetadata() async throws { + let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem) + + var valid = SendableItemMetadata(ocId: "valid-id", fileName: "good.txt", account: Self.account) + valid.serverUrl = Self.account.davFilesUrl + var poison = SendableItemMetadata(ocId: "", fileName: "bad.txt", account: Self.account) + poison.serverUrl = Self.account.davFilesUrl + + let items = try await [valid, poison].toFileProviderItems( + account: Self.account, + remoteInterface: remoteInterface, + dbManager: dbManager, + log: FileProviderLogMock() + ) + + XCTAssertFalse(items.contains { $0.itemIdentifier.rawValue.isEmpty }) + XCTAssertEqual(items.count, 1) + XCTAssertEqual(items.first?.itemIdentifier.rawValue, "valid-id") + } +} From d5ac72422b4c79eef7cc55b9ac31fa4d8f9fee26 Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Tue, 1 Sep 2026 14:29:48 +0000 Subject: [PATCH 2/2] fix(macos): skip empty ocId rows while enumerating a folder. Skip empty ocId rows while enumerating a folder, and clear any left from before. Also skip empty identifiers in the deletion batch. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Camila Ayres --- .../Database/FilesDatabaseManager.swift | 22 +++- .../Enumerator+ObserverReporting.swift | 6 +- .../Item/Item+Create.swift | 10 +- .../Tests/Interface/MockRemoteInterface.swift | 6 +- .../MissingIdentifierCrashGuardTests.swift | 113 ++++++++++++++++++ 5 files changed, 152 insertions(+), 5 deletions(-) diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift index 36c50c0f35398..4a333620dd3f3 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Database/FilesDatabaseManager.swift @@ -470,7 +470,24 @@ public final class FilesDatabaseManager: Sendable { } } + // This path writes directly to Realm, bypassing addItemMetadata's guard. Drop any + // empty-ocId row here so it never becomes an item with an empty identifier. See #10701. + let droppedCount = metadatasToCreate.filter { $0.ocId.isEmpty }.count + + metadatasToUpdate.filter { $0.ocId.isEmpty }.count + if droppedCount > 0 { + logger.error("Dropping \(droppedCount) metadata row(s) with empty ocId during depth-1 ingestion.", [.url: serverUrl]) + } + metadatasToCreate = metadatasToCreate.filter { !$0.ocId.isEmpty } + metadatasToUpdate = metadatasToUpdate.filter { !$0.ocId.isEmpty } + try database.write { + // Self-heal a database poisoned by an earlier build: an empty-ocId row is keyed by "" + // and is always invalid, so remove it while we are enumerating. + let poisoned = database.objects(RealmItemMetadata.self).where { $0.ocId == "" } + if !poisoned.isEmpty { + database.delete(poisoned) + } + // Evict any logical-address duplicates before persisting fresh // payloads, so an ocId rotation (or rename whose target collides // with a third row) does not leave two non-deleted siblings at @@ -482,8 +499,9 @@ public final class FilesDatabaseManager: Sendable { evictLogicalDuplicates(of: metadata, in: database) } - // Do not delete the metadatas that have been deleted - database.add(metadatasToDelete.map { RealmItemMetadata(value: $0) }, update: .modified) + // Do not delete the metadatas that have been deleted. Skip empty-ocId markers so the + // self-heal above is not immediately undone. + database.add(metadatasToDelete.filter { !$0.ocId.isEmpty }.map { RealmItemMetadata(value: $0) }, update: .modified) database.add(metadatasToUpdate.map { RealmItemMetadata(value: $0) }, update: .modified) database.add(metadatasToCreate.map { RealmItemMetadata(value: $0) }, update: .all) } diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+ObserverReporting.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+ObserverReporting.swift index dfa9883e524a8..50e87937f76d1 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+ObserverReporting.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Enumeration/Enumerator+ObserverReporting.swift @@ -132,7 +132,11 @@ extension Enumerator { remoteInterface: RemoteInterface, dbManager: FilesDatabaseManager ) { - let deletedFileProviderItemIdentifiers = deleted.map { NSFileProviderItemIdentifier($0.ocId) } + // Never report an empty identifier to the framework: a corrupt empty-ocId row must not reach + // didDeleteItems. See #10701. + let deletedFileProviderItemIdentifiers = deleted.compactMap { + $0.ocId.isEmpty ? nil : NSFileProviderItemIdentifier($0.ocId) + } // Per-item trace so a debug archive can reconstruct exactly which items each batch carried. for metadata in updated { 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 16593d9ac7b69..2aed4e0ae008d 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Create.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Sources/NextcloudFileProviderKit/Item/Item+Create.swift @@ -101,6 +101,13 @@ public extension Item { return (nil, NSFileProviderError(.cannotSynchronize)) } + // The server response must carry an ocId. Returning an item with an empty identifier would + // crash the framework. See #10701. + guard !directory.ocId.isEmpty else { + logger.error("Refusing to return created folder with empty ocId.", [.url: remotePath]) + return (nil, NSFileProviderError(.cannotSynchronize)) + } + directory.downloaded = true directory.keepDownloaded = parentKeepDownloaded dbManager.addItemMetadata(directory) @@ -161,7 +168,8 @@ public extension Item { progressHandler: { $0.copyCurrentStateToProgress(progress) } ) - guard error == .success, let ocId else { + // Reject an empty ocId too: an item with an empty identifier crashes the framework. See #10701. + guard error == .success, let ocId, !ocId.isEmpty else { logger.error( """ Could not upload item with filename: \(itemTemplate.filename), diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/Interface/MockRemoteInterface.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/Interface/MockRemoteInterface.swift index f41010ef83325..31792cb6a6c13 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/Interface/MockRemoteInterface.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/Interface/MockRemoteInterface.swift @@ -606,6 +606,10 @@ 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 set, the next created folder uses this as its identifier (including "") instead of a + /// random one. Simulates a server response lacking an ocId. See #10701. + public var createFolderIdentifierOverride: String? + /// Records the `If-Match` header the most recent upload call carried (nil if none). /// Lets tests assert the optimistic-concurrency precondition was sent, and with /// which etag. Captured before any injected `uploadError` short-circuit. @@ -762,7 +766,7 @@ public class MockRemoteInterface: RemoteInterface, @unchecked Sendable { } let item = MockRemoteItem( - identifier: randomIdentifier(), + identifier: createFolderIdentifierOverride ?? randomIdentifier(), name: itemName, remotePath: remotePath, directory: true, diff --git a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MissingIdentifierCrashGuardTests.swift b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MissingIdentifierCrashGuardTests.swift index ad4b04773ced4..21d966dbc17ff 100644 --- a/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MissingIdentifierCrashGuardTests.swift +++ b/shell_integration/MacOSX/NextcloudFileProviderKit/Tests/NextcloudFileProviderKitTests/MissingIdentifierCrashGuardTests.swift @@ -91,4 +91,117 @@ final class MissingIdentifierCrashGuardTests: NextcloudFileProviderKitTestCase { XCTAssertEqual(items.count, 1) XCTAssertEqual(items.first?.itemIdentifier.rawValue, "valid-id") } + + // Producer: depth-1 PROPFIND ingestion writes directly to Realm, bypassing addItemMetadata. An + // empty-ocId row here is the most likely source of the poison. It must be dropped on write. + func testDepthOneIngestionDropsEmptyOcIdRows() { + let folderPath = Self.account.davFilesUrl + "/folder" + + var target = SendableItemMetadata(ocId: "target-dir", fileName: "folder", account: Self.account) + target.directory = true + target.serverUrl = Self.account.davFilesUrl + var valid = SendableItemMetadata(ocId: "child-valid", fileName: "good.txt", account: Self.account) + valid.serverUrl = folderPath + var poison = SendableItemMetadata(ocId: "", fileName: "bad.txt", account: Self.account) + poison.serverUrl = folderPath + + let changeSet = dbManager.depth1ReadUpdateItemMetadatas( + account: Self.account.ncKitAccount, + serverUrl: folderPath, + updatedMetadatas: [target, valid, poison], + keepExistingDownloadState: false + ) + + XCTAssertNil(dbManager.itemMetadata(ocId: "")) + XCTAssertNotNil(dbManager.itemMetadata(ocId: "child-valid")) + XCTAssertFalse((changeSet?.created ?? []).contains { $0.ocId.isEmpty }) + } + + // Self-heal: a database already poisoned by a previous build must shed its empty-ocId row the + // next time the containing folder is enumerated, not keep it forever. + func testDepthOneIngestionPurgesExistingEmptyOcIdRow() { + let folderPath = Self.account.davFilesUrl + "/folder" + + var poison = SendableItemMetadata(ocId: "", fileName: "bad.txt", account: Self.account) + poison.serverUrl = folderPath + insertRaw(poison) + XCTAssertNotNil(dbManager.itemMetadata(ocId: "")) + + var target = SendableItemMetadata(ocId: "target-dir", fileName: "folder", account: Self.account) + target.directory = true + target.serverUrl = Self.account.davFilesUrl + + _ = dbManager.depth1ReadUpdateItemMetadatas( + account: Self.account.ncKitAccount, + serverUrl: folderPath, + updatedMetadatas: [target], + keepExistingDownloadState: false + ) + + XCTAssertNil(dbManager.itemMetadata(ocId: "")) + } + + // Deletion path: an empty-ocId row that disappears remotely must not be reported to + // didDeleteItems as an empty identifier. + func testChangeBatchSkipsEmptyOcIdDeletions() throws { + let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem) + let enumerator = try Enumerator( + enumeratedItemIdentifier: .workingSet, + account: Self.account, + remoteInterface: remoteInterface, + dbManager: dbManager, + log: FileProviderLogMock() + ) + let observer = MockChangeObserver(enumerator: enumerator) + + let valid = SendableItemMetadata(ocId: "del-valid", fileName: "a.txt", account: Self.account) + let poison = SendableItemMetadata(ocId: "", fileName: "b.txt", account: Self.account) + + enumerator.completeChangesBatch( + observer, + updated: [], + deleted: [valid, poison], + anchor: Enumerator.syncAnchor(at: Date(timeIntervalSince1970: 1)), + moreComing: false, + account: Self.account, + remoteInterface: remoteInterface, + dbManager: dbManager + ) + + XCTAssertEqual(observer.deletedItemIdentifiers.map(\.rawValue), ["del-valid"]) + } + + // Create callback: when the server yields an item without an identifier, create must return an + // error rather than hand the framework an item with an empty identifier. + func testCreateFolderReturnsErrorWhenServerOcIdEmpty() async throws { + let remoteInterface = MockRemoteInterface(account: Self.account, rootItem: rootItem) + remoteInterface.createFolderIdentifierOverride = "" + + var folderMeta = SendableItemMetadata(ocId: "template-id", fileName: "folder", account: Self.account) + folderMeta.directory = true + folderMeta.classFile = NKTypeClassFile.directory.rawValue + folderMeta.serverUrl = Self.account.davFilesUrl + + let template = Item( + metadata: folderMeta, + parentItemIdentifier: .rootContainer, + account: Self.account, + remoteInterface: remoteInterface, + dbManager: dbManager + ) + + let (created, error) = await Item.create( + basedOn: template, + contents: nil, + account: Self.account, + remoteInterface: remoteInterface, + progress: Progress(), + dbManager: dbManager, + log: FileProviderLogMock() + ) + + XCTAssertNil(created) + XCTAssertNotNil(error) + XCTAssertNil(dbManager.itemMetadata(ocId: "")) + } }