ensembles icon indicating copy to clipboard operation
ensembles copied to clipboard

[general] 'NSKeyedUnarchiveFromData' should not be used to for un-archiving and will be removed in a future release

Open dgwilson opened this issue 4 years ago • 14 comments

Xcode 12.3, built for iOS 12 target. I'm getting the warning message '[general] 'NSKeyedUnarchiveFromData' should not be used to for un-archiving and will be removed in a future release' - repeated in the console many many times.

Tracking this down, this post: https://www.kairadiagne.com/2020/01/13/nssecurecoding-and-transformable-properties-in-core-data.html

Looking into the ensembles model I've found this: image

This would also explain the other warnings I'm getting at build time. CDEObjectChange.propertyChangeValues is using a nil or insecure value transformer. Please switch to NSSecureUnarchiveFromDataTransformerName or a custom NSValueTransformer subclass of NSSecureUnarchiveFromDataTransformer [2]

...... ensembles_master/Framework/Resources/CDEEventStoreModel.xcdatamodeld/CDEEventStoreModel_2.xcdatamodel: warning: Misconfigured Property: CDEObjectChange.propertyChangeValues is using a nil or insecure value transformer. Please switch to NSSecureUnarchiveFromDataTransformerName or a custom NSValueTransformer subclass of NSSecureUnarchiveFromDataTransformer

Is the change as simple as adding 'NSSecureUnarchiveFromData' to the 'Transformer' field on the propertyChangeValues field of CDEObjectChange ? - Should the CDEEventStoreModel get a version bump? nope... I've tried that and have a crash on run... something does not confirm.... will work on tracking it down.

dgwilson avatar Jan 05 '21 20:01 dgwilson

For the record I've had a go at this... changing CDEPropertyChangeValue.h <NSCoding> to <NSSecureCoding> and CDEPropertyChangeValue.m by adding:

  • (BOOL)supportsSecureCoding { return TRUE; } and updating initWithCoder. self.propertyName = [aDecoder decodeObjectOfClass:[NSString class] forKey:@"propertyName"]; self.type = [aDecoder decodeIntegerForKey:@"type"]; self.filename = [aDecoder decodeObjectOfClass:[NSString class] forKey:@"filename"]; self.value = [aDecoder decodeObjectOfClass:[NSObject class] forKey:@"value"]; self.relatedIdentifier = [aDecoder decodeObjectOfClass:[NSObject class] forKey:@"relatedIdentifier"]; self.addedIdentifiers = [aDecoder decodeObjectOfClass:[NSSet class] forKey:@"addedIdentifiers"]; self.removedIdentifiers = [aDecoder decodeObjectOfClass:[NSSet class] forKey:@"removedIdentifiers"]; self.movedIdentifiersByIndex = [aDecoder decodeObjectOfClass:[NSDictionary class] forKey:@"movedIdentifiersByIndex"];

Though based on the runtime errors I'm now getting... it's not right. I suspect that [NSObject class] isn't good enough and I do not know what to do with these.

dgwilson avatar Jan 06 '21 00:01 dgwilson

The latest version of Ensembles should already have a custom transformer (CDEPropertyChangeValueTransformer) for CDEPropertyChangeValue, to stop this error. Are you on the latest Ensembles?

Screen Shot 2021-01-06 at 10 20 45

Note that the warnings can still appear for your own model transformables, but they should not appear for CDEPropertyChangeValue. I'm afraid you can't really just change the transformer in your model either, because you are dealing with old data that was serialized using the old transformer. You need to put in place something like CDEPropertyChangeValueTransformer for your own code, or migrate the property to one using a secure transformer.

drewmccormack avatar Jan 06 '21 09:01 drewmccormack

Hi Drew…

Thank you for the support. I’ve just downloaded the repository from GitHub (via direct download) and checked the model in the iOS project. It doesn’t have CDEPropertyChangeValueTransformer.

A download from ‘master’. Correct?

  • David

On 6/01/2021, at 10:24 PM, Drew McCormack [email protected] wrote:

The latest version of Ensembles should already have a custom transformer (CDEPropertyChangeValueTransformer) for CDEPropertyChangeValue, to stop this error. Are you on the latest Ensembles?

https://user-images.githubusercontent.com/77312/103751655-f4166180-5008-11eb-9662-33bdbfcb92f9.png Note that the warnings can still appear for your own model transformables, but they should not appear for CDEPropertyChangeValue. I'm afraid you can't really just change the transformer in your model either, because you are dealing with old data that was serialized using the old transformer. You need to put in place something like CDEPropertyChangeValueTransformer for your own code, or migrate the property to one using a secure transformer.

dgwilson avatar Jan 06 '21 10:01 dgwilson

Ah, wait. Sorry, I got confused. This has been fixed in Ensembles 2, but I didn't get a chance to backport the fix to Ensembles 1 yet. If anyone has access to both and wants to try cherrypicking a PR, I will happily consider it.

drewmccormack avatar Jan 06 '21 11:01 drewmccormack

I'd be happy to have a go at this, though I'd need the matching .m and .h files from Ensembles 2.

dgwilson avatar Jan 07 '21 21:01 dgwilson

I've invited you to the Ensembles 2 repo.

See these hashes for the changes: def40edc3a6d9b23c539501e8c1886fab193de1c 8ee18c8c7d4a3a45348d67aaa3a888a7961c0af1 641e307f996faae865ebf26d8f137229b77d3385

