bridge: Separate proxy validation from proxy setting

And expose an explicit setInvalidProxy, both for testing and in case
apps want to do their own connection-poisoning.

This makes it easier to be consistent about "if you try to set a proxy
that turns out not to be valid, the ConnectionManager should end up in
the invalid state until explicitly cleared", whether the validation is
done on the Rust side of the bridge or the app language side.

Author: jrose-signal

RELEASE_NOTES.md

@@ -3,4 +3,9 @@ v0.65.5
 - Introduces an overload of `Net.setProxy()` that supports HTTP and SOCKS proxies in addition to the
    "transparent TLS proxies" already supported. Supported schemes: "socks5" (or just "socks"),
    "socks5h", "socks4", "socks4a", "https", "http", and "org.signal.tls".
+
+    - `Net.setInvalidProxy()` disables new connections until the proxy settings are updated.
+
+    - Desktop: `Net.setProxyFromUrl()` translates from URL syntax for specifying a proxy.
+
 - keytrans: Verify consistency proofs

java/client/src/main/java/org/signal/libsignal/net/Network.java

@@ -93,11 +93,21 @@ public void setProxy(String host, int port) throws IOException {
     this.connectionManager.setProxy(SIGNAL_TLS_PROXY_SCHEME, host, port, username, null);
   }
 
