Use NSURLComponents to properly encode tokens

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<NSURLQueryItem *> *queryItems = [NSMutableArray array];
+  [queryItems addObject:[NSURLQueryItem queryItemWithName:kRevokeTokenParameter value:token]];
+  NSDictionary<NSString *, NSString *> *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) {

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];