fix(auth): validate wildcard permission patterns and warn on deny-list stripping (#7997)

* fix(auth): validate wildcard permission patterns and warn on deny-list stripping

Permission.IsValidPattern rejects inert wildcard spellings (such as
"workflows*:delete") that parse but can never match. The grant boundary,
stored-permission, and external-authentication options validators reject them
at authoring time, and PermissionGrantValidator applies the same check to
incoming grants.

ExternalAuthenticationOptionsValidator now warns (never fails) when
DeniedPermissions is non-empty, because any non-empty deny list refuses every
wildcard grant that could reach a denied permission -- including the seeded
administrator role's "*". The validator takes an ILogger, and
AddExternalAuthenticationServices registers logging alongside its other
framework dependencies (TryAdd-based, so host logging configuration wins).
The operational consequence is recorded in the authorization-model migration
guide.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(auth): report subtree grants whose verb nothing under them supports

'workflows/*:frobnicate' reached a non-empty subtree and was therefore
treated as resolved, so the startup audit stayed silent about a grant
that cannot authorize anything. Require at least one reached descriptor
to support a concrete verb; verb wildcards keep the reach-only check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Sipke Schoorstra 2026-08-27 11:45:54 +02:00 committed by GitHub
parent 7e8e7aa012
commit 168a8c76f0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
12 changed files with 286 additions and 11 deletions

View file

@ -61,7 +61,10 @@ by exact string, so they read the way a role does.
connection granting `workflows/*:delete` is denied by a deny list naming only `workflows/definitions:delete`.
Before this release both comparisons were exact, so either spelling slipped past the other and a deployment's
deny list did not hold. If you carried a deny list across the upgrade, re-read it: it may now deny more than it
used to, which is the intent.
used to, which is the intent. A consequence to plan for: any non-empty deny list refuses every wildcard grant
that could reach a denied permission, and `*` (which parses to `*:*`) reaches all of them — so a role holding
`*`, including the seeded administrator role, will not survive external issuance. Operators using
`DeniedPermissions` must give externally-authenticating administrators enumerated grants instead of `*`.
- **Allowed** is matched one way: an allow entry must cover the whole grant. `workflows/*:delete` admits
`workflows/definitions:delete`, but an allow list naming only `workflows/definitions:delete` refuses a
`workflows/*:delete` grant rather than admitting the part that overlaps.

View file

@ -37,6 +37,32 @@ public readonly record struct Permission(string Resource, string Verb)
/// <summary>Whether either axis carries a wildcard.</summary>
public bool HasWildcard => IsResourceWildcard || IsVerbWildcard || IsSubtree;
/// <summary>
/// Whether every <c>*</c> this permission carries sits where the matcher gives it meaning: the entire
/// resource (<c>*</c>), a trailing <c>/*</c> subtree segment with no other <c>*</c>, or the entire verb.
/// </summary>
/// <remarks>
/// <see cref="TryParse"/> stays lenient because stored roles may hold historical strings, so a stray
/// wildcard such as <c>workflows*</c> or <c>work*/foo</c> parses yet can never match anything. Validation
/// paths use this check to surface those entries instead of letting them silently match nothing — which
/// in a deny list would mean silently not denying.
/// </remarks>
public bool IsValidPattern
{
get
{
if (!IsVerbWildcard && Verb.Contains(Wildcard, StringComparison.Ordinal))
return false;
var first = Resource.IndexOf(Wildcard, StringComparison.Ordinal);
if (first < 0)
return true;
return first == Resource.LastIndexOf(Wildcard, StringComparison.Ordinal) && (IsResourceWildcard || IsSubtree);
}
}
/// <summary>
/// Parses <paramref name="value"/>, returning <c>false</c> when it is not a well-formed permission.
/// </summary>

View file

@ -48,6 +48,14 @@ public sealed class PermissionGrantValidator(IPermissionDescriptorRegistry regis
continue;
}
// An entry like 'workflows*:delete' parses but the matcher never satisfies it, so it would
// be persisted as a grant that silently reaches nothing.
if (!permission.IsValidPattern)
{
errors.Add(new(value, "Places '*' where it has no meaning and would match nothing. A wildcard may only be the entire resource ('*'), a trailing '/*' segment ('workflows/*'), or the entire verb ('workflows/definitions:*')."));
continue;
}
if (permission.IsResourceWildcard || permission.IsSubtree)
continue;

View file

@ -49,6 +49,10 @@ public static class ServiceCollectionExtensions
services.AddExternalAuthenticationExtension(ExternalAuthenticationExtensionKind.PermissionGrantSource, ClaimMappingPermissionGrantSource.SourceType);
services.AddExternalAuthenticationExtension(ExternalAuthenticationExtensionKind.PermissionGrantSource, GroupMappingPermissionGrantSource.SourceType);
services.AddExternalAuthenticationExtension(ExternalAuthenticationExtensionKind.PermissionGrantSource, ClaimPassThroughPermissionGrantSource.SourceType);
// The validator warns about grant-boundary configuration, and ValidateOnStart resolves it on any
// IOptions access, so a logger has to be resolvable even on a bare service collection. AddLogging is
// TryAdd-based, so a host that already configured logging keeps its own.
services.AddLogging();
services.TryAddEnumerable(ServiceDescriptor.Singleton<IValidateOptions<ExternalAuthenticationOptions>, ExternalAuthenticationOptionsValidator>());
services.AddDataProtection();
services.AddRateLimiter(_ => { });

View file

@ -2,6 +2,7 @@ using Elsa.Authorization;
using Elsa.ExternalAuthentication.Models;
using Elsa.ExternalAuthentication.Options;
using Elsa.ExternalAuthentication.Policies;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
namespace Elsa.ExternalAuthentication.Validation;
@ -10,7 +11,8 @@ namespace Elsa.ExternalAuthentication.Validation;
/// Validates the deployment-owned External Authentication configuration before it is used by the broker.
/// </summary>
public sealed class ExternalAuthenticationOptionsValidator(
IOptions<ExternalAuthenticationExtensionOptions> extensionOptions) : IValidateOptions<ExternalAuthenticationOptions>
IOptions<ExternalAuthenticationExtensionOptions> extensionOptions,
ILogger<ExternalAuthenticationOptionsValidator> logger) : IValidateOptions<ExternalAuthenticationOptions>
{
public ValidateOptionsResult Validate(string? name, ExternalAuthenticationOptions options)
{
@ -48,19 +50,33 @@ public sealed class ExternalAuthenticationOptionsValidator(
/// quietly stop denying what it names. Failing startup puts the mistake in front of whoever can fix it
/// instead of leaving it to be discovered from an issued token.
/// </remarks>
private static void ValidatePermissionGrantBoundary(PermissionGrantOptions? permissionGrants, ICollection<string> failures)
private void ValidatePermissionGrantBoundary(PermissionGrantOptions? permissionGrants, ICollection<string> failures)
{
if (permissionGrants is null)
return;
ValidatePermissionPatterns(permissionGrants.AllowedPermissions, "AllowedPermissions", failures);
ValidatePermissionPatterns(permissionGrants.DeniedPermissions, "DeniedPermissions", failures);
// Deny entries match in both directions, so a wildcard grant that could reach a denied permission is
// refused whole rather than narrowed — a role holding '*' does not survive external token issuance.
// That is intended, but surprising enough at sign-in time to be worth announcing at startup.
if (permissionGrants.DeniedPermissions is { Count: > 0 })
logger.LogWarning(
"ExternalAuthentication:PermissionGrants:DeniedPermissions is configured. Deny entries match wildcard grants in both directions, so any grant that could reach a denied permission is refused entirely — a role holding '*' will carry no permissions into an externally issued token. Give externally-authenticating administrators enumerated grants instead of '*'.");
}
private static void ValidatePermissionPatterns(IEnumerable<string>? permissions, string listName, ICollection<string> failures)
{
foreach (var permission in (permissions ?? []).Where(x => !Permission.TryParse(x, out _)))
failures.Add($"'{permission}' in ExternalAuthentication:PermissionGrants:{listName} is not a well-formed permission. Expected '{{resource}}:{{verb}}', for example 'workflows/*:delete'.");
foreach (var permission in permissions ?? [])
{
if (!Permission.TryParse(permission, out var parsed))
failures.Add($"'{permission}' in ExternalAuthentication:PermissionGrants:{listName} is not a well-formed permission. Expected '{{resource}}:{{verb}}', for example 'workflows/*:delete'.");
// An entry like 'workflows*:delete' parses but the matcher never satisfies it, so in a deny
// list it would silently stop denying what it names.
else if (!parsed.IsValidPattern)
failures.Add($"'{permission}' in ExternalAuthentication:PermissionGrants:{listName} places '*' where it has no meaning and would match nothing. A wildcard may only be the entire resource ('*'), a trailing '/*' segment ('workflows/*'), or the entire verb ('workflows/definitions:*').");
}
}
private static void ValidateExternalCallbackBaseUri(RedirectValidationOptions? redirects, ICollection<string> failures)

View file

@ -69,12 +69,26 @@ public class StoredPermissionValidator(IServiceScopeFactory scopeFactory, ILogge
private static bool Resolves(IPermissionDescriptorRegistry registry, string value)
{
if (!Permission.TryParse(value, out var permission))
if (!Permission.TryParse(value, out var permission) || !permission.IsValidPattern)
return false;
if (permission.IsResourceWildcard || permission.IsSubtree)
if (permission.IsResourceWildcard)
return true;
// A subtree grant reaching nothing is far more likely a typo ('workflow/*') than a grant for a
// module yet to be installed, so it is reported rather than assumed forward-reaching.
if (permission.IsSubtree)
{
var reached = registry.Reach(permission.Resource);
// A concrete verb is only resolved when something under the subtree actually supports it:
// 'workflows/*:frobnicate' reaches plenty and authorizes nothing, which is the same inert
// grant an unreachable subtree is, and deserves the same warning.
return permission.IsVerbWildcard
? reached.Count > 0
: reached.Any(x => registry.Find(x)?.Supports(permission.Verb) == true);
}
var descriptor = registry.Find(permission.Resource);
return descriptor is not null && (permission.IsVerbWildcard || descriptor.Supports(permission.Verb));

View file

@ -63,4 +63,21 @@ public class PermissionTests
Assert.Equal(subtree, permission.IsSubtree);
Assert.Equal(resourceWildcard || verbWildcard || subtree, permission.HasWildcard);
}
[Theory]
[InlineData("workflows/definitions:view", true)]
[InlineData("workflows/*:view", true)]
[InlineData("workflows/definitions:*", true)]
[InlineData("*:*", true)]
[InlineData("workflows*:delete", false)] // missing slash: not a subtree pattern
[InlineData("work*/foo:view", false)] // embedded wildcard mid-resource
[InlineData("work*/definitions/*:view", false)] // trailing '/*' does not redeem an embedded '*'
[InlineData("workflows/*/versions:view", false)] // '*' as a middle segment
[InlineData("workflows:del*", false)] // embedded wildcard in the verb
public void RecognizesWildcardsTheMatcherNeverSatisfies(string value, bool valid)
{
// Such strings parse — TryParse stays lenient for stored roles — but validation paths reject them,
// because a pattern that matches nothing in a deny list silently stops denying.
Assert.Equal(valid, Permission.Parse(value).IsValidPattern);
}
}

View file

@ -66,6 +66,21 @@ public class PermissionGrantValidatorTests
Assert.Contains("view, write", result.Errors.Single().Reason);
}
[Theory]
[InlineData("workflows*:delete")]
[InlineData("work*/foo/*:view")]
[InlineData("workflows/*/instances:view")]
[InlineData("workflows/definitions:vi*w")]
public void RejectsWildcardsTheMatcherNeverSatisfies(string permission)
{
// These parse, but the matcher never satisfies them; accepting them would persist a grant
// that silently reaches nothing.
var result = Validator.Validate([permission]);
Assert.False(result.IsValid);
Assert.Contains("would match nothing", result.Errors.Single().Reason);
}
[Theory]
[InlineData("not a permission")]
[InlineData("workflows/definitions")]

View file

@ -2,6 +2,7 @@ using Elsa.ExternalAuthentication.Contracts;
using Elsa.ExternalAuthentication.Models;
using Elsa.ExternalAuthentication.Options;
using Elsa.ExternalAuthentication.Validation;
using Microsoft.Extensions.Logging.Abstractions;
using Elsa.Extensions;
using Microsoft.Extensions.Configuration;
@ -206,7 +207,7 @@ public class ExternalAuthenticationOptionsTests
extensions.Registrations.Add(new(ExternalAuthenticationExtensionKind.Adapter, adapter.Type));
extensions.Registrations.Add(new(ExternalAuthenticationExtensionKind.UnlinkedIdentityPolicy, "reject"));
extensions.Registrations.Add(new(ExternalAuthenticationExtensionKind.PermissionGrantSource, "elsa-roles"));
return new(Microsoft.Extensions.Options.Options.Create(extensions));
return new(Microsoft.Extensions.Options.Options.Create(extensions), NullLogger<ExternalAuthenticationOptionsValidator>.Instance);
}
private sealed class TestAdapter(string type) : IExternalAuthenticationAdapter

View file

@ -1,11 +1,14 @@
using Elsa.ExternalAuthentication.Models;
using Elsa.ExternalAuthentication.Options;
using Elsa.ExternalAuthentication.Validation;
using Microsoft.Extensions.Logging;
namespace Elsa.ExternalAuthentication.UnitTests.Foundational;
public class ExternalAuthenticationOptionsValidatorTests
{
private readonly CapturingLogger<ExternalAuthenticationOptionsValidator> _logger = new();
[Theory]
// The legacy spelling carries two colons, so it parses as nothing and would silently stop bounding anything.
[InlineData("external-authentication:connections:read")]
@ -27,6 +30,29 @@ public class ExternalAuthenticationOptionsValidatorTests
Assert.Contains(deniedResult.Failures!, x => x.Contains("DeniedPermissions") && x.Contains("well-formed permission"));
}
[Theory]
// These parse, so they slipped past well-formedness — yet the matcher never satisfies them. In a deny
// list that is a silent un-denying, exactly what boundary validation exists to prevent.
[InlineData("workflows*:delete")]
[InlineData("work*/foo:view")]
[InlineData("work*/definitions/*:view")]
[InlineData("workflows:del*")]
public void RejectsAGrantBoundaryEntryWithAWildcardTheMatcherNeverSatisfies(string permission)
{
var allowed = new ExternalAuthenticationOptions();
allowed.PermissionGrants.AllowedPermissions = [permission];
var denied = new ExternalAuthenticationOptions();
denied.PermissionGrants.DeniedPermissions = [permission];
var allowedResult = CreateValidator().Validate(null, allowed);
var deniedResult = CreateValidator().Validate(null, denied);
Assert.False(allowedResult.Succeeded);
Assert.Contains(allowedResult.Failures!, x => x.Contains("AllowedPermissions") && x.Contains("would match nothing"));
Assert.False(deniedResult.Succeeded);
Assert.Contains(deniedResult.Failures!, x => x.Contains("DeniedPermissions") && x.Contains("would match nothing"));
}
[Fact]
public void AcceptsAGrantBoundaryOfWildcardPatterns()
{
@ -39,6 +65,37 @@ public class ExternalAuthenticationOptionsValidatorTests
Assert.DoesNotContain(result.Failures ?? [], x => x.Contains("well-formed permission"));
}
/// <remarks>
/// A deny list is not a failure, but it costs a wildcard grant everything it could have conferred: deny
/// matches in both directions, so '*' satisfies every deny entry and an externally-authenticated superuser
/// silently loses it while local login keeps working. The warning names that at startup rather than leaving
/// it to be diagnosed from an issued token.
/// </remarks>
[Fact]
public void WarnsThatANonEmptyDenyListRefusesWildcardGrantsWhole()
{
var options = new ExternalAuthenticationOptions();
options.PermissionGrants.DeniedPermissions = ["workflows/*:delete"];
var result = CreateValidator().Validate(null, options);
Assert.True(result.Succeeded);
var warning = Assert.Single(_logger.Entries, x => x.Level == LogLevel.Warning);
Assert.Contains("DeniedPermissions", warning.Message);
Assert.Contains("refused entirely", warning.Message);
}
[Fact]
public void DoesNotWarnWhenNoPermissionsAreDenied()
{
var options = new ExternalAuthenticationOptions();
options.PermissionGrants.AllowedPermissions = ["workflows/*:delete"];
CreateValidator().Validate(null, options);
Assert.DoesNotContain(_logger.Entries, x => x.Level == LogLevel.Warning);
}
[Fact]
public void RejectsDuplicateInstalledAdapterTypes()
{
@ -187,7 +244,7 @@ public class ExternalAuthenticationOptionsValidatorTests
Assert.True(result.Succeeded);
}
private static ExternalAuthenticationOptionsValidator CreateValidator(IEnumerable<StubAdapter>? adapters = null)
private ExternalAuthenticationOptionsValidator CreateValidator(IEnumerable<StubAdapter>? adapters = null)
{
var extensions = new ExternalAuthenticationExtensionOptions();
foreach (var adapter in adapters ?? [new StubAdapter("oidc")])
@ -198,6 +255,28 @@ public class ExternalAuthenticationOptionsValidatorTests
extensions.Registrations.Add(new(ExternalAuthenticationExtensionKind.PermissionGrantSource, "claim-mapping"));
extensions.Registrations.Add(new(ExternalAuthenticationExtensionKind.PermissionGrantSource, "group-mapping"));
extensions.Registrations.Add(new(ExternalAuthenticationExtensionKind.PermissionGrantSource, "claim-pass-through"));
return new(Microsoft.Extensions.Options.Options.Create(extensions));
return new(Microsoft.Extensions.Options.Options.Create(extensions), _logger);
}
private sealed class CapturingLogger<T> : ILogger<T>
{
public List<LogEntry> Entries { get; } = [];
public IDisposable BeginScope<TState>(TState state) where TState : notnull => NullScope.Instance;
public bool IsEnabled(LogLevel logLevel) => true;
public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func<TState, Exception?, string> formatter) =>
Entries.Add(new(logLevel, formatter(state, exception)));
}
private sealed record LogEntry(LogLevel Level, string Message);
private sealed class NullScope : IDisposable
{
public static readonly NullScope Instance = new();
public void Dispose()
{
}
}
}

