Disallow comma character in authMechanismProperties (#1412)

JAVA-5486
# Conflicts:
#	driver-sync/src/test/functional/com/mongodb/internal/connection/OidcAuthenticationProseTests.java

Author: katcharov

driver-core/src/main/com/mongodb/ConnectionString.java

@@ -38,7 +38,6 @@
 import java.net.URLDecoder;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -240,9 +239,7 @@
  * mechanism (the default).
  * </li>
  * <li>{@code authMechanismProperties=PROPERTY_NAME:PROPERTY_VALUE,PROPERTY_NAME2:PROPERTY_VALUE2}: This option allows authentication
- * mechanism properties to be set on the connection string. Property values must be percent-encoded individually, when
- * special characters are used, including {@code ,} (comma), {@code =}, {@code +}, {@code &}, and {@code %}. The
- * entire substring following the {@code =} should not itself be encoded.
+ * mechanism properties to be set on the connection string.
  * </li>
  * <li>{@code gssapiServiceName=string}: This option only applies to the GSSAPI mechanism and is used to alter the service name.
  *   Deprecated, please use {@code authMechanismProperties=SERVICE_NAME:string} instead.
@@ -908,7 +905,6 @@ private MongoCredential createCredentials(final Map<String, List<String>> option
             }
         }
 
