Handle non-200 HTTP responses in SNTSyncStage performRequest (#1392)

* Handle non-200 HTTP responses in SNTSyncStage performRequest If we receive a non-200 HTTP response, we should return an error instead of parsing the response to an empty protobuf message. * Fix nil check --------- Co-authored-by: Matt W <436037+mlw@users.noreply.github.com>
google · Jul 10, 2024 · 47648d2 · 47648d2
1 parent 208b4a6
commit 47648d2
Showing 1 changed file with 27 additions and 9 deletions.
diff --git a/Source/santasyncservice/SNTSyncStage.mm b/Source/santasyncservice/SNTSyncStage.mm
@@ -159,9 +159,11 @@ - (void)addExtraRequestHeaders:(NSMutableURLRequest *)req {
                                  }];
 }
 
-- (NSData *)dataFromRequest:(NSURLRequest *)request timeout:(NSTimeInterval)timeout {
+- (NSData *)dataFromRequest:(NSURLRequest *)request
+                    timeout:(NSTimeInterval)timeout
+                      error:(NSError **)error {
   NSHTTPURLResponse *response;
-  NSError *error;
+  NSError *requestError;
   NSData *data;
 
   int maxAttempts = 5;
@@ -174,14 +176,14 @@ - (NSData *)dataFromRequest:(NSURLRequest *)request timeout:(NSTimeInterval)time
     }
 
     SLOGD(@"Performing request, attempt %d (of %d maximum)...", attempt, maxAttempts);
-    data = [self performRequest:request timeout:timeout response:&response error:&error];
+    data = [self performRequest:request timeout:timeout response:&response error:&requestError];
     if (response.statusCode == 200) break;
 
     // If the original request failed because of an auth error, attempt to get a new XSRF token and
     // try again. Unfortunately some servers cause NSURLSession to return 'client cert required' or
     // 'could not parse response' when a 403 occurs and SSL cert auth is enabled.
-    if ((response.statusCode == 403 || error.code == NSURLErrorClientCertificateRequired ||
-         error.code == NSURLErrorCannotParseResponse) &&
+    if ((response.statusCode == 403 || requestError.code == NSURLErrorClientCertificateRequired ||
+         requestError.code == NSURLErrorCannotParseResponse) &&
         [self fetchXSRFToken]) {
       NSMutableURLRequest *mutableRequest = [request mutableCopy];
       NSString *xsrfHeader = self.syncState.xsrfTokenHeader ?: kDefaultXSRFTokenHeader;
@@ -199,10 +201,15 @@ - (NSData *)dataFromRequest:(NSURLRequest *)request timeout:(NSTimeInterval)time
       code = response.statusCode;
       errStr = [NSHTTPURLResponse localizedStringForStatusCode:response.statusCode];
     } else {
-      code = (long)error.code;
-      errStr = error.localizedDescription;
+      code = (long)requestError.code;
+      errStr = requestError.localizedDescription;
     }
     LOGE(@"HTTP Response: %ld %@", code, errStr);
+    if (error != NULL) {
+      *error = [NSError errorWithDomain:@"com.google.santa.syncservice"
+                                   code:code
+                               userInfo:@{NSLocalizedDescriptionKey : errStr}];
+    }
     return nil;
   }
   return data;
@@ -211,8 +218,19 @@ - (NSData *)dataFromRequest:(NSURLRequest *)request timeout:(NSTimeInterval)time
 - (NSError *)performRequest:(NSURLRequest *)request
                 intoMessage:(google::protobuf::Message *)message
                     timeout:(NSTimeInterval)timeout {
-  NSData *data = [self dataFromRequest:request timeout:timeout];
-  if (data.length == 0) return nil;
+  NSError *error;
+  NSData *data = [self dataFromRequest:request timeout:timeout error:&error];
+  if (error) {
+    SLOGE(@"Error performing request: %@", error.localizedDescription);
+    return error;
+  }
+  if (data.length == 0) {
+    NSString *errStr = @"Response is empty";
+    SLOGE(@"%@", errStr);
+    return [NSError errorWithDomain:@"com.google.santa.syncservice"
+                               code:4
+                           userInfo:@{NSLocalizedDescriptionKey : errStr}];
+  }
 
   if ([[SNTConfigurator configurator] syncEnableProtoTransfer]) {
     if (!message->ParseFromString(std::string((const char *)data.bytes, data.length))) {