drewmccormack / ensembles

A synchronization framework for Core Data.
MIT License
1.63k stars 132 forks source link

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

Open dgwilson opened 3 years ago

dgwilson commented 3 years ago

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 commented 3 years ago

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

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.

drewmccormack commented 3 years ago

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.

dgwilson commented 3 years ago

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?

On 6/01/2021, at 10:24 PM, Drew McCormack notifications@github.com 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.

drewmccormack commented 3 years ago

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.

dgwilson commented 3 years ago

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

drewmccormack commented 3 years ago

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 commented 3 years ago

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.

On 8/01/2021, at 10:08 PM, Drew McCormack notifications@github.com 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.

drewmccormack commented 3 years ago

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 commented 3 years ago

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.

On 8/01/2021, at 10:54 PM, Drew McCormack notifications@github.com 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 commented 3 years ago

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

On 8/01/2021, at 10:08 PM, Drew McCormack notifications@github.com 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.

UberJason commented 3 years ago

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.

dgwilson commented 3 years ago

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

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

On 9/01/2021, at 4:09 PM, Jason Ji notifications@github.com 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.

UberJason commented 3 years ago

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.

drewmccormack commented 3 years ago

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.