Skip to content

Commit

Permalink
Enforce the types for sessionUserInfo.
Browse files Browse the repository at this point in the history 
Ensure the types are correct for the api contract in all places possible.

Add some testing also to ensure things are enforced.
  • Loading branch information
@thomasvl
thomasvl committed Jan 29, 2025
1 parent 3cdb78e commit c8e886f
Show file tree
Hide file tree
Showing 2 changed files with 191 additions and 52 deletions.
118 changes: 66 additions & 52 deletions Sources/Core/GTMSessionFetcher.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1594,56 +1594,65 @@ - (void)setSessionIdentifierInternal:(nullable NSString *)sessionIdentifier {
if (_sessionUserInfo == nil) {
// We'll return the metadata dictionary with internal keys removed. This avoids the user
// re-using the userInfo dictionary later and accidentally including the internal keys.
// Just incase something got corrupted in storage and parsed back out differently, ensure
// the api contract on types is still valid.
NSMutableDictionary *metadata = [[self sessionIdentifierMetadataUnsynchronized] mutableCopy];
NSSet *keysToRemove = [metadata keysOfEntriesPassingTest:^BOOL(id key, id obj, BOOL *stop) {
return [key hasPrefix:@"_"];
return ![key isKindOfClass:[NSString class]] || ![obj isKindOfClass:[NSString class]] ||
[key hasPrefix:@"_"];
}];
[metadata removeObjectsForKeys:[keysToRemove allObjects]];
if (metadata.count > 0) {
_sessionUserInfo = metadata;

#if DEBUG
// If we're pruning, give warnings about the things that were invalid as some bug has slipped
// through.
if (keysToRemove.count) {
[metadata enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull obj,
BOOL *_Nonnull stop) {
GTMSESSION_ASSERT_DEBUG([key isKindOfClass:[NSString class]],
@"sessionUserInfo keys must be NSStrings: %@", key);
if (![obj isKindOfClass:[NSString class]]) {
GTMSESSION_LOG_DEBUG(@"WARNING: sessionUserInfo included a non String value, this will "
@"be an error in the future: %@: %@",
key, obj);
if (![key isKindOfClass:[NSString class]]) {
GTMSESSION_LOG_DEBUG(
@"InternalError: restoring sessionUserInfo is pruning a non String key: %@", key);
} else if (![obj isKindOfClass:[NSString class]]) {
GTMSESSION_LOG_DEBUG(
@"InternalError: restoring sessionUserInfo is pruning a non String value: %@: %@",
key, obj);
}
}];
}
#endif // DEBUG
[metadata removeObjectsForKeys:[keysToRemove allObjects]];

if (metadata.count > 0) {
_sessionUserInfo = metadata;
}
}
return _sessionUserInfo;
} // @synchronized(self)
}

- (void)setSessionUserInfo:(nullable NSDictionary<NSString *, NSString *> *)dictionary {
[dictionary enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull obj,
BOOL *_Nonnull stop) {
if (![key isKindOfClass:[NSString class]]) {
[NSException raise:NSInvalidArgumentException
format:@"sessionUserInfo keys must be NSStrings: %@", key];
}
if ([key hasPrefix:@"_"]) {
[NSException
raise:NSInvalidArgumentException
format:
@"sessionUserInfo keys starting with an underscore are reserved for the library: %@",
key];
}
if (![obj isKindOfClass:[NSString class]]) {
[NSException raise:NSInvalidArgumentException
format:@"Values in sessionUserInfo must be NSStrings: %@: %@", key, obj];
}
}];

