#1028 validate region string format before use in URL construction

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryProvider.java

@@ -15,6 +15,7 @@
 import java.util.Map;
 import java.util.HashMap;
 import java.util.concurrent.*;
+import java.util.regex.Pattern;
 
 class AadInstanceDiscoveryProvider {
 
@@ -27,6 +28,10 @@ class AadInstanceDiscoveryProvider {
     private static final String REGION_NAME = "REGION_NAME";
     private static final int PORT_NOT_SET = -1;
 
+    //Azure region names are lowercase alphanumeric characters and hyphens, starting with a letter (westus, east-us-2, etc.).
+    //Regions are used to build authority hosts, so anything outside of that set could produce a malformed URL
+    private static final Pattern VALID_REGION = Pattern.compile("^[a-z][a-z0-9-]*$");
+
     // For information of the current api-version refer: https://docs.microsoft.com/en-us/azure/virtual-machines/windows/instance-metadata-service#versioning
     private static final String DEFAULT_API_VERSION = "2021-02-01";
     private static final String IMDS_ENDPOINT = "http://169.254.169.254/metadata/instance/compute?api-version=" + DEFAULT_API_VERSION;
@@ -98,8 +103,10 @@ static InstanceDiscoveryMetadataEntry getMetadataEntry(URL authorityUrl,
                         && detectedRegion != null) {
                     LOG.debug("Region autodetection found {}, this region will be used for future calls.", detectedRegion);
 
+                    //The regional host is built first so that a region which cannot be used is not stored on the
+                    //  application, where it would affect every later request made by that application
+                    host = getRegionalizedHost(authorityUrl.getHost(), detectedRegion);
                     ((AbstractClientApplicationBase) msalRequest.application()).azureRegion = detectedRegion;
-                    host = getRegionalizedHost(authorityUrl.getHost(), ((AbstractClientApplicationBase) msalRequest.application()).azureRegion());
                 }
 
                 cacheRegionInstanceMetadata(authorityUrl.getHost(), host);
@@ -186,13 +193,37 @@ static void cacheRegionInstanceMetadata(String originalHost, String regionalHost
         cache.putIfAbsent(regionalHost,  new InstanceDiscoveryMetadataEntry(regionalHost, originalHost, aliases));
     }
 
+    /**
+     * Checks a region against the Azure region naming convention: lowercase alphanumeric characters
+     * and hyphens, starting with a letter.
+     */
+    static boolean isValidRegion(String region) {
+        return region != null && VALID_REGION.matcher(region).matches();
+    }
+
+    /**
+     * Regions which do not follow the Azure region naming convention are rejected, as they would otherwise be
+     * used to build a malformed authority host and lead to failed or misdirected token requests.
+     */
+    private static MsalClientException invalidRegionException(String region) {
+        return new MsalClientException(String.format("Invalid region '%s': region must start with a lowercase " +
+                "letter and contain only lowercase alphanumeric characters and hyphens", region),
+                AuthenticationErrorCode.INVALID_REGION);
+    }
+
     private static String getRegionalizedHost(String host, String region) {
         String regionalizedHost;
 
         if (region == null){
             return host;
         }
 
+        //Last check before the region is used to build a host: regions from any source, such as one set by an
+        //  app developer, must follow the Azure region naming convention
+        if (!isValidRegion(region)) {
+            throw invalidRegionException(region);
+        }
+
         //Cached calls may already have the regionalized authority, so if region info is already in the host just return as-is
         if (host.contains(region)) {
             return host;
@@ -303,10 +334,19 @@ static String discoverRegion(MsalRequest msalRequest, ServiceBundle serviceBundl
 
         //Check if the REGION_NAME environment variable has a value for the region
         if (System.getenv(REGION_NAME) != null) {
-            LOG.info("Region found in environment variable: {}", System.getenv(REGION_NAME));
+            String region = System.getenv(REGION_NAME);
+            LOG.info("Region found in environment variable: {}", region);
+
+            //A region is used to build an authority host, so an unexpected value is rejected rather than
+            //  used to make requests against a malformed URL
+            if (!isValidRegion(region)) {
+                currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_FAILED_AUTODETECT.telemetryValue);
+                throw invalidRegionException(region);
+            }
+
             currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_ENV_VARIABLE.telemetryValue);
 
-            return System.getenv(REGION_NAME);
+            return region;
         }
 
         //Check the IMDS endpoint to retrieve current region (will only work if application is running in an Azure VM)
@@ -316,21 +356,23 @@ static String discoverRegion(MsalRequest msalRequest, ServiceBundle serviceBundl
         ExecutorService executor = Executors.newSingleThreadExecutor();
         Future<IHttpResponse> future = executor.submit(() -> executeRequest(IMDS_ENDPOINT, headers, msalRequest, serviceBundle));
 
+        String detectedRegion = null;
+
         try {
             LOG.info("Starting call to IMDS endpoint.");
             IHttpResponse httpResponse = future.get(IMDS_TIMEOUT, IMDS_TIMEOUT_UNIT);
             //If call to IMDS endpoint was successful, parse the region from the JSON response body
             if (httpResponse.statusCode() == HttpStatus.HTTP_OK) {
-                String region = parseRegionFromImdsResponse(httpResponse.body());
-                if (!StringHelper.isBlank(region)) {
-                    LOG.info("Region retrieved from IMDS endpoint: {}", region);
-                    currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_IMDS.telemetryValue);
+                detectedRegion = parseRegionFromImdsResponse(httpResponse.body());
+            }
 
-                    return region;
-                }
+            if (!StringHelper.isBlank(detectedRegion)) {
+                LOG.info("Region retrieved from IMDS endpoint: {}", detectedRegion);
+                currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_IMDS.telemetryValue);
+            } else {
+                LOG.warn("Call to local IMDS failed with status code: {}, or response was empty or missing a region", httpResponse.statusCode());
+                currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_FAILED_AUTODETECT.telemetryValue);
             }
-            LOG.warn("Call to local IMDS failed with status code: {}, or response was empty or missing a region", httpResponse.statusCode());
-            currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_FAILED_AUTODETECT.telemetryValue);
         } catch (Exception ex) {
             // handle other exceptions
             //IMDS call failed, cannot find region
@@ -344,7 +386,19 @@ static String discoverRegion(MsalRequest msalRequest, ServiceBundle serviceBundl
             executor.shutdownNow();
         }
 
-        return null;
+        if (StringHelper.isBlank(detectedRegion)) {
+            return null;
+        }
+
+        //A region is used to build an authority host, so an unexpected value from IMDS is rejected rather than
+        //  used to make requests against a malformed URL. Validation happens outside of the try/catch above so
+        //  that it is not mistaken for an IMDS communication failure and silently swallowed
+        if (!isValidRegion(detectedRegion)) {
+            currentRequest.regionSource(RegionTelemetry.REGION_SOURCE_FAILED_AUTODETECT.telemetryValue);
+            throw invalidRegionException(detectedRegion);
+        }
+
+        return detectedRegion;
     }
 
     /**

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AbstractClientApplicationBase.java

@@ -421,6 +421,11 @@ public T autoDetectRegion(boolean val) {
          * Regions must be valid Azure regions and their short names should be used, such as 'westus' for the West US Azure region,
          * 'centralus' for the Central US Azure region, etc.
          * <p>
+         * Because the region is used to build the authority host, it must follow the Azure region naming convention:
+         * lowercase alphanumeric characters and hyphens, starting with a letter. A region which does not will cause a
+         * {@link MsalClientException} with the error code {@link AuthenticationErrorCode#INVALID_REGION} when a token
+         * request first attempts to use it.
+         * <p>
          * Although you can set a specific region here and enable autodetection with {@link AbstractClientApplicationBase#autoDetectRegion} at the same time
          * the specific region set here will take priority over the autodetected region if there is a mismatch.
          * <p>

msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java

@@ -166,4 +166,13 @@ public class AuthenticationErrorCode {
      * This is returned by the instance discovery endpoint when the provided authority host is unknown.
      */
     public static final String INVALID_INSTANCE = "invalid_instance";
