Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dgwilson opened this issue Jan 5, 2021 · 14 comments

Comments

@dgwilson
Copy link
Contributor

dgwilson commented Jan 5, 2021

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
Copy link
Contributor Author

dgwilson commented Jan 6, 2021

For the record I've had a go at this... changing CDEPropertyChangeValue.h to
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.

@drewmccormack
Copy link
Owner

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
Copy link
Contributor Author

dgwilson commented Jan 6, 2021 via email

@drewmccormack
Copy link
Owner

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
Copy link
Contributor Author

dgwilson commented Jan 7, 2021

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

@drewmccormack
Copy link
Owner

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
Copy link
Contributor Author

dgwilson commented Jan 8, 2021 via email

@drewmccormack
Copy link
Owner

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
Copy link
Contributor Author

dgwilson commented Jan 9, 2021 via email

@dgwilson
Copy link
Contributor Author

dgwilson commented Jan 9, 2021 via email

@UberJason
Copy link
Contributor

UberJason commented Jan 9, 2021

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
Copy link
Contributor Author

dgwilson commented Jan 9, 2021 via email

@UberJason
Copy link
Contributor

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants