Skip to content

Commit 49205ae

Browse files
crazytanclaude
andcommitted
Fix four data-loss bugs in KDBX round-trip serialization
- parseKPDate: try base64 decode first so timestamps containing 'T' or '-' are no longer misrouted through the ISO-8601 path and silently dropped (~25 entries affected on a real-world Strongbox database) - Stop trimming whitespace from <Value> text on parse — only trim the base64 ciphertext of protected values before decoding - Preserve the original `otp` (otpauth://) URI instead of decomposing into TimeOtp-* fields, which drops issuer/label and custom parameters - Emit empty <Tags></Tags> elements so they survive the round-trip instead of being silently dropped Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 4a8a219 commit 49205ae

5 files changed

Lines changed: 160 additions & 15 deletions

File tree

KeeForge/Models/DatabaseDraft.swift

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,7 @@ struct DatabaseDraft: Sendable {
222222
url: draft.url,
223223
notes: draft.notes,
224224
tags: draft.tags,
225+
hasTagsElement: !draft.tags.isEmpty,
225226
customFields: draft.customFields,
226227
totpConfig: try makeTOTPConfig(from: draft.totpConfig),
227228
creationTime: timestamp,
@@ -243,8 +244,10 @@ struct DatabaseDraft: Sendable {
243244
notes: draft.notes,
244245
iconID: originalEntry.iconID,
245246
tags: draft.tags,
247+
hasTagsElement: originalEntry.hasTagsElement || !draft.tags.isEmpty,
246248
customFields: draft.customFields,
247249
totpConfig: try makeTOTPConfig(from: draft.totpConfig),
250+
otpURL: preservedOtpURL(draft: draft, originalEntry: originalEntry),
248251
creationTime: originalEntry.creationTime,
249252
lastModificationTime: timestamp,
250253
unknownXML: originalEntry.unknownXML,
@@ -255,6 +258,27 @@ struct DatabaseDraft: Sendable {
255258
)
256259
}
257260

261+
private func preservedOtpURL(
262+
draft: EntryDraftPayload,
263+
originalEntry: KPEntry
264+
) -> String? {
265+
guard let url = originalEntry.otpURL,
266+
let draftConfig = draft.totpConfig,
267+
let originalConfig = originalEntry.totpConfig,
268+
let originalSecret = try? originalConfig.secret.decrypt(using: sessionKey)
269+
else {
270+
return nil
271+
}
272+
guard draftConfig.secret == originalSecret,
273+
draftConfig.period == originalConfig.period,
274+
draftConfig.digits == originalConfig.digits,
275+
draftConfig.algorithm == originalConfig.algorithm
276+
else {
277+
return nil
278+
}
279+
return url
280+
}
281+
258282
private func makeTOTPConfig(
259283
from draft: EntryDraftPayload.TOTPConfiguration?
260284
) throws -> TOTPConfig? {

KeeForge/Models/Entry.swift

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,18 @@ struct KPEntry: Identifiable, Sendable {
1010
var notes: String
1111
var iconID: Int
1212
var tags: [String]
13+
/// Whether the source XML had a `<Tags>` element, even if empty. Writers
14+
/// use this to preserve an empty `<Tags></Tags>` that would otherwise be
15+
/// indistinguishable from "no tags at all".
16+
var hasTagsElement: Bool
1317
var customFields: [String: String]
1418
/// Raw TOTP config: either otpauth:// URI or key/settings
1519
var totpConfig: TOTPConfig?
20+
/// Original `otp` key value (an `otpauth://` URI) if the source used the
21+
/// KeeWeb/legacy format. When present, the writer emits the original URI
22+
/// verbatim instead of decomposing into `TimeOtp-*` fields, which would
23+
/// drop the issuer/label and any custom query parameters.
24+
var otpURL: String?
1625
var creationTime: Date?
1726
var lastModificationTime: Date?
1827
var unknownXML: OpaqueXMLNodes
@@ -30,8 +39,10 @@ struct KPEntry: Identifiable, Sendable {
3039
notes: String = "",
3140
iconID: Int = 0,
3241
tags: [String] = [],
42+
hasTagsElement: Bool = false,
3343
customFields: [String: String] = [:],
3444
totpConfig: TOTPConfig? = nil,
45+
otpURL: String? = nil,
3546
creationTime: Date? = nil,
3647
lastModificationTime: Date? = nil,
3748
unknownXML: OpaqueXMLNodes = .empty,
@@ -45,8 +56,10 @@ struct KPEntry: Identifiable, Sendable {
4556
self.notes = notes
4657
self.iconID = iconID
4758
self.tags = tags
59+
self.hasTagsElement = hasTagsElement
4860
self.customFields = customFields
4961
self.totpConfig = totpConfig
62+
self.otpURL = otpURL
5063
self.creationTime = creationTime
5164
self.lastModificationTime = lastModificationTime
5265
self.unknownXML = unknownXML

KeeForge/Models/KDBXParser.swift

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -875,11 +875,21 @@ final class KDBXXMLParser: NSObject, XMLParserDelegate {
875875

876876
case "Value":
877877
if inValue {
878-
var val = currentText.trimmingCharacters(in: .whitespacesAndNewlines)
879-
if isProtected, let decoded = Data(base64Encoded: val) {
880-
val = decryptProtectedValue(decoded)
878+
// Preserve leading/trailing whitespace in stored values — a
879+
// username or custom field may intentionally contain spaces,
880+
// and trimming here silently destroys that data. Only the
881+
// base64 ciphertext of a protected value needs trimming before
882+
// decoding.
883+
if isProtected {
884+
let base64 = currentText.trimmingCharacters(in: .whitespacesAndNewlines)
885+
if let decoded = Data(base64Encoded: base64) {
886+
currentValue = decryptProtectedValue(decoded)
887+
} else {
888+
currentValue = currentText
889+
}
890+
} else {
891+
currentValue = currentText
881892
}
882-
currentValue = val
883893
inValue = false
884894
}
885895

@@ -925,6 +935,10 @@ final class KDBXXMLParser: NSObject, XMLParserDelegate {
925935

926936
case "Tags":
927937
if !isInsideHistory(), let entry = currentEntry {
938+
// Track element presence separately from content so that an
939+
// empty `<Tags></Tags>` element round-trips instead of being
940+
// silently dropped on save.
941+
entry.hasTagsElement = true
928942
let trimmed = currentText.trimmingCharacters(in: .whitespacesAndNewlines)
929943
if !trimmed.isEmpty {
930944
entry.tags = trimmed.components(separatedBy: CharacterSet([",", ";"])).map {
@@ -1174,14 +1188,12 @@ final class KDBXXMLParser: NSObject, XMLParserDelegate {
11741188
}
11751189

11761190
private func parseKPDate(_ string: String) -> Date? {
1177-
// KDBX4 can use base64-encoded binary date or ISO 8601
1178-
if string.contains("-") || string.contains("T") {
1179-
let formatter = ISO8601DateFormatter()
1180-
formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds]
1181-
return formatter.date(from: string) ?? ISO8601DateFormatter().date(from: string)
1182-
}
1183-
// Base64 binary timestamp (seconds since 0001-01-01)
1184-
if let data = Data(base64Encoded: string), data.count == 8 {
1191+
// KDBX4 stores timestamps as 8 bytes of little-endian seconds since
1192+
// year 0001, base64-encoded (always 12 characters with padding).
1193+
// Try this form first — base64 strings can legitimately contain the
1194+
// characters 'T' and '-', so a substring check is not a reliable way
1195+
// to distinguish binary from ISO-8601 form.
1196+
if string.count == 12, let data = Data(base64Encoded: string), data.count == 8 {
11851197
let seconds = data.withUnsafeBytes { $0.loadUnaligned(as: Int64.self).littleEndian }
11861198
guard let kpEpoch = DateComponents(
11871199
calendar: .init(identifier: .gregorian),
@@ -1193,7 +1205,10 @@ final class KDBXXMLParser: NSObject, XMLParserDelegate {
11931205
}
11941206
return kpEpoch.addingTimeInterval(TimeInterval(seconds))
11951207
}
1196-
return nil
1208+
// KDBX 3.x and some KDBX 4 writers use ISO-8601 text form.
1209+
let formatter = ISO8601DateFormatter()
1210+
formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds]
1211+
return formatter.date(from: string) ?? ISO8601DateFormatter().date(from: string)
11971212
}
11981213
}
11991214

@@ -1208,6 +1223,7 @@ private class EntryBuilder {
12081223
var notes = ""
12091224
var iconID = 0
12101225
var tags: [String] = []
1226+
var hasTagsElement = false
12111227
var customFields: [String: String] = [:]
12121228
var protectedStringKeys: Set<String> = []
12131229
var otpURL: String?
@@ -1229,8 +1245,10 @@ private class EntryBuilder {
12291245
notes: notes,
12301246
iconID: iconID,
12311247
tags: tags,
1248+
hasTagsElement: hasTagsElement,
12321249
customFields: customFields.filter { !$0.key.hasPrefix("TimeOtp-") && $0.key != "TOTP Settings" && $0.key != "TOTP Seed" },
12331250
totpConfig: totpConfig,
1251+
otpURL: otpURL,
12341252
creationTime: creationTime,
12351253
lastModificationTime: lastModificationTime,
12361254
unknownXML: unknownXML,

KeeForge/Models/KDBXXMLSerializer.swift

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ struct KDBXXMLSerializer {
131131
xml += element("IconID", value: String(entry.iconID))
132132
knownChildCount += 1
133133

134-
if !entry.tags.isEmpty {
134+
if entry.hasTagsElement || !entry.tags.isEmpty {
135135
xml += try opaqueXML(from: entry.unknownXML, path: [], insertionIndex: knownChildCount)
136136
xml += element("Tags", value: escape(entry.tags.joined(separator: ",")))
137137
knownChildCount += 1
@@ -171,7 +171,18 @@ struct KDBXXMLSerializer {
171171
xml += try serializeString(key: "Notes", value: entry.notes, isProtected: entry.protectedStringKeys.contains("Notes"))
172172
knownChildCount += 1
173173

174-
if let totpConfig = entry.totpConfig {
174+
if let otpURL = entry.otpURL {
175+
// Preserve the original otpauth:// URI so issuer/label and any
176+
// custom query parameters survive the round-trip. Splitting into
177+
// TimeOtp-* fields drops everything outside the canonical set.
178+
xml += try opaqueXML(from: entry.unknownXML, path: [], insertionIndex: knownChildCount)
179+
xml += try serializeString(
180+
key: "otp",
181+
value: otpURL,
182+
isProtected: entry.protectedStringKeys.contains("otp")
183+
)
184+
knownChildCount += 1
185+
} else if let totpConfig = entry.totpConfig {
175186
let secret = try totpConfig.secret.decrypt(using: sessionKey)
176187
let totpFields = [
177188
("TimeOtp-Secret-Base32", secret, true),

KeeForgeTests/KDBXRoundTripTests.swift

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -425,4 +425,83 @@ final class KDBXRoundTripTests: XCTestCase {
425425
return trimmed.hasPrefix("<\(elementName)") && trimmed.contains(expectedContent)
426426
}
427427
}
428+
429+
// MARK: - Regression: base64 dates containing 'T' must parse
430+
431+
func test_parseKPDate_base64WithLetterT_roundTrips() throws {
432+
// "9eT23g4AAAA=" is a valid KDBX4 base64 timestamp whose encoding
433+
// contains the letter 'T'. A naive "contains T → ISO-8601" heuristic
434+
// would misroute this string and silently lose the date.
435+
let entry = KPEntry(
436+
title: "Date-T",
437+
creationTime: Date(timeIntervalSinceReferenceDate: 0)
438+
)
439+
let root = KPGroup(name: "Root", entries: [entry])
440+
let parsed = (rootGroup: root, meta: KPMeta())
441+
let reparsed = try serializeAndParse(parsed)
442+
let reparsedEntry = try XCTUnwrap(reparsed.rootGroup.allEntries.first)
443+
XCTAssertNotNil(reparsedEntry.creationTime, "CreationTime should survive round-trip")
444+
XCTAssertEqual(
445+
entry.creationTime!.timeIntervalSinceReferenceDate,
446+
reparsedEntry.creationTime!.timeIntervalSinceReferenceDate,
447+
accuracy: 1.0
448+
)
449+
}
450+
451+
// MARK: - Regression: Value whitespace preserved
452+
453+
func test_valueWhitespace_trailingSpaces_preserved() throws {
454+
let entry = KPEntry(
455+
title: "Whitespace",
456+
username: "user "
457+
)
458+
let root = KPGroup(name: "Root", entries: [entry])
459+
let reparsed = try serializeAndParse((rootGroup: root, meta: KPMeta()))
460+
let reparsedEntry = try XCTUnwrap(reparsed.rootGroup.allEntries.first)
461+
XCTAssertEqual(reparsedEntry.username, "user ", "Trailing whitespace must be preserved")
462+
}
463+
464+
// MARK: - Regression: otp URL preserved
465+
466+
func test_otpURL_preservedOnRoundTrip() throws {
467+
let uri = "otpauth://totp/Example:user@example.com?secret=JBSWY3DPEHPK3PXP&issuer=Example&period=30&digits=6&algorithm=SHA1"
468+
let secret = "JBSWY3DPEHPK3PXP"
469+
let encryptedSecret = try EncryptedValue.encrypt(secret, using: roundTripSessionKey)
470+
let entry = KPEntry(
471+
title: "OTP",
472+
totpConfig: TOTPConfig(secret: encryptedSecret),
473+
otpURL: uri,
474+
protectedStringKeys: ["otp"]
475+
)
476+
let root = KPGroup(name: "Root", entries: [entry])
477+
let reparsed = try serializeAndParse((rootGroup: root, meta: KPMeta()))
478+
let reparsedEntry = try XCTUnwrap(reparsed.rootGroup.allEntries.first)
479+
XCTAssertEqual(reparsedEntry.otpURL, uri, "otp URL should survive round-trip")
480+
XCTAssertNotNil(reparsedEntry.totpConfig, "TOTP config should still be derived from the URL")
481+
}
482+
483+
// MARK: - Regression: empty Tags element preserved
484+
485+
func test_emptyTags_elementPreserved() throws {
486+
let entry = KPEntry(
487+
title: "Empty Tags",
488+
hasTagsElement: true
489+
)
490+
let root = KPGroup(name: "Root", entries: [entry])
491+
492+
var serializer = KDBXXMLSerializer(
493+
rootGroup: root,
494+
meta: KPMeta(),
495+
innerStreamKey: roundTripInnerStreamKey,
496+
sessionKey: roundTripSessionKey
497+
)
498+
let xmlData = try serializer.serialize()
499+
let xmlString = String(data: xmlData, encoding: .utf8)!
500+
XCTAssertTrue(xmlString.contains("<Tags></Tags>"), "Empty Tags element should be emitted")
501+
502+
let reparsed = try parseXML(xmlData)
503+
let reparsedEntry = try XCTUnwrap(reparsed.rootGroup.allEntries.first)
504+
XCTAssertTrue(reparsedEntry.hasTagsElement, "hasTagsElement should survive round-trip")
505+
XCTAssertTrue(reparsedEntry.tags.isEmpty, "tags array should remain empty")
506+
}
428507
}

0 commit comments

Comments
 (0)