These introduce the CDEPropertyChangeValueTransformer class. You then need to insert that in the Core Data model of Ensembles. Let me know if there are any problems. Should be mostly copy and paste work.

drewmccormack avatar Jan 08 '21 09:01 drewmccormack

Thank you. I’ve had an initial look around. Will look to begin integration work tomorrow.

For this change, does the core data model need a version change? Or can the transformer class be added to the existing version? I know how to do it, just not sure if that is required for that step.

  • David

On 8/01/2021, at 10:08 PM, Drew McCormack [email protected] wrote:

I've invited you to the Ensembles 2 repo.

See these hashes for the changes: def40edc3a6d9b23c539501e8c1886fab193de1c 8ee18c8c7d4a3a45348d67aaa3a888a7961c0af1 641e307f996faae865ebf26d8f137229b77d3385

These introduce the CDEPropertyChangeValueTransformer class. You then need to insert that in the Core Data model of Ensembles. Let me know if there are any problems. Should be mostly copy and paste work.

dgwilson avatar Jan 08 '21 09:01 dgwilson

No change to model version. The transformer is not part of the model hash if I recall properly (check the git change to be sure). The transformer in question is designed to also unarchive old data, so no special handling needed.

drewmccormack avatar Jan 08 '21 09:01 drewmccormack

Done. I’ve tested locally with my app across multiple devices in the iOS simulator. No error messages, changes replicated as expected.

My lack of GIT knowledge has meant that I pushed the changes to my master and thus they’ve been added to the existing pull request as per the image/screen shot below. So I’ve edited the heading accordingly. I trust this is OK.

  • David

On 8/01/2021, at 10:54 PM, Drew McCormack [email protected] wrote:

No change to model version. The transformer is not part of the model hash if I recall properly (check the git change to be sure). The transformer in question is designed to also unarchive old data, so no special handling needed.

dgwilson avatar Jan 09 '21 02:01 dgwilson

Additionally to confirm, I check all three of these commits to ensure that the changes have been incorporated.

  • David

On 8/01/2021, at 10:08 PM, Drew McCormack [email protected] wrote:

I've invited you to the Ensembles 2 repo.

See these hashes for the changes: def40edc3a6d9b23c539501e8c1886fab193de1c 8ee18c8c7d4a3a45348d67aaa3a888a7961c0af1 641e307f996faae865ebf26d8f137229b77d3385

These introduce the CDEPropertyChangeValueTransformer class. You then need to insert that in the Core Data model of Ensembles. Let me know if there are any problems. Should be mostly copy and paste work.

dgwilson avatar Jan 09 '21 02:01 dgwilson

Quick question, has anyone tested the impact of introducing the new transformer into an existing app that already has data? Say I have a version 1.0 of my app in production, with customers with data, and then I update my version of Ensembles in my app and push out a version 1.1 that uses the new transformer - will my customers' existing data still be okay, given that they used to be archived/unarchived without the secure transformer and now they're trying to be unarchived with a different transformer?

I'm asking because I vaguely recall running into this issue myself many months ago when working to fix the warning in my app (unrelated to Ensembles in this case), and existing data wasn't getting loaded correctly anymore. But I didn't have the time to look into it further to see if there was an easy fix (so the warning persists in my app for now). Just wanted to raise it here before any PR is merged, so that if this is an issue, unwitting Ensembles 1 users like myself won't update and find everything destroyed.

UberJason avatar Jan 09 '21 03:01 UberJason

Good question.

I’ve been testing with my existing app… in the simulator and it’s all working as far as I can tell.

If you take a look at ‘CDEPropertyChangeValue.m’ and the procedure

  • (id)attributeValueForAttributeDescription:(NSAttributeDescription *)attribute

you will see code that tests for the transform… and thus supporting the new secure and the old not secure.

  • David

On 9/01/2021, at 4:09 PM, Jason Ji [email protected] wrote:

Quick question, has anyone tested the impact of introducing the new transformer into an existing app that already has data? Say I have a version 1.0 of my app in production, with customers with data, and then I update my version of Ensembles in my app and push out a version 1.1 that uses the new transformer - will my customers' existing data still be okay, given that they used to be archived/unarchived without the secure transformer and now they're trying to be unarchived with a different transformer?

I'm asking because I vaguely recall running into this issue myself when working to fix the warning in my app (unrelated to Ensembles in this case), and existing data wasn't getting loaded correctly anymore. But I didn't have the time to look into it further to see if there was an easy fix. Just wanted to raise it here before any PR is merged, so that if this is an issue, unwitting Ensembles 1 users like myself won't update and find everything destroyed.

dgwilson avatar Jan 09 '21 03:01 dgwilson

Seems reasonable, admittedly with me not being familiar with the code. So long as you did some sort of test where you ran the app using the old, insecure transformer first, then pod-installed or otherwise updated Ensembles to the new version using the secure transformer, and then re-ran the app and were able to see the original data without issue, then that's probably good enough.

UberJason avatar Jan 09 '21 16:01 UberJason

The new transformer actually just does what the old one did, ie the default one, so it should all work. We have been using this code in Ensembles 2 for about 6 months, and no problems with legacy.

drewmccormack avatar Jan 11 '21 09:01 drewmccormack