Skip to content

Commit

Permalink
Fix for LIB-1416 & Github #846 (#853)
Browse files Browse the repository at this point in the history
* Fix for LIB-1416 & Github #846

- Stops blindly passing dictionaries.
- Property dictionaries are checked for NSCoding conformance to ensure they can be serialized.
- Property dictionaries are deep copied so contents can’t change while the pipeline is in progress.
- Puts a try/catch arrangement as a temporary guard against crashes for serialization failures until the storage format can be changed.

* Fixed missing ;

* Added test for deepCopy/conformance additions.
  • Loading branch information
bsneed authored Nov 20, 2019
1 parent f07232e commit 19d6c56
Show file tree
Hide file tree
Showing 7 changed files with 182 additions and 59 deletions.
5 changes: 5 additions & 0 deletions Analytics.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
3481C14AEC76E5A8DA64DA59 /* Pods_AnalyticsTests.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 89033CBF22319674E6CE7E61 /* Pods_AnalyticsTests.framework */; };
6E265C791FB1178C0030E08E /* IntegrationsManagerTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6E265C781FB1178C0030E08E /* IntegrationsManagerTest.swift */; };
6EEC1C712017EA370089C478 /* EndToEndTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EEC1C702017EA370089C478 /* EndToEndTests.swift */; };
A31958EF2385AC3A00A47EFA /* SerializationTests.m in Sources */ = {isa = PBXBuildFile; fileRef = A31958EE2385AC3A00A47EFA /* SerializationTests.m */; };
EA88A5981DED7608009FB66A /* SEGSerializableValue.h in Headers */ = {isa = PBXBuildFile; fileRef = EA88A5971DED7608009FB66A /* SEGSerializableValue.h */; settings = {ATTRIBUTES = (Public, ); }; };
EA8F09741E24C5C600B8B93F /* MiddlewareTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = EA8F09731E24C5C600B8B93F /* MiddlewareTests.swift */; };
EAA542771EB4035400945DA7 /* TrackingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = EAA542761EB4035400945DA7 /* TrackingTests.swift */; };
Expand Down Expand Up @@ -92,6 +93,7 @@
6E265C781FB1178C0030E08E /* IntegrationsManagerTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = IntegrationsManagerTest.swift; sourceTree = "<group>"; };
6EEC1C702017EA370089C478 /* EndToEndTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EndToEndTests.swift; sourceTree = "<group>"; };
89033CBF22319674E6CE7E61 /* Pods_AnalyticsTests.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_AnalyticsTests.framework; sourceTree = BUILT_PRODUCTS_DIR; };
A31958EE2385AC3A00A47EFA /* SerializationTests.m */ = {isa = PBXFileReference; indentWidth = 4; lastKnownFileType = sourcecode.c.objc; path = SerializationTests.m; sourceTree = "<group>"; tabWidth = 4; };
D3BF8AE673FE0FD91DF5B503 /* Pods-AnalyticsTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AnalyticsTests.release.xcconfig"; path = "Pods/Target Support Files/Pods-AnalyticsTests/Pods-AnalyticsTests.release.xcconfig"; sourceTree = "<group>"; };
EA88A5971DED7608009FB66A /* SEGSerializableValue.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SEGSerializableValue.h; sourceTree = "<group>"; };
EA8F09731E24C5C600B8B93F /* MiddlewareTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = MiddlewareTests.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -266,6 +268,7 @@
EADEB8EC1DECD335005322DA /* AnalyticsTests-Bridging-Header.h */,
6E265C781FB1178C0030E08E /* IntegrationsManagerTest.swift */,
6EEC1C702017EA370089C478 /* EndToEndTests.swift */,
A31958EE2385AC3A00A47EFA /* SerializationTests.m */,
);
indentWidth = 2;
path = AnalyticsTests;
Expand Down Expand Up @@ -486,6 +489,7 @@
developmentRegion = English;
hasScannedForEncodings = 0;
knownRegions = (
English,
en,
);
mainGroup = EADEB8511DECD080005322DA;
Expand Down Expand Up @@ -604,6 +608,7 @@
EADEB8F21DECD335005322DA /* UserDefaultsStorageTest.swift in Sources */,
EAA542771EB4035400945DA7 /* TrackingTests.swift in Sources */,
EADEB8F41DECD335005322DA /* ContextTest.swift in Sources */,
A31958EF2385AC3A00A47EFA /* SerializationTests.m in Sources */,
EADEB8EE1DECD335005322DA /* HTTPClientTest.swift in Sources */,
EADEB8F31DECD335005322DA /* AnalyticsTests.swift in Sources */,
EADEB8F11DECD335005322DA /* FileStorageTest.swift in Sources */,
Expand Down
24 changes: 9 additions & 15 deletions Analytics.xcodeproj/xcshareddata/xcschemes/Analytics.xcscheme
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,17 @@
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
shouldUseLaunchSchemeArgsEnv = "YES"
codeCoverageEnabled = "YES">
<MacroExpansion>
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "EADEB85A1DECD080005322DA"
BuildableName = "Analytics.framework"
BlueprintName = "Analytics"
ReferencedContainer = "container:Analytics.xcodeproj">
</BuildableReference>
</MacroExpansion>
<Testables>
<TestableReference
skipped = "NO">
Expand All @@ -41,23 +49,11 @@
</BuildableReference>
</TestableReference>
</Testables>
<MacroExpansion>
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "EADEB85A1DECD080005322DA"
BuildableName = "Analytics.framework"
BlueprintName = "Analytics"
ReferencedContainer = "container:Analytics.xcodeproj">
</BuildableReference>
</MacroExpansion>
<AdditionalOptions>
</AdditionalOptions>
</TestAction>
<LaunchAction
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
launchStyle = "0"
useCustomWorkingDirectory = "NO"
ignoresPersistentStateOnLaunch = "NO"
Expand All @@ -73,8 +69,6 @@
ReferencedContainer = "container:Analytics.xcodeproj">
</BuildableReference>
</MacroExpansion>
<AdditionalOptions>
</AdditionalOptions>
</LaunchAction>
<ProfileAction
buildConfiguration = "Release"
Expand Down
30 changes: 12 additions & 18 deletions Analytics.xcodeproj/xcshareddata/xcschemes/AnalyticsTests.xcscheme
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,7 @@
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
shouldUseLaunchSchemeArgsEnv = "NO">
<Testables>
<TestableReference
skipped = "NO">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "EADEB8691DECD0EF005322DA"
BuildableName = "AnalyticsTests.xctest"
BlueprintName = "AnalyticsTests"
ReferencedContainer = "container:Analytics.xcodeproj">
</BuildableReference>
</TestableReference>
</Testables>
<MacroExpansion>
<BuildableReference
BuildableIdentifier = "primary"
Expand All @@ -45,22 +32,29 @@
isEnabled = "YES">
</EnvironmentVariable>
</EnvironmentVariables>
<AdditionalOptions>
</AdditionalOptions>
<Testables>
<TestableReference
skipped = "NO">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "EADEB8691DECD0EF005322DA"
BuildableName = "AnalyticsTests.xctest"
BlueprintName = "AnalyticsTests"
ReferencedContainer = "container:Analytics.xcodeproj">
</BuildableReference>
</TestableReference>
</Testables>
</TestAction>
<LaunchAction
buildConfiguration = "Debug"
selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
language = ""
launchStyle = "0"
useCustomWorkingDirectory = "NO"
ignoresPersistentStateOnLaunch = "NO"
debugDocumentVersioning = "YES"
debugServiceExtension = "internal"
allowLocationSimulation = "YES">
<AdditionalOptions>
</AdditionalOptions>
</LaunchAction>
<ProfileAction
buildConfiguration = "Release"
Expand Down
15 changes: 15 additions & 0 deletions Analytics/Classes/Internal/SEGAnalyticsUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ NS_ASSUME_NONNULL_BEGIN

