diff --git a/GoogleSignIn/Sources/GIDSignIn.m b/GoogleSignIn/Sources/GIDSignIn.m index 82a8b039..f910e72d 100644 --- a/GoogleSignIn/Sources/GIDSignIn.m +++ b/GoogleSignIn/Sources/GIDSignIn.m @@ -80,11 +80,20 @@ // The URL template for the token endpoint. static NSString *const kTokenURLTemplate = @"https://%@/token"; -// The URL template for the URL to get user info. -static NSString *const kUserInfoURLTemplate = @"https://%@/oauth2/v3/userinfo?access_token=%@"; +// The path for the endpoint to get user info. +static NSString *const kUserInfoPath = @"/oauth2/v3/userinfo"; -// The URL template for the URL to revoke the token. -static NSString *const kRevokeTokenURLTemplate = @"https://%@/o/oauth2/revoke?token=%@"; +// The name of the query parameter carrying the access token for the user info request. +static NSString *const kAccessTokenParameter = @"access_token"; + +// The path for the endpoint to revoke the token. +static NSString *const kRevokeTokenPath = @"/o/oauth2/revoke"; + +// The name of the query parameter carrying the token to be revoked. +static NSString *const kRevokeTokenParameter = @"token"; + +// The scheme used for requests to Google's servers. +static NSString *const kHTTPSScheme = @"https"; // Expected path in the URL scheme to be handled. static NSString *const kBrowserCallbackPath = @"/oauth2callback"; @@ -550,6 +559,15 @@ - (void)signOut { [self removeAllKeychainEntries]; } +// OAuth parameters are application/x-www-form-urlencoded (RFC 6749 Appendix B), where "+" +// means a space, but NSURLComponents leaves "+" literal because RFC 3986 permits it in a +// query. Percent-encode it so a "+" in a token survives form decoding on the server. +static void GIDPercentEncodePlusInQuery(NSURLComponents *components) { + components.percentEncodedQuery = + [components.percentEncodedQuery stringByReplacingOccurrencesOfString:@"+" + withString:@"%2B"]; +} + - (void)disconnectWithCompletion:(nullable GIDDisconnectCompletion)completion { OIDAuthState *authState = _currentUser.authState; if (!authState) { @@ -573,17 +591,23 @@ - (void)disconnectWithCompletion:(nullable GIDDisconnectCompletion)completion { } return; } - NSString *revokeURLString = [NSString stringWithFormat:kRevokeTokenURLTemplate, - [GIDSignInPreferences googleAuthorizationServer], token]; - // Append logging parameter - revokeURLString = [NSString stringWithFormat:@"%@&%@=%@&%@=%@", - revokeURLString, - kSDKVersionLoggingParameter, - [GIDSignInPreferences sdkVersion], - kEnvironmentLoggingParameter, - [GIDSignInPreferences environment]]; - NSURL *revokeURL = [NSURL URLWithString:revokeURLString]; - [self startFetchURL:revokeURL + NSURLComponents *revokeURLComponents = [[NSURLComponents alloc] init]; + revokeURLComponents.scheme = kHTTPSScheme; + revokeURLComponents.host = [GIDSignInPreferences googleAuthorizationServer]; + revokeURLComponents.path = kRevokeTokenPath; + + NSMutableArray *queryItems = [NSMutableArray array]; + [queryItems addObject:[NSURLQueryItem queryItemWithName:kRevokeTokenParameter value:token]]; + NSDictionary *loggingParameters = + [GIDSignInPreferences loggingParameters]; + for (NSString *name in [loggingParameters.allKeys sortedArrayUsingSelector:@selector(compare:)]) { + [queryItems addObject:[NSURLQueryItem queryItemWithName:name + value:loggingParameters[name]]]; + } + revokeURLComponents.queryItems = queryItems; + GIDPercentEncodePlusInQuery(revokeURLComponents); + + [self startFetchURL:revokeURLComponents.URL fromAuthState:authState withComment:@"GIDSignIn: revoke tokens" withCompletionHandler:^(NSData *data, NSError *error) { @@ -1135,11 +1159,16 @@ - (void)addDecodeIdTokenCallback:(GIDAuthFlow *)authFlow { // If we can't retrieve profile data from the ID token, make a userInfo request to fetch them. if (!handlerAuthFlow.profileData) { [handlerAuthFlow wait]; - NSURL *infoURL = [NSURL URLWithString: - [NSString stringWithFormat:kUserInfoURLTemplate, - [GIDSignInPreferences googleUserInfoServer], - authState.lastTokenResponse.accessToken]]; - [self startFetchURL:infoURL + NSURLComponents *infoURLComponents = [[NSURLComponents alloc] init]; + infoURLComponents.scheme = kHTTPSScheme; + infoURLComponents.host = [GIDSignInPreferences googleUserInfoServer]; + infoURLComponents.path = kUserInfoPath; + infoURLComponents.queryItems = @[ + [NSURLQueryItem queryItemWithName:kAccessTokenParameter + value:authState.lastTokenResponse.accessToken], + ]; + GIDPercentEncodePlusInQuery(infoURLComponents); + [self startFetchURL:infoURLComponents.URL fromAuthState:authState withComment:@"GIDSignIn: fetch basic profile info" withCompletionHandler:^(NSData *data, NSError *error) { diff --git a/GoogleSignIn/Tests/Unit/GIDSignInTest.m b/GoogleSignIn/Tests/Unit/GIDSignInTest.m index 36cb3047..a98cdf25 100644 --- a/GoogleSignIn/Tests/Unit/GIDSignInTest.m +++ b/GoogleSignIn/Tests/Unit/GIDSignInTest.m @@ -1349,6 +1349,104 @@ - (void)testDisconnectNoCallback_accessToken { [_tokenResponse verify]; } +// Verifies a token containing characters that are reserved in a URL query is percent-encoded +// in the revoke URL, so that it arrives at the server intact. +- (void)testDisconnectNoCallback_tokenWithReservedCharacters { + NSString *tokenWithReservedCharacters = @"token&with=reserved#characters"; + [[[_authorization expect] andReturn:_authState] authState]; + [[[_authState expect] andReturn:_tokenResponse] lastTokenResponse]; + [[[_tokenResponse expect] andReturn:tokenWithReservedCharacters] accessToken]; + [[[_authorization expect] andReturn:_fetcherService] fetcherService]; + [_signIn disconnectWithCompletion:nil]; + [self verifyAndRevokeToken:tokenWithReservedCharacters + hasCallback:NO + waitingForExpectations:@[]]; + [_authorization verify]; + [_authState verify]; + [_tokenResponse verify]; +} + +// OAuth parameters use application/x-www-form-urlencoded (RFC 6749 Appendix B), where "+" +// means a space, so a literal "+" in a token is sent as "%2B" even though RFC 3986 +// would permit it unescaped in a query. +- (void)testDisconnectNoCallback_tokenWithPlusCharacter { + NSString *tokenWithPlusCharacter = @"token+with+plus"; + [[[_authorization expect] andReturn:_authState] authState]; + [[[_authState expect] andReturn:_tokenResponse] lastTokenResponse]; + [[[_tokenResponse expect] andReturn:tokenWithPlusCharacter] accessToken]; + [[[_authorization expect] andReturn:_fetcherService] fetcherService]; + [_signIn disconnectWithCompletion:nil]; + + XCTAssertTrue([self isFetcherStarted], @"should start fetching"); + NSURL *url = [self fetchedURL]; + XCTAssertEqualObjects([url scheme], @"https", @"scheme must match"); + XCTAssertEqualObjects([url host], @"accounts.google.com", @"host must match"); + XCTAssertEqualObjects([url path], @"/o/oauth2/revoke", @"path must match"); + + NSString *query = [[self fetchedURL] query]; + XCTAssertTrue([query containsString:@"token=token%2Bwith%2Bplus"], + @"'+' should be percent-encoded in the query string"); + XCTAssertFalse([query containsString:@"token=token+with+plus"], + @"'+' should not be literal in the query string"); + + NSURLComponents *components = + [NSURLComponents componentsWithURL:[self fetchedURL] resolvingAgainstBaseURL:NO]; + NSURLQueryItem *tokenItem; + for (NSURLQueryItem *item in components.queryItems) { + if ([item.name isEqualToString:@"token"]) { + tokenItem = item; + break; + } + } + XCTAssertEqualObjects(tokenItem.value, tokenWithPlusCharacter); + + [self didFetch:nil error:nil]; + XCTAssertTrue(_keychainRemoved, @"should clear saved keychain name"); + + [_authorization verify]; + [_authState verify]; + [_tokenResponse verify]; +} + +// Guard the "+" to "%2B" rewrite, which is only safe if a space encodes as "%20", never as "+". +// Whilst this is technically testing Foundation behaviour, it's undocumented behaviour. +- (void)testDisconnectNoCallback_tokenWithSpace { + NSString *tokenWithSpace = @"token with space"; + [[[_authorization expect] andReturn:_authState] authState]; + [[[_authState expect] andReturn:_tokenResponse] lastTokenResponse]; + [[[_tokenResponse expect] andReturn:tokenWithSpace] accessToken]; + [[[_authorization expect] andReturn:_fetcherService] fetcherService]; + [_signIn disconnectWithCompletion:nil]; + + NSString *query = [[self fetchedURL] query]; + XCTAssertTrue([query containsString:@"token=token%20with%20space"], + @"a space should be percent-encoded in the query string"); + XCTAssertFalse([query containsString:@"+"], @"a space should never be encoded as '+'"); + + [self didFetch:nil error:nil]; + XCTAssertTrue(_keychainRemoved, @"should clear saved keychain name"); + [_authorization verify]; + [_authState verify]; + [_tokenResponse verify]; +} + +// Round-trip the revoke URL through OIDURLQueryComponent, a pretend server, to check "+" survives. +- (void)testDisconnectNoCallback_tokenWithPlusCharacterFormDecoded { + NSString *tokenWithPlusCharacter = @"token+with+plus"; + [[[_authorization expect] andReturn:_authState] authState]; + [[[_authState expect] andReturn:_tokenResponse] lastTokenResponse]; + [[[_tokenResponse expect] andReturn:tokenWithPlusCharacter] accessToken]; + [[[_authorization expect] andReturn:_fetcherService] fetcherService]; + [_signIn disconnectWithCompletion:nil]; + + [self verifyAndRevokeToken:tokenWithPlusCharacter + hasCallback:NO + waitingForExpectations:@[]]; + [_authorization verify]; + [_authState verify]; + [_tokenResponse verify]; +} + // Verifies disconnect calls callback with no errors if refresh token is present. - (void)testDisconnect_refreshToken { [[[_authorization expect] andReturn:_authState] authState];