From 7a804d7d1692ebee9945d343141e0d6b953a1247 Mon Sep 17 00:00:00 2001 From: Bradley Mackey Date: Tue, 15 Sep 2026 09:22:24 +0400 Subject: [PATCH] Normalize killphrase queries so trailing whitespace still fires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Digests were built from trimmed phrases but matched against the raw search-bar text, so a trailing space silently stopped a killphrase from firing. Trim in the digester on both sides (no case/canonical fold — existing digests were computed without one), and pass the sanitized query to the deleter so match input equals the search predicate input. Co-Authored-By: Claude Fable 5 --- .../Encryption/KillphraseDigester.swift | 20 ++++-- .../VaultFeed/Storage/VaultDataModel.swift | 5 +- .../Encryption/KillphraseDigesterTests.swift | 28 +++++++++ .../PersistedLocalVaultStoreTests.swift | 19 ++++++ .../Storage/VaultDataModelTests.swift | 62 +++++++++++++++++++ 5 files changed, 129 insertions(+), 5 deletions(-) diff --git a/Vault/Sources/VaultFeed/Encryption/KillphraseDigester.swift b/Vault/Sources/VaultFeed/Encryption/KillphraseDigester.swift index 0370f5b1f..07c3129b3 100644 --- a/Vault/Sources/VaultFeed/Encryption/KillphraseDigester.swift +++ b/Vault/Sources/VaultFeed/Encryption/KillphraseDigester.swift @@ -4,7 +4,8 @@ import FoundationExtensions /// Computes and verifies one-way killphrase digests for a vault. /// -/// Digests are `HMAC-SHA256(K, salt || phrase)` where `K` is a 256-bit key +/// Digests are `HMAC-SHA256(K, salt || normalize(phrase))` where +/// `normalize(...)` trims surrounding whitespace and `K` is a 256-bit key /// that lives in the device keychain (see `KillphraseKeyStore`). The same /// `K` is reused across every item; per-item randomness comes from `salt`, /// which is regenerated on every set. @@ -25,18 +26,18 @@ public struct KillphraseDigester: KillphraseMatcher, Sendable { /// Produce a digest for the given plaintext phrase, using a fresh random salt. public func makeDigest(phrase: String) -> KillphraseDigest { let salt = Data.random(count: Self.saltLength) - let digest = computeDigest(phrase: phrase, salt: salt) + let digest = computeDigest(phrase: Self.normalize(phrase), salt: salt) return KillphraseDigest(salt: salt, digest: digest) } - /// Returns `true` iff `HMAC(K, salt || query)` equals `digest`. + /// Returns `true` iff `HMAC(K, salt || normalize(query))` equals `digest`. /// /// Uses CryptoKit's `isValidAuthenticationCode` which performs a /// constant-time comparison. Callers must not branch on this result in /// any externally observable way beyond performing the deletion itself. public func matches(query: String, salt: Data, digest: Data) -> Bool { var message = salt - message.append(Data(query.utf8)) + message.append(Data(Self.normalize(query).utf8)) return HMAC.isValidAuthenticationCode( digest, authenticating: message, @@ -44,6 +45,17 @@ public struct KillphraseDigester: KillphraseMatcher, Sendable { ) } + /// Both sides of the comparison must apply the same normalization or + /// the HMAC will not match: digests have always been created from + /// trimmed phrases, but the live search query arrives untrimmed, so a + /// trailing space from the keyboard would silently stop a killphrase + /// from firing. Trim only — no canonical/case fold, because existing + /// digests were computed without one and killphrases are intentionally + /// exact-match otherwise. + static func normalize(_ phrase: String) -> String { + phrase.trimmingCharacters(in: .whitespacesAndNewlines) + } + private func computeDigest(phrase: String, salt: Data) -> Data { var message = salt message.append(Data(phrase.utf8)) diff --git a/Vault/Sources/VaultFeed/Storage/VaultDataModel.swift b/Vault/Sources/VaultFeed/Storage/VaultDataModel.swift index cb8f4c41b..8f451cf46 100644 --- a/Vault/Sources/VaultFeed/Storage/VaultDataModel.swift +++ b/Vault/Sources/VaultFeed/Storage/VaultDataModel.swift @@ -322,9 +322,12 @@ extension VaultDataModel { // only available when the vault is unlocked. If we don't have // one yet (vault still locked, key load failed), skip the // delete pass — the search itself remains functional. + // Match on the same sanitized text the search predicate uses, + // so a phrase the user can see matching in the feed also fires + // the killphrase (digests are built from trimmed phrases). let didDeleteKillphraseItems: Bool = if let digester = killphraseDigester { await vaultKillphraseDeleter - .deleteItems(matchingKillphrase: itemsSearchQuery, using: digester) + .deleteItems(matchingKillphrase: itemsSanitizedQuery ?? itemsSearchQuery, using: digester) } else { false } diff --git a/Vault/Tests/VaultFeedTests/Encryption/KillphraseDigesterTests.swift b/Vault/Tests/VaultFeedTests/Encryption/KillphraseDigesterTests.swift index 5612e522e..a9b98592f 100644 --- a/Vault/Tests/VaultFeedTests/Encryption/KillphraseDigesterTests.swift +++ b/Vault/Tests/VaultFeedTests/Encryption/KillphraseDigesterTests.swift @@ -94,6 +94,34 @@ struct KillphraseDigesterTests { #expect(sut.matches(query: "", salt: digest.salt, digest: digest.digest) == false) } + + @Test + func matches_trimsWhitespaceFromQuery() { + let sut = makeSUT() + let digest = sut.makeDigest(phrase: "phrase") + + // The search bar delivers the query untrimmed; a trailing space + // from the keyboard must not stop the killphrase firing. + #expect(sut.matches(query: "phrase ", salt: digest.salt, digest: digest.digest)) + #expect(sut.matches(query: " phrase\n", salt: digest.salt, digest: digest.digest)) + } + + @Test + func makeDigest_trimsWhitespaceFromPhrase() { + let sut = makeSUT() + let digest = sut.makeDigest(phrase: " phrase ") + + #expect(sut.matches(query: "phrase", salt: digest.salt, digest: digest.digest)) + } + + @Test + func matches_doesNotTrimInteriorWhitespace() { + let sut = makeSUT() + let digest = sut.makeDigest(phrase: "two words") + + #expect(sut.matches(query: "twowords", salt: digest.salt, digest: digest.digest) == false) + #expect(sut.matches(query: "two words", salt: digest.salt, digest: digest.digest)) + } } extension KillphraseDigesterTests { diff --git a/Vault/Tests/VaultFeedTests/Storage/PersistedLocalVaultStoreTests.swift b/Vault/Tests/VaultFeedTests/Storage/PersistedLocalVaultStoreTests.swift index 866e88da3..5de938149 100644 --- a/Vault/Tests/VaultFeedTests/Storage/PersistedLocalVaultStoreTests.swift +++ b/Vault/Tests/VaultFeedTests/Storage/PersistedLocalVaultStoreTests.swift @@ -1731,6 +1731,25 @@ final class PersistedLocalVaultStoreTests { try await assertStoreContains(exactlyItems: [item2, item3]) } + @Test + func deleteItemsMatchingKillphrase_matchesQueryWithSurroundingWhitespace() async throws { + let item1 = uniqueVaultItem(killphrase: "phrase") + let item2 = uniqueVaultItem(killphrase: "other") + let payload = VaultApplicationPayload( + userDescription: "Hello world", + items: [item1, item2], + tags: [], + ) + try await sut.importAndOverrideVault(payload: payload) + + // The search bar delivers untrimmed text; a trailing space must + // not stop the killphrase firing. + let didDelete = await sut.deleteItems(matchingKillphrase: "phrase ", using: testDigester) + + #expect(didDelete == true) + try await assertStoreContains(exactlyItems: [item2]) + } + @Test func deleteItemsMatchingKillphrase_doesNotDeleteEmptyKillphraseItems() async throws { let item1 = uniqueVaultItem(killphrase: nil) diff --git a/Vault/Tests/VaultFeedTests/Storage/VaultDataModelTests.swift b/Vault/Tests/VaultFeedTests/Storage/VaultDataModelTests.swift index 4e1eaed48..cf9647408 100644 --- a/Vault/Tests/VaultFeedTests/Storage/VaultDataModelTests.swift +++ b/Vault/Tests/VaultFeedTests/Storage/VaultDataModelTests.swift @@ -382,6 +382,68 @@ final class VaultDataModelTests { } } + @Test + func reloadItems_matchesKillphraseWhenSearchQueryHasTrailingWhitespace() async { + let store = VaultStoreStub() + let killphraseDeleter = VaultStoreKillphraseDeleterMock() + let keyStore = KillphraseKeyStoreMock() + keyStore.loadOrCreateHandler = { + (try? KeyData<32>(data: Data(repeating: 0xAA, count: 32))) ?? .zero() + } + let sut = makeSUT( + vaultStore: store, + vaultKillphraseDeleter: killphraseDeleter, + killphraseKeyStore: keyStore, + ) + await sut.setup() + // Untrimmed query, as delivered by the search bar. The deleter + // must receive the same sanitized text the search predicate uses, + // since digests are built from trimmed phrases. + sut.itemsSearchQuery = " hello world \n" + + await confirmation("Delete called", expectedCount: 1) { confirmDelete in + killphraseDeleter.deleteItemsHandler = { query, _ in + #expect(query == "hello world") + confirmDelete() + return false + } + + await sut.reloadItems() + } + } + + @Test + func reloadItems_doesNotInvokeKillphraseDeleterWhenDigesterNeverLoaded() async { + let killphraseDeleter = VaultStoreKillphraseDeleterMock() + let sut = makeSUT(vaultKillphraseDeleter: killphraseDeleter) + // No setup(): the digester is never loaded, matching the + // vault-still-locked state. The delete pass must be skipped. + sut.itemsSearchQuery = "hello world" + + await sut.reloadItems() + + #expect(killphraseDeleter.deleteItemsCallCount == 0) + } + + @Test + func reloadItems_doesNotInvokeKillphraseDeleterWhenKeyStoreFails() async { + let killphraseDeleter = VaultStoreKillphraseDeleterMock() + let keyStore = KillphraseKeyStoreMock() + keyStore.loadOrCreateHandler = { throw TestError() } + let sut = makeSUT( + vaultKillphraseDeleter: killphraseDeleter, + killphraseKeyStore: keyStore, + ) + // Setup runs, but the key load fails, so the digester stays nil + // and killphrase deletion must remain a no-op. + await sut.setup() + sut.itemsSearchQuery = "hello world" + + await sut.reloadItems() + + #expect(killphraseDeleter.deleteItemsCallCount == 0) + } + @Test func reloadItems_syncsAutofillAndNotifiesWhenKillphraseDeletesItems() async { let store = VaultStoreStub()