Skip to content
Draft
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 @@ -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
Expand All @@ -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)
}
Expand Down Expand Up @@ -538,6 +556,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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -762,7 +766,7 @@ public class MockRemoteInterface: RemoteInterface, @unchecked Sendable {
}

let item = MockRemoteItem(
identifier: randomIdentifier(),
identifier: createFolderIdentifierOverride ?? randomIdentifier(),
name: itemName,
remotePath: remotePath,
directory: true,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
// 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")
}

// 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: ""))
}
}