NSString *GenerateUUIDString(void);

// Validation Utils
BOOL serializableDictionaryTypes(NSDictionary *dict);

// Date Utils
NSString *iso8601FormattedString(NSDate *date);

Expand Down Expand Up @@ -33,4 +36,16 @@ NSString *_Nullable SEGIDFA(void);

NSString *SEGEventNameForScreenTitle(NSString *title);

// Deep copy and check NSCoding conformance
@protocol SEGSerializableDeepCopy <NSObject>
-(id _Nullable) serializableDeepCopy;
@end

@interface NSDictionary(SerializableDeepCopy) <SEGSerializableDeepCopy>
@end

@interface NSArray(SerializableDeepCopy) <SEGSerializableDeepCopy>
@end


NS_ASSUME_NONNULL_END
93 changes: 73 additions & 20 deletions Analytics/Classes/Internal/SEGAnalyticsUtils.m
Original file line number Diff line number Diff line change
Expand Up @@ -160,31 +160,12 @@ static id SEGCoerceJSONObject(id obj)
return [obj description];
}

static void AssertDictionaryTypes(id dict)
{
#ifdef DEBUG
assert([dict isKindOfClass:[NSDictionary class]]);
for (id key in dict) {
assert([key isKindOfClass:[NSString class]]);
id value = dict[key];

assert([value isKindOfClass:[NSString class]] ||
[value isKindOfClass:[NSNumber class]] ||
[value isKindOfClass:[NSNull class]] ||
[value isKindOfClass:[NSArray class]] ||
[value isKindOfClass:[NSDictionary class]] ||
[value isKindOfClass:[NSDate class]] ||
[value isKindOfClass:[NSURL class]]);
}
#endif
}