@synchronized(self) {
GTMSessionMonitorSynchronized(self);

GTMSESSION_ASSERT_DEBUG(_sessionIdentifier == nil, @"Too late to assign userInfo");
_sessionUserInfo = dictionary;

#if DEBUG
[dictionary enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull obj,
BOOL *_Nonnull stop) {
GTMSESSION_ASSERT_DEBUG([key isKindOfClass:[NSString class]],
@"sessionUserInfo keys must be NSStrings: %@", key);
if ([key hasPrefix:@"_"]) {
GTMSESSION_LOG_DEBUG(@"WARNING: sessionUserInfo keys starting with an underscore are "
@"reserved for the library, this will become an error in the future: "
@"%@: %@", key, obj);
}
if (![obj isKindOfClass:[NSString class]]) {
GTMSESSION_LOG_DEBUG(@"WARNING: sessionUserInfo included a non String value, this will be "
@"an error in the future: %@: %@",
key, obj);
}
}];
#endif // DEBUG
} // @synchronized(self)
}

Expand Down Expand Up @@ -1734,34 +1743,39 @@ - (NSString *)createSessionIdentifierWithMetadata:(nullable NSDictionary *)metad
_sessionIdentifier =
[NSString stringWithFormat:@"%@_%@", kGTMSessionIdentifierPrefix, _sessionIdentifierUUID];

#if DEBUG
// _sessionUserInfo was declared as `strong` (not `copy`, so it could have been modifed after
// having been set.
[_sessionUserInfo enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull obj,
BOOL *_Nonnull stop) {
GTMSESSION_ASSERT_DEBUG([key isKindOfClass:[NSString class]],
@"sessionUserInfo keys must be NSStrings: %@", key);
if ([key hasPrefix:@"_"]) {
GTMSESSION_LOG_DEBUG(@"WARNING: sessionUserInfo keys starting with an underscore are "
@"reserved for the library, this will become an error in the future: "
@"%@: %@", key, obj);
}
if (![obj isKindOfClass:[NSString class]]) {
GTMSESSION_LOG_DEBUG(@"WARNING: sessionUserInfo included a non String value, this will "
@"be an error in the future: %@: %@",
key, obj);
}
}];
#endif // DEBUG

// Start with user-supplied keys so they cannot accidentally override the fetcher's keys.
NSMutableDictionary *metadataDict =
[NSMutableDictionary dictionaryWithDictionary:(NSDictionary *_Nonnull)_sessionUserInfo];

// sessionUserInfo was declared as `strong` (not `copy`), so it could have been modifed after
// having been set. So remove anything that breaks the contract.
NSSet *keysToRemove = [metadataDict keysOfEntriesPassingTest:^BOOL(id key, id obj, BOOL *stop) {
return ![key isKindOfClass:[NSString class]] || ![obj isKindOfClass:[NSString class]] ||
[key hasPrefix:@"_"];
}];
#if DEBUG
// If we're pruning, give warnings about the things that were invalid as some bug has slipped
// through.
if (keysToRemove.count) {
[metadataDict enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull obj,
BOOL *_Nonnull stop) {
if (![key isKindOfClass:[NSString class]]) {
GTMSESSION_LOG_DEBUG(
@"Warning: sessionUserInfo has been modifed to include a non String key: %@", key);
} else if (![obj isKindOfClass:[NSString class]]) {
GTMSESSION_LOG_DEBUG(
@"Warning: sessionUserInfo has been modifed to include a non String value: %@: %@",
key, obj);
}
}];
}
#endif // DEBUG
[metadataDict removeObjectsForKeys:[keysToRemove allObjects]];

