From 75322ebfaa80fa0995a157753446e3c46e129578 Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Tue, 25 Jan 2022 13:43:37 -0500 Subject: [PATCH 01/13] bug for loading api resources by scope --- .../Stores/Caching/CachingResourceStore.cs | 115 ++++++++++++--- .../Default/CachingResourceStoreTests.cs | 132 ++++++++++++++++-- 2 files changed, 222 insertions(+), 25 deletions(-) diff --git a/src/IdentityServer/Stores/Caching/CachingResourceStore.cs b/src/IdentityServer/Stores/Caching/CachingResourceStore.cs index eb6beb45e..a3996b3e2 100644 --- a/src/IdentityServer/Stores/Caching/CachingResourceStore.cs +++ b/src/IdentityServer/Stores/Caching/CachingResourceStore.cs @@ -28,9 +28,21 @@ public class CachingResourceStore : IResourceStore private readonly ICache _apiScopeCache; private readonly ICache _apiResourceCache; private readonly ICache _allCache; - + private readonly ICache _apiResourceNames; + private readonly IResourceStore _inner; + /// + /// Used to cache the ApiResource names for ApiScopes requested. + /// + public class ApiResourceNames + { + /// + /// The ApiResource names. + /// + public IEnumerable Names { get; set; } + } + /// /// Initializes a new instance of the class. /// @@ -40,11 +52,14 @@ public class CachingResourceStore : IResourceStore /// The ApiResource cache. /// The ApiScope cache. /// All Resources cache. - public CachingResourceStore(IdentityServerOptions options, T inner, + /// + public CachingResourceStore(IdentityServerOptions options, + T inner, ICache identityCache, ICache apisCache, ICache scopeCache, - ICache allCache) + ICache allCache, + ICache apiResourceNames) { _options = options; _inner = inner; @@ -52,6 +67,7 @@ public CachingResourceStore(IdentityServerOptions options, T inner, _apiResourceCache = apisCache; _apiScopeCache = scopeCache; _allCache = allCache; + _apiResourceNames = apiResourceNames; } private string GetKey(IEnumerable names) @@ -71,29 +87,97 @@ public async Task GetAllResourcesAsync() return all; } - + /// - public async Task> FindApiResourcesByNameAsync(IEnumerable apiResourceNames) + public async Task> FindApiResourcesByScopeNameAsync(IEnumerable scopeNames) { - return await FindItemsAsync(apiResourceNames, _apiResourceCache, async names => new Resources(null, await _inner.FindApiResourcesByNameAsync(names), null), x => x.ApiResources, x => x.Name, "ApiResources-"); + var apiResourceNames = new HashSet(); + var uncachedScopes = new List(); + foreach (var scope in scopeNames) + { + var apiResourceName = await _apiResourceNames.GetAsync(scope); + if (apiResourceName != null) + { + foreach(var name in apiResourceName.Names) + { + apiResourceNames.Add(name); + } + } + else + { + uncachedScopes.Add(scope); + } + } + + if (uncachedScopes.Any()) + { + // now we need to lookup the remaining items. it's possible this is happening concurrently, so + // we're going to use the "allcache" to throttle this lookup since the cache has concurrency lock. + // also, the "allcache" conveniently holds Resources objects so it can handle all three of our resource types. + // the results will then be put into the correct and specific cache as individual items for subsequent lookups. + // this means the cache item in the "allcache" should not really be used again and thus can have a very short lifetime. + // as the cache key we'll derive a key from the remaining names, and then hash it to not confuse admins with a meaningful name. + // + // create a key based on the names we're about to lookup + var allCacheItemsKey = "ApiResourcesByScopeName-" + GetKey(uncachedScopes); + // expire this entry much faster than the normal items + var itemsDuration = _options.Caching.ResourceStoreExpiration / 20; + // do the cache/DB lookup + var resources = await _allCache.GetOrAddAsync(allCacheItemsKey, itemsDuration, async () => + { + var results = await _inner.FindApiResourcesByScopeNameAsync(uncachedScopes); + return new Resources(null, results, null); + }); + + // get the specific items from the Resources object + var uncachedItems = resources.ApiResources; + + // add the ApiResource names for each scope we didn't have cached above + foreach(var scope in uncachedScopes) + { + var names = uncachedItems.Where(x => x.Scopes.Contains(scope)).Select(x => x.Name).ToArray(); + var apiResourceNamesCacheItem = new ApiResourceNames { Names = names }; + await _apiResourceNames.SetAsync(scope, apiResourceNamesCacheItem, _options.Caching.ResourceStoreExpiration); + } + + // add each one to the specific cache + foreach (var item in uncachedItems) + { + // this adds to the ApiResource cache in the same way when FindApiResourcesByNameAsync is used + await _apiResourceCache.SetAsync(item.Name, item, _options.Caching.ResourceStoreExpiration); + + // add this name + apiResourceNames.Add(item.Name); + } + } + + // now that we have all the ApiResource names, just use our other API (that should find the cacted items) + return await FindApiResourcesByNameAsync(apiResourceNames); } + /// - public async Task> FindIdentityResourcesByScopeNameAsync(IEnumerable scopeNames) + public async Task> FindApiResourcesByNameAsync(IEnumerable apiResourceNames) { - return await FindItemsAsync(scopeNames, _identityCache, async names => new Resources(await _inner.FindIdentityResourcesByScopeNameAsync(names), null, null), x => x.IdentityResources, x => x.Name, "IdentityResources-"); + return await FindItemsAsync(apiResourceNames, _apiResourceCache, + async names => new Resources(null, await _inner.FindApiResourcesByNameAsync(names), null), + x => x.ApiResources, x => x.Name, "ApiResources-"); } /// - public async Task> FindApiResourcesByScopeNameAsync(IEnumerable scopeNames) + public async Task> FindIdentityResourcesByScopeNameAsync(IEnumerable scopeNames) { - return await FindItemsAsync(scopeNames, _apiResourceCache, async names => new Resources(null, await _inner.FindApiResourcesByScopeNameAsync(names), null), x => x.ApiResources, x => x.Name, "ApiResourcesByScopeNames-", "ApiResourcesByScopeNames-"); + return await FindItemsAsync(scopeNames, _identityCache, + async names => new Resources(await _inner.FindIdentityResourcesByScopeNameAsync(names), null, null), + x => x.IdentityResources, x => x.Name, "IdentityResources-"); } /// public async Task> FindApiScopesByNameAsync(IEnumerable scopeNames) { - return await FindItemsAsync(scopeNames, _apiScopeCache, async names => new Resources(null, null, await _inner.FindApiScopesByNameAsync(names)), x => x.ApiScopes, x => x.Name, "ApiScopes-"); + return await FindItemsAsync(scopeNames, _apiScopeCache, + async names => new Resources(null, null, await _inner.FindApiScopesByNameAsync(names)), + x => x.ApiScopes, x => x.Name, "ApiScopes-"); } @@ -103,8 +187,7 @@ async Task> FindItemsAsync( Func, Task> getResourcesFunc, Func> getFromResourcesFunc, Func getNameFunc, - string allCachePrefix, - string keyPrefix = null + string allCachePrefix ) where TItem : class { @@ -112,7 +195,7 @@ async Task> FindItemsAsync( var cachedItems = new List(); foreach (var name in names) { - var item = await cache.GetAsync(keyPrefix + name); + var item = await cache.GetAsync(name); if (item != null) { cachedItems.Add(item); @@ -133,7 +216,7 @@ async Task> FindItemsAsync( // as the cache key we'll derive a key from the remaining names, and then hash it to not confuse admins with a meaningful name. // create a key based on the names we're about to lookup - var allCacheItemsKey = allCachePrefix + keyPrefix + GetKey(uncachedNames); + var allCacheItemsKey = allCachePrefix + GetKey(uncachedNames); // expire this entry much faster than the normal items var itemsDuration = _options.Caching.ResourceStoreExpiration / 20; // do the cache/DB lookup @@ -144,7 +227,7 @@ async Task> FindItemsAsync( // add each one to the specific cache foreach (var item in uncachedItems) { - await cache.SetAsync(keyPrefix + getNameFunc(item), item, _options.Caching.ResourceStoreExpiration); + await cache.SetAsync(getNameFunc(item), item, _options.Caching.ResourceStoreExpiration); } // add these to our result diff --git a/test/IdentityServer.UnitTests/Stores/Default/CachingResourceStoreTests.cs b/test/IdentityServer.UnitTests/Stores/Default/CachingResourceStoreTests.cs index 0fa8f3ca3..094137801 100644 --- a/test/IdentityServer.UnitTests/Stores/Default/CachingResourceStoreTests.cs +++ b/test/IdentityServer.UnitTests/Stores/Default/CachingResourceStoreTests.cs @@ -22,31 +22,35 @@ public class CachingResourceStoreTests InMemoryResourcesStore _store; IdentityServerOptions _options = new IdentityServerOptions(); + MockCache _apiCache = new MockCache(); + MockCache _identityCache = new MockCache(); MockCache _scopeCache = new MockCache(); MockCache _resourceCache = new MockCache(); + MockCache.ApiResourceNames> _apiResourceNamesCache = new MockCache.ApiResourceNames>(); CachingResourceStore _subject; public CachingResourceStoreTests() { - _apiScopes.Add(new ApiScope("scope1")); - _apiScopes.Add(new ApiScope("scope2")); - _apiScopes.Add(new ApiScope("scope3")); - _apiScopes.Add(new ApiScope("scope4")); - _store = new InMemoryResourcesStore(_identityResources, _apiResources, _apiScopes); _subject = new CachingResourceStore( _options, _store, - new MockCache(), - new MockCache(), + _identityCache, + _apiCache, _scopeCache, - _resourceCache); + _resourceCache, + _apiResourceNamesCache); } [Fact] public async Task FindApiScopesByNameAsync_should_populate_cache() { + _apiScopes.Add(new ApiScope("scope1")); + _apiScopes.Add(new ApiScope("scope2")); + _apiScopes.Add(new ApiScope("scope3")); + _apiScopes.Add(new ApiScope("scope4")); + _scopeCache.Items.Count.Should().Be(0); var items = await _subject.FindApiScopesByNameAsync(new[] { "scope3", "scope1", "scope2", "invalid" }); @@ -58,6 +62,11 @@ public async Task FindApiScopesByNameAsync_should_populate_cache() [Fact] public async Task FindApiScopesByNameAsync_should_populate_missing_cache_items() { + _apiScopes.Add(new ApiScope("scope1")); + _apiScopes.Add(new ApiScope("scope2")); + _apiScopes.Add(new ApiScope("scope3")); + _apiScopes.Add(new ApiScope("scope4")); + _scopeCache.Items.Count.Should().Be(0); var items = await _subject.FindApiScopesByNameAsync(new[] { "scope1" }); @@ -80,4 +89,109 @@ public async Task FindApiScopesByNameAsync_should_populate_missing_cache_items() items.Count().Should().Be(3); _scopeCache.Items.Count.Should().Be(4); } -} \ No newline at end of file + + [Fact] + public async Task FindApiResourcesByScopeNameAsync_should_populate_cache() + { + _apiResources.Add(new ApiResource("foo") { Scopes = { "foo2", "foo1" } }); + _apiResources.Add(new ApiResource("bar") { Scopes = { "bar2", "bar1" } }); + _apiScopes.Add(new ApiScope("foo2")); + _apiScopes.Add(new ApiScope("foo1")); + _apiScopes.Add(new ApiScope("bar2")); + _apiScopes.Add(new ApiScope("bar1")); + + { + _apiCache.Items.Count.Should().Be(0); + _apiResourceNamesCache.Items.Count().Should().Be(0); + var items = await _subject.FindApiResourcesByScopeNameAsync(new[] { "invalid" }); + items.Count().Should().Be(0); + _apiCache.Items.Count.Should().Be(0); + _apiResourceNamesCache.Items.Count().Should().Be(1); + } + + { + _apiCache.Items.Clear(); + _apiResourceNamesCache.Items.Clear(); + _resourceCache.Items.Clear(); + + _apiCache.Items.Count.Should().Be(0); + _apiResourceNamesCache.Items.Count().Should().Be(0); + var items = await _subject.FindApiResourcesByScopeNameAsync(new[] { "foo1" }); + items.Count().Should().Be(1); + items.Select(x => x.Name).Should().BeEquivalentTo(new[] { "foo" }); + _apiCache.Items.Count.Should().Be(1); + _apiResourceNamesCache.Items.Count().Should().Be(1); + } + + { + var items = await _subject.FindApiResourcesByScopeNameAsync(new[] { "foo2" }); + items.Count().Should().Be(1); + items.Select(x => x.Name).Should().BeEquivalentTo(new[] { "foo" }); + _apiCache.Items.Count.Should().Be(1); + _apiResourceNamesCache.Items.Count().Should().Be(2); + } + + { + var items = await _subject.FindApiResourcesByScopeNameAsync(new[] { "foo1", "bar1" }); + items.Count().Should().Be(2); + items.Select(x => x.Name).Should().BeEquivalentTo(new[] { "foo", "bar" }); + _apiCache.Items.Count.Should().Be(2); + _apiResourceNamesCache.Items.Count().Should().Be(3); + } + + { + var items = await _subject.FindApiResourcesByScopeNameAsync(new[] { "foo2", "foo1", "bar2", "bar1" }); + items.Count().Should().Be(2); + items.Select(x => x.Name).Should().BeEquivalentTo(new[] { "foo", "bar" }); + _apiCache.Items.Count.Should().Be(2); + _apiResourceNamesCache.Items.Count().Should().Be(4); + } + + { + _apiCache.Items.Clear(); + _apiResourceNamesCache.Items.Clear(); + _resourceCache.Items.Clear(); + + var items = await _subject.FindApiResourcesByScopeNameAsync(new[] { "foo2", "foo1", "bar2", "bar1" }); + items.Count().Should().Be(2); + items.Select(x => x.Name).Should().BeEquivalentTo(new[] { "foo", "bar" }); + _apiCache.Items.Count.Should().Be(2); + _apiResourceNamesCache.Items.Count().Should().Be(4); + } + + { + // should not need go to db + _apiResources.Clear(); + _apiScopes.Clear(); + _identityResources.Clear(); + + var items = await _subject.FindApiResourcesByScopeNameAsync(new[] { "foo2", "foo1", "bar2", "bar1" }); + items.Count().Should().Be(2); + items.Select(x => x.Name).Should().BeEquivalentTo(new[] { "foo", "bar" }); + _apiCache.Items.Count.Should().Be(2); + _apiResourceNamesCache.Items.Count().Should().Be(4); + } + } + + [Fact] + public async Task FindApiResourcesByScopeNameAsync_should_return_same_results_twice() + { + _apiResources.Add(new ApiResource("foo") { Scopes = { "foo", "foo1" } }); + _apiResources.Add(new ApiResource("bar") { Scopes = { "bar", "bar1" } }); + _apiScopes.Add(new ApiScope("foo")); + _apiScopes.Add(new ApiScope("foo1")); + _apiScopes.Add(new ApiScope("bar")); + _apiScopes.Add(new ApiScope("bar1")); + + { + var items = await _subject.FindApiResourcesByScopeNameAsync(new[] { "foo", "foo1", "bar", "bar1" }); + items.Count().Should().Be(2); + items.Select(x => x.Name).Should().BeEquivalentTo(new[] { "foo", "bar" }); + } + { + var items = await _subject.FindApiResourcesByScopeNameAsync(new[] { "foo", "foo1", "bar", "bar1" }); + items.Count().Should().Be(2); + items.Select(x => x.Name).Should().BeEquivalentTo(new[] { "foo", "bar" }); + } + } +} From 535bc8809ba9880ff4d51885ea8338ca6275ea8d Mon Sep 17 00:00:00 2001 From: Dominick Baier Date: Thu, 27 Jan 2022 23:26:07 +0100 Subject: [PATCH 02/13] Add CIBA grant type to discovery when endpoint is enabled. --- .../ResponseHandling/Default/DiscoveryResponseGenerator.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/IdentityServer/ResponseHandling/Default/DiscoveryResponseGenerator.cs b/src/IdentityServer/ResponseHandling/Default/DiscoveryResponseGenerator.cs index b29a0c5d8..b5a62b95e 100644 --- a/src/IdentityServer/ResponseHandling/Default/DiscoveryResponseGenerator.cs +++ b/src/IdentityServer/ResponseHandling/Default/DiscoveryResponseGenerator.cs @@ -279,6 +279,11 @@ where scope.ShowInDiscoveryDocument { standardGrantTypes.Add(OidcConstants.GrantTypes.DeviceCode); } + + if (Options.Endpoints.EnableBackchannelAuthenticationEndpoint) + { + standardGrantTypes.Add(OidcConstants.GrantTypes.Ciba); + } var showGrantTypes = new List(standardGrantTypes); From ac616b00cffc50d04dbb7e5efdc48aae9daa3aa4 Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Tue, 1 Feb 2022 15:12:11 -0500 Subject: [PATCH 03/13] revert client store query to v5 style (multiple queries to reduce multi table join cartesian explosion) --- .../Stores/ClientStore.cs | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/EntityFramework.Storage/Stores/ClientStore.cs b/src/EntityFramework.Storage/Stores/ClientStore.cs index dad1e15c3..63307077e 100644 --- a/src/EntityFramework.Storage/Stores/ClientStore.cs +++ b/src/EntityFramework.Storage/Stores/ClientStore.cs @@ -58,23 +58,23 @@ public ClientStore(IConfigurationDbContext context, ILogger logger, /// public virtual async Task FindClientByIdAsync(string clientId) { - var query = Context.Clients - .Where(x => x.ClientId == clientId) - .Include(x => x.AllowedCorsOrigins) - .Include(x => x.AllowedGrantTypes) - .Include(x => x.AllowedScopes) - .Include(x => x.Claims) - .Include(x => x.ClientSecrets) - .Include(x => x.IdentityProviderRestrictions) - .Include(x => x.PostLogoutRedirectUris) - .Include(x => x.Properties) - .Include(x => x.RedirectUris) - .AsNoTracking(); - - var client = (await query.ToArrayAsync(CancellationTokenProvider.CancellationToken)) + IQueryable baseQuery = Context.Clients + .Where(x => x.ClientId == clientId); + + var client = (await baseQuery.ToArrayAsync()) .SingleOrDefault(x => x.ClientId == clientId); if (client == null) return null; + await baseQuery.Include(x => x.AllowedCorsOrigins).SelectMany(c => c.AllowedCorsOrigins).LoadAsync(); + await baseQuery.Include(x => x.AllowedGrantTypes).SelectMany(c => c.AllowedGrantTypes).LoadAsync(); + await baseQuery.Include(x => x.AllowedScopes).SelectMany(c => c.AllowedScopes).LoadAsync(); + await baseQuery.Include(x => x.Claims).SelectMany(c => c.Claims).LoadAsync(); + await baseQuery.Include(x => x.ClientSecrets).SelectMany(c => c.ClientSecrets).LoadAsync(); + await baseQuery.Include(x => x.IdentityProviderRestrictions).SelectMany(c => c.IdentityProviderRestrictions).LoadAsync(); + await baseQuery.Include(x => x.PostLogoutRedirectUris).SelectMany(c => c.PostLogoutRedirectUris).LoadAsync(); + await baseQuery.Include(x => x.Properties).SelectMany(c => c.Properties).LoadAsync(); + await baseQuery.Include(x => x.RedirectUris).SelectMany(c => c.RedirectUris).LoadAsync(); + var model = client.ToModel(); Logger.LogDebug("{clientId} found in database: {clientIdFound}", clientId, model != null); From 67ad46d3e8bb410e2a9e23bb49118e89e1d022db Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Tue, 1 Feb 2022 15:12:22 -0500 Subject: [PATCH 04/13] update CC sample to use net6.0 --- .../ConsoleClientCredentialsFlow.csproj | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clients/src/ConsoleClientCredentialsFlow/ConsoleClientCredentialsFlow.csproj b/clients/src/ConsoleClientCredentialsFlow/ConsoleClientCredentialsFlow.csproj index 64178956d..a0c37b85b 100644 --- a/clients/src/ConsoleClientCredentialsFlow/ConsoleClientCredentialsFlow.csproj +++ b/clients/src/ConsoleClientCredentialsFlow/ConsoleClientCredentialsFlow.csproj @@ -1,7 +1,7 @@ - + - net5.0 + net6.0 Exe From 87aa0fbfc1c392da521999d013c3201069afd665 Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Tue, 1 Feb 2022 15:14:39 -0500 Subject: [PATCH 05/13] fix bug in admin pages: missing client secret and offline_access --- hosts/EntityFramework/Pages/Admin/Clients/ClientRepository.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hosts/EntityFramework/Pages/Admin/Clients/ClientRepository.cs b/hosts/EntityFramework/Pages/Admin/Clients/ClientRepository.cs index 9d1b64eb3..de5a785fa 100644 --- a/hosts/EntityFramework/Pages/Admin/Clients/ClientRepository.cs +++ b/hosts/EntityFramework/Pages/Admin/Clients/ClientRepository.cs @@ -121,6 +121,8 @@ public async Task CreateAsync(CreateClientModel model) client.ClientId = model.ClientId.Trim(); client.ClientName = model.Name?.Trim(); + client.ClientSecrets.Add(new Duende.IdentityServer.Models.Secret(model.Secret.Sha256())); + if (model.Flow == Flow.ClientCredentials) { client.AllowedGrantTypes = GrantTypes.ClientCredentials; @@ -128,6 +130,7 @@ public async Task CreateAsync(CreateClientModel model) else { client.AllowedGrantTypes = GrantTypes.Code; + client.AllowOfflineAccess = true; } _context.Clients.Add(client.ToEntity()); From 3e7aec7f47a8b3fe2f00408ec823857a9f73899f Mon Sep 17 00:00:00 2001 From: Brock Allen Date: Wed, 2 Feb 2022 08:45:37 -0500 Subject: [PATCH 06/13] use AsSplitQuery to reuse connection for child tables --- .../Stores/ClientStore.cs | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/EntityFramework.Storage/Stores/ClientStore.cs b/src/EntityFramework.Storage/Stores/ClientStore.cs index 63307077e..c853fc400 100644 --- a/src/EntityFramework.Storage/Stores/ClientStore.cs +++ b/src/EntityFramework.Storage/Stores/ClientStore.cs @@ -58,23 +58,24 @@ public ClientStore(IConfigurationDbContext context, ILogger logger, /// public virtual async Task FindClientByIdAsync(string clientId) { - IQueryable baseQuery = Context.Clients - .Where(x => x.ClientId == clientId); + var query = Context.Clients + .Where(x => x.ClientId == clientId) + .Include(x => x.AllowedCorsOrigins) + .Include(x => x.AllowedGrantTypes) + .Include(x => x.AllowedScopes) + .Include(x => x.Claims) + .Include(x => x.ClientSecrets) + .Include(x => x.IdentityProviderRestrictions) + .Include(x => x.PostLogoutRedirectUris) + .Include(x => x.Properties) + .Include(x => x.RedirectUris) + .AsNoTracking() + .AsSplitQuery(); - var client = (await baseQuery.ToArrayAsync()) - .SingleOrDefault(x => x.ClientId == clientId); + var client = (await query.ToArrayAsync(CancellationTokenProvider.CancellationToken)). + SingleOrDefault(x => x.ClientId == clientId); if (client == null) return null; - await baseQuery.Include(x => x.AllowedCorsOrigins).SelectMany(c => c.AllowedCorsOrigins).LoadAsync(); - await baseQuery.Include(x => x.AllowedGrantTypes).SelectMany(c => c.AllowedGrantTypes).LoadAsync(); - await baseQuery.Include(x => x.AllowedScopes).SelectMany(c => c.AllowedScopes).LoadAsync(); - await baseQuery.Include(x => x.Claims).SelectMany(c => c.Claims).LoadAsync(); - await baseQuery.Include(x => x.ClientSecrets).SelectMany(c => c.ClientSecrets).LoadAsync(); - await baseQuery.Include(x => x.IdentityProviderRestrictions).SelectMany(c => c.IdentityProviderRestrictions).LoadAsync(); - await baseQuery.Include(x => x.PostLogoutRedirectUris).SelectMany(c => c.PostLogoutRedirectUris).LoadAsync(); - await baseQuery.Include(x => x.Properties).SelectMany(c => c.Properties).LoadAsync(); - await baseQuery.Include(x => x.RedirectUris).SelectMany(c => c.RedirectUris).LoadAsync(); - var model = client.ToModel(); Logger.LogDebug("{clientId} found in database: {clientIdFound}", clientId, model != null); From c8b6e0218c287afdf4e07bfd1a5a5389869adef2 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 11 Jun 2024 16:53:15 -0500 Subject: [PATCH 07/13] Added failing tests that show IsLocalUrl bug --- .../Validation/IsLocalUrl.cs | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 test/IdentityServer.UnitTests/Validation/IsLocalUrl.cs diff --git a/test/IdentityServer.UnitTests/Validation/IsLocalUrl.cs b/test/IdentityServer.UnitTests/Validation/IsLocalUrl.cs new file mode 100644 index 000000000..a311ef4d1 --- /dev/null +++ b/test/IdentityServer.UnitTests/Validation/IsLocalUrl.cs @@ -0,0 +1,96 @@ +using Duende.IdentityServer.Configuration; +using Duende.IdentityServer.Extensions; +using Duende.IdentityServer.Services; +using Duende.IdentityServer.Validation; +using FluentAssertions; +using Microsoft.Extensions.Logging; +using UnitTests.Common; +using UnitTests.Endpoints.Authorize; +using Xunit; + +public class IsLocalUrlTests +{ + private const string queryParameters = "?client_id=mvc.code" + + "&redirect_uri=https%3A%2F%2Flocalhost%3A44302%2Fsignin-oidc" + + "&response_type=code" + + "&scope=openid%20profile%20email%20custom.profile%20resource1.scope1%20resource2.scope1%20offline_access" + + "&code_challenge=LcJN1shWmezC0J5EU7QOi7N_amBuvMDb6PcTY0sB2YY" + + "&code_challenge_method=S256" + + "&response_mode=form_post" + + "&nonce=nonce" + + "&state=state"; + private const string ExternalWithControlCharacters = + "/ /evil.com/connect/authorize/callback" + // Note tab character between slashes + queryParameters; + private const string ExternalWithoutControlCharacters = + "//evil.com/" + + queryParameters; + private const string Local = + "/connect/authorize/callback" + + queryParameters; + + [Fact] + public void IsLocalUrl() + { + Local.IsLocalUrl().Should().BeTrue(); + ExternalWithoutControlCharacters.IsLocalUrl().Should().BeFalse(); + ExternalWithControlCharacters.IsLocalUrl().Should().BeFalse(); + } + + [Fact] + public void GetIdentityServerRelativeUrl() + { + var serverUrls = new MockServerUrls + { + Origin = "https://localhost:5001", + BasePath = "/" + }; + + serverUrls.GetIdentityServerRelativeUrl(Local).Should().NotBeNull(); + serverUrls.GetIdentityServerRelativeUrl(ExternalWithoutControlCharacters).Should().BeNull(); + serverUrls.GetIdentityServerRelativeUrl(ExternalWithControlCharacters).Should().BeNull(); + } + + [Fact] + public async void OidcReturnUrlParser() + { + var oidcParser = GetOidcParser(); + + (await oidcParser.ParseAsync(Local)).Should().NotBeNull(); + oidcParser.IsValidReturnUrl(Local).Should().BeTrue(); + (await oidcParser.ParseAsync(ExternalWithoutControlCharacters)).Should().BeNull(); + oidcParser.IsValidReturnUrl(ExternalWithoutControlCharacters).Should().BeFalse(); + (await oidcParser.ParseAsync(ExternalWithControlCharacters)).Should().BeNull(); + oidcParser.IsValidReturnUrl(ExternalWithControlCharacters).Should().BeFalse(); + } + + [Fact] + public async void ReturnUrlParser() + { + var oidcParser = GetOidcParser(); + var parser = new ReturnUrlParser([oidcParser]); + + (await parser.ParseAsync(Local)).Should().NotBeNull(); + parser.IsValidReturnUrl(Local).Should().BeTrue(); + (await parser.ParseAsync(ExternalWithoutControlCharacters)).Should().BeNull(); + parser.IsValidReturnUrl(ExternalWithoutControlCharacters).Should().BeFalse(); + (await parser.ParseAsync(ExternalWithControlCharacters)).Should().BeNull(); + parser.IsValidReturnUrl(ExternalWithControlCharacters).Should().BeFalse(); + } + + private static OidcReturnUrlParser GetOidcParser() + { + return new OidcReturnUrlParser( + new IdentityServerOptions(), + new StubAuthorizeRequestValidator + { + Result = new AuthorizeRequestValidationResult + ( + new ValidatedAuthorizeRequest() + ) + }, + new MockUserSession(), + new MockServerUrls(), + new LoggerFactory().CreateLogger()); + } +} \ No newline at end of file From ce3ccbd3096a7775854d13d47b5c331d5d9ba350 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 11 Jun 2024 20:20:45 -0500 Subject: [PATCH 08/13] Add more failing tests of IsLocalUrl --- test/EntityFramework.Tests/IsLocalUrlTests.cs | 39 +++++++++++++++++++ .../{IsLocalUrl.cs => IsLocalUrlTests.cs} | 28 +++++++++++-- 2 files changed, 63 insertions(+), 4 deletions(-) create mode 100644 test/EntityFramework.Tests/IsLocalUrlTests.cs rename test/IdentityServer.UnitTests/Validation/{IsLocalUrl.cs => IsLocalUrlTests.cs} (75%) diff --git a/test/EntityFramework.Tests/IsLocalUrlTests.cs b/test/EntityFramework.Tests/IsLocalUrlTests.cs new file mode 100644 index 000000000..8beec5e63 --- /dev/null +++ b/test/EntityFramework.Tests/IsLocalUrlTests.cs @@ -0,0 +1,39 @@ +using Duende.IdentityServer.Configuration; +using Duende.IdentityServer.Extensions; +using Duende.IdentityServer.Services; +using Duende.IdentityServer.Validation; +using FluentAssertions; +using Microsoft.Extensions.Logging; +using UnitTests.Common; +using UnitTests.Endpoints.Authorize; +using Xunit; + +public class IsLocalUrlTests +{ + private const string queryParameters = "?client_id=mvc.code" + + "&redirect_uri=https%3A%2F%2Flocalhost%3A44302%2Fsignin-oidc" + + "&response_type=code" + + "&scope=openid%20profile%20email%20custom.profile%20resource1.scope1%20resource2.scope1%20offline_access" + + "&code_challenge=LcJN1shWmezC0J5EU7QOi7N_amBuvMDb6PcTY0sB2YY" + + "&code_challenge_method=S256" + + "&response_mode=form_post" + + "&nonce=nonce" + + "&state=state"; + private const string ExternalWithControlCharacters = + "/ /evil.com/connect/authorize/callback" + // Note tab character between slashes + queryParameters; + private const string ExternalWithoutControlCharacters = + "//evil.com/" + + queryParameters; + private const string Local = + "/connect/authorize/callback" + + queryParameters; + + [Fact] + public void IsLocalUrl() + { + Local.IsLocalUrl().Should().BeTrue(); + ExternalWithoutControlCharacters.IsLocalUrl().Should().BeFalse(); + ExternalWithControlCharacters.IsLocalUrl().Should().BeFalse(); + } +} \ No newline at end of file diff --git a/test/IdentityServer.UnitTests/Validation/IsLocalUrl.cs b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs similarity index 75% rename from test/IdentityServer.UnitTests/Validation/IsLocalUrl.cs rename to test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs index a311ef4d1..bfe2c67b2 100644 --- a/test/IdentityServer.UnitTests/Validation/IsLocalUrl.cs +++ b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs @@ -30,6 +30,18 @@ public class IsLocalUrlTests + queryParameters; [Fact] + public async void GetAuthorizationContextAsync() + { + var interactionService = new DefaultIdentityServerInteractionService(null, null, null, null, null, null, null, + GetReturnUrlParser(), new LoggerFactory().CreateLogger()); + + (await interactionService.GetAuthorizationContextAsync(Local)).Should().NotBeNull(); + (await interactionService.GetAuthorizationContextAsync(ExternalWithoutControlCharacters)).Should().BeNull(); + (await interactionService.GetAuthorizationContextAsync(ExternalWithControlCharacters)).Should().BeNull(); + } + + [Fact] + // TODO - Test the duplicated method in the EF package in later release branches public void IsLocalUrl() { Local.IsLocalUrl().Should().BeTrue(); @@ -54,7 +66,7 @@ public void GetIdentityServerRelativeUrl() [Fact] public async void OidcReturnUrlParser() { - var oidcParser = GetOidcParser(); + var oidcParser = GetOidcReturnUrlParser(); (await oidcParser.ParseAsync(Local)).Should().NotBeNull(); oidcParser.IsValidReturnUrl(Local).Should().BeTrue(); @@ -67,8 +79,7 @@ public async void OidcReturnUrlParser() [Fact] public async void ReturnUrlParser() { - var oidcParser = GetOidcParser(); - var parser = new ReturnUrlParser([oidcParser]); + var parser = GetReturnUrlParser(); (await parser.ParseAsync(Local)).Should().NotBeNull(); parser.IsValidReturnUrl(Local).Should().BeTrue(); @@ -78,7 +89,14 @@ public async void ReturnUrlParser() parser.IsValidReturnUrl(ExternalWithControlCharacters).Should().BeFalse(); } - private static OidcReturnUrlParser GetOidcParser() + private static ReturnUrlParser GetReturnUrlParser() + { + var oidcParser = GetOidcReturnUrlParser(); + var parser = new ReturnUrlParser(new IReturnUrlParser[] { oidcParser }); + return parser; + } + + private static OidcReturnUrlParser GetOidcReturnUrlParser() { return new OidcReturnUrlParser( new IdentityServerOptions(), @@ -93,4 +111,6 @@ private static OidcReturnUrlParser GetOidcParser() new MockServerUrls(), new LoggerFactory().CreateLogger()); } + + } \ No newline at end of file From fa6d0554ab203c332bc3888cc5dbc6041ae1066b Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 11 Jun 2024 20:23:11 -0500 Subject: [PATCH 09/13] Use tab instead of spaces in test data --- test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs index bfe2c67b2..cb0992a9e 100644 --- a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs +++ b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs @@ -20,7 +20,7 @@ public class IsLocalUrlTests "&nonce=nonce" + "&state=state"; private const string ExternalWithControlCharacters = - "/ /evil.com/connect/authorize/callback" + // Note tab character between slashes + "/ /evil.com/connect/authorize/callback" + // Note tab character between slashes queryParameters; private const string ExternalWithoutControlCharacters = "//evil.com/" From 8be6dee4e978c3582e448adca60b8faeb68e4822 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 12 Jun 2024 14:25:42 -0500 Subject: [PATCH 10/13] Refactored IsLocalUrl tests into memberdata, added many more cases --- test/EntityFramework.Tests/IsLocalUrlTests.cs | 39 ----- .../Validation/IsLocalUrlTests.cs | 139 ++++++++++++------ 2 files changed, 97 insertions(+), 81 deletions(-) delete mode 100644 test/EntityFramework.Tests/IsLocalUrlTests.cs diff --git a/test/EntityFramework.Tests/IsLocalUrlTests.cs b/test/EntityFramework.Tests/IsLocalUrlTests.cs deleted file mode 100644 index 8beec5e63..000000000 --- a/test/EntityFramework.Tests/IsLocalUrlTests.cs +++ /dev/null @@ -1,39 +0,0 @@ -using Duende.IdentityServer.Configuration; -using Duende.IdentityServer.Extensions; -using Duende.IdentityServer.Services; -using Duende.IdentityServer.Validation; -using FluentAssertions; -using Microsoft.Extensions.Logging; -using UnitTests.Common; -using UnitTests.Endpoints.Authorize; -using Xunit; - -public class IsLocalUrlTests -{ - private const string queryParameters = "?client_id=mvc.code" + - "&redirect_uri=https%3A%2F%2Flocalhost%3A44302%2Fsignin-oidc" + - "&response_type=code" + - "&scope=openid%20profile%20email%20custom.profile%20resource1.scope1%20resource2.scope1%20offline_access" + - "&code_challenge=LcJN1shWmezC0J5EU7QOi7N_amBuvMDb6PcTY0sB2YY" + - "&code_challenge_method=S256" + - "&response_mode=form_post" + - "&nonce=nonce" + - "&state=state"; - private const string ExternalWithControlCharacters = - "/ /evil.com/connect/authorize/callback" + // Note tab character between slashes - queryParameters; - private const string ExternalWithoutControlCharacters = - "//evil.com/" - + queryParameters; - private const string Local = - "/connect/authorize/callback" - + queryParameters; - - [Fact] - public void IsLocalUrl() - { - Local.IsLocalUrl().Should().BeTrue(); - ExternalWithoutControlCharacters.IsLocalUrl().Should().BeFalse(); - ExternalWithControlCharacters.IsLocalUrl().Should().BeFalse(); - } -} \ No newline at end of file diff --git a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs index cb0992a9e..1c3e0b352 100644 --- a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs +++ b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs @@ -1,3 +1,4 @@ +using System.Collections.Generic; using Duende.IdentityServer.Configuration; using Duende.IdentityServer.Extensions; using Duende.IdentityServer.Services; @@ -8,6 +9,8 @@ using UnitTests.Endpoints.Authorize; using Xunit; +namespace UnitTests.Validation; + public class IsLocalUrlTests { private const string queryParameters = "?client_id=mvc.code" + @@ -19,74 +22,126 @@ public class IsLocalUrlTests "&response_mode=form_post" + "&nonce=nonce" + "&state=state"; - private const string ExternalWithControlCharacters = - "/ /evil.com/connect/authorize/callback" + // Note tab character between slashes - queryParameters; - private const string ExternalWithoutControlCharacters = - "//evil.com/" - + queryParameters; - private const string Local = - "/connect/authorize/callback" - + queryParameters; - [Fact] - public async void GetAuthorizationContextAsync() + public static IEnumerable TestCases => + new List + { + new object[] { "/connect/authorize/callback" + queryParameters, true }, + new object[] { "//evil.com/" + queryParameters, false }, + // Tab character + new object[] { "/\t/evil.com/connect/authorize/callback" + queryParameters, false }, + // Spaces + //new object[] { "/ /evil.com/connect/authorize/callback" + queryParameters, false }, + //new object[] { "/ /evil.com/connect/authorize/callback" + queryParameters, false }, + //new object[] { "/ /evil.com/connect/authorize/callback" + queryParameters, false }, + // Tabs and Spaces + new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/\t /evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/\t /evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/\t /evil.com/connect/authorize/callback" + queryParameters, false }, + // Various new line related things + new object[] { "/\n/evil.com/" + queryParameters, false }, + new object[] { "/\n\n/evil.com/" + queryParameters, false }, + new object[] { "/\r/evil.com/" + queryParameters, false }, + new object[] { "/\r\r/evil.com/" + queryParameters, false }, + new object[] { "/\r\n/evil.com/" + queryParameters, false }, + new object[] { "/\r\n\r\n/evil.com/" + queryParameters, false }, + }; + + [Theory] + [MemberData(nameof(TestCases))] + public async void GetAuthorizationContextAsync(string returnUrl, bool expected) { var interactionService = new DefaultIdentityServerInteractionService(null, null, null, null, null, null, null, GetReturnUrlParser(), new LoggerFactory().CreateLogger()); - - (await interactionService.GetAuthorizationContextAsync(Local)).Should().NotBeNull(); - (await interactionService.GetAuthorizationContextAsync(ExternalWithoutControlCharacters)).Should().BeNull(); - (await interactionService.GetAuthorizationContextAsync(ExternalWithControlCharacters)).Should().BeNull(); + var actual = await interactionService.GetAuthorizationContextAsync(returnUrl); + if (expected) + { + actual.Should().NotBeNull(); + } + else + { + actual.Should().BeNull(); + } } - [Fact] + [Theory] + [MemberData(nameof(TestCases))] // TODO - Test the duplicated method in the EF package in later release branches - public void IsLocalUrl() + public void IsLocalUrl(string returnUrl, bool expected) { - Local.IsLocalUrl().Should().BeTrue(); - ExternalWithoutControlCharacters.IsLocalUrl().Should().BeFalse(); - ExternalWithControlCharacters.IsLocalUrl().Should().BeFalse(); + returnUrl.IsLocalUrl().Should().Be(expected); } - [Fact] - public void GetIdentityServerRelativeUrl() + [Theory] + [MemberData(nameof(TestCases))] + public void GetIdentityServerRelativeUrl(string returnUrl, bool expected) { var serverUrls = new MockServerUrls { Origin = "https://localhost:5001", BasePath = "/" }; - - serverUrls.GetIdentityServerRelativeUrl(Local).Should().NotBeNull(); - serverUrls.GetIdentityServerRelativeUrl(ExternalWithoutControlCharacters).Should().BeNull(); - serverUrls.GetIdentityServerRelativeUrl(ExternalWithControlCharacters).Should().BeNull(); + var actual = serverUrls.GetIdentityServerRelativeUrl(returnUrl); + if (expected) + { + actual.Should().NotBeNull(); + } + else + { + actual.Should().BeNull(); + } } - [Fact] - public async void OidcReturnUrlParser() + [Theory] + [MemberData(nameof(TestCases))] + public async void OidcReturnUrlParser_ParseAsync(string returnUrl, bool expected) { var oidcParser = GetOidcReturnUrlParser(); + var actual = await oidcParser.ParseAsync(returnUrl); + if (expected) + { + actual.Should().NotBeNull(); + } + else + { + actual.Should().BeNull(); + } + } - (await oidcParser.ParseAsync(Local)).Should().NotBeNull(); - oidcParser.IsValidReturnUrl(Local).Should().BeTrue(); - (await oidcParser.ParseAsync(ExternalWithoutControlCharacters)).Should().BeNull(); - oidcParser.IsValidReturnUrl(ExternalWithoutControlCharacters).Should().BeFalse(); - (await oidcParser.ParseAsync(ExternalWithControlCharacters)).Should().BeNull(); - oidcParser.IsValidReturnUrl(ExternalWithControlCharacters).Should().BeFalse(); + [Theory] + [MemberData(nameof(TestCases))] + public void OidcReturnUrlParser_IsValidReturnUrl(string returnUrl, bool expected) + { + var oidcParser = GetOidcReturnUrlParser(); + oidcParser.IsValidReturnUrl(returnUrl).Should().Be(expected); } - [Fact] - public async void ReturnUrlParser() + + [Theory] + [MemberData(nameof(TestCases))] + public void ReturnUrlParser_IsValidReturnUrl(string returnUrl, bool expected) { var parser = GetReturnUrlParser(); + parser.IsValidReturnUrl(returnUrl).Should().Be(expected); + } - (await parser.ParseAsync(Local)).Should().NotBeNull(); - parser.IsValidReturnUrl(Local).Should().BeTrue(); - (await parser.ParseAsync(ExternalWithoutControlCharacters)).Should().BeNull(); - parser.IsValidReturnUrl(ExternalWithoutControlCharacters).Should().BeFalse(); - (await parser.ParseAsync(ExternalWithControlCharacters)).Should().BeNull(); - parser.IsValidReturnUrl(ExternalWithControlCharacters).Should().BeFalse(); + [Theory] + [MemberData(nameof(TestCases))] + public async void ReturnUrlParser_ParseAsync(string returnUrl, bool expected) + { + var parser = GetReturnUrlParser(); + var actual = await parser.ParseAsync(returnUrl); + if (expected) + { + actual.Should().NotBeNull(); + } + else + { + actual.Should().BeNull(); + } } private static ReturnUrlParser GetReturnUrlParser() From 97b298fe62133d5c53d2aaba0de127151a3625fa Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 12 Jun 2024 14:47:51 -0500 Subject: [PATCH 11/13] Sync IsLocalUrl with Microsoft.AspNetCore.Mvc.Routing.UrlHelperBase --- .../Extensions/StringsExtensions.cs | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/IdentityServer/Extensions/StringsExtensions.cs b/src/IdentityServer/Extensions/StringsExtensions.cs index 290191889..8dff245e6 100644 --- a/src/IdentityServer/Extensions/StringsExtensions.cs +++ b/src/IdentityServer/Extensions/StringsExtensions.cs @@ -146,6 +146,8 @@ public static string CleanUrlPath(this string url) [DebuggerStepThrough] public static bool IsLocalUrl(this string url) { + // This implementation is a copy of a https://github.com/dotnet/aspnetcore/blob/3f1acb59718cadf111a0a796681e3d3509bb3381/src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs#L315 + // We originally copied that code to avoid a dependency, but we could potentially remove this entirely by switching to the Microsoft.NET.Sdk.Web sdk. if (string.IsNullOrEmpty(url)) { return false; @@ -163,7 +165,7 @@ public static bool IsLocalUrl(this string url) // url doesn't start with "//" or "/\" if (url[1] != '/' && url[1] != '\\') { - return true; + return !HasControlCharacter(url.AsSpan(1)); } return false; @@ -181,13 +183,27 @@ public static bool IsLocalUrl(this string url) // url doesn't start with "~//" or "~/\" if (url[2] != '/' && url[2] != '\\') { - return true; + return !HasControlCharacter(url.AsSpan(2)); } return false; } return false; + + static bool HasControlCharacter(ReadOnlySpan readOnlySpan) + { + // URLs may not contain ASCII control characters. + for (var i = 0; i < readOnlySpan.Length; i++) + { + if (char.IsControl(readOnlySpan[i])) + { + return true; + } + } + + return false; + } } [DebuggerStepThrough] From 5af78623ab61afb0c3f056fc2a61ff75252447d6 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 12 Jun 2024 15:21:20 -0500 Subject: [PATCH 12/13] Clean up and add more test cases for IsLocalUrl --- .../Validation/IsLocalUrlTests.cs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs index 1c3e0b352..d9835cfcf 100644 --- a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs +++ b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs @@ -30,10 +30,6 @@ public class IsLocalUrlTests new object[] { "//evil.com/" + queryParameters, false }, // Tab character new object[] { "/\t/evil.com/connect/authorize/callback" + queryParameters, false }, - // Spaces - //new object[] { "/ /evil.com/connect/authorize/callback" + queryParameters, false }, - //new object[] { "/ /evil.com/connect/authorize/callback" + queryParameters, false }, - //new object[] { "/ /evil.com/connect/authorize/callback" + queryParameters, false }, // Tabs and Spaces new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, @@ -48,6 +44,19 @@ public class IsLocalUrlTests new object[] { "/\r\r/evil.com/" + queryParameters, false }, new object[] { "/\r\n/evil.com/" + queryParameters, false }, new object[] { "/\r\n\r\n/evil.com/" + queryParameters, false }, + // Tabs and Newlines + new object[] { "/\t\n/evil.com/" + queryParameters, false }, + new object[] { "/\t\n\n/evil.com/" + queryParameters, false }, + new object[] { "/\t\r/evil.com/" + queryParameters, false }, + new object[] { "/\t\r\r/evil.com/" + queryParameters, false }, + new object[] { "/\t\r\n/evil.com/" + queryParameters, false }, + new object[] { "/\t\r\n\r\n/evil.com/" + queryParameters, false }, + new object[] { "/\n/evil.com\t/" + queryParameters, false }, + new object[] { "/\n\n/evil.com\t/" + queryParameters, false }, + new object[] { "/\r/evil.com\t/" + queryParameters, false }, + new object[] { "/\r\r/evil.com\t/" + queryParameters, false }, + new object[] { "/\r\n/evil.com\t/" + queryParameters, false }, + new object[] { "/\r\n\r\n/evil.com\t/" + queryParameters, false }, }; [Theory] From 4a861f24449bef7b2bd78990a60d37645202c5c1 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 12 Jun 2024 16:19:51 -0500 Subject: [PATCH 13/13] Fix IsLocalUrl in EF Note that in 6.1, IsLocalUrl got copied into the EF package --- .../Extensions/StringsExtensions.cs | 20 +++++- .../IsLocalUrlTests.cs | 62 +++++++++++++++++++ .../Validation/IsLocalUrlTests.cs | 1 - 3 files changed, 80 insertions(+), 3 deletions(-) create mode 100644 test/EntityFramework.Storage.UnitTests/IsLocalUrlTests.cs diff --git a/src/EntityFramework.Storage/Extensions/StringsExtensions.cs b/src/EntityFramework.Storage/Extensions/StringsExtensions.cs index d0b295f3a..9f26cb0b4 100644 --- a/src/EntityFramework.Storage/Extensions/StringsExtensions.cs +++ b/src/EntityFramework.Storage/Extensions/StringsExtensions.cs @@ -136,6 +136,8 @@ public static string CleanUrlPath(this string url) [DebuggerStepThrough] public static bool IsLocalUrl(this string url) { + // This implementation is a copy of a https://github.com/dotnet/aspnetcore/blob/3f1acb59718cadf111a0a796681e3d3509bb3381/src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs#L315 + // We originally copied that code to avoid a dependency, but we could potentially remove this entirely by switching to the Microsoft.NET.Sdk.Web sdk. if (string.IsNullOrEmpty(url)) { return false; @@ -153,7 +155,7 @@ public static bool IsLocalUrl(this string url) // url doesn't start with "//" or "/\" if (url[1] != '/' && url[1] != '\\') { - return true; + return !HasControlCharacter(url.AsSpan(1)); } return false; @@ -171,13 +173,27 @@ public static bool IsLocalUrl(this string url) // url doesn't start with "~//" or "~/\" if (url[2] != '/' && url[2] != '\\') { - return true; + return !HasControlCharacter(url.AsSpan(2)); } return false; } return false; + + static bool HasControlCharacter(ReadOnlySpan readOnlySpan) + { + // URLs may not contain ASCII control characters. + for (var i = 0; i < readOnlySpan.Length; i++) + { + if (char.IsControl(readOnlySpan[i])) + { + return true; + } + } + + return false; + } } [DebuggerStepThrough] diff --git a/test/EntityFramework.Storage.UnitTests/IsLocalUrlTests.cs b/test/EntityFramework.Storage.UnitTests/IsLocalUrlTests.cs new file mode 100644 index 000000000..8f93f495b --- /dev/null +++ b/test/EntityFramework.Storage.UnitTests/IsLocalUrlTests.cs @@ -0,0 +1,62 @@ +using System.Collections.Generic; +using FluentAssertions; +using Xunit; +using Duende.IdentityServer.EntityFramework.Extensions; + +namespace UnitTests.Validation; + +public class IsLocalUrlTests +{ + private const string queryParameters = "?client_id=mvc.code" + + "&redirect_uri=https%3A%2F%2Flocalhost%3A44302%2Fsignin-oidc" + + "&response_type=code" + + "&scope=openid%20profile%20email%20custom.profile%20resource1.scope1%20resource2.scope1%20offline_access" + + "&code_challenge=LcJN1shWmezC0J5EU7QOi7N_amBuvMDb6PcTY0sB2YY" + + "&code_challenge_method=S256" + + "&response_mode=form_post" + + "&nonce=nonce" + + "&state=state"; + + public static IEnumerable TestCases => + new List + { + new object[] { "/connect/authorize/callback" + queryParameters, true }, + new object[] { "//evil.com/" + queryParameters, false }, + // Tab character + new object[] { "/\t/evil.com/connect/authorize/callback" + queryParameters, false }, + // Tabs and Spaces + new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/\t /evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/\t /evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/\t /evil.com/connect/authorize/callback" + queryParameters, false }, + // Various new line related things + new object[] { "/\n/evil.com/" + queryParameters, false }, + new object[] { "/\n\n/evil.com/" + queryParameters, false }, + new object[] { "/\r/evil.com/" + queryParameters, false }, + new object[] { "/\r\r/evil.com/" + queryParameters, false }, + new object[] { "/\r\n/evil.com/" + queryParameters, false }, + new object[] { "/\r\n\r\n/evil.com/" + queryParameters, false }, + // Tabs and Newlines + new object[] { "/\t\n/evil.com/" + queryParameters, false }, + new object[] { "/\t\n\n/evil.com/" + queryParameters, false }, + new object[] { "/\t\r/evil.com/" + queryParameters, false }, + new object[] { "/\t\r\r/evil.com/" + queryParameters, false }, + new object[] { "/\t\r\n/evil.com/" + queryParameters, false }, + new object[] { "/\t\r\n\r\n/evil.com/" + queryParameters, false }, + new object[] { "/\n/evil.com\t/" + queryParameters, false }, + new object[] { "/\n\n/evil.com\t/" + queryParameters, false }, + new object[] { "/\r/evil.com\t/" + queryParameters, false }, + new object[] { "/\r\r/evil.com\t/" + queryParameters, false }, + new object[] { "/\r\n/evil.com\t/" + queryParameters, false }, + new object[] { "/\r\n\r\n/evil.com\t/" + queryParameters, false }, + }; + + [Theory] + [MemberData(nameof(TestCases))] + public void IsLocalUrl(string returnUrl, bool expected) + { + returnUrl.IsLocalUrl().Should().Be(expected); + } +} \ No newline at end of file diff --git a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs index d9835cfcf..6319748ac 100644 --- a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs +++ b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs @@ -78,7 +78,6 @@ public async void GetAuthorizationContextAsync(string returnUrl, bool expected) [Theory] [MemberData(nameof(TestCases))] - // TODO - Test the duplicated method in the EF package in later release branches public void IsLocalUrl(string returnUrl, bool expected) { returnUrl.IsLocalUrl().Should().Be(expected);