NSDictionary *SEGCoerceDictionary(NSDictionary *dict)
{
// make sure that a new dictionary exists even if the input is null
dict = dict ?: @{};
// assert that the proper types are in the dictionary
AssertDictionaryTypes(dict);
dict = [dict serializableDeepCopy];
// coerce urls, and dates to the proper format
return SEGCoerceJSONObject(dict);
}
Expand Down Expand Up @@ -214,3 +195,75 @@ static void AssertDictionaryTypes(id dict)
{
return [[NSString alloc] initWithFormat:@"Viewed %@ Screen", title];
}


@implementation NSDictionary(SerializableDeepCopy)

- (NSDictionary *)serializableDeepCopy
{
NSMutableDictionary *returnDict = [[NSMutableDictionary alloc] initWithCapacity:self.count];
NSArray *keys = [self allKeys];
for (id key in keys) {
id aValue = [self objectForKey:key];
id theCopy = nil;

if (![aValue conformsToProtocol:@protocol(NSCoding)]) {
#ifdef DEBUG
NSAssert(FALSE, @"key `%@` doesn't conform to NSCoding and can't be serialized for delivery.", key);
#else
SEGLog(@"key `%@` doesn't conform to NSCoding and can't be serialized for delivery.", key);
// simply leave it out since we can't encode it anyway.
continue;
#endif
}

if ([aValue conformsToProtocol:@protocol(SEGSerializableDeepCopy)]) {
theCopy = [aValue serializableDeepCopy];
} else if ([aValue conformsToProtocol:@protocol(NSCopying)]) {
theCopy = [aValue copy];
} else {
theCopy = aValue;
}

[returnDict setValue:theCopy forKey:key];
}

return [returnDict copy];
}

@end


@implementation NSArray(SerializableDeepCopy)

