granoff/Strongbox

unarchive(objectForKey:) should throw

Closed this issue · 9 comments

Could you please change

public func unarchive(objectForKey key:String) -> Any? {
        guard let data = self.data(forKey: key) else {
            return nil
        }

        let unarchiver = NSKeyedUnarchiver(forReadingWith: data as Data)
        return unarchiver.decodeObject(forKey: key)
}

to

public func unarchive(objectForKey key:String) -> Any? throws {
        guard let data = self.data(forKey: key) else {
            return nil
        }

        let unarchiver = NSKeyedUnarchiver(forReadingWith: data as Data)
        return unarchiver.decodeTopLevelObject(forKey: key)

It allows to catch the exception in case of failure to decode data.

In my case module name was changed (it's being used as kind of workspace) and I got crash
when trying to decode the object encoded with old module name.

Error Domain=NSCocoaErrorDomain Code=4864 "*** -[NSKeyedUnarchiver decodeObjectForKey:]: cannot decode object of class (MyOldTargetName.MyClass) for key (root); the class may be defined in source code or a library that is not linked" UserInfo={NSDebugDescription=*** -[NSKeyedUnarchiver decodeObjectForKey:]: cannot decode object of class (MyOldTargetName.MyClass) for key (root); the class may be defined in source code or a library that is not linked}

Thanks in advance.

I am not sure that making this api throw is the solution to your issue. Does the class MyOldTargetName (or whatever your class is named) confirm to NSSecureCoding?

I Just re-read what you wrote. Yah, if you change the name of the class your trying to decode, that's a problem in your code. Making this api throw is a breaking change, meaning it would be incompatible with previous versions. I'll have to think about this more carefully.

@granoff Another option may be to leave existing method w/o any changes and just introduce another with throw. WDYT?

I think I'd rather add a new API that throws, instead of changing this existing API. That would not break existing implementations...

@granoff
Am also facing the same issue.

Below is the trace.

0 libsystem_malloc.dylib nanov2_allocate_from_block.cold.1 + 40
1 libsystem_malloc.dylib nanov2_find_block_and_allocate + 554
2 libsystem_malloc.dylib nanov2_allocate + 128
3 libsystem_malloc.dylib nanov2_calloc + 140
4 libsystem_malloc.dylib default_zone_calloc + 84
5 libsystem_malloc.dylib malloc_zone_calloc + 148
6 CoreFoundation CFAllocatorAllocate + 108
7 CoreFoundation _CFRuntimeCreateInstance + 340
8 CoreFoundation __CFDataInit + 168
9 Security add_sequence_to_array + 76
10 CoreFoundation -[__NSDictionaryM __apply:context:] + 132
11 Security der_encode_dictionary + 108
12 Security SecXPCDictionarySetPList + 100
13 Security securityd_send_sync_and_do + 72
14 Security __SecItemCopyMatching_block_invoke_2 + 264
15 Security __SecItemAuthDoQuery_block_invoke + 320
16 Security SecItemAuthDo + 140
17 Security SecItemAuthDoQuery + 520
18 Security __SecItemCopyMatching_block_invoke + 120
19 Security SecOSStatusWith + 56
20 Security SecItemCopyMatching + 364
21 Strongbox Strongbox.swift line 152Strongbox.data(forKey:) + 152
22 Strongbox Strongbox.swift line 95Strongbox.unarchive(objectForKey:) + 95

Is there any fix for this?

@alexeylubyanoy Can you give me a little more detail about what your code looked like before you changed the module name, and then what the code looked like after you changed the module name and you got the crash you reported?

I am trying to write some unit tests to replicate the issue before I try to change any of the APIs in the library.

import Security
import Strongbox
import Foundation

@objc class Credentials: NSObject, NSSecureCoding {
    static var supportsSecureCoding: Bool = true

    func encode(with aCoder: NSCoder) {
        aCoder.encode(username, forKey: "username")
        aCoder.encode(password, forKey: "password")
    }

    required init?(coder aDecoder: NSCoder) {
        guard
            let username = aDecoder.decodeObject(forKey: "username") as? String,
            let password = aDecoder.decodeObject(forKey: "password") as? String else {
            return nil
        }

        self.username = username
        self.password = password

        super.init()
    }

    @objc let username: String
    @objc let password: String

    @objc init(with username: String, password: String) {
        self.username = username
        self.password = password
    }
}

@objc class KeychainHelper: NSObject {
    @discardableResult
    @objc class func store(credentials: Credentials) -> Bool {
        KeychainHelper.removeCredentials()

        return Strongbox().archive(credentials, key: "defaultUser")
    }

    @objc class var storedCredentials: Credentials? {
        return Strongbox().unarchive(objectForKey: "defaultUser") as? Credentials
    }
}

So, nothing special here.

Issue appears when I call following code with old target name

let creds = Credentials(with: "username", password: "password")
KeychainHelper.store(creds)

and then change target name to the new one and call

let creds = KeychainHelper.storedCredentials

Please let me know if you need more details.
My workaround so far is to modify project settings to hardcode module name, so module name remains despite target name changed

A couple more thoughts. The bundle identifier is integral to storing the data in the keychain if you do not specify a key prefix yourself. So if your target changed, then the bundle identifier changed. You can get around this by initializing your Strongbox instance with a keyPrefix argument:
Strongbox(keyPrefix: <the-old-bundle-identifier>). That might solve your problem.

I'm also curious about what your KeychainHelper.removeCredentials() method is doing. Strongbox has a convenience remove method for this purpose, and understands the key prefix stuff too.

@discardableResult
    @objc class func removeCredentials() -> Bool {
        return Strongbox().remove(key: "defaultUser")
    }