From cd6dd43dae684950a20156268ebf496365e4f14f Mon Sep 17 00:00:00 2001 From: Lysann Tranvouez Date: Thu, 12 Mar 2026 22:44:42 +0100 Subject: [PATCH 1/2] do not search too far for .gpg-id + lots of tests for that --- passKit/Models/PasswordStore.swift | 49 ++++--- passKitTests/Models/PasswordStoreTest.swift | 150 +++++++++++++++++++- 2 files changed, 177 insertions(+), 22 deletions(-) diff --git a/passKit/Models/PasswordStore.swift b/passKit/Models/PasswordStore.swift index 742f13e..65bf31c 100644 --- a/passKit/Models/PasswordStore.swift +++ b/passKit/Models/PasswordStore.swift @@ -411,8 +411,7 @@ public class PasswordStore { return [keyID] } if Defaults.isEnableGPGIDOn { - let encryptedDataPath = password.fileURL(in: storeURL) - return findGPGIDs(from: encryptedDataPath) + return findGPGIDs(underPath: password.path) } return [] }() @@ -430,6 +429,37 @@ public class PasswordStore { AppKeychain.shared.removeContent(for: SSHKey.PRIVATE.getKeychainKey()) gitSSHPrivateKeyPassphrase = nil } + + func findGPGIDs(underPath relativePath: String) -> [String] { + guard let gpgIDFileURL = findGPGIDFile(atPath: relativePath) else { + return [] + } + let allKeysSeparatedByNewline = (try? String(contentsOf: gpgIDFileURL)) ?? "" + return allKeysSeparatedByNewline + .split(separator: "\n") + .map { String($0).trimmed } + .filter { !$0.isEmpty } + } + + func findGPGIDFile(atPath relativePath: String) -> URL? { + // Walk up the directory hierarchy, but never escape the store root + let storeRoot = storeURL.absoluteURL.resolvingSymlinksInPath() + var current = storeRoot.appendingPathComponent(relativePath).resolvingSymlinksInPath() + + while current.path.hasPrefix(storeRoot.path) { + let candidate = current.appendingPathComponent(".gpg-id") + var isDir: ObjCBool = false + if FileManager.default.fileExists(atPath: candidate.path, isDirectory: &isDir), !isDir.boolValue { + return candidate.standardizedFileURL + } + let parent = current.deletingLastPathComponent().resolvingSymlinksInPath() + if parent.path == current.path { + break + } + current = parent + } + return nil + } } extension PasswordStore { @@ -462,18 +492,3 @@ extension PasswordStore { return try gitRepository.commit(signature: gitSignatureForNow, message: message) } } - -func findGPGIDs(from url: URL) -> [String] { - var path = url - while !FileManager.default.fileExists(atPath: path.appendingPathComponent(".gpg-id").path), - path.path != "file:///" { - path = path.deletingLastPathComponent() - } - path = path.appendingPathComponent(".gpg-id") - - let allKeysSeparatedByNewline = (try? String(contentsOf: path)) ?? "" - return allKeysSeparatedByNewline - .split(separator: "\n") - .map { String($0).trimmed } - .filter { !$0.isEmpty } -} diff --git a/passKitTests/Models/PasswordStoreTest.swift b/passKitTests/Models/PasswordStoreTest.swift index e9b3c44..52b683c 100644 --- a/passKitTests/Models/PasswordStoreTest.swift +++ b/passKitTests/Models/PasswordStoreTest.swift @@ -332,12 +332,85 @@ final class PasswordStoreTest: XCTestCase { waitForExpectations(timeout: 1, handler: nil) } - // MARK: - .gpg-id support + // MARK: - Find .gpg-id + + func testFindGPGIDFile() throws { + try FileManager.default.createDirectory(at: localRepoURL, withIntermediateDirectories: true) + XCTAssertTrue(FileManager.default.createFile(atPath: localRepoURL.appendingPathComponent(".gpg-id").path, contents: Data("under root".utf8))) + + try FileManager.default.createDirectory(at: localRepoURL.appendingPathComponent("foo/bar/baz"), withIntermediateDirectories: true) + XCTAssertTrue(FileManager.default.createFile(atPath: localRepoURL.appendingPathComponent("foo/.gpg-id").path, contents: Data("under foo".utf8))) + + try FileManager.default.createDirectory(at: localRepoURL.appendingPathComponent("weird-subdir/.gpg-id/"), withIntermediateDirectories: true) + try FileManager.default.createDirectory(at: localRepoURL.appendingPathComponent("weird-subdir/.gpg-id/hey"), withIntermediateDirectories: true) + XCTAssertTrue(FileManager.default.createFile(atPath: localRepoURL.appendingPathComponent("weird-subdir/.gpg-id/hey/.gpg-id").path, contents: Data("under hey".utf8))) + + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent(".gpg-id").path)) + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "/")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent(".gpg-id").path)) + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "doesnt-exist")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent(".gpg-id").path)) + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "foo/..")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent(".gpg-id").path)) + + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "foo")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent("foo/.gpg-id").path)) + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "foo/bar")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent("foo/.gpg-id").path)) + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "foo/bar/baz")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent("foo/.gpg-id").path)) + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "foo/doesnt-exist")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent("foo/.gpg-id").path)) + + // there is a _drectory_ called .gpg-id in here + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "weird-subdir")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent(".gpg-id").path)) + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "weird-subdir/.gpg-id")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent(".gpg-id").path)) + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "weird-subdir/.gpg-id/hey")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent("weird-subdir/.gpg-id/hey/.gpg-id").path)) + + // "foo/bar/../../baz" resolves to "baz" which has no .gpg-id, so should find root's. + // Without path resolution, the walk ["foo","bar","..","..","baz"] → remove "baz" → remove ".." → + // "foo/bar/.." → remove ".." → "foo/bar" → finds foo/.gpg-id (wrong). + try FileManager.default.createDirectory(at: localRepoURL.appendingPathComponent("baz"), withIntermediateDirectories: true) + XCTAssertEqual(passwordStore.findGPGIDFile(atPath: "foo/bar/../../baz")?.absoluteURL, URL(fileURLWithPath: localRepoURL.appendingPathComponent(".gpg-id").path)) + } + + func testMissingGPGIDFile() throws { + XCTAssertFalse(FileManager.default.fileExists(atPath: localRepoURL.appendingPathComponent(".gpg-id").path)) + try FileManager.default.createDirectory(at: localRepoURL.appendingPathComponent("subdir"), withIntermediateDirectories: true) + + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "")) + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "subdir")) + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "missing")) + } + + func testFindGPGIDFileStopsAtRoot() throws { + // Place a .gpg-id file ABOVE the store root, this should not be found + let parentDir = localRepoURL.deletingLastPathComponent() + let escapedGPGIDURL = parentDir.appendingPathComponent(".gpg-id") + XCTAssertTrue(FileManager.default.createFile(atPath: escapedGPGIDURL.path, contents: Data("ESCAPED_KEY".utf8))) + defer { try? FileManager.default.removeItem(at: escapedGPGIDURL) } + + // Store has no .gpg-id at all + try FileManager.default.createDirectory(at: localRepoURL.appendingPathComponent("sub/deep"), withIntermediateDirectories: true) + // Direct paths, should not find the escaped .gpg-id since it's outside the store root + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "")) + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "sub")) + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "sub/deep")) + + // Path traversal attempts via ".." + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "..")) + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "../..")) + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "sub/../..")) + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "sub/deep/../../..")) + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "sub/deep/../../../../../etc")) + + // Symlink escape: create a symlink inside the store pointing outside + let evilDir = parentDir.appendingPathComponent("evil") + try FileManager.default.createDirectory(at: evilDir, withIntermediateDirectories: true) + XCTAssertTrue(FileManager.default.createFile(atPath: evilDir.appendingPathComponent(".gpg-id").path, contents: Data("EVIL_KEY".utf8))) + defer { try? FileManager.default.removeItem(at: evilDir) } + try FileManager.default.createSymbolicLink(at: localRepoURL.appendingPathComponent("sub/escape"), withDestinationURL: evilDir) + // Following the symlink would find evil/.gpg-id — must not happen + XCTAssertNil(passwordStore.findGPGIDFile(atPath: "sub/escape")) + } + + // MARK: Parse .gpg-id func testReadGPGIDFile() throws { try cloneRepository(.withGPGID) - Defaults.isEnableGPGIDOn = true - [ ("", [RSA4096.longFingerprint]), ("family", [String(NISTP384.longFingerprint.suffix(16))]), @@ -345,11 +418,36 @@ final class PasswordStoreTest: XCTestCase { ("shared", [RSA2048.longFingerprint, RSA4096.longFingerprint]), ("work", [RSA2048.longFingerprint]), ].forEach { path, expectedKeyIDs in - let foundKeyIDs = findGPGIDs(from: localRepoURL.appendingPathComponent(path)) - XCTAssertEqual(foundKeyIDs, expectedKeyIDs.map { $0.uppercased() }) + let foundKeyIDs = passwordStore.findGPGIDs(underPath: path) + XCTAssertEqual(foundKeyIDs, expectedKeyIDs.map { $0.uppercased() }, "Failed for path: \(path)") } } + func testReadEmptyGPGIDFile() throws { + try FileManager.default.createDirectory(at: localRepoURL, withIntermediateDirectories: true) + + XCTAssertTrue(FileManager.default.createFile(atPath: localRepoURL.appendingPathComponent(".gpg-id").path, contents: nil)) + XCTAssertEqual(passwordStore.findGPGIDs(underPath: ""), []) + + XCTAssertTrue(FileManager.default.createFile(atPath: localRepoURL.appendingPathComponent(".gpg-id").path, contents: Data(" \n\t".utf8))) + XCTAssertEqual(passwordStore.findGPGIDs(underPath: ""), []) + } + + func testReadGPGIDFileWithWhitespace() throws { + try FileManager.default.createDirectory(at: localRepoURL, withIntermediateDirectories: true) + + XCTAssertTrue(FileManager.default.createFile(atPath: localRepoURL.appendingPathComponent(".gpg-id").path, contents: nil)) + XCTAssertEqual(passwordStore.findGPGIDs(underPath: ""), []) + + XCTAssertTrue(FileManager.default.createFile(atPath: localRepoURL.appendingPathComponent(".gpg-id").path, contents: Data(" \n\t".utf8))) + XCTAssertEqual(passwordStore.findGPGIDs(underPath: ""), []) + + XCTAssertTrue(FileManager.default.createFile(atPath: localRepoURL.appendingPathComponent(".gpg-id").path, contents: Data(" \nbar foo\n\tbaz\n \n".utf8))) + XCTAssertEqual(passwordStore.findGPGIDs(underPath: ""), ["bar foo", "baz"]) + } + + // MARK: Handle .gpg-id + func testAddPasswordInRoot_WithSingleEntryInPGPIDFile_EncryptsWithThatKey() throws { let mockPGPInterface = setUpMockedPGPInterface() mockPGPInterface.publicKeyIDs = Set(RSA2048_RSA4096.fingerprints) @@ -436,6 +534,48 @@ final class PasswordStoreTest: XCTestCase { } } + func testGPGIDDisabledIgnoresGPGIDFile() throws { + try cloneRepository(.withGPGID) + try importSinglePGPKey() // Only import RSA4096, but not RSA2048 + Defaults.isEnableGPGIDOn = false + + // /work uses RSA2048, but we didn't import that one + let testPassword = Password(name: "test", path: "work/test.gpg", plainText: "testpassword") + // this would throw if isEnableGPGIDOn was true, since we are missing the key to encrypt it + _ = try passwordStore.add(password: testPassword) + + // check that we can decrypt it with the key we have, which confirms that it was encrypted without using the .gpg-id file + let decryptedPassword = try decrypt(path: "work/test.gpg", keyID: RSA4096.longFingerprint) + XCTAssertEqual(decryptedPassword.plainText, "testpassword") + + // we can't even decrypt it with RSA2048 + try importMultiplePGPKeys() + XCTAssertThrowsError(try decrypt(path: "work/test.gpg", keyID: RSA2048.longFingerprint)) { + XCTAssertEqual($0 as? AppError, .keyExpiredOrIncompatible) + } + } + + func testEncryptWithExplicitKeyID_OverridesGPGIDFile() throws { + continueAfterFailure = false // avoid index out of bounds error below + + let mockPGPInterface = setUpMockedPGPInterface() + mockPGPInterface.publicKeyIDs = Set(RSA2048_RSA4096.fingerprints) + try cloneRepository(.withGPGID) + Defaults.isEnableGPGIDOn = true + + // Even though /personal would normally use RSA4096 from the root .gpg-id file, if we explicitly specify a key ID then that should be used instead + let testPassword1 = Password(name: "test1", path: "personal/test1.gpg", plainText: "testpassword1") + _ = try passwordStore.add(password: testPassword1) + let testPassword2 = Password(name: "test2", path: "personal/test2.gpg", plainText: "testpassword2") + _ = try passwordStore.add(password: testPassword2, keyID: RSA2048.longFingerprint) + + XCTAssertEqual(mockPGPInterface.encryptMultiKeyCalls.count, 2) + XCTAssertEqual(mockPGPInterface.encryptMultiKeyCalls[0].plainData, testPassword1.plainData) + XCTAssertEqual(mockPGPInterface.encryptMultiKeyCalls[0].keyIDs, [RSA4096.longFingerprint].map { $0.uppercased() }) + XCTAssertEqual(mockPGPInterface.encryptMultiKeyCalls[1].plainData, testPassword2.plainData) + XCTAssertEqual(mockPGPInterface.encryptMultiKeyCalls[1].keyIDs, [RSA2048.longFingerprint]) + } + // MARK: - Helpers private enum RemoteRepo { From 4d564f570b55be77cd8a323f39dbea75894b4bf2 Mon Sep 17 00:00:00 2001 From: Lysann Tranvouez Date: Thu, 12 Mar 2026 22:51:28 +0100 Subject: [PATCH 2/2] plan updates --- plans/02-multi-recipient-encryption-plan.md | 37 ++++++++++----------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/plans/02-multi-recipient-encryption-plan.md b/plans/02-multi-recipient-encryption-plan.md index 28defd4..132cfce 100644 --- a/plans/02-multi-recipient-encryption-plan.md +++ b/plans/02-multi-recipient-encryption-plan.md @@ -30,7 +30,7 @@ The codebase does **not** support encrypting to multiple public keys. Every laye ### 1. `findGPGID(from:) -> [String]` -Split file contents by newline, trim each line, filter empty lines. Return array of key IDs. Callers that only need a single key (e.g. for decryption routing) can use `.first`. +Split file contents by newline, trim each line, filter empty lines. Return array of key IDs. ### 2. `PGPInterface` protocol @@ -60,10 +60,10 @@ When a store lists multiple key IDs in `.gpg-id`, the user needs the public keys ### Current state -- The keychain holds exactly **one** `pgpPublicKey` blob and **one** `pgpPrivateKey` blob. -- The import UI (armor paste, URL, file picker) has one public key field + one private key field. Importing **replaces** the previous key pair entirely. -- Both `GopenPGPInterface` and `ObjectivePGPInterface` *can* parse multiple keys from a single armored blob (e.g. concatenated armor blocks). So if the user pastes multiple public keys into the single field, they would be parsed — but the encrypt path only uses one key, and the UI doesn't communicate this. -- There is no UI for viewing which key IDs are loaded. +- The keychain holds **one** `pgpPublicKey` blob and **one** `pgpPrivateKey` blob, but each can contain **multiple concatenated armored key blocks**. Both interface implementations parse all keys from these blobs. +- The import UI (armor paste, URL, file picker) has one public key field + one private key field. Importing **replaces** the set of keys entirely, there is no append mode for adding additional keys or managing existing keys. +- There is no UI for viewing which key IDs are loaded or for importing additional recipient-only public keys, nor for viewing the key metadata. +- There is no UI for viewing or editing `.gpg-id` files, which are the source of truth for which keys are used for encryption. ### Key storage approach @@ -142,20 +142,19 @@ This can be expensive for large directories. Show progress and allow cancellatio ## Implementation Order -| Step | Description | Depends On | -|------|-------------|------------| -| 1 | `findGPGID` returns `[String]` + update callers | — | -| 2 | `PGPInterface` protocol change (`keyIDs: [String]?`) | — | -| 3 | `GopenPGPInterface` multi-key encryption | Step 2 | -| 4 | `ObjectivePGPInterface` multi-key encryption | Step 2 | -| 5 | `PGPAgent` updated overloads | Steps 2-4 | -| 6 | `PasswordStore.encrypt()` uses `[String]` from `findGPGID` | Steps 1+5 | -| 7 | UI: import additional recipient public keys | Step 5 | -| 8 | UI: view loaded key IDs | Step 5 | -| 9a | UI: view `.gpg-id` in password detail / folder view | Step 1 | -| 9b | UI: edit `.gpg-id` | Step 9a | -| 10 | Re-encryption when `.gpg-id` changes | Steps 6+9b | -| T | Tests (see testing section) | Steps 1-10 | +| Step | Description | Status | Depends On | +|------|-------------|--------|------------| +| 1 | `findGPGIDs` returns `[String]` + update callers | ✅ Done | — | +| 2 | `PGPInterface` protocol change (`keyIDs: [String]`) | ✅ Done | — | +| 3 | `GopenPGPInterface` multi-key encryption | ✅ Done | Step 2 | +| 4 | `ObjectivePGPInterface` multi-key encryption | ✅ Done | Step 2 | +| 5 | `PGPAgent` updated overloads | ✅ Done | Steps 2-4 | +| 6 | `PasswordStore.encrypt()` uses `[String]` from `findGPGIDs` | ✅ Done | Steps 1+5 | +| 7 | UI: import additional recipient public keys | Not started | Step 5 | +| 8 | UI: view loaded key IDs and metadata | Not started | Step 5 | +| 9a | UI: view `.gpg-id` in password detail / folder view | Not started | Step 1 | +| 9b | UI: edit `.gpg-id` | Not started | Step 9a | +| 10 | Re-encryption when `.gpg-id` changes | Not started | Steps 6+9b | ---