mapbox / mapbox-events-ios

Mapbox Events Framework for iOS
Other
20 stars 38 forks source link

🐞 [iOS] decodeObjectOfClass warning on iOS 15 #295

Closed julianrex closed 3 years ago

julianrex commented 3 years ago

Configuration

Steps to Reproduce Any app/example (on device) should trigger this, but I've been running the Show 3D Terrain example:

When [unarchiver decodeObjectOfClass:MMEEvent.class forKey:NSKeyedArchiveRootObjectKey]; is called in in loadPendingTelemetryMetricsEvent. We see a lot of the following warnings:

2021-08-11 10:11:29.026132-0400 Examples[390:8566] [general] *** -[NSKeyedUnarchiver validateAllowedClass:forKey:] allowed unarchiving safe plist type ''NSNumber' (0x1f17a2a08) [/System/Library/Frameworks/Foundation.framework]', even though it was not explicitly included in the client allowed classes set: '{(
    "'NSDictionary' (0x1f17977a8) [/System/Library/Frameworks/CoreFoundation.framework]"
)}'. This will be disallowed in the future.
2021-08-11 10:11:29.027164-0400 Examples[390:8566] [general] *** -[NSKeyedUnarchiver validateAllowedClass:forKey:] allowed unarchiving safe plist type ''NSNumber' (0x1f17a2a08) [/System/Library/Frameworks/Foundation.framework]', even though it was not explicitly included in the client allowed classes set: '{(
    "'NSDictionary' (0x1f17977a8) [/System/Library/Frameworks/CoreFoundation.framework]"
)}'. This will be disallowed in the future.

Changing this call to [unarchiver decodeObjectOfClasses:classes forKey:NSKeyedArchiveRootObjectKey]; (where classes include the types in the warning log), made no difference. I'm not sure where we need to specify the "allowed classes".

Note that other calls that use secure coding (archive and unarchive) do not trigger the warnings, but I think that's just luck, and we should address each call site.

NSCoder has an allowedClasses property (and similarly NSSecureUnarchiveFromDataTransformer has allowedTopLevelClasses), but this is read-only;

Subclassing NSKeyedUnarchiver and setting the allowedClasses does appear to fix this issue.

References:

macdrevx commented 3 years ago

After a bit more investigation, I've identified the root cause of this issue…

In loadPendingTelemetryMetricsEvent which @julianrex identified above, MMEEvent encodes its _attributesStorage NSDictionary. This dictionary contains numbers, strings, and possibly other types, so in the corresponding decode invocation, we need to allow whichever types could potentially be nested within that dictionary:

When I was testing this locally, I only saw NSNumber and NSString in this dictionary, but someone more familiar with MMEEvent should audit the list of types that could potentially be included:

_attributesStorage = [aDecoder decodeObjectOfClasses:[NSSet setWithObjects:NSDictionary.class, NSNumber.class, NSString.class, nil] forKey:MMEEventAttributesKey];

macdrevx commented 3 years ago

The documentation comments indicate that attributes must be "a dictionary for which [NSJSONSerialization isValidJSONObject:] returns YES".

NSJSONSerialization's docs indicate that the list of allowed types are:

NSArray, NSDictionary, NSString, NSNumber, NSNull

Based on that, we should update the decode invocation to:

_attributesStorage = [aDecoder decodeObjectOfClasses:[NSSet setWithObjects:NSArray.class, NSDictionary.class, NSString.class, NSNumber.class, NSNull.class, nil] forKey:MMEEventAttributesKey];

macdrevx commented 3 years ago

PR: https://github.com/mapbox/mapbox-events-ios/pull/297