-(NSArray *)serializableDeepCopy
{
NSMutableArray *returnArray = [[NSMutableArray alloc] initWithCapacity:self.count];

for (id aValue in self) {
id theCopy = nil;

if (![aValue conformsToProtocol:@protocol(NSCoding)]) {
#ifdef DEBUG
NSAssert(FALSE, @"type `%@` doesn't conform to NSCoding and can't be serialized for delivery.", NSStringFromClass([aValue class]));
#else
SEGLog(@"type `%@` doesn't conform to NSCoding and can't be serialized for delivery.", NSStringFromClass([aValue class]));
// simply leave it out since we can't encode it anyway.
continue;
#endif
}

if ([aValue conformsToProtocol:@protocol(SEGSerializableDeepCopy)]) {
theCopy = [aValue serializableDeepCopy];
} else if ([aValue conformsToProtocol:@protocol(NSCopying)]) {
theCopy = [aValue copy];
} else {
theCopy = aValue;
}
[returnArray addObject:theCopy];
}

return [returnArray copy];
}

@end
19 changes: 13 additions & 6 deletions Analytics/Classes/Internal/SEGFileStorage.m
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,19 @@ - (void)setPlist:(id _Nonnull)plist forKey:(NSString *)key
- (NSData *_Nullable)dataFromPlist:(nonnull id)plist
{
NSError *error = nil;
NSData *data = [NSPropertyListSerialization dataWithPropertyList:plist
format:NSPropertyListXMLFormat_v1_0
options:0
error:&error];
if (error) {
SEGLog(@"Unable to serialize data from plist object", error, plist);
NSData *data = nil;
// Temporary just-in-case fix for issue #846; Follow-on PR to move away from plist storage.
@try {
data = [NSPropertyListSerialization dataWithPropertyList:plist
format:NSPropertyListXMLFormat_v1_0
options:0
error:&error];
} @catch (NSException *e) {
SEGLog(@"Unable to serialize data from plist object; Exception: %@, plist: %@", e, plist);
} @finally {
if (error) {
SEGLog(@"Unable to serialize data from plist object; Error: %@, plist: %@", error, plist);
}
}
return data;
}
Expand Down
55 changes: 55 additions & 0 deletions AnalyticsTests/SerializationTests.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
//
// PropSerializationTests.m
// AnalyticsTests
//
// Created by Brandon Sneed on 11/20/19.
// Copyright © 2019 Segment. All rights reserved.
//

#import <XCTest/XCTest.h>
@import Analytics;

@protocol SEGSerializableDeepCopy <NSObject>
-(id _Nullable) serializableDeepCopy;
@end

@interface NSDictionary(SerializableDeepCopy) <SEGSerializableDeepCopy>
@end

@interface NSArray(SerializableDeepCopy) <SEGSerializableDeepCopy>
@end

@interface SerializationTests : XCTestCase

@end

@implementation SerializationTests

- (void)setUp {
// Put setup code here. This method is called before the invocation of each test method in the class.
}

- (void)tearDown {
// Put teardown code here. This method is called after the invocation of each test method in the class.
}

- (void)testDeepCopyAndConformance {
NSDictionary *nonserializable = @{@"test": @1, @"nonserializable": self, @"nested": @{@"nonserializable": self}, @"array": @[@1, @2, @3, self]};
NSDictionary *serializable = @{@"test": @1, @"nonserializable": @0, @"nested": @{@"nonserializable": @0}, @"array": @[@1, @2, @3, @0]};

NSDictionary *aCopy = [serializable serializableDeepCopy];
XCTAssert(aCopy != serializable);

NSDictionary *sub = [serializable objectForKey:@"nested"];
NSDictionary *subCopy = [aCopy objectForKey:@"nested"];
XCTAssert(sub != subCopy);

NSDictionary *array = [serializable objectForKey:@"array"];
NSDictionary *arrayCopy = [aCopy objectForKey:@"array"];
XCTAssert(array != arrayCopy);

XCTAssertNoThrow([serializable serializableDeepCopy]);
XCTAssertThrows([nonserializable serializableDeepCopy]);
}

@end

0 comments on commit 19d6c56

Please sign in to comment.