From 88e40449e1641be8db5d092498a9a4cb84c37eaf Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Mon, 31 Aug 2026 18:10:27 +0700 Subject: [PATCH] fix(sync): bound a CloudKit record name so a long database path cannot crash the app Claude-Session: https://claude.ai/code/session_01KhHdFvjmq8f8cEFyx5WGiv --- CHANGELOG.md | 1 + .../SyncRecordType.swift | 33 ++++++- .../SyncRecordNameConstructionTests.swift | 76 ++++++++++++++++ .../SyncRecordTypeTests.swift | 59 +++++++++++++ TablePro/Core/Sync/SyncCoordinator.swift | 23 ++++- TablePro/Core/Sync/SyncRecordIdentity.swift | 14 +++ TablePro/Core/Sync/SyncRecordMapper.swift | 17 ++++ .../Core/Storage/ColumnLayoutSyncTests.swift | 21 +++++ .../Core/Sync/SyncRecordIdentityTests.swift | 77 ++++++++++++++++ scripts/check-cloudkit-record-name-limit.sh | 87 +++++++++++++++++++ 10 files changed, 403 insertions(+), 5 deletions(-) create mode 100644 Packages/TableProCore/Tests/TableProSyncTests/SyncRecordNameConstructionTests.swift create mode 100644 TablePro/Core/Sync/SyncRecordIdentity.swift create mode 100644 TableProTests/Core/Sync/SyncRecordIdentityTests.swift create mode 100755 scripts/check-cloudkit-record-name-limit.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index fac3bf2fc..0e37dbd63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Cell editor opening off screen when `Tab` wrapped onto a row below the visible ones. - Cell cursor left on the old column after `Tab` carried the editor to the next one. - Every data grid switching to its accessibility layout after one `Tab` press, with no assistive app attached. +- Crash loop on every launch after resizing a column on a database with a long file path, with iCloud sync on. (#2575) ## [0.69.0] - 2026-08-27 diff --git a/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordType.swift b/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordType.swift index ec291b62b..d3f38b610 100644 --- a/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordType.swift +++ b/Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordType.swift @@ -1,3 +1,4 @@ +import CryptoKit import Foundation public enum SyncRecordType: String, CaseIterable, Sendable { @@ -25,10 +26,23 @@ public enum SyncRecordType: String, CaseIterable, Sendable { } } + /// A name longer than `SyncRecordName.maximumLength` carries a digest of the identifier in + /// place of the identifier. `CKRecord.ID(recordName:)` raises `CKException` past that length, + /// and an Objective-C exception raised inside a Swift task leaves the concurrency runtime + /// unwound, which crashes the app seconds later from an unrelated call site. A settings + /// category embeds a database name, so on SQLite it embeds a percent-encoded file path and + /// has no bound at all (#2575). + /// + /// Every name that already fits is returned unchanged, so records that reached iCloud keep + /// their identity. A name that did not fit could never be written, so nothing is orphaned. public func recordName(for id: String) -> String { - recordNamePrefix + id + let name = recordNamePrefix + id + guard (name as NSString).length > SyncRecordName.maximumLength else { return name } + return recordNamePrefix + SyncRecordName.digestPrefix + SyncRecordName.digest(of: id) } + /// The identifier is only recoverable when `recordName(for:)` did not shorten it, so the push + /// path resolves a saved record through the identifiers it sent rather than through this. public static func parse(recordName: String) -> (type: SyncRecordType, id: String)? { for type in longestPrefixFirst where recordName.hasPrefix(type.recordNamePrefix) { return (type, String(recordName.dropFirst(type.recordNamePrefix.count))) @@ -39,3 +53,20 @@ public enum SyncRecordType: String, CaseIterable, Sendable { private static let longestPrefixFirst: [SyncRecordType] = allCases .sorted { $0.recordNamePrefix.count > $1.recordNamePrefix.count } } + +/// CloudKit's own limit on a record name, and how a name that would exceed it is shortened. +public enum SyncRecordName { + /// Measured against the CloudKit framework: 255 UTF-16 code units pass and 256 raise, and the + /// count is of UTF-16 units rather than characters or bytes (250 two-byte characters pass at + /// 500 UTF-8 bytes; 128 emoji raise at 256 UTF-16 units). `scripts/check-cloudkit-record-name-limit.sh` + /// re-measures it. + public static let maximumLength = 255 + + /// Names the shortening in the CloudKit dashboard, and keeps a digest from colliding with an + /// identifier that is genuinely 64 hexadecimal characters, such as a favorite's sync id. + public static let digestPrefix = "sha256-" + + public static func digest(of id: String) -> String { + SHA256.hash(data: Data(id.utf8)).map { String(format: "%02x", $0) }.joined() + } +} diff --git a/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordNameConstructionTests.swift b/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordNameConstructionTests.swift new file mode 100644 index 000000000..e4e0fd3cc --- /dev/null +++ b/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordNameConstructionTests.swift @@ -0,0 +1,76 @@ +import Foundation +import Testing + +@Suite("Record names are only built where CloudKit's length limit is enforced") +struct SyncRecordNameConstructionTests { + private static let repositoryRoot = URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() + .deletingLastPathComponent() + .deletingLastPathComponent() + .deletingLastPathComponent() + .deletingLastPathComponent() + + private static let sourceRoots = [ + "TablePro", + "TableProMobile/TableProMobile", + "Packages/TableProCore/Sources" + ] + + private static let permittedPaths: Set = [ + "TablePro/Core/Sync/SyncRecordMapper.swift", + "Packages/TableProCore/Sources/TableProSync/SyncRecordMapper.swift" + ] + + @Test("Every source root the check covers is where the check expects it") + func sourceRootsExist() { + for path in Self.sourceRoots { + let url = Self.repositoryRoot.appendingPathComponent(path) + #expect(FileManager.default.fileExists(atPath: url.path), """ + \(path) has moved, so this check would pass vacuously. Update sourceRoots. + """) + } + } + + @Test("Every mapper the check permits is where the check expects it") + func permittedMappersExist() { + for path in Self.permittedPaths { + let url = Self.repositoryRoot.appendingPathComponent(path) + #expect(FileManager.default.fileExists(atPath: url.path), """ + \(path) has moved. Update permittedPaths. + """) + } + } + + @Test("No shipping source constructs a CKRecord.ID outside the mappers") + func recordIdsComeFromTheMappers() { + var offenders: [String] = [] + + for root in Self.sourceRoots { + let rootURL = Self.repositoryRoot.appendingPathComponent(root) + guard let files = FileManager.default.enumerator( + at: rootURL, + includingPropertiesForKeys: nil + ) else { continue } + + for case let url as URL in files where url.pathExtension == "swift" { + let path = url.path.replacingOccurrences(of: Self.repositoryRoot.path + "/", with: "") + guard !Self.permittedPaths.contains(path), + let source = try? String(contentsOf: url, encoding: .utf8) else { continue } + + for (offset, line) in source.components(separatedBy: .newlines).enumerated() { + let code = line.trimmingCharacters(in: .whitespaces) + guard !code.hasPrefix("//"), code.contains("CKRecord.ID(recordName:") else { continue } + offenders.append("\(path):\(offset + 1): \(code)") + } + } + } + + #expect(offenders.isEmpty, """ + CKRecord.ID(recordName:) raises CKException past 255 UTF-16 code units, and an \ + Objective-C exception raised inside a Swift task crashes the app from an unrelated call \ + site seconds later. SyncRecordType.recordName(for:) is the only thing that bounds the \ + name, so go through SyncRecordMapper.recordID(type:id:in:). + \(offenders.joined(separator: "\n")) + """) + } +} diff --git a/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordTypeTests.swift b/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordTypeTests.swift index 0d927a47f..c8d75fe04 100644 --- a/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordTypeTests.swift +++ b/Packages/TableProCore/Tests/TableProSyncTests/SyncRecordTypeTests.swift @@ -62,4 +62,63 @@ struct SyncRecordTypeTests { let prefixes = SyncRecordType.allCases.map(\.recordNamePrefix) #expect(Set(prefixes).count == prefixes.count) } + + @Test("A name that already fits is returned unchanged", arguments: SyncRecordType.allCases) + func namesThatFitAreUnchanged(_ type: SyncRecordType) { + let id = String(repeating: "a", count: SyncRecordName.maximumLength - type.recordNamePrefix.count) + let name = type.recordName(for: id) + #expect(name == type.recordNamePrefix + id) + #expect((name as NSString).length == SyncRecordName.maximumLength) + } + + @Test("A name one unit too long is shortened", arguments: SyncRecordType.allCases) + func namesPastTheLimitAreShortened(_ type: SyncRecordType) { + let id = String(repeating: "a", count: SyncRecordName.maximumLength - type.recordNamePrefix.count + 1) + let name = type.recordName(for: id) + #expect((name as NSString).length <= SyncRecordName.maximumLength) + #expect(name.hasPrefix(type.recordNamePrefix + SyncRecordName.digestPrefix)) + } + + /// The limit CloudKit enforces counts UTF-16 code units, so a name of 128 emoji is over it at + /// 128 characters. `scripts/check-cloudkit-record-name-limit.sh` measures that. + @Test("The limit counts UTF-16 code units, not characters") + func theLimitCountsUTF16CodeUnits() { + let id = String(repeating: "😀", count: 200) + #expect(id.count < SyncRecordName.maximumLength) + let name = SyncRecordType.settings.recordName(for: id) + #expect((name as NSString).length <= SyncRecordName.maximumLength) + #expect(name.hasPrefix("Settings_" + SyncRecordName.digestPrefix)) + } + + @Test("Shortening is stable, so two devices agree on the record") + func shorteningIsDeterministic() { + let id = String(repeating: "path/to/database.sqlite", count: 40) + #expect(SyncRecordType.settings.recordName(for: id) == SyncRecordType.settings.recordName(for: id)) + #expect( + SyncRecordType.settings.recordName(for: id) == "Settings_sha256-" + + SyncRecordName.digest(of: id) + ) + } + + @Test("Two long identifiers do not collapse onto one record") + func distinctLongIdentifiersStayDistinct() { + let base = String(repeating: "a", count: 300) + #expect(SyncRecordType.settings.recordName(for: base) != SyncRecordType.settings.recordName(for: base + "b")) + } + + /// The column layout category that produced #2575: a connection UUID, a percent-encoded + /// SQLite file path, an empty schema and a table name. + @Test("A long SQLite path produces a name CloudKit accepts") + func aLongSQLitePathFits() { + let path = "/Users/example/projects/acme/api/.wrangler/state/v3/d1" + + "/miniflare-D1DatabaseObject/" + + String(repeating: "f", count: 64) + ".sqlite" + let parts = [UUID().uuidString, path, "", "d1_migrations"] + .map { $0.addingPercentEncoding(withAllowedCharacters: .alphanumerics) ?? $0 } + let category = "columnLayout." + parts.joined(separator: ".") + + #expect((("Settings_" + category) as NSString).length > SyncRecordName.maximumLength) + #expect((SyncRecordType.settings.recordName(for: category) as NSString).length + <= SyncRecordName.maximumLength) + } } diff --git a/TablePro/Core/Sync/SyncCoordinator.swift b/TablePro/Core/Sync/SyncCoordinator.swift index a033745b6..67e027f3e 100644 --- a/TablePro/Core/Sync/SyncCoordinator.swift +++ b/TablePro/Core/Sync/SyncCoordinator.swift @@ -391,19 +391,20 @@ final class SyncCoordinator { guard !recordsToSave.isEmpty || !uniqueDeletions.isEmpty else { return } + let identities = SyncRecordMapper.identities(for: pushedLocalIds(), in: zoneID) let outcome = try await engine.push(records: recordsToSave, deletions: uniqueDeletions) recordCache.store(Array(outcome.savedRecords.values)) recordCache.remove(Array(outcome.deletedRecordIDs)) for recordID in outcome.savedRecords.keys { - guard let parsed = SyncRecordMapper.parse(recordName: recordID.recordName) else { continue } - changeTracker.clearDirty(parsed.type, id: parsed.id) + guard let identity = identities[recordID] else { continue } + changeTracker.clearDirty(identity.type, id: identity.id) } for recordID in outcome.deletedRecordIDs { - guard let parsed = SyncRecordMapper.parse(recordName: recordID.recordName) else { continue } - metadataStorage.removeTombstone(parsed.id, type: parsed.type) + guard let identity = identities[recordID] else { continue } + metadataStorage.removeTombstone(identity.id, type: identity.type) } let savedCount = outcome.savedRecords.count @@ -417,6 +418,20 @@ final class SyncCoordinator { throw SyncError.pushRejected(count: outcome.failures.count, detail: firstFailure.message) } + /// Every local identifier this push can have sent. `SyncChangeTracker` is not isolated to this + /// actor, so the sets can move under an await; a record whose identifier is missing from the + /// snapshot is left dirty and pushed again rather than cleared against the wrong entry. + private func pushedLocalIds() -> [SyncRecordType: Set] { + var localIds: [SyncRecordType: Set] = [:] + for type in SyncRecordType.allCases { + let ids = changeTracker.dirtyRecords(for: type) + .union(metadataStorage.tombstones(for: type).map(\.id)) + guard !ids.isEmpty else { continue } + localIds[type] = ids + } + return localIds + } + // MARK: - Pull nonisolated static func isTokenExpired(_ error: Error) -> Bool { diff --git a/TablePro/Core/Sync/SyncRecordIdentity.swift b/TablePro/Core/Sync/SyncRecordIdentity.swift new file mode 100644 index 000000000..ac7bbcbe1 --- /dev/null +++ b/TablePro/Core/Sync/SyncRecordIdentity.swift @@ -0,0 +1,14 @@ +// +// SyncRecordIdentity.swift +// TablePro +// + +import Foundation +import TableProSyncTransport + +/// The local identifier behind a record the push sent, kept because a CloudKit record name is an +/// identity rather than an encoding of that identifier. +struct SyncRecordIdentity: Hashable, Sendable { + let type: SyncRecordType + let id: String +} diff --git a/TablePro/Core/Sync/SyncRecordMapper.swift b/TablePro/Core/Sync/SyncRecordMapper.swift index 86b7b0767..a63c2c2e9 100644 --- a/TablePro/Core/Sync/SyncRecordMapper.swift +++ b/TablePro/Core/Sync/SyncRecordMapper.swift @@ -45,6 +45,23 @@ struct SyncRecordMapper { SyncRecordType.parse(recordName: recordName) } + /// Maps every record the push is about to send back onto the local identifier it was built + /// from. `SyncRecordType.recordName(for:)` shortens an identifier that would take the name + /// past CloudKit's limit, so a saved record cannot be read back through `parse(recordName:)` + /// without clearing the wrong dirty entry and pushing the same record on every sync forever. + static func identities( + for localIds: [SyncRecordType: Set], + in zone: CKRecordZone.ID + ) -> [CKRecord.ID: SyncRecordIdentity] { + var identities: [CKRecord.ID: SyncRecordIdentity] = [:] + for (type, ids) in localIds { + for id in ids { + identities[recordID(type: type, id: id, in: zone)] = SyncRecordIdentity(type: type, id: id) + } + } + return identities + } + // MARK: - Connection static func toCKRecord( diff --git a/TableProTests/Core/Storage/ColumnLayoutSyncTests.swift b/TableProTests/Core/Storage/ColumnLayoutSyncTests.swift index ed1aaa3b4..f165b45f6 100644 --- a/TableProTests/Core/Storage/ColumnLayoutSyncTests.swift +++ b/TableProTests/Core/Storage/ColumnLayoutSyncTests.swift @@ -59,4 +59,25 @@ struct ColumnLayoutSyncTests { func categoryPrefix() { #expect(FileColumnLayoutPersister.syncCategory(for: "abc").hasPrefix(FileColumnLayoutPersister.syncCategoryPrefix)) } + + /// A SQLite database name is a file path, and the storage key percent-encodes every character + /// that is not alphanumeric, so a wrangler path takes the record name past what CloudKit + /// accepts. `CKRecord.ID(recordName:)` raised there, and the app crashed seconds later from an + /// unrelated call site, on every launch (#2575). + @Test("A long SQLite path still yields a record name CloudKit accepts") + func longSQLitePathYieldsAcceptableRecordName() { + let path = "/Users/example/projects/acme/api/.wrangler/state/v3/d1" + + "/miniflare-D1DatabaseObject/" + String(repeating: "f", count: 64) + ".sqlite" + let tableKey = ColumnLayoutTableKey( + connectionId: UUID(), + databaseName: path, + schemaName: nil, + tableName: "d1_migrations" + ) + let category = FileColumnLayoutPersister.syncCategory(for: tableKey.storageKey) + + #expect((("Settings_" + category) as NSString).length > SyncRecordName.maximumLength) + #expect((SyncRecordType.settings.recordName(for: category) as NSString).length + <= SyncRecordName.maximumLength) + } } diff --git a/TableProTests/Core/Sync/SyncRecordIdentityTests.swift b/TableProTests/Core/Sync/SyncRecordIdentityTests.swift new file mode 100644 index 000000000..d73727f8c --- /dev/null +++ b/TableProTests/Core/Sync/SyncRecordIdentityTests.swift @@ -0,0 +1,77 @@ +// +// SyncRecordIdentityTests.swift +// TableProTests +// + +import CloudKit +import Foundation +@testable import TablePro +import TableProSyncTransport +import Testing + +@Suite("Push identities survive a shortened record name") +@MainActor +struct SyncRecordIdentityTests { + private static let zone = CKRecordZone.ID(zoneName: "TableProZone", ownerName: CKCurrentUserDefaultName) + + private static func longColumnLayoutCategory() -> String { + let path = "/Users/example/projects/acme/api/.wrangler/state/v3/d1" + + "/miniflare-D1DatabaseObject/" + String(repeating: "f", count: 64) + ".sqlite" + let key = ColumnLayoutTableKey( + connectionId: UUID(), + databaseName: path, + schemaName: nil, + tableName: "d1_migrations" + ) + return FileColumnLayoutPersister.syncCategory(for: key.storageKey) + } + + @Test("A category too long for a record name still resolves back to itself") + func longCategoryResolvesBack() { + let category = Self.longColumnLayoutCategory() + let identities = SyncRecordMapper.identities(for: [.settings: [category]], in: Self.zone) + let recordID = SyncRecordMapper.recordID(type: .settings, id: category, in: Self.zone) + + #expect(identities[recordID] == SyncRecordIdentity(type: .settings, id: category)) + } + + /// The record name carries a digest once it is shortened, so the identifier the push needs + /// back is not in it. Reading it out of the name clears the wrong dirty entry, which leaves + /// the real one dirty and pushes the same record on every sync forever. + @Test("The shortened record name no longer carries the category") + func aShortenedNameDoesNotCarryTheCategory() { + let category = Self.longColumnLayoutCategory() + #expect((("Settings_" + category) as NSString).length > SyncRecordName.maximumLength, """ + The fixture no longer exceeds CloudKit's limit, so this check would pass vacuously. + """) + + let recordID = SyncRecordMapper.recordID(type: .settings, id: category, in: Self.zone) + let parsed = SyncRecordMapper.parse(recordName: recordID.recordName) + + #expect((recordID.recordName as NSString).length <= SyncRecordName.maximumLength) + #expect(parsed?.type == .settings) + #expect(parsed?.id != category) + } + + @Test("A short identifier resolves back without being shortened") + func shortIdentifierResolvesBack() { + let id = UUID().uuidString + let identities = SyncRecordMapper.identities(for: [.connection: [id]], in: Self.zone) + let recordID = SyncRecordMapper.recordID(type: .connection, id: id, in: Self.zone) + + #expect(recordID.recordName == "Connection_" + id) + #expect(identities[recordID] == SyncRecordIdentity(type: .connection, id: id)) + } + + @Test("Identities keep every type apart") + func identitiesKeepTypesApart() { + let id = UUID().uuidString + let identities = SyncRecordMapper.identities( + for: [.connection: [id], .group: [id], .tag: [id]], + in: Self.zone + ) + + #expect(identities.count == 3) + #expect(identities[SyncRecordMapper.recordID(type: .group, id: id, in: Self.zone)]?.type == .group) + } +} diff --git a/scripts/check-cloudkit-record-name-limit.sh b/scripts/check-cloudkit-record-name-limit.sh new file mode 100755 index 000000000..b574aeee7 --- /dev/null +++ b/scripts/check-cloudkit-record-name-limit.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +set -euo pipefail + +# CloudKit record name limit gate. +# +# SyncRecordName.maximumLength hard-codes what CKRecord.ID(recordName:) accepts. The value is not +# in a header and CloudKit raises an Objective-C exception rather than returning nil, so a wrong +# number is a crash rather than a rejected write, and the crash lands seconds later on an unrelated +# thread (#2575). This measures the real framework and compares it against the constant. +# +# Usage: scripts/check-cloudkit-record-name-limit.sh + +cd "$(dirname "$0")/.." + +SOURCE="Packages/TableProCore/Sources/TableProSyncTransport/SyncRecordType.swift" + +if [[ -z "${DEVELOPER_DIR:-}" ]]; then + for candidate in /Applications/Xcode.app /Applications/Xcode-beta.app; do + if [[ -d "$candidate" ]]; then + export DEVELOPER_DIR="$candidate/Contents/Developer" + break + fi + done +fi + +declared=$(sed -n 's/.*maximumLength = \([0-9][0-9]*\).*/\1/p' "$SOURCE" | head -1) +if [[ -z "$declared" ]]; then + echo "FAIL: could not read maximumLength from $SOURCE" >&2 + exit 1 +fi + +workdir=$(mktemp -d) +trap 'rm -rf "$workdir"' EXIT + +cat > "$workdir/probe.m" <<'OBJC' +#import +#import + +static BOOL accepts(NSUInteger length) { + NSString *name = [@"" stringByPaddingToLength:length withString:@"a" startingAtIndex:0]; + CKRecordZoneID *zone = [[CKRecordZoneID alloc] initWithZoneName:@"probe" + ownerName:CKCurrentUserDefaultName]; + @try { + (void)[[CKRecordID alloc] initWithRecordName:name zoneID:zone]; + return YES; + } @catch (NSException *exception) { + return NO; + } +} + +int main(void) { + @autoreleasepool { + NSUInteger low = 1; + NSUInteger high = 4096; + if (accepts(high)) { + printf("%lu\n", (unsigned long)high); + return 0; + } + while (low + 1 < high) { + NSUInteger mid = (low + high) / 2; + if (accepts(mid)) { + low = mid; + } else { + high = mid; + } + } + printf("%lu\n", (unsigned long)low); + } + return 0; +} +OBJC + +clang -fobjc-arc -framework CloudKit -framework Foundation \ + -o "$workdir/probe" "$workdir/probe.m" + +measured=$("$workdir/probe") + +echo "declared SyncRecordName.maximumLength: $declared" +echo "measured CKRecord.ID limit: $measured" + +if [[ "$declared" != "$measured" ]]; then + echo "FAIL: CloudKit accepts $measured UTF-16 code units, the source declares $declared." >&2 + echo "Update SyncRecordName.maximumLength in $SOURCE." >&2 + exit 1 +fi + +echo "PASS"