Skip to content

Allow Bundle IDs that have a valid prefix - strict validation #2515

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Mar 12, 2019
Merged

Allow Bundle IDs that have a valid prefix - strict validation #2515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c797071
Allow Bundle IDs that have a valid prefix
bstpierr Feb 22, 2019
cdf452a
Validate extension Bundle IDs more strictly.
maksymmalyhin Mar 11, 2019
0ce2482
Run ./scripts/style.sh
maksymmalyhin Mar 11, 2019
fbc0881
Merge branch 'master'
maksymmalyhin Mar 11, 2019
8338fcd
Code cleanup and comments update
maksymmalyhin Mar 11, 2019
fdd44e2
FirebaseCore.podspec: 'GoogleUtilities/Environment' dependency added.
maksymmalyhin Mar 11, 2019
1d1e1f6
FirebaseCore.podspec: dependencies alphabetic order
maksymmalyhin Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions Example/Core/Tests/FIRBundleUtilTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#import "FIRTestCase.h"

#import <FirebaseCore/FIRBundleUtil.h>
#import <GoogleUtilities/GULAppEnvironmentUtil.h>

static NSString *const kResultPath = @"resultPath";
static NSString *const kResourceName = @"resourceName";
Expand Down Expand Up @@ -69,16 +70,53 @@ - (void)testFindOptionsDictionaryPath_secondBundle {
- (void)testBundleIdentifierExistsInBundles {
NSString *bundleID = @"com.google.test";
[OCMStub([self.mockBundle bundleIdentifier]) andReturn:bundleID];
XCTAssertTrue([FIRBundleUtil hasBundleIdentifier:bundleID inBundles:@[ self.mockBundle ]]);
XCTAssertTrue([FIRBundleUtil hasBundleIdentifierPrefix:bundleID inBundles:@[ self.mockBundle ]]);
}

- (void)testBundleIdentifierExistsInBundles_notExist {
[OCMStub([self.mockBundle bundleIdentifier]) andReturn:@"com.google.test"];
XCTAssertFalse([FIRBundleUtil hasBundleIdentifier:@"not-exist" inBundles:@[ self.mockBundle ]]);
XCTAssertFalse([FIRBundleUtil hasBundleIdentifierPrefix:@"not-exist"
inBundles:@[ self.mockBundle ]]);
}

- (void)testBundleIdentifierExistsInBundles_emptyBundlesArray {
XCTAssertFalse([FIRBundleUtil hasBundleIdentifier:@"com.google.test" inBundles:@[]]);
XCTAssertFalse([FIRBundleUtil hasBundleIdentifierPrefix:@"com.google.test" inBundles:@[]]);
}

- (void)testBundleIdentifierHasPrefixInBundlesForExtension {
id environmentUtilsMock = [OCMockObject mockForClass:[GULAppEnvironmentUtil class]];
[[[environmentUtilsMock stub] andReturnValue:@(YES)] isAppExtension];

[OCMStub([self.mockBundle bundleIdentifier]) andReturn:@"com.google.test"];
XCTAssertTrue([FIRBundleUtil hasBundleIdentifierPrefix:@"com.google.test.someextension"
inBundles:@[ self.mockBundle ]]);

[environmentUtilsMock stopMocking];
}

- (void)testBundleIdentifierHasPrefixInBundlesNotValidExtension {
id environmentUtilsMock = [OCMockObject mockForClass:[GULAppEnvironmentUtil class]];
[[[environmentUtilsMock stub] andReturnValue:@(YES)] isAppExtension];

[OCMStub([self.mockBundle bundleIdentifier]) andReturn:@"com.google.test"];
XCTAssertFalse([FIRBundleUtil hasBundleIdentifierPrefix:@"com.google.test.someextension.some"
inBundles:@[ self.mockBundle ]]);

XCTAssertFalse([FIRBundleUtil hasBundleIdentifierPrefix:@"com.google.testsomeextension"
inBundles:@[ self.mockBundle ]]);

XCTAssertFalse([FIRBundleUtil hasBundleIdentifierPrefix:@"com.google.testsomeextension.some"
inBundles:@[ self.mockBundle ]]);

XCTAssertFalse([FIRBundleUtil hasBundleIdentifierPrefix:@"not-exist"
inBundles:@[ self.mockBundle ]]);

// Should be NO, since if @"com.google.tests" is an app extension identifier, then the app bundle
// identifier is @"com.google"
XCTAssertFalse([FIRBundleUtil hasBundleIdentifierPrefix:@"com.google.tests"
inBundles:@[ self.mockBundle ]]);

[environmentUtilsMock stopMocking];
}

@end
4 changes: 2 additions & 2 deletions Firebase/Core/FIRApp.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,8 +525,8 @@ - (void)checkExpectedBundleID {
NSString *expectedBundleID = [self expectedBundleID];
// The checking is only done when the bundle ID is provided in the serviceInfo dictionary for
// backward compatibility.
if (expectedBundleID != nil && ![FIRBundleUtil hasBundleIdentifier:expectedBundleID
inBundles:bundles]) {
if (expectedBundleID != nil && ![FIRBundleUtil hasBundleIdentifierPrefix:expectedBundleID
inBundles:bundles]) {
FIRLogError(kFIRLoggerCore, @"I-COR000008",
@"The project's Bundle ID is inconsistent with "
@"either the Bundle ID in '%@.%@', or the Bundle ID in the options if you are "
Expand Down
22 changes: 20 additions & 2 deletions Firebase/Core/FIRBundleUtil.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

#import "Private/FIRBundleUtil.h"

#import <GoogleUtilities/GULAppEnvironmentUtil.h>

@implementation FIRBundleUtil

+ (NSArray *)relevantBundles {
Expand Down Expand Up @@ -45,13 +47,29 @@ + (NSArray *)relevantURLSchemes {
return result;
}

+ (BOOL)hasBundleIdentifier:(NSString *)bundleIdentifier inBundles:(NSArray *)bundles {
+ (BOOL)hasBundleIdentifierPrefix:(NSString *)bundleIdentifier inBundles:(NSArray *)bundles {
for (NSBundle *bundle in bundles) {
if ([bundle.bundleIdentifier isEqualToString:bundleIdentifier]) {
// This allows app extensions that have the app's bundle as their prefix to pass this test.
NSString *applicationBundleIdentifier =
[GULAppEnvironmentUtil isAppExtension]
? [self bundleIdentifierByRemovingLastPartFrom:bundleIdentifier]
: bundleIdentifier;

if ([applicationBundleIdentifier isEqualToString:bundle.bundleIdentifier]) {
return YES;
}
}
return NO;
}

+ (NSString *)bundleIdentifierByRemovingLastPartFrom:(NSString *)bundleIdentifier {
NSString *bundleIDComponentsSeparator = @".";

NSMutableArray<NSString *> *bundleIDComponents =
[[bundleIdentifier componentsSeparatedByString:bundleIDComponentsSeparator] mutableCopy];
[bundleIDComponents removeLastObject];

return [bundleIDComponents componentsJoinedByString:bundleIDComponentsSeparator];
}

@end
5 changes: 3 additions & 2 deletions Firebase/Core/Private/FIRBundleUtil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@
+ (NSArray *)relevantURLSchemes;

/**
* Checks if the bundle identifier exists in the given bundles.
* Checks if any of the given bundles have a matching bundle identifier prefix (removing extension
* suffixes).
*/
+ (BOOL)hasBundleIdentifier:(NSString *)bundleIdentifier inBundles:(NSArray *)bundles;
+ (BOOL)hasBundleIdentifierPrefix:(NSString *)bundleIdentifier inBundles:(NSArray *)bundles;

@end
1 change: 1 addition & 0 deletions FirebaseCore.podspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Firebase Core includes FIRApp and FIROptions which provide central configuration
s.public_header_files = 'Firebase/Core/Public/*.h', 'Firebase/Core/Private/*.h'
s.private_header_files = 'Firebase/Core/Private/*.h'
s.framework = 'Foundation'
s.dependency 'GoogleUtilities/Environment', '~> 5.2'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanwilson Not sure we catch missing dependencies in our tests...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch - I'm not sure why this wouldn't be caught by pod lib lint - @paulb777 any ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's always a transitive dep - https://github.com/firebase/firebase-ios-sdk/blob/master/GoogleUtilities.podspec#L38

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! Can you please move it up one to be above Logger (alphabetical order)?

s.dependency 'GoogleUtilities/Logger', '~> 5.2'
s.pod_target_xcconfig = {
'GCC_C_LANGUAGE_STANDARD' => 'c99',
Expand Down