if (metadataToInclude) {
#if DEBUG
[metadataToInclude enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull obj,
BOOL * _Nonnull stop) {
[metadataToInclude enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull obj,
BOOL *_Nonnull stop) {
GTMSESSION_ASSERT_DEBUG([key isKindOfClass:[NSString class]],
@"metadataToInclude keys must be NSStrings: %@", key);
GTMSESSION_ASSERT_DEBUG([key hasPrefix:@"_"],
Expand All @@ -1774,7 +1788,7 @@ - (NSString *)createSessionIdentifierWithMetadata:(nullable NSDictionary *)metad
NSDictionary *defaultMetadataDict = [self sessionIdentifierDefaultMetadata];
if (defaultMetadataDict) {
#if DEBUG
[defaultMetadataDict enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull obj,
[defaultMetadataDict enumerateKeysAndObjectsUsingBlock:^(id _Nonnull key, id _Nonnull obj,
BOOL * _Nonnull stop) {
GTMSESSION_ASSERT_DEBUG([key isKindOfClass:[NSString class]],
@"defaultMetadataDict keys must be NSStrings: %@", key);
Expand Down
125 changes: 125 additions & 0 deletions UnitTests/GTMSessionFetcherFetchingTest.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@
@interface GTMSessionFetcher (ExposedForTesting)
+ (nullable NSURL *)redirectURLWithOriginalRequestURL:(nullable NSURL *)originalRequestURL
redirectRequestURL:(nullable NSURL *)redirectRequestURL;
- (NSString *)createSessionIdentifierWithMetadata:(NSDictionary *)metadata;
@end

@interface TestIdentifierMetadataFecher : GTMSessionFetcher
@property(strong, nullable) NSDictionary *testIdentifierMetadata;
@end

@implementation TestIdentifierMetadataFecher
- (nullable NSDictionary *)sessionIdentifierMetadataUnsynchronized {
return self.testIdentifierMetadata;
}
@end

// Base class for fetcher and chunked upload tests.
Expand Down Expand Up @@ -258,6 +269,120 @@ @interface GTMSessionFetcherFetchingTest : GTMSessionFetcherBaseTest

@implementation GTMSessionFetcherFetchingTest

#pragma mark - SessionUserInfo Tests

- (void)testSetSessionUserInfo {
GTMSessionFetcher *fetcher = [GTMSessionFetcher fetcherWithURLString:@"file://not_needed"];

@try {
NSDictionary *dict = @{@1 : @"invalid non string key"};
fetcher.sessionUserInfo = dict;
XCTFail(@"Shouldn't get here");
} @catch (NSException *exception) {
XCTAssertEqualObjects(exception.name, NSInvalidArgumentException);
XCTAssertEqualObjects(exception.reason, @"sessionUserInfo keys must be NSStrings: 1");
}
XCTAssertNil(fetcher.sessionUserInfo);

// Key with a leading underscore
@try {
fetcher.sessionUserInfo = @{@"_invalidUnderscore" : @"value"};
XCTFail(@"Shouldn't get here");
} @catch (NSException *exception) {
XCTAssertEqualObjects(exception.name, NSInvalidArgumentException);
XCTAssertEqualObjects(exception.reason, @"sessionUserInfo keys starting with an underscore are "
@"reserved for the library: _invalidUnderscore");
}
XCTAssertNil(fetcher.sessionUserInfo);

@try {
NSDictionary *dict = @{@"InvalidNonStringValue" : @1};
fetcher.sessionUserInfo = dict;
XCTFail(@"Shouldn't get here");
} @catch (NSException *exception) {
XCTAssertEqualObjects(exception.name, NSInvalidArgumentException);
XCTAssertEqualObjects(exception.reason,
@"Values in sessionUserInfo must be NSStrings: InvalidNonStringValue: 1");
}
XCTAssertNil(fetcher.sessionUserInfo);

NSDictionary *validDict = @{@"key" : @"value"};
@try {
fetcher.sessionUserInfo = validDict;
} @catch (NSException *exception) {
XCTFail(@"Shouldn't have gotten here: %@", exception);
}
XCTAssertEqualObjects(fetcher.sessionUserInfo, validDict);
XCTAssertTrue(fetcher.sessionUserInfo == validDict); // Ptr equality, property is strong.
}

- (void)testCreateSessionIdentifierWithMetadata {
// Since `sessionUserInfo` is a `strong` property, the value can be modified after being set
// and thus `createSessionIdentifierWithMetadata:` has to deal with late arriving invalide
// values. Everything else should get encoded into the identifier.

GTMSessionFetcher *fetcher = [GTMSessionFetcher fetcherWithURLString:@"file://not_needed"];
NSMutableDictionary *dict = [NSMutableDictionary dictionaryWithObject:@"GoodValue"
forKey:@"GoodKey"];
fetcher.sessionUserInfo = dict;

// Values that make it through end up in the identifier
NSString *identifier = [fetcher createSessionIdentifierWithMetadata:nil];
XCTAssertTrue([identifier containsString:@"{\"GoodKey\":\"GoodValue\"}"], @"%@", identifier);

[dict removeAllObjects];
fetcher = [GTMSessionFetcher fetcherWithURLString:@"file://not_needed"];
fetcher.sessionUserInfo = dict;
[dict setObject:@"NonStringKey" forKey:@2];
identifier = [fetcher createSessionIdentifierWithMetadata:nil];
XCTAssertFalse([identifier containsString:@"NonStringKey"], @"%@", identifier);
XCTAssertTrue(fetcher.sessionUserInfo == dict); // Ptr compared, dict still set.

[dict removeAllObjects];
fetcher = [GTMSessionFetcher fetcherWithURLString:@"file://not_needed"];
fetcher.sessionUserInfo = dict;
[dict setObject:@"KeysCantHaveLeadingUnderscore" forKey:@"_InvalidKey"];
identifier = [fetcher createSessionIdentifierWithMetadata:nil];
XCTAssertFalse([identifier containsString:@"InvalidKey"], @"%@", identifier);
XCTAssertFalse([identifier containsString:@"KeysCantHaveLeadingUnderscore"], @"%@", identifier);
XCTAssertTrue(fetcher.sessionUserInfo == dict); // Ptr compared, dict still set.

[dict removeAllObjects];
fetcher = [GTMSessionFetcher fetcherWithURLString:@"file://not_needed"];
fetcher.sessionUserInfo = dict;
[dict setObject:@1 forKey:@"ValuesMustBeStrings"];
identifier = [fetcher createSessionIdentifierWithMetadata:nil];
XCTAssertFalse([identifier containsString:@"ValuesMustBeStrings"], @"%@", identifier);
XCTAssertTrue(fetcher.sessionUserInfo == dict); // Ptr compared, dict still set.
}

- (void)testSessionUserInfoFromSessionIdentifierMetadata {
NSDictionary *expected = @{@"GoodKey" : @"GoodValue"};
NSMutableDictionary *dict = [expected mutableCopy];

TestIdentifierMetadataFecher *fetcher =
[TestIdentifierMetadataFecher fetcherWithURLString:@"file://not_needed"];
fetcher.testIdentifierMetadata = dict;
XCTAssertEqualObjects(fetcher.sessionUserInfo, expected);

// Add these additions will get dropped

[dict setObject:@"NonStringKey" forKey:@1];
fetcher = [TestIdentifierMetadataFecher fetcherWithURLString:@"file://not_needed"];
fetcher.testIdentifierMetadata = dict;
XCTAssertEqualObjects(fetcher.sessionUserInfo, expected);

[dict setObject:@"InvalidKey" forKey:@"_UnderscorePrefixedKey"];
fetcher = [TestIdentifierMetadataFecher fetcherWithURLString:@"file://not_needed"];
fetcher.testIdentifierMetadata = dict;
XCTAssertEqualObjects(fetcher.sessionUserInfo, expected);

[dict setObject:@5 forKey:@"NonStringValue"];
fetcher = [TestIdentifierMetadataFecher fetcherWithURLString:@"file://not_needed"];
fetcher.testIdentifierMetadata = dict;
XCTAssertEqualObjects(fetcher.sessionUserInfo, expected);
}

#pragma mark - Fetcher Tests

- (void)assertSuccessfulGettysburgFetchWithFetcher:(GTMSessionFetcher *)fetcher
Expand Down

0 comments on commit c8e886f

Please sign in to comment.