fix: prevent action crashes and reduce SOCKS5 save churn

WebDriverAgentLib/Routing/FBRouteRequest.m

@@ -74,10 +74,15 @@ static id FBRedactedRequestLogValue(id value)
   return value;
 }
 
-static NSDictionary *FBRedactedRequestArguments(NSURL *URL, NSDictionary *arguments)
+static id FBRedactedRequestArguments(NSURL *URL, id arguments)
 {
-  NSMutableDictionary *redacted = [(NSDictionary *)FBRedactedRequestLogValue(arguments) mutableCopy];
-  id uri = arguments[@"uri"];
+  id redactedValue = FBRedactedRequestLogValue(arguments);
+  if (![redactedValue isKindOfClass:NSDictionary.class]
+      || ![arguments isKindOfClass:NSDictionary.class]) {
+    return redactedValue;
+  }
+  NSMutableDictionary *redacted = [(NSDictionary *)redactedValue mutableCopy];
+  id uri = ((NSDictionary *)arguments)[@"uri"];
   if ([URL.path hasSuffix:@"/mobilerun/socks5/connect"] && [uri isKindOfClass:NSString.class]) {
     redacted[@"uri"] = FBRedactedProxyURIString((NSString *)uri);
   }

WebDriverAgentLib/Utilities/FBSocks5TunnelManager.m

@@ -10,6 +10,7 @@
 
 #include <errno.h>
 #import <stdatomic.h>
+#import <NetworkExtension/NetworkExtension.h>
 
 #import "FBSocks5TunnelProtocol.h"
 #import "FBSocks5URI.h"
@@ -26,6 +27,44 @@ static BOOL FBSocks5Fail(NSError **error, FBSocks5TunnelManagerError code, NSStr
   return NO;
 }
 
+static BOOL FBSocks5ObjectsEqual(id left, id right)
+{
+  return left == right || [left isEqual:right];
+}
+
+BOOL FBSocks5TunnelManagerConfigurationNeedsSave(NETunnelProviderManager *manager,
+                                                 NETunnelProviderProtocol *desiredProtocol,
+                                                 NSString *desiredDescription)
+{
+  NETunnelProviderProtocol *currentProtocol = (NETunnelProviderProtocol *)manager.protocolConfiguration;
+  if (![currentProtocol isKindOfClass:NETunnelProviderProtocol.class]) {
+    return YES;
+  }
+  return !manager.enabled
+    || !FBSocks5ObjectsEqual(manager.localizedDescription, desiredDescription)
+    || !FBSocks5ObjectsEqual(currentProtocol.providerBundleIdentifier,
+                             desiredProtocol.providerBundleIdentifier)
+    || !FBSocks5ObjectsEqual(currentProtocol.serverAddress, desiredProtocol.serverAddress)
+    || !FBSocks5ObjectsEqual(currentProtocol.providerConfiguration,
+                             desiredProtocol.providerConfiguration)
+    || currentProtocol.disconnectOnSleep != desiredProtocol.disconnectOnSleep;
+}
+
+NSString *FBSocks5TunnelManagerStartRejectedMessage(FBSocks5URI *uri)
+{
+  if (!uri.remoteDNS) {
+    return [NSString stringWithFormat:
+      @"The SOCKS5 tunnel stopped right after starting. Plain socks5:// DNS requires UDP "
+       "ASSOCIATE support from the proxy at %@:%lu; use socks5h:// for TCP-only proxies. "
+       "The proxy may also be unreachable, invalid, or rejecting the credentials",
+      uri.host, (unsigned long)uri.port];
+  }
+  return [NSString stringWithFormat:
+    @"The SOCKS5 tunnel stopped right after starting. The proxy at %@:%lu is unreachable, "
+     "is not a SOCKS5 proxy, or rejected the credentials",
+    uri.host, (unsigned long)uri.port];
+}
+
 // The tunnel appex is only embedded by the WebDriverAgentRunnerTunnel schemes (the default
 // runner schemes build without it so the hev submodule and paid-team signing stay optional);
 // its presence in the host app is what decides whether SOCKS5 support exists in this build.
@@ -632,84 +671,90 @@ - (BOOL)lockedConnectWithURI:(FBSocks5URI *)uri
   protocol.serverAddress = uri.host;
   protocol.providerConfiguration = [uri providerConfigurationWithControlAddress:controlAddress];
   protocol.disconnectOnSleep = NO;
-  manager.protocolConfiguration = protocol;
-  manager.localizedDescription = FBSocks5TunnelDescription;
-  manager.enabled = YES;
-
-  __block BOOL consentTapped = NO;
-  NSUInteger staleRetries = 0;
-  while (YES) {
-    if (deadline.timeIntervalSinceNow <= 0) {
-      return FBSocks5Fail(error, FBSocks5TunnelManagerErrorTimeout,
-                          @"Timed out before saving the VPN configuration");
-    }
-    __block volatile atomic_bool saveDone = false;
-    __block NSError *saveError = nil;
-    dispatch_semaphore_t saveSignal = dispatch_semaphore_create(0);
-    self.lifecycle.pendingSaveSignal = saveSignal;
-    [manager saveToPreferencesWithCompletionHandler:^(NSError *err) {
-      saveError = err;
-      atomic_store_explicit(&saveDone, true, memory_order_release);
-      dispatch_semaphore_signal(saveSignal);
-    }];
-    // Keep re-attempting for as long as the alert is still up: a dispatched tap can be shed by the
-    // system, so 'we dispatched one' is not evidence that it landed. tapConsentButtonWithLabels:
-    // paces the re-attempts itself and answers NO once the alert is gone.
-    // No MAX(..., 1.0) floor here: granting an already-exhausted request another second is
-    // exactly the overshoot the caller's timeout is supposed to prevent.
-    [[[[FBRunLoopSpinner new] timeout:deadline.timeIntervalSinceNow] interval:0.3] spinUntilTrue:^BOOL{
-      if (atomic_load_explicit(&saveDone, memory_order_acquire)) {
-        return YES;
+  BOOL needsConfigurationSave = manager.connection.status != NEVPNStatusDisconnected
+    || FBSocks5TunnelManagerConfigurationNeedsSave(manager, protocol, FBSocks5TunnelDescription);
+  if (needsConfigurationSave) {
+    manager.protocolConfiguration = protocol;
+    manager.localizedDescription = FBSocks5TunnelDescription;
+    manager.enabled = YES;
+
+    __block BOOL consentTapped = NO;
+    NSUInteger staleRetries = 0;
+    while (YES) {
+      if (deadline.timeIntervalSinceNow <= 0) {
+        return FBSocks5Fail(error, FBSocks5TunnelManagerErrorTimeout,
+                            @"Timed out before saving the VPN configuration");
       }
-      if ([self tapConsentButtonWithLabels:labels deadline:deadline]) {
-        consentTapped = YES;
-      }
-      return NO;
-    }];
-    if (!atomic_load_explicit(&saveDone, memory_order_acquire)) {
-      return FBSocks5Fail(error, FBSocks5TunnelManagerErrorTimeout,
-                          [NSString stringWithFormat:
-                           @"Timed out saving the VPN configuration. The consent alert was %@; "
-                           "pass 'consentButtonLabels' if the device language is not English, and note that "
-                           "devices with a passcode cannot confirm the VPN consent automatically",
-                           consentTapped ? @"confirmed" : @"not confirmed"]);
-    }
-    self.lifecycle.pendingSaveSignal = nil;
-    if (nil == saveError) {
-      break;
-    }
-    FBSocks5TunnelManagerSaveDisposition disposition =
-      FBSocks5TunnelManagerSaveDispositionForError(saveError);
-    if (FBSocks5TunnelManagerSaveDispositionRetryStale == disposition && 0 == staleRetries) {
-      staleRetries++;
-      [FBLogger log:@"socks5/connect: VPN configuration became stale; reloading and retrying the save once"];
-      if (![self reloadManager:manager
-                      deadline:deadline
-                       context:@"reloading the stale VPN configuration"
-                         error:error]) {
+      __block volatile atomic_bool saveDone = false;
+      __block NSError *saveError = nil;
+      dispatch_semaphore_t saveSignal = dispatch_semaphore_create(0);
+      self.lifecycle.pendingSaveSignal = saveSignal;
+      [manager saveToPreferencesWithCompletionHandler:^(NSError *err) {
+        saveError = err;
+        atomic_store_explicit(&saveDone, true, memory_order_release);
+        dispatch_semaphore_signal(saveSignal);
+      }];
+      // Keep re-attempting for as long as the alert is still up: a dispatched tap can be shed by the
+      // system, so 'we dispatched one' is not evidence that it landed. tapConsentButtonWithLabels:
+      // paces the re-attempts itself and answers NO once the alert is gone.
+      // No MAX(..., 1.0) floor here: granting an already-exhausted request another second is
+      // exactly the overshoot the caller's timeout is supposed to prevent.
+      [[[[FBRunLoopSpinner new] timeout:deadline.timeIntervalSinceNow] interval:0.3] spinUntilTrue:^BOOL{
+        if (atomic_load_explicit(&saveDone, memory_order_acquire)) {
+          return YES;
+        }
+        if ([self tapConsentButtonWithLabels:labels deadline:deadline]) {
+          consentTapped = YES;
+        }
         return NO;
+      }];
+      if (!atomic_load_explicit(&saveDone, memory_order_acquire)) {
+        return FBSocks5Fail(error, FBSocks5TunnelManagerErrorTimeout,
+                            [NSString stringWithFormat:
+                             @"Timed out saving the VPN configuration. The consent alert was %@; "
+                             "pass 'consentButtonLabels' if the device language is not English, and note that "
+                             "devices with a passcode cannot confirm the VPN consent automatically",
+                             consentTapped ? @"confirmed" : @"not confirmed"]);
       }
-      manager.protocolConfiguration = protocol;
-      manager.localizedDescription = FBSocks5TunnelDescription;
-      manager.enabled = YES;
-      continue;
+      self.lifecycle.pendingSaveSignal = nil;
+      if (nil == saveError) {
+        break;
+      }
+      FBSocks5TunnelManagerSaveDisposition disposition =
+        FBSocks5TunnelManagerSaveDispositionForError(saveError);
+      if (FBSocks5TunnelManagerSaveDispositionRetryStale == disposition && 0 == staleRetries) {
+        staleRetries++;
+        [FBLogger log:@"socks5/connect: VPN configuration became stale; reloading and retrying the save once"];
+        if (![self reloadManager:manager
+                        deadline:deadline
+                         context:@"reloading the stale VPN configuration"
+                           error:error]) {
+          return NO;
+        }
+        manager.protocolConfiguration = protocol;
+        manager.localizedDescription = FBSocks5TunnelDescription;
+        manager.enabled = YES;
+        continue;
+      }
+      FBSocks5TunnelManagerError code = FBSocks5TunnelManagerSaveDispositionNotAuthorized == disposition
+        ? FBSocks5TunnelManagerErrorNotAuthorized
+        : FBSocks5TunnelManagerErrorInternal;
+      NSString *prefix = FBSocks5TunnelManagerSaveDispositionNotAuthorized == disposition
+        ? @"The VPN configuration was not authorized"
+        : @"Cannot save the VPN configuration";
+      return FBSocks5Fail(error, code,
+                          [NSString stringWithFormat:@"%@: %@", prefix, saveError.localizedDescription]);
     }
-    FBSocks5TunnelManagerError code = FBSocks5TunnelManagerSaveDispositionNotAuthorized == disposition
-      ? FBSocks5TunnelManagerErrorNotAuthorized
-      : FBSocks5TunnelManagerErrorInternal;
-    NSString *prefix = FBSocks5TunnelManagerSaveDispositionNotAuthorized == disposition
-      ? @"The VPN configuration was not authorized"
-      : @"Cannot save the VPN configuration";
-    return FBSocks5Fail(error, code,
-                        [NSString stringWithFormat:@"%@: %@", prefix, saveError.localizedDescription]);
-  }
-
-  // A freshly saved configuration must be re-loaded before the tunnel can be started.
-  if (![self reloadManager:manager
-                  deadline:deadline
-                   context:@"reloading the saved VPN configuration"
-                     error:error]) {
-    return NO;
+
+    // A freshly saved configuration must be re-loaded before the tunnel can be started.
+    if (![self reloadManager:manager
+                    deadline:deadline
+                     context:@"reloading the saved VPN configuration"
+                       error:error]) {
+      return NO;
+    }
+  } else {
+    [FBLogger log:@"socks5/connect: reusing the unchanged VPN configuration"];
   }
 
   if (deadline.timeIntervalSinceNow <= 0) {
@@ -752,10 +797,7 @@ - (BOOL)lockedConnectWithURI:(FBSocks5URI *)uri
     [manager.connection stopVPNTunnel];
     return FBSocks5Fail(error, FBSocks5TunnelManagerErrorTimeout,
                         startRejected
-                        ? [NSString stringWithFormat:
-                           @"The SOCKS5 tunnel stopped right after starting. The proxy at %@:%lu "
-                           "is unreachable, is not a SOCKS5 proxy, or rejected the credentials",
-                           uri.host, (unsigned long)uri.port]
+                        ? FBSocks5TunnelManagerStartRejectedMessage(uri)
                         : [NSString stringWithFormat:
                            @"The SOCKS5 tunnel did not connect within %.0fs (status %ld). "
                            "Check that the proxy at %@:%lu is reachable from the device",

WebDriverAgentTests/UnitTests/FBRouteTests.m

@@ -186,6 +186,26 @@ - (void)testRequestDescriptionRedactsCredentialsFromMalformedProxyURIs
   }
 }
 
+- (void)testRequestDescriptionAcceptsArrayArguments
+{
+  NSArray *arguments = @[
+    @{
+      @"type": @"pointerDown",
+      @"uri": @"https://alice:secret@example.com/path",
+    },
+  ];
+  FBRouteRequest *request = [FBRouteRequest
+    routeRequestWithURL:[NSURL URLWithString:@"/mobilerun/actions"]
+    parameters:@{}
+    arguments:(NSDictionary *)(id)arguments];
+
+  NSString *description = request.description;
+  XCTAssertFalse([description containsString:@"alice"]);
+  XCTAssertFalse([description containsString:@"secret"]);
+  XCTAssertTrue([description containsString:@"example.com/path"]);
+  XCTAssertTrue([description containsString:@"pointerDown"]);
+}
+
 - (void)testSocks5ConnectTimeoutIsFiniteAndBounded
 {
   NSTimeInterval timeout = 0;

WebDriverAgentTests/UnitTests/FBSocks5ConfigTests.m

@@ -30,6 +30,10 @@ typedef NS_ENUM(NSInteger, FBSocks5TunnelManagerSaveDisposition) {
 
 extern FBSocks5TunnelManagerSaveDisposition FBSocks5TunnelManagerSaveDispositionForError(NSError *error);
 extern NSDictionary<NSString *, id> *_Nullable FBSocks5TunnelManagerDisconnectedStatsIfExtensionUnavailable(NSBundle *bundle);
+extern BOOL FBSocks5TunnelManagerConfigurationNeedsSave(NETunnelProviderManager *manager,
+                                                        NETunnelProviderProtocol *desiredProtocol,
+                                                        NSString *desiredDescription);
+extern NSString *FBSocks5TunnelManagerStartRejectedMessage(FBSocks5URI *uri);
 
 @interface FBSocks5ConfigTests : XCTestCase
 @property (nonatomic, nullable, copy) NSString *tempBundleRoot;
@@ -404,4 +408,63 @@ - (void)testOnlyPermissionFailuresAreClassifiedAsNotAuthorized
                  FBSocks5TunnelManagerSaveDispositionInternal);
 }
 
+- (void)testMatchingEnabledVPNConfigurationDoesNotNeedAnotherSave
+{
+  NETunnelProviderProtocol *protocol = [[NETunnelProviderProtocol alloc] init];
+  protocol.providerBundleIdentifier = @"com.example.runner.tunnel";
+  protocol.serverAddress = @"proxy.example.com";
+  protocol.providerConfiguration = @{
+    FBSocks5KeyHost: @"proxy.example.com",
+    FBSocks5KeyPort: @1080,
+    FBSocks5KeyRemoteDNS: @YES,
+  };
+  protocol.disconnectOnSleep = NO;
+
+  NETunnelProviderManager *manager = [[NETunnelProviderManager alloc] init];
+  manager.protocolConfiguration = protocol;
+  manager.localizedDescription = @"mobilerun SOCKS5";
+  manager.enabled = YES;
+
+  NETunnelProviderProtocol *desiredProtocol = [protocol copy];
+  XCTAssertFalse(FBSocks5TunnelManagerConfigurationNeedsSave(manager,
+                                                             desiredProtocol,
+                                                             @"mobilerun SOCKS5"));
+}
+
+- (void)testChangedVPNConfigurationStillNeedsSave
+{
+  NETunnelProviderProtocol *protocol = [[NETunnelProviderProtocol alloc] init];
+  protocol.providerBundleIdentifier = @"com.example.runner.tunnel";
+  protocol.serverAddress = @"proxy.example.com";
+  protocol.providerConfiguration = @{FBSocks5KeyHost: @"proxy.example.com"};
+  protocol.disconnectOnSleep = NO;
+
+  NETunnelProviderManager *manager = [[NETunnelProviderManager alloc] init];
+  manager.protocolConfiguration = protocol;
+  manager.localizedDescription = @"mobilerun SOCKS5";
+  manager.enabled = YES;
+
+  NETunnelProviderProtocol *desiredProtocol = [protocol copy];
+  desiredProtocol.providerConfiguration = @{FBSocks5KeyHost: @"other.example.com"};
+  XCTAssertTrue(FBSocks5TunnelManagerConfigurationNeedsSave(manager,
+                                                            desiredProtocol,
+                                                            @"mobilerun SOCKS5"));
+  manager.enabled = NO;
+  desiredProtocol.providerConfiguration = protocol.providerConfiguration;
+  XCTAssertTrue(FBSocks5TunnelManagerConfigurationNeedsSave(manager,
+                                                            desiredProtocol,
+                                                            @"mobilerun SOCKS5"));
+}
+
+- (void)testLocalDNSStartupFailureNamesUDPAssociateRequirement
+{
+  FBSocks5URI *localDNS = [FBSocks5URI parse:@"socks5://proxy.example.com:1080" error:nil];
+  FBSocks5URI *remoteDNS = [FBSocks5URI parse:@"socks5h://proxy.example.com:1080" error:nil];
+
+  NSString *localMessage = FBSocks5TunnelManagerStartRejectedMessage(localDNS);
+  XCTAssertTrue([localMessage containsString:@"UDP ASSOCIATE"]);
+  XCTAssertTrue([localMessage containsString:@"socks5h://"]);
+  XCTAssertFalse([FBSocks5TunnelManagerStartRejectedMessage(remoteDNS) containsString:@"UDP ASSOCIATE"]);
+}
+
 @end

docs/socks5-tunnel.md

@@ -22,7 +22,7 @@ Network Extension capability.
 
 | Endpoint | Method | Description |
 |---|---|---|
-| `/mobilerun/socks5/connect` | POST | Installs/updates the VPN configuration (auto-accepting the system consent alert via UI automation) and starts the tunnel. Replaces an already running tunnel. Returns once the tunnel reports connected. |
+| `/mobilerun/socks5/connect` | POST | Installs/updates the VPN configuration (auto-accepting the system consent alert via UI automation) and starts the tunnel. Reuses an unchanged installed configuration without saving it again. Replaces an already running tunnel. Returns once the tunnel reports connected. |
 | `/mobilerun/socks5/disconnect` | POST | Stops the running tunnel. The VPN profile stays installed. Succeeds when no tunnel is running. Send `{}` as the body (WDA rejects body-less POSTs with HTTP 400). |
 | `/mobilerun/socks5/stats` | GET | Connection state plus traffic counters queried from the extension. Never fails; counters fall back to zero when the extension cannot be reached. |