View file

@ -1,6 +1,7 @@
using Elsa.ExternalAuthentication.Options;
using Elsa.ExternalAuthentication.Services;
using Elsa.ExternalAuthentication.Validation;
using Microsoft.Extensions.Logging.Abstractions;
namespace Elsa.ExternalAuthentication.UnitTests.Foundational;
@ -51,7 +52,7 @@ public class HmacExternalAuthenticationHandleHasherTests
extensions.Registrations.Add(new(ExternalAuthenticationExtensionKind.PermissionGrantSource, "claim-mapping"));
extensions.Registrations.Add(new(ExternalAuthenticationExtensionKind.PermissionGrantSource, "group-mapping"));
extensions.Registrations.Add(new(ExternalAuthenticationExtensionKind.PermissionGrantSource, "claim-pass-through"));
var validator = new ExternalAuthenticationOptionsValidator(Microsoft.Extensions.Options.Options.Create(extensions));
var validator = new ExternalAuthenticationOptionsValidator(Microsoft.Extensions.Options.Options.Create(extensions), NullLogger<ExternalAuthenticationOptionsValidator>.Instance);
var result = validator.Validate(null, options);

View file

@ -0,0 +1,91 @@
using Elsa.Identity.Contracts;
using Elsa.Identity.Entities;
using Elsa.Identity.HostedServices;
using Elsa.Identity.Models;
using Elsa.Permissions;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
namespace Elsa.Identity.UnitTests.HostedServices;
public class StoredPermissionValidatorTests
{
private readonly CapturingLogger<StoredPermissionValidator> _logger = new();
[Theory]
[InlineData("*")]
[InlineData("workflows/definitions:view")]
[InlineData("workflows/definitions:*")]
[InlineData("workflows/*:view")] // reaches 'workflows/definitions'
[InlineData("workflows/definitions/*:view")] // reaches the prefix resource itself
public async Task DoesNotWarnAboutAPermissionThatResolves(string permission)
{
await StartAsync(permission);
Assert.DoesNotContain(_logger.Entries, x => x.Level == LogLevel.Warning);
}
[Theory]
[InlineData("workflow/*:view")] // typo'd subtree: reaches nothing, silently authorizes nothing
[InlineData("secrets/*:view")] // subtree over an unregistered branch
[InlineData("workflows*:delete")] // embedded wildcard: parses, but the matcher never satisfies it
[InlineData("work*/foo/*:view")]
[InlineData("workflows/definitions:frobnicate")]
[InlineData("workflows/*:frobnicate")] // reaches 'workflows/definitions', which supports no such verb
public async Task WarnsAboutAPermissionThatDoesNotResolve(string permission)
{
await StartAsync(permission);
var warning = _logger.Entries.First(x => x.Level == LogLevel.Warning);
Assert.Contains("editors", warning.Message);
Assert.Contains(permission, warning.Message);
}
private async Task StartAsync(string permission)
{
var role = new Role { Id = "role-1", Name = "editors", Permissions = [permission] };
var registry = new DefaultPermissionDescriptorRegistry([new StubDescriptorProvider()]);
var services = new ServiceCollection()
.AddSingleton<IRoleProvider>(new StubRoleProvider(role))
.AddSingleton<IPermissionDescriptorRegistry>(registry)
.BuildServiceProvider();
var validator = new StoredPermissionValidator(services.GetRequiredService<IServiceScopeFactory>(), _logger);
await validator.StartAsync(CancellationToken.None);
}
private sealed class StubDescriptorProvider : IPermissionDescriptorProvider
{
public IEnumerable<PermissionDescriptor> GetDescriptors() =>
[new("workflows/definitions", ["view"], "Workflow definitions", "Workflow definitions.", "Workflows")];
}
private sealed class StubRoleProvider(params Role[] roles) : IRoleProvider
{
public ValueTask<IEnumerable<Role>> FindManyAsync(RoleFilter filter, CancellationToken cancellationToken = default) =>
ValueTask.FromResult<IEnumerable<Role>>(roles);
}
private sealed class CapturingLogger<T> : ILogger<T>
{
public List<LogEntry> Entries { get; } = [];
public IDisposable BeginScope<TState>(TState state) where TState : notnull => NullScope.Instance;
public bool IsEnabled(LogLevel logLevel) => true;
public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func<TState, Exception?, string> formatter) =>
Entries.Add(new(logLevel, formatter(state, exception)));
}
private sealed record LogEntry(LogLevel Level, string Message);
private sealed class NullScope : IDisposable
{
public static readonly NullScope Instance = new();
public void Dispose()
{
}
}
}