-
         MongoCredential credential = null;
         if (mechanism != null) {
             credential = createMongoCredentialWithMechanism(mechanism, userName, password, authSource, gssapiServiceName);
@@ -926,9 +922,6 @@ private MongoCredential createCredentials(final Map<String, List<String>> option
                 }
                 String key = mechanismPropertyKeyValue[0].trim().toLowerCase();
                 String value = mechanismPropertyKeyValue[1].trim();
-                if (decodeValueOfKeyValuePair(credential.getMechanism())) {
-                    value = urldecode(value);
-                }
                 if (MECHANISM_KEYS_DISALLOWED_IN_CONNECTION_STRING.contains(key)) {
                     throw new IllegalArgumentException(format("The connection string contains disallowed mechanism properties. "
                             + "'%s' must be set on the credential programmatically.", key));
@@ -944,27 +937,6 @@ private MongoCredential createCredentials(final Map<String, List<String>> option
         return credential;
     }
 
-    private static boolean decodeWholeOptionValue(final boolean isOidc, final String key) {
-        // The "whole option value" is the entire string following = in an option,
-        // including separators when the value is a list or list of key-values.
-        // This is the original parsing behaviour, but implies that users can
-        // encode separators (much like they might with URL parameters). This
-        // behaviour implies that users cannot encode "key-value" values that
-        // contain a comma, because this will (after this "whole value decoding)
-        // be parsed as a key-value separator, rather than part of a value.
-        return !(isOidc && key.equals("authmechanismproperties"));
-    }
-
-    private static boolean decodeValueOfKeyValuePair(@Nullable final String mechanismName) {
-        // Only authMechanismProperties should be individually decoded, and only
-        // when the mechanism is OIDC. These will not have been decoded.
-        return AuthenticationMechanism.MONGODB_OIDC.getMechanismName().equals(mechanismName);
-    }
-
-    private static boolean isOidc(final List<String> options) {
-        return options.contains("authMechanism=" + AuthenticationMechanism.MONGODB_OIDC.getMechanismName());
-    }
-
     private MongoCredential createMongoCredentialWithMechanism(final AuthenticationMechanism mechanism, final String userName,
                                                                @Nullable final char[] password,
                                                                @Nullable final String authSource,
@@ -1049,9 +1021,7 @@ private Map<String, List<String>> parseOptions(final String optionsPart) {
             return optionsMap;
         }
 
-        List<String> options = Arrays.asList(optionsPart.split("&|;"));
-        boolean isOidc = isOidc(options);
-        for (final String part : options) {
+        for (final String part : optionsPart.split("&|;")) {
             if (part.isEmpty()) {
                 continue;
             }
@@ -1063,10 +1033,7 @@ private Map<String, List<String>> parseOptions(final String optionsPart) {
                 if (valueList == null) {
                     valueList = new ArrayList<>(1);
                 }
-                if (decodeWholeOptionValue(isOidc, key)) {
-                    value = urldecode(value);
-                }
-                valueList.add(value);
+                valueList.add(urldecode(value));
                 optionsMap.put(key, valueList);
             } else {
                 throw new IllegalArgumentException(format("The connection string contains an invalid option '%s'. "

driver-core/src/main/com/mongodb/MongoCredential.java

@@ -267,6 +267,9 @@ public final class MongoCredential {
      * Mechanism property key for specifying he URI of the target resource (sometimes called the audience),
      * used in some OIDC environments.
      *
+     * <p>A TOKEN_RESOURCE with a comma character must be given as a `MongoClient` configuration and not as
+     * part of the connection string. The TOKEN_RESOURCE value can contain a colon character.
+     *
      * @see MongoCredential#ENVIRONMENT_KEY
      * @see #createOidcCredential(String)
      * @since 5.1

driver-core/src/test/resources/auth/legacy/connection-string.json

@@ -565,7 +565,7 @@
     },
     {
       "description": "should handle a complicated url-encoded TOKEN_RESOURCE (MONGODB-OIDC)",
-      "uri": "mongodb://user@localhost/?authMechanism=MONGODB-OIDC&authMechanismProperties=ENVIRONMENT:azure,TOKEN_RESOURCE:abc%2Cd%25ef%3Ag%26hi",
+      "uri": "mongodb://user@localhost/?authMechanism=MONGODB-OIDC&authMechanismProperties=ENVIRONMENT:azure,TOKEN_RESOURCE:abcd%25ef%3Ag%26hi",
       "valid": true,
       "credential": {
         "username": "user",
@@ -574,7 +574,7 @@
         "mechanism": "MONGODB-OIDC",
         "mechanism_properties": {
           "ENVIRONMENT": "azure",
-          "TOKEN_RESOURCE": "abc,d%ef:g&hi"
+          "TOKEN_RESOURCE": "abcd%ef:g&hi"
         }
       }
     },

driver-core/src/test/resources/connection-string/valid-options.json

@@ -20,6 +20,25 @@
       "options": {
         "authmechanism": "MONGODB-CR"
       }
+    },
+    {
+      "description": "Colon in a key value pair",
+      "uri": "mongodb://example.com/?authMechanism=MONGODB-OIDC&authMechanismProperties=TOKEN_RESOURCE:mongodb://test-cluster",
+      "valid": true,
+      "warning": false,
+      "hosts": [
+        {
+          "type": "hostname",
+          "host": "example.com",
+          "port": null
+        }
+      ],
+      "auth": null,
+      "options": {
+        "authmechanismProperties": {
+          "TOKEN_RESOURCE": "mongodb://test-cluster"
+        }
+      }
     }
   ]
 }

driver-core/src/test/unit/com/mongodb/ConnectionStringUnitTest.java

@@ -39,20 +39,6 @@ void defaults() {
         assertAll(() -> assertNull(connectionStringDefault.getServerMonitoringMode()));
     }
 
-    @Test
-    public void mustDecodeOidcIndividually() {
-        String string = "abc,d!@#$%^&*;ef:ghi";
-        // encoded tags will fail parsing with an "invalid read preference tag"
-        // error if decoding is skipped.
-        String encodedTags = encode("dc:ny,rack:1");
-        ConnectionString cs = new ConnectionString(
-                "mongodb://localhost/?readPreference=primaryPreferred&readPreferenceTags=" + encodedTags
-                + "&authMechanism=MONGODB-OIDC&authMechanismProperties="
-                + "ENVIRONMENT:azure,TOKEN_RESOURCE:" + encode(string));
-        MongoCredential credential = Assertions.assertNotNull(cs.getCredential());
-        assertEquals(string, credential.getMechanismProperty("TOKEN_RESOURCE", null));
-    }
-
     @Test
     public void mustDecodeNonOidcAsWhole()  {
         // this string allows us to check if there is no double decoding