From 2a6b3efae466dd17888d14738cc840a86a37b169 Mon Sep 17 00:00:00 2001 From: Nilesh Choudhary Date: Thu, 29 Oct 2026 15:08:48 +0000 Subject: [PATCH 1/3] fix: scope OIDC issuer fallback eligibility Restrict regional issuer patterns to built-in Microsoft authorities and prevent their CIAM/B2C fallback. Preserve native parsing behavior, reject before endpoint use, and evict rejected discovery metadata for recovery. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- RELEASES.md | 16 ++ msal/application.py | 14 ++ msal/authority.py | 40 ++-- msal/throttled_http_client.py | 12 +- tests/test_authority.py | 391 +++++++++++++++++++++++++++++++--- 5 files changed, 425 insertions(+), 48 deletions(-) diff --git a/RELEASES.md b/RELEASES.md index 3e65b976..fd9e9310 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -2,6 +2,22 @@ We have adopted the semantic versioning flow that is industry standard for OSS projects. It gives the maximum amount of control on what risk you take with what versions. If you know how semantic versioning works with node.js, java, and ruby none of this will be new. +## Unreleased behavior changes + +For explicit `oidc_authority`, custom authorities no longer accept regional +prefixes on Microsoft issuer hosts or on their own host. Exact built-in +Microsoft authority hosts (case-insensitive) still accept any single nonempty +regional label before a built-in Microsoft issuer host; no region allowlist +is introduced. Microsoft authorities can no longer use the B2C/CIAM issuer +fallback. Exact Microsoft issuer hosts, same-host matching, and existing +B2C/CIAM mappings for non-Microsoft authorities remain supported. + +Rejected issuers raise the existing `ValueError` before advertised endpoints +are consumed, and their discovery responses are removed from MSAL's HTTP cache +so later valid metadata can succeed. Ordinary `authority` behavior and existing +URL parsing and scheme handling are unchanged. This is an issuer-policy +behavior change, not a complete credential-theft remediation. + ## Semantic Versioning and API stability promises Microsoft Authentication libraries are independent open source libraries that are used by partners both internal and external to Microsoft. As with the rest of Microsoft, we have moved to a rapid iteration model where bugs are fixed daily and new versions are produced as required. To communicate these frequent changes to external partners and customers, we use semantic versioning for all our public Microsoft Authentication SDK libraries. This follows the practices of other open source libraries on the internet. This allows us to support our downstream partners which will lock on certain versions for stability purposes, as well as providing for the distribution over NuGet, CocoaPods, and Maven. diff --git a/msal/application.py b/msal/application.py index 1aa14c28..8628588c 100644 --- a/msal/application.py +++ b/msal/application.py @@ -675,6 +675,20 @@ def get_client_assertion(): MSAL will append ".well-known/openid-configuration" to the authority and retrieve the OIDC metadata from there, to figure out the endpoints. + Issuer validation raises ``ValueError`` before using the advertised + endpoints when the issuer does not match an accepted pattern. + Exact known Microsoft issuer hosts remain accepted for custom + authorities. Regional issuer hosts are accepted only when the + configured authority host is an exact built-in Microsoft host + (case-insensitive). Any single nonempty regional label is still + accepted for those authorities; there is no region allowlist. + Custom authorities no longer accept regional prefixes on Microsoft + hosts or on their own host. Same-host matching and existing + B2C/CIAM issuer mappings for non-Microsoft authorities are preserved, + but Microsoft authorities cannot use the B2C/CIAM fallback. + Rejected discovery metadata is removed from MSAL's HTTP cache so a + later valid response can succeed. + Note: Broker will NOT be used for OIDC authority. """ self.client_id = client_id diff --git a/msal/authority.py b/msal/authority.py index ee16868f..1d8d78f7 100644 --- a/msal/authority.py +++ b/msal/authority.py @@ -5,6 +5,8 @@ from urlparse import urlparse import logging +from .throttled_http_client import ThrottledHttpClient + logger = logging.getLogger(__name__) # Endpoints were copied from here # https://docs.microsoft.com/en-us/azure/active-directory/develop/authentication-national-cloud#azure-ad-authentication-endpoints @@ -113,13 +115,11 @@ def __init__( ) + " Also please double check your tenant name or GUID is correct." raise ValueError(error_message) self._issuer = openid_config.get('issuer') - self.authorization_endpoint = openid_config['authorization_endpoint'] - self.token_endpoint = openid_config['token_endpoint'] - self.device_authorization_endpoint = openid_config.get('device_authorization_endpoint') - _, _, self.tenant = canonicalize(self.token_endpoint) # Usually a GUID # Validate the issuer if using OIDC authority if self._oidc_authority_url and not self.has_valid_issuer(): + if isinstance(self._http_client, ThrottledHttpClient): + self._http_client._forget_get(tenant_discovery_endpoint) raise ValueError(( "The issuer '{iss}' does not match the authority '{auth}' or a known pattern. " "When using the 'oidc_authority' parameter in ClientApplication, the authority " @@ -128,6 +128,11 @@ def __init__( "'authority' parameter should be used instead of 'oidc_authority'. " "" ).format(iss=self._issuer, auth=oidc_authority_url)) + self.authorization_endpoint = openid_config['authorization_endpoint'] + self.token_endpoint = openid_config['token_endpoint'] + self.device_authorization_endpoint = openid_config.get('device_authorization_endpoint') + _, _, self.tenant = canonicalize(self.token_endpoint) # Usually a GUID + def _initialize_oidc_authority(self, oidc_authority_url): authority, self.instance, tenant = canonicalize(oidc_authority_url) self.is_adfs = tenant.lower() == 'adfs' # As a convention @@ -208,8 +213,11 @@ def has_valid_issuer(self): - It exactly matches the authority URL (with/without trailing slash) - It has the same scheme and host as the authority (path can be different) - The issuer host is a well-known Microsoft authority host - - The issuer host is a regional variant of a well-known host (e.g., westus2.login.microsoft.com) - - For CIAM, hosts that end with well-known B2C hosts (e.g., tenant.b2clogin.com) are accepted as valid issuers + - For a well-known Microsoft authority only, the issuer host has + exactly one nonempty label before a well-known Microsoft host. + The label is not checked against a list of regions. + - For other authorities, issuer hosts that end with well-known + B2C/CIAM hosts (e.g., tenant.b2clogin.com) are accepted. """ if not self._issuer or not self._oidc_authority_url: return False @@ -224,25 +232,20 @@ def has_valid_issuer(self): if not issuer_host: return False + authority_host = authority_parsed.hostname.lower() if authority_parsed.hostname else "" + is_microsoft_authority = authority_host in WELL_KNOWN_AUTHORITY_HOSTS # Case 2: Issuer is from a trusted Microsoft host - O(1) lookup if issuer_host in WELL_KNOWN_AUTHORITY_HOSTS: return True - # Case 3: Regional variant check - O(1) lookup + # Case 3: Only built-in Microsoft authorities accept regional issuers. # e.g., westus2.login.microsoft.com -> extract "login.microsoft.com" dot_index = issuer_host.find(".") - if dot_index > 0: + if is_microsoft_authority and dot_index > 0: potential_base = issuer_host[dot_index + 1:] - if "." not in issuer_host[:dot_index]: - # 3a: Base host is a trusted Microsoft host - if potential_base in WELL_KNOWN_AUTHORITY_HOSTS: - return True - # 3b: Issuer has a region prefix on the authority host - # e.g. issuer=us.someweb.com, authority=someweb.com - authority_host = authority_parsed.hostname.lower() if authority_parsed.hostname else "" - if potential_base == authority_host: - return True + if potential_base in WELL_KNOWN_AUTHORITY_HOSTS: + return True # Case 4: Same scheme and host (path can differ) if (authority_parsed.scheme == issuer_parsed.scheme and @@ -252,7 +255,8 @@ def has_valid_issuer(self): # Case 5: Check if issuer host is a subdomain of a well-known B2C host # e.g., tenant.b2clogin.com matches .b2clogin.com # but fakeb2clogin.com does not - if any(issuer_host.endswith("." + h) for h in WELL_KNOWN_B2C_HOSTS): + if not is_microsoft_authority and any( + issuer_host.endswith("." + h) for h in WELL_KNOWN_B2C_HOSTS): return True return False diff --git a/msal/throttled_http_client.py b/msal/throttled_http_client.py index 8cabe261..44508181 100644 --- a/msal/throttled_http_client.py +++ b/msal/throttled_http_client.py @@ -171,10 +171,14 @@ def __init__(self, *args, default_throttle_time=None, **kwargs): self.get = IndividualCache( # Typically those discovery GETs mapping=self._expiring_mapping, - key_maker=lambda func, args, kwargs: "GET {} hash={} 2xx".format( - args[0], # It is the url, sometimes containing inline params - self._hash(kwargs.get("params", "")), - ), + key_maker=lambda func, args, kwargs: self._get_cache_key( + args[0], kwargs.get("params", "")), expires_in=lambda result=None, **ignored: 3600*24 if 200 <= result.status_code < 300 else 0, )(self.get) + + def _get_cache_key(self, url, params=""): + return "GET {} hash={} 2xx".format(url, self._hash(params)) + + def _forget_get(self, url): + self._expiring_mapping.pop(self._get_cache_key(url), None) diff --git a/tests/test_authority.py b/tests/test_authority.py index 2f9d1605..2d1dc370 100644 --- a/tests/test_authority.py +++ b/tests/test_authority.py @@ -1,3 +1,4 @@ +import json import os try: from unittest.mock import patch @@ -5,10 +6,34 @@ from mock import patch import msal +from requests.exceptions import Timeout from msal.authority import * from msal.authority import _CIAM_DOMAIN_SUFFIX +from msal.throttled_http_client import ThrottledHttpClient from tests import unittest -from tests.http_client import MinimalHttpClient +from tests.http_client import MinimalHttpClient, MinimalResponse, RecordingHttpClient + + +_TENANT = "11111111-2222-3333-4444-555555555555" +_ISSUER_POLICY_CASES = ( + ("S01", "https://custom.example.com/{0}/v2.0", + "https://login.microsoftonline.com/{0}/v2.0", True), + ("S02", "https://contoso.ciamlogin.com/{0}/v2.0", + "https://login.microsoftonline.com/{0}/v2.0", True), + ("S03", "https://contoso.ciamlogin.com/{0}/v2.0", + "https://contoso.ciamlogin.com/{0}/v2.0", True), + ("S04", "https://custom.example.com/{0}", + "https://eastus.login.microsoftonline.com/{0}", False), + ("S05", "https://custom.example.com/{0}", + "https://evil.login.microsoftonline.com/{0}", False), + ("S06", "https://login.microsoftonline.com/{0}", + "https://evil.login.microsoftonline.com/{0}", True), + ("S07", "https://login.microsoftonline.com/attacker.onmicrosoft.com", + "https://attacker.onmicrosoft.com.ciamlogin.com/attacker.onmicrosoft.com/v2.0", + False), + ("S08", "https://custom.example.com/{0}", + "https://login.microsoftonline.evil.com/{0}", False), +) @unittest.skipIf(os.getenv("TRAVIS_TAG"), "Skip network io during tagged release") @@ -420,6 +445,181 @@ def _test_turning_off_instance_discovery_should_skip_authority_validation_and_in instance_metadata.assert_not_called() +class OidcAuthorityIssuerPolicyTestCase(unittest.TestCase): + authorization_endpoint = "https://advertised.example.com/tenant/authorize" + token_endpoint = "https://advertised.example.com/tenant/token" + + def _metadata(self, issuer): + return { + "issuer": issuer, + "authorization_endpoint": self.authorization_endpoint, + "token_endpoint": self.token_endpoint, + } + + def _http_client(self, authority, metadata): + client = RecordingHttpClient() + discovery_url = authority + "/.well-known/openid-configuration" + client.add_get_route( + lambda call: call["url"] == discovery_url, + lambda call: MinimalResponse(status_code=200, + text=json.dumps(metadata))) + client.add_post_route( + lambda call: call["url"] == self.token_endpoint, + lambda call: MinimalResponse(status_code=200, text=json.dumps({ + "access_token": "test-token", "token_type": "Bearer", + "expires_in": 3600}))) + return client + + def _application(self, authority, client, http_cache, token_cache): + return msal.ConfidentialClientApplication( + "id", client_credential="test-secret", oidc_authority=authority, + http_client=client, http_cache=http_cache, token_cache=token_cache, + validate_authority=False, instance_discovery=False) + + def test_shared_cases_through_public_oidc_construction_and_acquisition(self): + for case, authority, issuer, expected in _ISSUER_POLICY_CASES: + with self.subTest(case=case): + authority, issuer = authority.format(_TENANT), issuer.format(_TENANT) + client = self._http_client(authority, self._metadata(issuer)) + http_cache, token_cache = {}, msal.TokenCache() + if expected: + app = self._application(authority, client, http_cache, token_cache) + self.assertTrue(app.initiate_auth_code_flow( + ["scope"])["auth_uri"].startswith(self.authorization_endpoint + "?")) + self.assertEqual("test-token", + app.acquire_token_for_client(["scope"])["access_token"]) + self.assertEqual([self.token_endpoint], + [call["url"] for call in client.post_calls]) + self.assertTrue(list(token_cache.search(msal.TokenCache.CredentialType.ACCESS_TOKEN))) + self._application(authority, client, http_cache, token_cache) + else: + with patch.object(msal.ClientApplication, "_build_client") as build_client: + with self.assertRaisesRegex(ValueError, "The issuer"): + self._application(authority, client, http_cache, token_cache) + build_client.assert_not_called() + self.assertFalse(client.post_calls) + self.assertFalse(list(token_cache.search(msal.TokenCache.CredentialType.ACCESS_TOKEN))) + self.assertFalse([key for key in http_cache if key != "_index_"]) + self.assertEqual( + [authority + "/.well-known/openid-configuration"], + [call["url"] for call in client.get_calls]) + + def test_rejected_discovery_is_not_cached_and_valid_response_can_recover(self): + authority = "https://custom.example.com/" + _TENANT + metadata = self._metadata("https://eastus.login.microsoftonline.com/" + _TENANT) + client = self._http_client(authority, metadata) + http_cache, token_cache = {}, msal.TokenCache() + for attempt in range(2): + with self.subTest(attempt=attempt): + with self.assertRaisesRegex(ValueError, "The issuer"): + self._application(authority, client, http_cache, token_cache) + self.assertEqual(attempt + 1, len(client.get_calls)) + self.assertFalse(client.post_calls) + self.assertFalse(list(token_cache.search(msal.TokenCache.CredentialType.ACCESS_TOKEN))) + self.assertFalse([key for key in http_cache if key != "_index_"]) + + metadata["issuer"] = "https://login.microsoftonline.com/" + _TENANT + app = self._application(authority, client, http_cache, token_cache) + self.assertEqual(3, len(client.get_calls)) + self.assertTrue(app.initiate_auth_code_flow( + ["scope"])["auth_uri"].startswith(self.authorization_endpoint + "?")) + self.assertEqual("test-token", + app.acquire_token_for_client(["scope"])["access_token"]) + self._application(authority, client, http_cache, token_cache) + self.assertEqual(3, len(client.get_calls), "Validated discovery should stay cached") + self.assertEqual(1, len(client.post_calls)) + + def test_preexisting_bad_http_cache_entry_is_revalidated_and_evicted(self): + authority = "https://custom.example.com/" + _TENANT + discovery_url = authority + "/.well-known/openid-configuration" + metadata = self._metadata("https://evil.login.microsoftonline.com/" + _TENANT) + client = self._http_client(authority, metadata) + http_cache, token_cache = {}, msal.TokenCache() + cached_client = ThrottledHttpClient(client, http_cache=http_cache) + cached_client.get(discovery_url) + unrelated_url = "https://other.example.com/discovery" + client.add_get_route( + lambda call: call["url"] == unrelated_url, + lambda call: MinimalResponse(status_code=200, text="unrelated")) + cached_client.get(unrelated_url) + metadata["issuer"] = "https://login.microsoftonline.com/" + _TENANT + + with self.assertRaisesRegex(ValueError, "The issuer"): + self._application(authority, client, http_cache, token_cache) + self.assertEqual(2, len(client.get_calls), "The old cached issuer must be checked") + self.assertFalse(client.post_calls) + self.assertEqual("unrelated", cached_client.get(unrelated_url).text) + self.assertEqual(2, len(client.get_calls), "Other cache entries must be preserved") + + app = self._application(authority, client, http_cache, token_cache) + self.assertEqual(3, len(client.get_calls)) + self.assertEqual("test-token", + app.acquire_token_for_client(["scope"])["access_token"]) + + def test_runtime_alias_metadata_does_not_make_custom_authority_microsoft(self): + authority = "https://custom.example.com/" + _TENANT + client = self._http_client(authority, + self._metadata("https://evil.login.microsoftonline.com/" + _TENANT)) + with patch.object(msal.ClientApplication, "_get_instance_metadata", + return_value=[{"aliases": ["custom.example.com", WORLD_WIDE], + "preferred_network": "custom.example.com", + "preferred_cache": "custom.example.com"}]) as instance_metadata: + with self.assertRaisesRegex(ValueError, "The issuer"): + self._application(authority, client, {}, msal.TokenCache()) + instance_metadata.assert_not_called() + self.assertFalse(client.post_calls) + + def test_explicit_oidc_transport_errors_and_interruptions_propagate(self): + authority = "https://custom.example.com/" + _TENANT + discovery_url = authority + "/.well-known/openid-configuration" + for error in (OSError("network unavailable"), Timeout("timeout"), + KeyboardInterrupt("interrupted")): + with self.subTest(error=type(error).__name__): + client = self._http_client(authority, self._metadata(authority)) + http_cache, token_cache = {}, msal.TokenCache() + with patch.object(client, "get", side_effect=error) as get: + with self.assertRaises(type(error)) as context: + self._application(authority, client, http_cache, token_cache) + self.assertIs(error, context.exception) + get.assert_called_once_with(discovery_url) + self.assertFalse(client.post_calls) + self.assertFalse([key for key in http_cache if key != "_index_"]) + self._application(authority, client, http_cache, token_cache) + self.assertEqual([discovery_url], + [call["url"] for call in client.get_calls]) + + def test_discovery_errors_preserve_native_error_contract(self): + authority = "https://custom.example.com/" + _TENANT + for body, message in ( + ("not-json", "Unable to get OIDC authority configuration"), + ("{}", "The issuer"), + (json.dumps(self._metadata(None)), "The issuer"), + (json.dumps(self._metadata("")), "The issuer")): + with self.subTest(body=body): + client = RecordingHttpClient() + client.add_get_route( + lambda call: True, + lambda call: MinimalResponse(status_code=200, text=body)) + with self.assertRaisesRegex(ValueError, message): + self._application(authority, client, {}, msal.TokenCache()) + self.assertEqual([authority + "/.well-known/openid-configuration"], + [call["url"] for call in client.get_calls]) + self.assertFalse(client.post_calls) + + def test_ordinary_authority_does_not_apply_explicit_oidc_issuer_policy(self): + authority = "https://login.microsoftonline.com/" + _TENANT + client = self._http_client(authority + "/v2.0", + self._metadata("https://tenant.ciamlogin.com/" + _TENANT)) + app = msal.ConfidentialClientApplication( + "id", client_credential="test-secret", authority=authority, + http_client=client) + self.assertEqual("test-token", + app.acquire_token_for_client(["scope"])["access_token"]) + self.assertEqual( + [authority + "/v2.0/.well-known/openid-configuration"], + [call["url"] for call in client.get_calls]) + + class TestAuthorityIssuerValidation(unittest.TestCase): """Test cases for authority.has_valid_issuer method """ @@ -438,6 +638,160 @@ def _create_authority_with_issuer(self, oidc_authority_url, issuer, tenant_disco oidc_authority_url=oidc_authority_url ) return authority + + def _assert_issuer_policy(self, authority_url, issuer, expected, discovery): + if expected: + authority = self._create_authority_with_issuer( + authority_url, issuer, discovery) + self.assertTrue(authority.has_valid_issuer()) + else: + with self.assertRaisesRegex(ValueError, "The issuer"): + self._create_authority_with_issuer( + authority_url, issuer, discovery) + + @patch("msal.authority.tenant_discovery") + def test_shared_issuer_policy_cases(self, discovery): + for case, authority, issuer, expected in _ISSUER_POLICY_CASES: + with self.subTest(case=case): + self._assert_issuer_policy( + authority.format(_TENANT), issuer.format(_TENANT), + expected, discovery) + + @patch("msal.authority.tenant_discovery") + def test_known_hosts_and_aliases_are_case_insensitive(self, discovery): + for host in WELL_KNOWN_AUTHORITY_HOSTS: + for authority_host in (host, host.upper()): + for issuer_host in (WORLD_WIDE.upper(), "EVIL." + host.upper()): + with self.subTest(authority=authority_host, issuer=issuer_host): + self._assert_issuer_policy( + "https://{}/tenant".format(authority_host), + "https://{}/other/v2.0".format(issuer_host), + True, discovery) + + @patch("msal.authority.tenant_discovery") + def test_known_authorities_reject_both_ciam_tenant_spellings(self, discovery): + for authority_host in WELL_KNOWN_AUTHORITY_HOSTS: + for tenant in ("attacker.onmicrosoft.com", _TENANT): + for issuer_host in ( + "attacker.onmicrosoft.com.ciamlogin.com", + "attacker.ciamlogin.com"): + with self.subTest(authority=authority_host, + tenant=tenant, issuer=issuer_host): + self._assert_issuer_policy( + "https://{}/{}".format(authority_host.upper(), tenant), + "https://{}/{}/v2.0".format(issuer_host, tenant), + False, discovery) + + @patch("msal.authority.tenant_discovery") + def test_ppe_hosts_are_not_in_native_microsoft_catalog(self, discovery): + for host in ("login.windows-ppe.net", "login.microsoftonline-p.com"): + self.assertNotIn(host, WELL_KNOWN_AUTHORITY_HOSTS) + for issuer, expected in ( + ("https://{}/other".format(host), True), + ("https://eastus.{}/tenant".format(host), False), + ("https://eastus.login.microsoftonline.com/tenant", False), + ("https://tenant.ciamlogin.com/tenant", True)): + with self.subTest(authority=host, issuer=issuer): + self._assert_issuer_policy( + "https://{}/tenant".format(host), issuer, + expected, discovery) + + @patch("msal.authority.tenant_discovery") + def test_native_issuer_parsing_boundaries(self, discovery): + for authority, issuer, expected in ( + ("https://custom.example.com/tenant", + "https://custom.example.com/tenant/", True), + ("https://custom.example.com/tenant/", + "https://custom.example.com/tenant", True), + ("https://custom.example.com/tenant", + "https://custom.example.com/other", True), + ("https://custom.example.com/tenant", + "https://CUSTOM.EXAMPLE.COM/other", False), + ("https://custom.example.com:443/tenant", + "https://custom.example.com:443/other", True), + ("https://custom.example.com/tenant", + "https://custom.example.com:443/other", False), + ("https://custom.example.com:8443/tenant", + "https://custom.example.com:443/other", False), + ("https://custom.example.com/tenant", + "http://custom.example.com/other", False), + ("https://custom.example.com/tenant", "/relative/issuer", False), + ("https://custom.example.com/tenant", "https:///tenant", False), + ("https://custom.example.com/tenant", + "https://login.microsoftonline.com:8443/other", True), + ("https://login.microsoftonline.com:8443/tenant", + "https://eastus.login.microsoftonline.com:443/other", True)): + with self.subTest(authority=authority, issuer=issuer): + self._assert_issuer_policy(authority, issuer, expected, discovery) + + @patch("msal.authority.tenant_discovery") + def test_malformed_issuer_preserves_parser_exceptions(self, discovery): + for issuer, error in (("https://[invalid/tenant", ValueError), + (123, AttributeError)): + with self.subTest(issuer=issuer): + with self.assertRaises(error): + self._create_authority_with_issuer( + "https://custom.example.com/tenant", issuer, discovery) + + @patch("msal.authority.tenant_discovery") + def test_regional_boundaries(self, discovery): + for authority_host in (WORLD_WIDE, "custom.example.com", + "eastus." + WORLD_WIDE, WORLD_WIDE + ".evil.com"): + for issuer_host in ( + "eastus." + WORLD_WIDE, "evil." + WORLD_WIDE, + "east.us." + WORLD_WIDE, "." + WORLD_WIDE, + "eastus." + WORLD_WIDE + ".evil.com", + "eastus.custom.example.com"): + authority = "https://{}/tenant".format(authority_host) + issuer = "https://{}/other".format(issuer_host) + expected = ( + authority_host == issuer_host or + authority_host == WORLD_WIDE and + issuer_host in ("eastus." + WORLD_WIDE, "evil." + WORLD_WIDE)) + with self.subTest(authority=authority, issuer=issuer): + self._assert_issuer_policy(authority, issuer, expected, discovery) + + @patch("msal.authority.tenant_discovery") + def test_ciam_and_b2c_fallback_only_for_non_microsoft_authorities(self, discovery): + for authority_host in (WORLD_WIDE, "LOGIN.WINDOWS.NET", + "custom.example.com", "contoso.ciamlogin.com"): + for suffix in WELL_KNOWN_B2C_HOSTS: + for issuer_host in ("tenant." + suffix, "tenant.nested." + suffix, + suffix, "fake" + suffix, "tenant." + suffix + ".evil.com"): + expected = ( + authority_host.lower() not in WELL_KNOWN_AUTHORITY_HOSTS and + issuer_host.endswith("." + suffix)) + with self.subTest(authority=authority_host, issuer=issuer_host): + self._assert_issuer_policy( + "https://{}/tenant".format(authority_host), + "https://{}/different/v2.0".format(issuer_host), + expected, discovery) + + @patch("msal.authority.tenant_discovery") + def test_existing_scheme_handling_is_preserved_with_new_host_restrictions(self, discovery): + for authority_host in (WORLD_WIDE, "custom.example.com"): + for issuer_host in (WORLD_WIDE, "eastus." + WORLD_WIDE, + "tenant.ciamlogin.com", "tenant.b2clogin.com", + "eastus.custom.example.com"): + expected = ( + issuer_host == WORLD_WIDE or + authority_host == WORLD_WIDE and issuer_host == "eastus." + WORLD_WIDE or + authority_host != WORLD_WIDE and issuer_host in ( + "tenant.ciamlogin.com", "tenant.b2clogin.com")) + for scheme in ("https", "http", "ftp"): + with self.subTest(authority=authority_host, + issuer=issuer_host, scheme=scheme): + self._assert_issuer_policy( + "https://{}/tenant".format(authority_host), + "{}://{}/tenant".format(scheme, issuer_host), + expected, discovery) + + @patch("msal.authority.tenant_discovery") + def test_invalid_issuer_is_rejected_before_endpoint_consumption(self, discovery): + discovery.return_value = {"issuer": "https://evil.example.com/tenant"} + with self.assertRaisesRegex(ValueError, "The issuer"): + Authority(None, self.http_client, + oidc_authority_url="https://custom.example.com/tenant") @patch("msal.authority.tenant_discovery") def test_exact_match_issuer(self, tenant_discovery_mock): @@ -464,7 +818,7 @@ def test_no_issuer(self, tenant_discovery_mock): def test_same_scheme_and_host_different_path(self, tenant_discovery_mock): """Test when issuer has same scheme and host but different path""" authority_url = "https://example.com/tenant" - issuer = f"https://{WORLD_WIDE}/tenant" + issuer = "https://example.com/different/path" authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) self.assertTrue(authority.has_valid_issuer(), "Issuer should be valid when it has the same scheme and host") @@ -539,36 +893,28 @@ def test_regional_issuer_westus2_login_microsoft(self, tenant_discovery_mock): """Test regional variant: westus2.login.microsoft.com""" authority_url = "https://custom-authority.com/tenant" issuer = "https://westus2.login.microsoftonline.com/tenant" - authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) - self.assertTrue(authority.has_valid_issuer(), - "Regional issuer westus2.login.microsoftonline.com should be valid") + self._assert_issuer_policy(authority_url, issuer, False, tenant_discovery_mock) @patch("msal.authority.tenant_discovery") def test_regional_issuer_eastus_login_microsoftonline(self, tenant_discovery_mock): """Test regional variant: eastus.login.microsoftonline.com""" authority_url = "https://custom-authority.com/tenant" issuer = "https://eastus.login.microsoftonline.com/tenant" - authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) - self.assertTrue(authority.has_valid_issuer(), - "Regional issuer eastus.login.microsoftonline.com should be valid") + self._assert_issuer_policy(authority_url, issuer, False, tenant_discovery_mock) @patch("msal.authority.tenant_discovery") def test_regional_issuer_for_china_cloud(self, tenant_discovery_mock): """Test regional variant for China cloud: region.login.chinacloudapi.cn""" authority_url = "https://custom-authority.com/tenant" issuer = "https://chinanorth.login.chinacloudapi.cn/tenant" - authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) - self.assertTrue(authority.has_valid_issuer(), - "Regional issuer for China cloud should be valid") + self._assert_issuer_policy(authority_url, issuer, False, tenant_discovery_mock) @patch("msal.authority.tenant_discovery") def test_regional_issuer_for_us_government(self, tenant_discovery_mock): """Test regional variant for US Government: region.login.microsoftonline.us""" authority_url = "https://custom-authority.com/tenant" issuer = "https://usgovvirginia.login.microsoftonline.us/tenant" - authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) - self.assertTrue(authority.has_valid_issuer(), - "Regional issuer for US Government should be valid") + self._assert_issuer_policy(authority_url, issuer, False, tenant_discovery_mock) @patch("msal.authority.tenant_discovery") def test_invalid_regional_pattern_with_dots_in_region(self, tenant_discovery_mock): @@ -617,30 +963,26 @@ def test_issuer_with_trailing_slash_match(self, tenant_discovery_mock): @patch("msal.authority.tenant_discovery") def test_issuer_case_sensitivity_host(self, tenant_discovery_mock): """Test that host comparison is case-insensitive for regional check""" - authority_url = "https://custom-authority.com/tenant" + authority_url = "https://LOGIN.MICROSOFTONLINE.COM/tenant" issuer = "https://WESTUS2.LOGIN.MICROSOFTONLINE.COM/tenant" # Uppercase authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) self.assertTrue(authority.has_valid_issuer(), "Host comparison should be case-insensitive") - # Case 3b: Regional prefix on authority host tests + # Regional prefixes on custom authority hosts are no longer accepted. @patch("msal.authority.tenant_discovery") def test_regional_issuer_matching_authority_host(self, tenant_discovery_mock): """Test issuer with region prefix on the authority host: us.someweb.com -> someweb.com""" authority_url = "https://someweb.com/tenant" issuer = "https://us.someweb.com/tenant" - authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) - self.assertTrue(authority.has_valid_issuer(), - "Issuer with region prefix on authority host should be valid") + self._assert_issuer_policy(authority_url, issuer, False, tenant_discovery_mock) @patch("msal.authority.tenant_discovery") def test_regional_issuer_westus2_on_custom_authority(self, tenant_discovery_mock): """Test issuer westus2.myidp.example.com with authority myidp.example.com""" authority_url = "https://myidp.example.com/tenant" issuer = "https://westus2.myidp.example.com/tenant" - authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) - self.assertTrue(authority.has_valid_issuer(), - "Regional prefix westus2 on custom authority host should be valid") + self._assert_issuer_policy(authority_url, issuer, False, tenant_discovery_mock) @patch("msal.authority.tenant_discovery") def test_regional_issuer_does_not_match_different_authority(self, tenant_discovery_mock): @@ -660,9 +1002,7 @@ def test_regional_issuer_on_authority_with_different_path(self, tenant_discovery """Test issuer eastus.someweb.com/v2 with authority someweb.com/tenant""" authority_url = "https://someweb.com/tenant" issuer = "https://eastus.someweb.com/v2" - authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) - self.assertTrue(authority.has_valid_issuer(), - "Regional issuer with different path should still match by host") + self._assert_issuer_policy(authority_url, issuer, False, tenant_discovery_mock) # Case 5: B2C host suffix tests @patch("msal.authority.tenant_discovery") @@ -762,4 +1102,3 @@ def test_valid_b2c_subdomain_should_be_accepted(self, tenant_discovery_mock): authority = self._create_authority_with_issuer(authority_url, issuer, tenant_discovery_mock) self.assertTrue(authority.has_valid_issuer(), "Legitimate subdomain of b2clogin.com should be valid") - From 1e82ba37bde26c781868734b81d9fc87da88372f Mon Sep 17 00:00:00 2001 From: Nilesh Choudhary Date: Thu, 29 Oct 2026 16:09:58 +0000 Subject: [PATCH 2/3] fix: evict discovery on issuer validation errors Evict rejected discovery metadata when issuer parsing raises, without changing the original exception. Cover repeated malformed-issuer rejection, valid recovery, and accepted metadata caching. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- msal/authority.py | 9 +++++++-- tests/test_authority.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/msal/authority.py b/msal/authority.py index 1d8d78f7..8a7fb0ec 100644 --- a/msal/authority.py +++ b/msal/authority.py @@ -117,9 +117,14 @@ def __init__( self._issuer = openid_config.get('issuer') # Validate the issuer if using OIDC authority - if self._oidc_authority_url and not self.has_valid_issuer(): - if isinstance(self._http_client, ThrottledHttpClient): + is_valid_issuer = not self._oidc_authority_url + try: + if self._oidc_authority_url: + is_valid_issuer = self.has_valid_issuer() + finally: + if not is_valid_issuer and isinstance(self._http_client, ThrottledHttpClient): self._http_client._forget_get(tenant_discovery_endpoint) + if not is_valid_issuer: raise ValueError(( "The issuer '{iss}' does not match the authority '{auth}' or a known pattern. " "When using the 'oidc_authority' parameter in ClientApplication, the authority " diff --git a/tests/test_authority.py b/tests/test_authority.py index 2d1dc370..5c1a605d 100644 --- a/tests/test_authority.py +++ b/tests/test_authority.py @@ -529,6 +529,37 @@ def test_rejected_discovery_is_not_cached_and_valid_response_can_recover(self): self.assertEqual(3, len(client.get_calls), "Validated discovery should stay cached") self.assertEqual(1, len(client.post_calls)) + def _assert_malformed_issuer_recovery(self, issuer, error_type, message): + authority = "https://custom.example.com/" + _TENANT + metadata = self._metadata(issuer) + client = self._http_client(authority, metadata) + http_cache, token_cache = {}, msal.TokenCache() + for attempt in range(2): + with self.assertRaisesRegex(error_type, message): + self._application(authority, client, http_cache, token_cache) + self.assertEqual(attempt + 1, len(client.get_calls)) + self.assertFalse(client.post_calls) + self.assertFalse(list(token_cache.search(msal.TokenCache.CredentialType.ACCESS_TOKEN))) + self.assertFalse([key for key in http_cache if key != "_index_"]) + + metadata["issuer"] = "https://login.microsoftonline.com/" + _TENANT + app = self._application(authority, client, http_cache, token_cache) + self.assertEqual(3, len(client.get_calls)) + self.assertEqual("test-token", + app.acquire_token_for_client(["scope"])["access_token"]) + self._application(authority, client, http_cache, token_cache) + self.assertEqual(3, len(client.get_calls), "Validated discovery should stay cached") + self.assertEqual([self.token_endpoint], + [call["url"] for call in client.post_calls]) + + def test_malformed_string_issuer_is_evicted_and_valid_metadata_can_recover(self): + self._assert_malformed_issuer_recovery( + "https://[invalid/tenant", ValueError, "Invalid IPv6 URL") + + def test_nonstring_issuer_is_evicted_and_valid_metadata_can_recover(self): + self._assert_malformed_issuer_recovery( + 123, AttributeError, "rstrip") + def test_preexisting_bad_http_cache_entry_is_revalidated_and_evicted(self): authority = "https://custom.example.com/" + _TENANT discovery_url = authority + "/.well-known/openid-configuration" From 41d12911824f0b4e8ce870b211772d805f550ab2 Mon Sep 17 00:00:00 2001 From: Nilesh Choudhary Date: Thu, 29 Oct 2026 16:23:04 +0000 Subject: [PATCH 3/3] fix: compact rejected discovery cache index Invalidate rejected responses and rebuild the expiry heap from live entries under the existing cache lock. Preserve accepted responses, expiry ordering, and capacity eviction without accumulating orphans. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- msal/individual_cache.py | 16 ++++++++++++- msal/throttled_http_client.py | 2 +- tests/test_authority.py | 36 ++++++++++++++++++++++++++++ tests/test_individual_cache.py | 43 +++++++++++++++++++++++++++++++++- 4 files changed, 94 insertions(+), 3 deletions(-) diff --git a/msal/individual_cache.py b/msal/individual_cache.py index 34f275dd..1aa91ce2 100644 --- a/msal/individual_cache.py +++ b/msal/individual_cache.py @@ -163,6 +163,21 @@ def __delitem__(self, key): # O(1) del timestamps[key] # O(1) self._mapping[self._INDEX] = sequence, timestamps + def _invalidate(self, key): + """Remove an entry and compact its expiry index under the same lock.""" + self._validate_key(key) + with self._lock: + sequence, timestamps = self._peek() + self._mapping.pop(key, None) + timestamps.pop(key, None) + # Rebuild from live entries, including cleanup of previously orphaned records. + sequence[:] = [ + [expires_at, created_at, entry_key] + for entry_key, (expires_at, created_at) in timestamps.items() + ] + heapq.heapify(sequence) + self._mapping[self._INDEX] = sequence, timestamps + def __len__(self): # O(logN) """Drop all expired items and return the remaining length""" with self._lock: @@ -287,4 +302,3 @@ def wrapper(*args, **kwargs): return value return wrapper - diff --git a/msal/throttled_http_client.py b/msal/throttled_http_client.py index 44508181..84942b8c 100644 --- a/msal/throttled_http_client.py +++ b/msal/throttled_http_client.py @@ -181,4 +181,4 @@ def _get_cache_key(self, url, params=""): return "GET {} hash={} 2xx".format(url, self._hash(params)) def _forget_get(self, url): - self._expiring_mapping.pop(self._get_cache_key(url), None) + self._expiring_mapping._invalidate(self._get_cache_key(url)) diff --git a/tests/test_authority.py b/tests/test_authority.py index 5c1a605d..863a55b0 100644 --- a/tests/test_authority.py +++ b/tests/test_authority.py @@ -587,6 +587,42 @@ def test_preexisting_bad_http_cache_entry_is_revalidated_and_evicted(self): self.assertEqual("test-token", app.acquire_token_for_client(["scope"])["access_token"]) + def test_repeated_rejections_do_not_accumulate_expiry_index_records(self): + authority = "https://custom.example.com/" + _TENANT + metadata = self._metadata("https://evil.login.microsoftonline.com/" + _TENANT) + client = self._http_client(authority, metadata) + http_cache, token_cache = {}, msal.TokenCache() + cached_client = ThrottledHttpClient(client, http_cache=http_cache) + unrelated_url = "https://other.example.com/discovery" + client.add_get_route( + lambda call: call["url"] == unrelated_url, + lambda call: MinimalResponse(status_code=200, text="unrelated")) + with patch("msal.individual_cache.time.time", return_value=1000): + cached_client.get(unrelated_url) + for attempt in range(1025): + issuer, error = ( + ("https://evil.login.microsoftonline.com/" + _TENANT, ValueError), + ("https://[invalid/tenant", ValueError), + (123, AttributeError), + )[attempt % 3] + metadata["issuer"] = issuer + with self.assertRaises(error): + self._application(authority, client, http_cache, token_cache) + sequence, timestamps = http_cache["_index_"] + self.assertEqual(1, len(sequence), "Rejected expiry records must be removed") + self.assertEqual(1, len(timestamps)) + self.assertEqual(2, len(http_cache), "Only unrelated response and index remain") + self.assertEqual(1026, len(client.get_calls)) + self.assertFalse(client.post_calls) + self.assertEqual("unrelated", cached_client.get(unrelated_url).text) + self.assertEqual(1026, len(client.get_calls)) + + metadata["issuer"] = "https://login.microsoftonline.com/" + _TENANT + self._application(authority, client, http_cache, token_cache) + self._application(authority, client, http_cache, token_cache) + self.assertEqual(1027, len(client.get_calls)) + self.assertEqual(2, len(http_cache["_index_"][0])) + def test_runtime_alias_metadata_does_not_make_custom_authority_microsoft(self): authority = "https://custom.example.com/" + _TENANT client = self._http_client(authority, diff --git a/tests/test_individual_cache.py b/tests/test_individual_cache.py index ce4aa993..116ddd5d 100644 --- a/tests/test_individual_cache.py +++ b/tests/test_individual_cache.py @@ -1,6 +1,7 @@ from time import sleep from random import random import unittest +from unittest.mock import patch from msal.individual_cache import _ExpiringMapping as ExpiringMapping from msal.individual_cache import _IndividualCache as IndividualCache @@ -91,6 +92,47 @@ def test_oversized_input_should_purge_most_aging_item(self): self.assertEqual(2, len(self.m), "It contains 2 items") self.assertNotIn("thing one", self.m) + def test_invalidation_compacts_orphans_and_preserves_live_expiry(self): + with patch("msal.individual_cache.time.time", return_value=1000): + self.m.set("keep", "value", 100) + self.m.set("reject", "old", 200) + self.m.set("reject", "new", 300) + self.m.pop("reject") + self.m.set("reject", "latest", 400) + self.assertEqual(4, len(self.m._peek()[0])) + + self.m._invalidate("reject") + self.assertEqual(([[1100, 1000, "keep"]], {"keep": [1100, 1000]}), + self.m._peek()) + self.assertNotIn("reject", self.mapping) + self.assertEqual("value", self.m["keep"]) + self.m._invalidate("reject") + self.assertEqual(1, len(self.m._peek()[0])) + + with patch("msal.individual_cache.time.time", return_value=1100): + self.assertEqual(0, len(self.m)) + self.assertEqual(([], {}), self.m._peek()) + + def test_invalidation_rejects_reserved_index(self): + self.m["keep"] = "value" + with self.assertRaises(ValueError): + self.m._invalidate(ExpiringMapping._INDEX) + self.assertEqual("value", self.m["keep"]) + + def test_invalidation_preserves_heap_order_and_capacity_eviction(self): + mapping = ExpiringMapping(capacity=3) + with patch("msal.individual_cache.time.time", return_value=1000): + mapping.set("later", "later", 200) + mapping.set("earlier", "earlier", 100) + mapping.set("reject", "reject", 300) + mapping._invalidate("reject") + self.assertEqual("earlier", mapping._peek()[0][0][2]) + mapping.set("third", "third", 300) + mapping.set("fourth", "fourth", 400) + self.assertNotIn("earlier", mapping) + self.assertEqual({"later", "third", "fourth"}, set(mapping)) + self.assertEqual(3, len(mapping._peek()[0])) + class TestIndividualCache(unittest.TestCase): mapping = {} @@ -107,4 +149,3 @@ def test_memorize_a_function_call(self): "Subsequent run should obtain same result from cache") # Note: In Python 3.7+, dict is ordered, so the following is typically True: #self.assertNotEqual(self.foo(a=1, b=2), self.foo(b=2, a=1)) -