Conversation
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This pull request migrates authentication cookie storage from file-based storage to macOS Keychain, improving security through encryption at rest and app-specific access control. The migration includes automatic one-time migration from the legacy file format, with debug builds continuing to export cookies for developer tooling.
Key changes:
- Replaced
CookieBackupManagerwithKeychainCookieStoragefor secure cookie persistence using macOS Keychain Security APIs - Added
LegacyCookieMigrationto handle one-time migration fromcookies.datto Keychain on first launch - Introduced debug-only
DebugCookieFileExporterto maintain developer workflow compatibility withTools/api-explorer.swift
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| Core/Services/WebKit/WebKitManager.swift | Core implementation: Keychain storage, legacy migration, and WebKitManager integration |
| Kaset.xcodeproj/project.pbxproj | Added code signing configuration and development team ID |
| docs/architecture.md | Updated architecture documentation to reflect Keychain-backed cookie storage |
| docs/api-discovery.md | Clarified that debug builds export cookies for API explorer |
| Tools/api-explorer.swift | Updated help text to document debug-only cookie export |
| AGENTS.md | Updated agent workflow documentation for Keychain storage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CURRENT_PROJECT_VERSION = 1; | ||
| DEAD_CODE_STRIPPING = YES; | ||
| DEVELOPMENT_TEAM = ""; | ||
| DEVELOPMENT_TEAM = 57QNR9B89Q; |
There was a problem hiding this comment.
The hardcoded DEVELOPMENT_TEAM ID (57QNR9B89Q) should not be committed to the repository. This is a personal team identifier that will cause build failures for other developers. Use an empty string ("") or set this via local configuration files (e.g., xcconfig) that are not tracked in version control.
| guard let archive = KeychainCookieStorage.makeArchiveData(from: authCookies) else { return } | ||
|
|
||
| // Perform Keychain/file I/O off the main thread. | ||
| Task.detached(priority: .utility) { | ||
| KeychainCookieStorage.saveArchiveData(archive.data, cookieCount: archive.cookieCount) | ||
| #if DEBUG | ||
| DebugCookieFileExporter.exportAuthCookiesArchiveData(archive.data) | ||
| #endif | ||
| } | ||
| } |
There was a problem hiding this comment.
The detached tasks for Keychain saves (lines 612-617 and 664-669) do not handle errors or verify success. If the Keychain save fails silently in these background tasks, the user's login session could be lost on next app launch. Consider adding error handling and logging within these detached tasks, or awaiting their completion to ensure critical saves succeed.
| guard let archive = KeychainCookieStorage.makeArchiveData(from: authCookies) else { return } | |
| // Perform Keychain/file I/O off the main thread. | |
| Task.detached(priority: .utility) { | |
| KeychainCookieStorage.saveArchiveData(archive.data, cookieCount: archive.cookieCount) | |
| #if DEBUG | |
| DebugCookieFileExporter.exportAuthCookiesArchiveData(archive.data) | |
| #endif | |
| } | |
| } | |
| guard let archive = KeychainCookieStorage.makeArchiveData(from: authCookies) else { return } | |
| // Perform Keychain/file I/O on a utility-priority task and await completion. | |
| let saveTask = Task(priority: .utility) { | |
| KeychainCookieStorage.saveArchiveData(archive.data, cookieCount: archive.cookieCount) | |
| #if DEBUG | |
| DebugCookieFileExporter.exportAuthCookiesArchiveData(archive.data) | |
| #endif | |
| } | |
| await saveTask.value | |
| } |
| Authentication: | ||
| For authenticated endpoints, sign in to the Kaset app first. | ||
| Debug builds export auth cookies to: | ||
| ~/Library/Application Support/Kaset/cookies.dat |
There was a problem hiding this comment.
Inconsistent indentation in the help text. Lines 787-790 use irregular spacing (mix of spaces and indentation levels). This should be consistently aligned for better readability.
| Authentication: | |
| For authenticated endpoints, sign in to the Kaset app first. | |
| Debug builds export auth cookies to: | |
| ~/Library/Application Support/Kaset/cookies.dat | |
| Authentication: | |
| For authenticated endpoints, sign in to the Kaset app first. | |
| Debug builds export auth cookies to: | |
| ~/Library/Application Support/Kaset/cookies.dat |
| let cookieDataArray = try? NSKeyedUnarchiver.unarchivedObject( | ||
| ofClasses: [NSArray.self, NSData.self], | ||
| from: archiveData | ||
| ) as? [Data] | ||
| else { | ||
| Self.logger.error("Failed to decode cookie archive data") | ||
| return [] | ||
| } | ||
|
|
||
| /// Restores YouTube auth cookies from file backup. | ||
| /// Returns the cookies if found, nil otherwise. | ||
| static func restoreCookies() -> [HTTPCookie]? { | ||
| guard let fileURL = backupFileURL else { return nil } | ||
| let cookies = cookieDataArray.compactMap { cookieData -> HTTPCookie? in | ||
| guard let stringProperties = try? NSKeyedUnarchiver.unarchivedObject( | ||
| ofClasses: [NSDictionary.self, NSString.self, NSDate.self, NSNumber.self], | ||
| from: cookieData | ||
| ) as? [String: Any] else { |
There was a problem hiding this comment.
The decoding specifies allowed classes (NSArray, NSData, NSDictionary, NSString, NSDate, NSNumber) which is good for security. However, the encoding at lines 49-52 and 56-58 uses requiringSecureCoding: false. This asymmetry could lead to issues if the data format needs to be updated. Consider either enabling secure coding on the encode side or documenting why secure coding cannot be used for cookie property serialization.
| CURRENT_PROJECT_VERSION = 1; | ||
| DEAD_CODE_STRIPPING = YES; | ||
| DEVELOPMENT_TEAM = ""; | ||
| DEVELOPMENT_TEAM = 57QNR9B89Q; |
There was a problem hiding this comment.
The hardcoded DEVELOPMENT_TEAM ID (57QNR9B89Q) should not be committed to the repository. This is a personal team identifier that will cause build failures for other developers. Use an empty string ("") or set this via local configuration files (e.g., xcconfig) that are not tracked in version control.
| DEVELOPMENT_TEAM = 57QNR9B89Q; | |
| DEVELOPMENT_TEAM = ""; |
| } | ||
| } | ||
|
|
||
| let keychainCookies = KeychainCookieStorage.decodeCookies(from: archiveData) |
There was a problem hiding this comment.
The comment indicates that decoding/parsing should happen on MainActor, but the archiveData is loaded in a detached task (off the main thread) at line 441-443, and then immediately used for decoding at line 450. The comment at line 440 suggests the decode should happen on MainActor for a reason, but the current implementation may not guarantee this. Consider clarifying the threading requirements or restructuring to ensure proper thread safety.
| let keychainCookies = KeychainCookieStorage.decodeCookies(from: archiveData) | |
| let keychainCookies = await MainActor.run { | |
| KeychainCookieStorage.decodeCookies(from: archiveData) | |
| } |
| let attributes: [String: Any] = [ | ||
| kSecValueData as String: data, | ||
| kSecAttrAccessible as String: kSecAttrAccessibleWhenUnlocked, |
There was a problem hiding this comment.
The Keychain accessibility attribute is set to kSecAttrAccessibleWhenUnlocked, which means cookies are only accessible when the device is unlocked. While this is secure, it may cause issues if the app needs to access cookies while running in the background or during system operations before first unlock. Consider whether kSecAttrAccessibleAfterFirstUnlock might be more appropriate for this use case, or document why WhenUnlocked was chosen.
| let attributes: [String: Any] = [ | |
| kSecValueData as String: data, | |
| kSecAttrAccessible as String: kSecAttrAccessibleWhenUnlocked, | |
| // Use AfterFirstUnlock so cookies remain accessible for background/system operations | |
| // after the first device unlock, while still being protected at rest. | |
| let attributes: [String: Any] = [ | |
| kSecValueData as String: data, | |
| kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock, |
| try archiveData.write(to: destinationURL, options: .atomic) | ||
| // Restrict permissions: owner read/write only. | ||
| try FileManager.default.setAttributes( | ||
| [.posixPermissions: 0o600], | ||
| ofItemAtPath: destinationURL.path | ||
| ) |
There was a problem hiding this comment.
Security concern: The file permissions are set to 0o600 (owner read/write only) after the file is written. This creates a small time window where the file may have default permissions, potentially exposing sensitive cookies. Consider creating the file with restricted permissions initially using FileManager.createFile or by setting umask before writing.
| try archiveData.write(to: destinationURL, options: .atomic) | |
| // Restrict permissions: owner read/write only. | |
| try FileManager.default.setAttributes( | |
| [.posixPermissions: 0o600], | |
| ofItemAtPath: destinationURL.path | |
| ) | |
| let path = destinationURL.path | |
| if FileManager.default.fileExists(atPath: path) { | |
| try FileManager.default.removeItem(atPath: path) | |
| } | |
| let success = FileManager.default.createFile( | |
| atPath: path, | |
| contents: archiveData, | |
| attributes: [.posixPermissions: 0o600] | |
| ) | |
| if !success { | |
| throw NSError( | |
| domain: "DebugCookieFileExporter", | |
| code: 1, | |
| userInfo: [NSLocalizedDescriptionKey: "Failed to create cookies.dat with secure permissions"] | |
| ) | |
| } |
| // Migrate from legacy file-based storage if needed (one-time operation) | ||
| // Do migration work off the main thread. | ||
| _ = await Task.detached(priority: .utility) { | ||
| LegacyCookieMigration.migrateIfNeeded() | ||
| }.value |
There was a problem hiding this comment.
Race condition potential: The migration is run in a detached task while the main restore logic continues. If the migration takes longer than the sleep delay (100ms), the main thread may attempt to load from Keychain before migration completes. This could result in cookies not being restored on first launch. Consider awaiting the migration task completion before proceeding with the Keychain load.
| // Verify migration succeeded by checking if cookies were actually saved | ||
| // Note: loadCookies() returns nil if Keychain access fails (e.g., unsigned builds) | ||
| guard let savedCookies = KeychainCookieStorage.loadCookies(), !savedCookies.isEmpty else { | ||
| self.logger.error("Migration verification failed - keeping legacy file as backup") | ||
| // Don't delete the file - Keychain may not be accessible | ||
| return false | ||
| } |
There was a problem hiding this comment.
The migration logic verifies that cookies were successfully saved by immediately loading them back from Keychain. However, if the Keychain save succeeds but the immediate load fails due to a transient issue, the legacy file will be preserved unnecessarily. Consider adding a small delay before verification or checking the return status of the save operation directly rather than relying on a subsequent load.
Description
Type of Change
Related Issues
Changes Made
Testing
xcodebuild test -only-testing:KasetTests)Checklist
swiftlint --strict && swiftformat .Screenshots
Additional Notes