+
+    /**
+     * Indicates that a region does not follow the Azure region naming convention, which is lowercase alphanumeric
+     * characters and hyphens starting with a letter, such as 'westus' or 'east-us-2'. Regions are used to build
+     * authority hosts, so any other value could result in requests being made against a malformed URL. The region
+     * may have been set with the azureRegion API, or detected from the REGION_NAME environment variable or the
+     * IMDS endpoint. For more details, see https://aka.ms/msal4j-azure-regions
+     */
+    public static final String INVALID_REGION = "invalid_region";
 }

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AadInstanceDiscoveryTest.java

@@ -11,13 +11,15 @@
 import org.mockito.MockedStatic;
 import org.mockito.junit.jupiter.MockitoExtension;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.CALLS_REAL_METHODS;
 import static org.mockito.Mockito.mockStatic;
 
 import java.net.URI;
 import java.net.URL;
+import java.util.Collections;
 
 @ExtendWith(MockitoExtension.class)
 @TestInstance(TestInstance.Lifecycle.PER_CLASS)
@@ -137,6 +139,89 @@ void aadInstanceDiscoveryTest_AutoDetectRegion_NoRegionDetected() throws Excepti
         }
     }
 
+    @Test
+    void aadInstanceDiscoveryTest_RegionSetByDeveloper_invalidRegion_throws() throws Exception {
+
+        ConfidentialClientApplication app = ConfidentialClientApplication.builder(
+                "client_id", ClientCredentialFactory.createFromSecret("secret"))
+                .aadInstanceDiscoveryResponse(instanceDiscoveryValidResponse)
+                .azureRegion("east.us")
+                .build();
+
+        MsalRequest msalRequest = clientCredentialRequest(app);
+        URL authority = new URL(app.authority());
+
+        //A region containing a dot would add an unexpected subdomain to the authority host, so it must be rejected
+        //  before it is used to build a URL
+        MsalClientException ex = assertThrows(MsalClientException.class, () ->
+                AadInstanceDiscoveryProvider.getMetadataEntry(authority, false, msalRequest, app.serviceBundle()));
+
+        assertEquals(AuthenticationErrorCode.INVALID_REGION, ex.errorCode());
+    }
+
+    @Test
+    void aadInstanceDiscoveryTest_AutoDetectRegion_invalidRegionDetected_throws() throws Exception {
+
+        ConfidentialClientApplication app = ConfidentialClientApplication.builder(
+                "client_id", ClientCredentialFactory.createFromSecret("secret"))
+                .autoDetectRegion(true)
+                .build();
+
+        MsalRequest msalRequest = clientCredentialRequest(app);
+        URL authority = new URL(app.authority());
+
+        try (MockedStatic<AadInstanceDiscoveryProvider> mocked = mockStatic(AadInstanceDiscoveryProvider.class, CALLS_REAL_METHODS)) {
+
+            mocked.when(() -> AadInstanceDiscoveryProvider.discoverRegion(msalRequest,
+                    app.serviceBundle())).thenReturn("east.us");
+
+            MsalClientException ex = assertThrows(MsalClientException.class, () ->
+                    AadInstanceDiscoveryProvider.getMetadataEntry(authority, false, msalRequest, app.serviceBundle()));
+
+            assertEquals(AuthenticationErrorCode.INVALID_REGION, ex.errorCode());
+            //An unusable region must not be stored on the application, where it would affect every later request
+            assertNull(app.azureRegion());
+        }
+    }
+
+    @Test
+    void aadInstanceDiscoveryTest_RegionSetByDeveloper_validRegion_buildsRegionalHost() throws Exception {
+
+        ConfidentialClientApplication app = ConfidentialClientApplication.builder(
+                "client_id", ClientCredentialFactory.createFromSecret("secret"))
+                .authority("https://login.microsoftonline.com/my_tenant")
+                .azureRegion("eastus")
+                .build();
+
+        MsalRequest msalRequest = clientCredentialRequest(app);
+        URL authority = new URL(app.authority());
+        AadInstanceDiscoveryResponse expectedResponse = JsonHelper.convertJsonStringToJsonSerializableObject(
+                instanceDiscoveryValidResponse, AadInstanceDiscoveryResponse::fromJson);
+
+        try (MockedStatic<AadInstanceDiscoveryProvider> mocked = mockStatic(AadInstanceDiscoveryProvider.class, CALLS_REAL_METHODS)) {
+
+            mocked.when(() -> AadInstanceDiscoveryProvider.discoverRegion(msalRequest,
+                    app.serviceBundle())).thenReturn(null);
+            mocked.when(() -> AadInstanceDiscoveryProvider.sendInstanceDiscoveryRequest(authority,
+                    msalRequest, app.serviceBundle())).thenReturn(expectedResponse);
+
+            InstanceDiscoveryMetadataEntry entry = AadInstanceDiscoveryProvider.getMetadataEntry(
+                    authority, false, msalRequest, app.serviceBundle());
+
+            assertEquals("eastus.login.microsoft.com", entry.preferredNetwork());
+            assertEquals("login.microsoftonline.com", entry.preferredCache());
+        }
+    }
+
+    private MsalRequest clientCredentialRequest(ConfidentialClientApplication app) {
+        //Regions are only used by the client credential flow, see AadInstanceDiscoveryProvider.shouldUseRegionalEndpoint
+        ClientCredentialParameters parameters = ClientCredentialParameters.builder(
+                Collections.singleton("scopes")).build();
+
+        return new ClientCredentialRequest(parameters, app,
+                new RequestContext(app, PublicApi.ACQUIRE_TOKEN_FOR_CLIENT, parameters), null);
+    }
+
     void assertValidResponse(InstanceDiscoveryMetadataEntry entry) {
         assertEquals(entry.preferredNetwork(), "login.microsoftonline.com");
         assertEquals(entry.preferredCache(), "login.windows.net");

msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RegionDiscoveryTest.java

@@ -5,9 +5,14 @@
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInstance;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullAndEmptySource;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 @TestInstance(TestInstance.Lifecycle.PER_CLASS)
 class RegionDiscoveryTest {
@@ -54,4 +59,38 @@ void parseRegionFromImdsResponse_emptyBody_returnsNull() {
         assertNull(AadInstanceDiscoveryProvider.parseRegionFromImdsResponse(""));
         assertNull(AadInstanceDiscoveryProvider.parseRegionFromImdsResponse(null));
     }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"eastus", "westus2", "east-us-2", "centralus", "a", "a1", "a-1"})
+    void isValidRegion_validRegionNames_returnsTrue(String region) {
+        assertTrue(AadInstanceDiscoveryProvider.isValidRegion(region));
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+            "east.us",      //dot would add a subdomain to the authority host
+            "east/us",      //slash would add a path segment to the authority URL
+            "../evil",      //path traversal attempt
+            "EastUS",       //uppercase characters
+            "east us",      //whitespace
+            "1eastus",      //does not start with a letter
+            "-eastus",      //does not start with a letter
+            "east_us",      //underscore
+            "east$us"})     //other special characters
+    void isValidRegion_invalidRegionNames_returnsFalse(String region) {
+        assertFalse(AadInstanceDiscoveryProvider.isValidRegion(region));
+    }
+
+    @ParameterizedTest
+    @NullAndEmptySource
+    void isValidRegion_nullOrEmpty_returnsFalse(String region) {
+        assertFalse(AadInstanceDiscoveryProvider.isValidRegion(region));
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"West US", "West US 2"})
+    void isValidRegion_displayName_returnsFalse(String region) {
+        //Regions must be given as their short name ("westus"), not their display name ("West US")
+        assertFalse(AadInstanceDiscoveryProvider.isValidRegion(region));
+    }
 }