+  /**
+   * Refuses to make any new connections until a new proxy configuration is set or {@link
+   * #clearProxy} is called.
+   *
+   * <p>Existing connections will not be affected.
+   */
+  public void setInvalidProxy() {
+    this.connectionManager.setInvalidProxy();
+  }
+
   /**
    * Ensures that future connections will be made directly, not through a proxy.
    *
-   * <p>Clears any proxy configuration set via {@link #setProxy}. If none was set, calling this
-   * method is a no-op.
+   * <p>Clears any proxy configuration set via {@link #setProxy} or {@link #setInvalidProxy}. If
+   * none was set, calling this method is a no-op.
    *
    * <p>Existing connections and services will continue with the setting they were created with. (In
    * particular, changing this setting will not affect any existing {@link ChatService
@@ -236,20 +246,34 @@ private ConnectionManager(Environment env, String userAgent) {
     private void setProxy(
         String scheme, String host, Integer port, String username, String password)
         throws IOException {
-      filterExceptions(
-          IOException.class,
-          () ->
-              guardedRunChecked(
-                  h ->
-                      Native.ConnectionManager_set_proxy(
-                          h,
-                          scheme,
-                          host,
-                          // Integer.MIN_VALUE represents "no port provided"; we don't expect anyone
-                          // to pass that manually.
-                          port != null ? port : Integer.MIN_VALUE,
-                          username,
-                          password)));
+      long rawProxyConfig;
+      try {
+        rawProxyConfig =
+            filterExceptions(
+                IOException.class,
+                () ->
+                    Native.ConnectionProxyConfig_new(
+                        scheme,
+                        host,
+                        // Integer.MIN_VALUE represents "no port provided"; we don't expect anyone
+                        // to pass that manually.
+                        port != null ? port : Integer.MIN_VALUE,
+                        username,
+                        password));
+      } catch (IOException | RuntimeException | Error e) {
+        setInvalidProxy();
+        throw e;
+      }
+
+      try {
+        guardedRun(h -> Native.ConnectionManager_set_proxy(h, rawProxyConfig));
+      } finally {
+        Native.ConnectionProxyConfig_Destroy(rawProxyConfig);
+      }
+    }
+
+    private void setInvalidProxy() {
+      guardedRun(Native::ConnectionManager_set_invalid_proxy);
     }
 
     private void clearProxy() {

java/client/src/test/java/org/signal/libsignal/net/ChatServiceTest.java

@@ -323,6 +323,12 @@ public void testInvalidProxyRejected() throws Exception {
     check.accept(() -> net.setProxy("signalfoundation.org", -1));
 
     check.accept(() -> net.setProxy("socks+shoes", "signalfoundation.org", null, null, null));
+
+    check.accept(
+        () -> {
+          net.setInvalidProxy();
+          throw new IOException("to match all the other test cases");
+        });
   }
 
   private void injectServerRequest(ChatService chat, String requestBase64) {

java/shared/java/org/signal/libsignal/internal/Native.java

@@ -250,7 +250,11 @@ private Native() {}
   public static native long ConnectionManager_new(int environment, String userAgent);
   public static native void ConnectionManager_on_network_change(long connectionManager);
   public static native void ConnectionManager_set_censorship_circumvention_enabled(long connectionManager, boolean enabled);
-  public static native void ConnectionManager_set_proxy(long connectionManager, String scheme, String host, int port, String username, String password) throws Exception;
+  public static native void ConnectionManager_set_invalid_proxy(long connectionManager);
+  public static native void ConnectionManager_set_proxy(long connectionManager, long proxy);
+
+  public static native void ConnectionProxyConfig_Destroy(long handle);
+  public static native long ConnectionProxyConfig_new(String scheme, String host, int port, String username, String password) throws Exception;
 
   public static native void CreateCallLinkCredentialPresentation_CheckValidContents(byte[] presentationBytes) throws Exception;
   public static native void CreateCallLinkCredentialPresentation_Verify(byte[] presentationBytes, byte[] roomId, long now, byte[] serverParamsBytes, byte[] callLinkParamsBytes) throws Exception;

node/Native.d.ts

@@ -233,8 +233,10 @@ export function ConnectionManager_clear_proxy(connectionManager: Wrapper<Connect
 export function ConnectionManager_new(environment: number, userAgent: string): ConnectionManager;
 export function ConnectionManager_on_network_change(connectionManager: Wrapper<ConnectionManager>): void;
 export function ConnectionManager_set_censorship_circumvention_enabled(connectionManager: Wrapper<ConnectionManager>, enabled: boolean): void;
+export function ConnectionManager_set_invalid_proxy(connectionManager: Wrapper<ConnectionManager>): void;
 export function ConnectionManager_set_ipv6_enabled(connectionManager: Wrapper<ConnectionManager>, ipv6Enabled: boolean): void;
-export function ConnectionManager_set_proxy(connectionManager: Wrapper<ConnectionManager>, scheme: string, host: string, port: number, username: string | null, password: string | null): void;
+export function ConnectionManager_set_proxy(connectionManager: Wrapper<ConnectionManager>, proxy: Wrapper<ConnectionProxyConfig>): void;
+export function ConnectionProxyConfig_new(scheme: string, host: string, port: number, username: string | null, password: string | null): ConnectionProxyConfig;
 export function CreateCallLinkCredentialPresentation_CheckValidContents(presentationBytes: Buffer): void;
 export function CreateCallLinkCredentialPresentation_Verify(presentationBytes: Buffer, roomId: Buffer, now: Timestamp, serverParamsBytes: Buffer, callLinkParamsBytes: Buffer): void;
 export function CreateCallLinkCredentialRequestContext_CheckValidContents(contextBytes: Buffer): void;
@@ -617,6 +619,7 @@ interface CiphertextMessage { readonly __type: unique symbol; }
 interface ComparableBackup { readonly __type: unique symbol; }
 interface ComparableBackup { readonly __type: unique symbol; }
 interface ConnectionManager { readonly __type: unique symbol; }
+interface ConnectionProxyConfig { readonly __type: unique symbol; }
 interface DecryptionErrorMessage { readonly __type: unique symbol; }
 interface ExpiringProfileKeyCredential { readonly __type: unique symbol; }
 interface ExpiringProfileKeyCredentialResponse { readonly __type: unique symbol; }

node/ts/net.ts

@@ -907,15 +907,22 @@ export class Net {
       };
     }
     const { scheme, host, port, username, password } = hostOrOptions;
-    Native.ConnectionManager_set_proxy(
-      this._connectionManager,
-      scheme,
-      host,
-      // i32::MIN represents "no port provided"; we don't expect anyone to pass that manually.
-      port ?? -0x8000_0000,
-      username ?? null,
-      password ?? null
-    );
+    try {
+      const proxyConfig = newNativeHandle(
+        Native.ConnectionProxyConfig_new(
+          scheme,
+          host,
+          // i32::MIN represents "no port provided"; we don't expect anyone to pass that manually.
+          port ?? -0x8000_0000,
+          username ?? null,
+          password ?? null
+        )
+      );
+      Native.ConnectionManager_set_proxy(this._connectionManager, proxyConfig);
+    } catch (e) {
+      this.setInvalidProxy();
+      throw e;
+    }
   }
 
   /**
@@ -936,11 +943,7 @@ export class Net {
     } catch (e) {
       // Make sure we set an invalid proxy on error,
       // so no connection can be made until the problem is fixed.
-      try {
-        this.setProxy({ scheme: 'https', host: '' });
-      } catch (_) {
-        // Ignore any errors here, we want to rethrow the original error.
-      }
+      this.setInvalidProxy();
       throw e;
     }
 
@@ -985,11 +988,21 @@ export class Net {
     return { scheme, username, password, host, port };
   }
 
+  /**
+   * Refuses to make any new connections until a new proxy configuration is set or
+   * {@link #clearProxy} is called.
+   *
+   * Existing connections will not be affected.
+   */
+  setInvalidProxy(): void {
+    Native.ConnectionManager_set_invalid_proxy(this._connectionManager);
+  }
+
   /**
    * Ensures that future connections will be made directly, not through a proxy.
    *
-   * Clears any proxy configuration set via {@link #setProxy}. If none was set, calling this
-   * method is a no-op.
+   * Clears any proxy configuration set via {@link #setProxy} or {@link #setInvalidProxy}. If none
+   * was set, calling this method is a no-op.
    */
   clearProxy(): void {
     Native.ConnectionManager_clear_proxy(this._connectionManager);

node/ts/test/NetTest.ts

@@ -180,34 +180,24 @@ describe('chat service api', () => {
       userAgent: userAgent,
     });
 
-    function check(callback: () => void, expectedProxyMode: number = -1): void {
+    function check(callback: () => void): void {
       expect(
         TESTING_ConnectionManager_isUsingProxy(net._connectionManager)
       ).equals(0);
       expect(callback).throws(Error);
       expect(
         TESTING_ConnectionManager_isUsingProxy(net._connectionManager)
-      ).equals(expectedProxyMode);
+      ).equals(-1);
       net.clearProxy();
     }
 
     check(() => net.setProxy('signalfoundation.org', 0));
     check(() => net.setProxy('signalfoundation.org', 100_000));
     check(() => net.setProxy('signalfoundation.org', -1));
-
-    // Ideally these would poison the Net instance like all the other invalid options,
-    // but unfortunately it's checked at a layer that makes that awkward.
-    // Hopefully no one actually tries to set a fractional or very large port!
-    check(() => net.setProxy('signalfoundation.org', 0.1), 0);
-    check(
-      () => net.setProxy('signalfoundation.org', Number.MAX_SAFE_INTEGER),
-      0
-    );
-    check(() => net.setProxy('signalfoundation.org', Number.MAX_VALUE), 0);
-    check(
-      () => net.setProxy('signalfoundation.org', Number.POSITIVE_INFINITY),
-      0
-    );
+    check(() => net.setProxy('signalfoundation.org', 0.1));
+    check(() => net.setProxy('signalfoundation.org', Number.MAX_SAFE_INTEGER));
+    check(() => net.setProxy('signalfoundation.org', Number.MAX_VALUE));
+    check(() => net.setProxy('signalfoundation.org', Number.POSITIVE_INFINITY));
 
     check(() =>
       net.setProxy({ scheme: 'socks+shoes', host: 'signalfoundation.org' })
@@ -227,6 +217,11 @@ describe('chat service api', () => {
         'https://signalfoundation.org#fragment-for-some-reason'
       )
     );
+
+    check(() => {
+      net.setInvalidProxy();
+      throw new Error('to match the behavior of all the other calls');
+    });
   });
 
   it('parses proxy URLs the way we expect, if not always ideally', () => {

rust/bridge/shared/src/net.rs

@@ -10,6 +10,8 @@ use libsignal_bridge_macros::bridge_fn;
 pub use libsignal_bridge_types::net::{ConnectionManager, Environment, TokioAsyncContext};
 use libsignal_net::auth::Auth;
 use libsignal_net::chat::ConnectionInfo;
+use libsignal_net::infra::errors::LogSafeDisplay;
+use libsignal_net::infra::route::ConnectionProxyConfig;
 
 use crate::support::*;
 use crate::*;
@@ -21,43 +23,86 @@ mod tokio;
 
 bridge_handle_fns!(ConnectionInfo, clone = false, jni = false);
 
-bridge_handle_fns!(ConnectionManager, clone = false);
-
-#[bridge_fn]
-fn ConnectionManager_new(
-    environment: AsType<Environment, u8>,
-    user_agent: String,
-) -> ConnectionManager {
-    ConnectionManager::new(environment.into_inner(), user_agent.as_str())
-}
+bridge_handle_fns!(ConnectionProxyConfig);
 
 #[bridge_fn]
-fn ConnectionManager_set_proxy(
-    connection_manager: &ConnectionManager,
+fn ConnectionProxyConfig_new(
     scheme: String,
     host: String,
     port: i32,
     username: Option<String>,
     password: Option<String>,
-) -> Result<(), std::io::Error> {
+) -> Result<ConnectionProxyConfig, std::io::Error> {
     // We take port as an i32 because Java 'short' is signed and thus can't represent all port
     // numbers, and we want too-large port numbers to be handled the same way as 0. However, we
     // *also* want to have a representation that means "no port provided". We'll use something
     // unlikely for anyone to have typed manually, especially in decimal: `i32::MIN`. (We're not
     // using 0 as the placeholder because an explicitly-specified zero should be diagnosed as
-    // invalid.) Note that we also *don't* return early from this function if the port is invalid;
-    // that should poison the connection manager. See [`ConnectionManager::set_proxy`].
-    let port: Option<Result<NonZeroU16, i32>> = if port == i32::MIN {
+    // invalid.)
+    let port = if port == i32::MIN {
         None
     } else {
         Some(
             u16::try_from(port)
                 .ok()
                 .and_then(NonZeroU16::new)
-                .ok_or(port),
+                .ok_or_else(|| {
+                    std::io::Error::new(
+                        std::io::ErrorKind::InvalidInput,
+                        format!("invalid port '{port}'"),
+                    )
+                })?,
         )
     };
-    connection_manager.set_proxy(&scheme, &host, port, username, password)
+
+    let auth = match (username, password) {
+        (None, None) => None,
+        (None, Some(_)) => {
+            return Err(std::io::Error::new(
+                std::io::ErrorKind::InvalidInput,
+                "cannot have password without username",
+            ));
+        }
+        (Some(username), password) => Some((username, password.unwrap_or_default())),
+    };
+
+    ConnectionProxyConfig::from_parts(&scheme, &host, port, auth).map_err(|e| {
+        use libsignal_net::infra::route::ProxyFromPartsError;
+        static_assertions::assert_impl_all!(ProxyFromPartsError: LogSafeDisplay);
+        match e {
+            ProxyFromPartsError::UnsupportedScheme(_) => {
+                std::io::Error::new(std::io::ErrorKind::Unsupported, e.to_string())
+            }
+            ProxyFromPartsError::MissingHost
+            | ProxyFromPartsError::SchemeDoesNotSupportUsernames(_)
+            | ProxyFromPartsError::SchemeDoesNotSupportPasswords(_) => {
+                std::io::Error::new(std::io::ErrorKind::InvalidInput, e.to_string())
+            }
+        }
+    })
+}
+
+bridge_handle_fns!(ConnectionManager, clone = false);
+
+#[bridge_fn]
+fn ConnectionManager_new(
+    environment: AsType<Environment, u8>,
+    user_agent: String,
+) -> ConnectionManager {
+    ConnectionManager::new(environment.into_inner(), user_agent.as_str())
+}
+
+#[bridge_fn]
+fn ConnectionManager_set_proxy(
+    connection_manager: &ConnectionManager,
+    proxy: &ConnectionProxyConfig,
+) {
+    connection_manager.set_proxy(proxy.clone())
+}
+
+#[bridge_fn]
+fn ConnectionManager_set_invalid_proxy(connection_manager: &ConnectionManager) {
+    connection_manager.set_invalid_proxy()
 }
 
 #[bridge_fn]
@@ -93,16 +138,3 @@ fn CreateOTPFromBase64(username: String, secret: String) -> String {
     let secret = BASE64_STANDARD.decode(secret).expect("valid base64");
     Auth::otp(&username, &secret, std::time::SystemTime::now())
 }
-
-#[cfg(test)]
-mod test {
-    use test_case::test_case;
-
-    use super::*;
-
-    #[test_case(Environment::Staging; "staging")]
-    #[test_case(Environment::Prod; "prod")]
-    fn can_create_connection_manager(env: Environment) {
-        let _ = ConnectionManager::new(env, "test-user-agent");
-    }
-}