Create separate lists of characters that are allowed within a tenant id vs scopes in AzureCliCredential. (#5086)

* Create separate lists of characters that are allowed within a tenant id vs scopes in AzureCliCredential.

* Update test to catch the particular exception we expect to be thrown for
tenant id but not for scopes.

* Address PR feedback.

Author: ahsonkhan

sdk/identity/azure-identity/CHANGELOG.md

@@ -12,6 +12,8 @@
 
 ### Other Changes
 
+- Create separate lists of characters that are allowed within tenant ids and scopes in `AzureCliCredential`.
+
 ## 1.6.0-beta.3 (2023-10-12)
 
 ### Bugs Fixed

sdk/identity/azure-identity/inc/azure/identity/azure_cli_credential.hpp

@@ -74,7 +74,10 @@ namespace Azure { namespace Identity {
         DateTime::duration cliProcessTimeout,
         std::vector<std::string> additionallyAllowedTenants);
 
-    void ThrowIfNotSafeCmdLineInput(std::string const& input, std::string const& description) const;
+    void ThrowIfNotSafeCmdLineInput(
+        std::string const& input,
+        std::string const& allowedChars,
+        std::string const& description) const;
 
   public:
     /**

sdk/identity/azure-identity/src/azure_cli_credential.cpp

@@ -57,26 +57,20 @@ using Azure::Identity::_detail::TokenCredentialImpl;
 
 void AzureCliCredential::ThrowIfNotSafeCmdLineInput(
     std::string const& input,
+    std::string const& allowedChars,
     std::string const& description) const
 {
   for (auto const c : input)
   {
-    switch (c)
+    if (allowedChars.find(c) != std::string::npos)
     {
-      case ':':
-      case '/':
-      case '.':
-      case '-':
-      case '_':
-        break;
-
-      default:
-        if (!StringExtensions::IsAlphaNumeric(c))
-        {
-          throw AuthenticationException(
-              GetCredentialName() + ": Unsafe command line input found in " + description + ": "
-              + input);
-        }
+      continue;
+    }
+    if (!StringExtensions::IsAlphaNumeric(c))
+    {
+      throw AuthenticationException(
+          GetCredentialName() + ": Unsafe command line input found in " + description + ": "
+          + input);
     }
   }
 }
@@ -119,8 +113,10 @@ AzureCliCredential::AzureCliCredential(const Core::Credentials::TokenCredentialO
 std::string AzureCliCredential::GetAzCommand(std::string const& scopes, std::string const& tenantId)
     const
 {
-  ThrowIfNotSafeCmdLineInput(scopes, "Scopes");
-  ThrowIfNotSafeCmdLineInput(m_tenantId, "TenantID");
+  // The OAuth 2.0 RFC (https://datatracker.ietf.org/doc/html/rfc6749#section-3.3) allows space as
+  // well for a list of scopes, but that isn't currently required.
+  ThrowIfNotSafeCmdLineInput(scopes, ".-:/_", "Scopes");
+  ThrowIfNotSafeCmdLineInput(m_tenantId, ".-", "TenantID");
   std::string command = "az account get-access-token --output json --scope \"" + scopes + "\"";
 
   if (!tenantId.empty())

sdk/identity/azure-identity/test/ut/azure_cli_credential_test.cpp

@@ -344,19 +344,33 @@ TEST(AzureCliCredential, UnsafeChars)
   }
 }
 
-TEST(AzureCliCredential, SpaceNotAllowed)
+class ParameterizedTestForDisallowedChars : public ::testing::TestWithParam<std::string> {
+protected:
+  std::string value;
+};
+
+TEST_P(ParameterizedTestForDisallowedChars, DisallowedCharsForScopeAndTenantId)
 {
-  std::string const invalid = "space character";
+  std::string const InvalidValue = GetParam();
 
   {
     AzureCliCredentialOptions options;
     options.TenantId = "01234567-89AB-CDEF-0123-456789ABCDEF";
-    options.TenantId += invalid;
+    options.TenantId += InvalidValue;
     AzureCliCredential azCliCred(options);
 
     TokenRequestContext trc;
     trc.Scopes.push_back(std::string("https://storage.azure.com/.default"));
     EXPECT_THROW(static_cast<void>(azCliCred.GetToken(trc, {})), AuthenticationException);
+
+    try
+    {
+      auto const token = azCliCred.GetToken(trc, {});
+    }
+    catch (AuthenticationException const& e)
+    {
+      EXPECT_TRUE(std::string(e.what()).find("Unsafe") != std::string::npos) << e.what();
+    }
   }
 
   {
@@ -365,12 +379,134 @@ TEST(AzureCliCredential, SpaceNotAllowed)
     AzureCliCredential azCliCred(options);
 
     TokenRequestContext trc;
-    trc.Scopes.push_back(std::string("https://storage.azure.com/.default") + invalid);
+    trc.Scopes.push_back(std::string("https://storage.azure.com/.default") + InvalidValue);
+    EXPECT_THROW(static_cast<void>(azCliCred.GetToken(trc, {})), AuthenticationException);
+
+    try
+    {
+      auto const token = azCliCred.GetToken(trc, {});
+    }
+    catch (AuthenticationException const& e)
+    {
+      EXPECT_TRUE(std::string(e.what()).find("Unsafe") != std::string::npos) << e.what();
+    }
+  }
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    AzureCliCredential,
+    ParameterizedTestForDisallowedChars,
+    ::testing::Values(" ", "|", "`", "\"", "'", ";", "&"));
+
+class ParameterizedTestForCharDifferences : public ::testing::TestWithParam<std::string> {
+protected:
+  std::string value;
+};
+
+TEST_P(ParameterizedTestForCharDifferences, ValidCharsForScopeButNotTenantId)
+{
+  std::string const ValidScopeButNotTenantId = GetParam();
+
+  {
+    AzureCliCredentialOptions options;
+    options.TenantId = "01234567-89AB-CDEF-0123-456789ABCDEF";
+    options.TenantId += ValidScopeButNotTenantId;
+    AzureCliCredential azCliCred(options);
 
+    TokenRequestContext trc;
+    trc.Scopes.push_back(std::string("https://storage.azure.com/.default"));
     EXPECT_THROW(static_cast<void>(azCliCred.GetToken(trc, {})), AuthenticationException);
+
+    try
+    {
+      auto const token = azCliCred.GetToken(trc, {});
+    }
+    catch (AuthenticationException const& e)
+    {
+      EXPECT_TRUE(std::string(e.what()).find("Unsafe") != std::string::npos) << e.what();
+    }
+  }
+
+  {
+    AzureCliCredentialOptions options;
+    options.CliProcessTimeout = std::chrono::hours(24);
+    AzureCliCredential azCliCred(options);
+
+    TokenRequestContext trc;
+    trc.Scopes.push_back(
+        std::string("https://storage.azure.com/.default") + ValidScopeButNotTenantId);
+
+    // We expect the GetToken to fail, but not because of the unsafe chars.
+    try
+    {
+      auto const token = azCliCred.GetToken(trc, {});
+    }
+    catch (AuthenticationException const& e)
+    {
+      EXPECT_TRUE(std::string(e.what()).find("Unsafe") == std::string::npos) << e.what();
+    }
   }
 }
 
+INSTANTIATE_TEST_SUITE_P(
+    AzureCliCredential,
+    ParameterizedTestForCharDifferences,
+    ::testing::Values(":", "/", "_"));
+
+class ParameterizedTestForAllowedChars : public ::testing::TestWithParam<std::string> {
+protected:
+  std::string value;
+};
+
+TEST_P(ParameterizedTestForAllowedChars, ValidCharsForScopeAndTenantId)
+{
+  std::string const ValidChars = GetParam();
+
+  {
+    AzureCliCredentialOptions options;
+    options.TenantId = "01234567-89AB-CDEF-0123-456789ABCDEF";
+    options.TenantId += ValidChars;
+    AzureCliCredential azCliCred(options);
+
+    TokenRequestContext trc;
+    trc.Scopes.push_back(std::string("https://storage.azure.com/.default"));
+
+    // We expect the GetToken to fail, but not because of the unsafe chars.
+    try
+    {
+      auto const token = azCliCred.GetToken(trc, {});
+    }
+    catch (AuthenticationException const& e)
+    {
+      EXPECT_TRUE(std::string(e.what()).find("Unsafe") == std::string::npos) << e.what();
+    }
+  }
+
+  {
+    AzureCliCredentialOptions options;
+    options.CliProcessTimeout = std::chrono::hours(24);
+    AzureCliCredential azCliCred(options);
+
+    TokenRequestContext trc;
+    trc.Scopes.push_back(std::string("https://storage.azure.com/.default") + ValidChars);
+
+    // We expect the GetToken to fail, but not because of the unsafe chars.
+    try
+    {
+      auto const token = azCliCred.GetToken(trc, {});
+    }
+    catch (AuthenticationException const& e)
+    {
+      EXPECT_TRUE(std::string(e.what()).find("Unsafe") == std::string::npos) << e.what();
+    }
+  }
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    AzureCliCredential,
+    ParameterizedTestForAllowedChars,
+    ::testing::Values(".", "-", "A", "9"));
+
 TEST(AzureCliCredential, StrictIso8601TimeFormat)
 {
   constexpr auto Token = "{\"accessToken\":\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\","