diff --git a/src/AzureCosmos/AzureCosmosCache.cs b/src/AzureCosmos/AzureCosmosCache.cs index 87ee9bc..6775706 100644 --- a/src/AzureCosmos/AzureCosmosCache.cs +++ b/src/AzureCosmos/AzureCosmosCache.cs @@ -59,7 +59,7 @@ await Cache.DeleteItemAsync( return default!; } - if (ActionCacheEntryOptions.HasExpiredSlidingExpiration(response.Resource.SlidingExpiration)) + if (ActionCacheEntryOptions.HasSlidingExpiration(response.Resource.SlidingExpiration)) { await Cache.ReplaceItemAsync( response.Resource, @@ -94,7 +94,7 @@ public override Task SetAsync(string key, TValue value) Value = CacheJsonSerializer.Serialize(value), AbsoluteExpiration = absoluteExpiration, SlidingExpiration = slidingExpiration, - TTL = ttl == ActionCacheEntryOptions.NoExpiration ? -1 : ttl / 1000 // The deconstructed TTL is in milliseconds, we need to convert it to seconds. + TTL = ttl == ActionCacheEntryOptions.NoExpiration ? -1 : (long)Math.Ceiling(ttl / 1000.0) }, PartitionKey); } diff --git a/src/Common/Caching/ActionCacheEntryOptions.cs b/src/Common/Caching/ActionCacheEntryOptions.cs index ad925ce..9e18409 100644 --- a/src/Common/Caching/ActionCacheEntryOptions.cs +++ b/src/Common/Caching/ActionCacheEntryOptions.cs @@ -81,14 +81,13 @@ public static bool HasExpiredAbsoluteExpiration(long absoluteExpirationUnix) => DateTimeOffset.UtcNow >= DateTimeOffset.FromUnixTimeMilliseconds(absoluteExpirationUnix); /// - /// Determines if the sliding expiration has passed based on the provided sliding expiration value. + /// Determines if a sliding expiration duration is configured for the cache entry. /// - /// The sliding expiration value, which represents the time in milliseconds. + /// The sliding expiration duration in milliseconds. /// - /// true if the sliding expiration time has passed; otherwise, false. - /// If is less than or equal to , expiration is not enforced. + /// true if the sliding expiration duration is set (greater than ); otherwise, false. /// - public static bool HasExpiredSlidingExpiration(long slidingExpiration) => HasExpirationValue(slidingExpiration); + public static bool HasSlidingExpiration(long slidingExpiration) => HasExpirationValue(slidingExpiration); /// /// Determines if the provided expiration value is valid (greater than the value). diff --git a/src/Common/Concurrency/CacheLockerBase.cs b/src/Common/Concurrency/CacheLockerBase.cs index 841621d..c35ba3d 100644 --- a/src/Common/Concurrency/CacheLockerBase.cs +++ b/src/Common/Concurrency/CacheLockerBase.cs @@ -96,7 +96,7 @@ public virtual async Task WaitForLockThenAsync(string resource, Func thenF } else { - // Handle lock acquisition failure + throw new InvalidOperationException($"Failed to acquire lock for resource '{resource}' within the configured timeout."); } } @@ -129,7 +129,7 @@ public virtual async Task WaitForLockThenAsync(string resource, Func thenF } else { - // Handle lock acquisition failure + throw new InvalidOperationException($"Failed to acquire lock for resource '{resource}' within the configured timeout."); } return result; diff --git a/src/Common/Keys/ActionCacheKeyBuilder.cs b/src/Common/Keys/ActionCacheKeyBuilder.cs index 070247e..0ed9bc0 100644 --- a/src/Common/Keys/ActionCacheKeyBuilder.cs +++ b/src/Common/Keys/ActionCacheKeyBuilder.cs @@ -40,6 +40,7 @@ public ActionCacheKeyBuilder WithRouteValues(RouteValueDictionary routeValues) /// Returns itself for chaining. public ActionCacheKeyBuilder WithActionArguments(IDictionary actionArguments) { + if (actionArguments is null) return this; KeyComponents.ActionArguments = actionArguments.ToDictionary(); return this; } diff --git a/src/Common/Serialization/CacheJsonSerializer.cs b/src/Common/Serialization/CacheJsonSerializer.cs index cc16967..bd18938 100644 --- a/src/Common/Serialization/CacheJsonSerializer.cs +++ b/src/Common/Serialization/CacheJsonSerializer.cs @@ -14,7 +14,11 @@ internal static class CacheJsonSerializer /// internal static readonly JsonSerializerSettings SerializerSettings = new JsonSerializerSettings { - TypeNameHandling = TypeNameHandling.All, + // Auto only embeds $type when the runtime type differs from the declared type + // (e.g. IActionResult → OkObjectResult). A binder restricts which types may be + // instantiated from a $type field, blocking known deserialization gadget chains. + TypeNameHandling = TypeNameHandling.Auto, + SerializationBinder = new SafeSerializationBinder(), ReferenceLoopHandling = ReferenceLoopHandling.Ignore, Converters = new List { new ActionArgumentsConverter() } }; @@ -25,8 +29,8 @@ internal static class CacheJsonSerializer /// The type of the object to serialize. /// The object to serialize. Can be null. /// A JSON string representation of the object. - internal static string Serialize(T? obj) => - JsonConvert.SerializeObject(obj, SerializerSettings); + internal static string Serialize(T? obj) => + JsonConvert.SerializeObject(obj, typeof(T), SerializerSettings); /// /// Deserializes a JSON string to an object of type diff --git a/src/Common/Serialization/Converters/ActionArgumentsConverter.cs b/src/Common/Serialization/Converters/ActionArgumentsConverter.cs index 5fd60c3..aec1696 100644 --- a/src/Common/Serialization/Converters/ActionArgumentsConverter.cs +++ b/src/Common/Serialization/Converters/ActionArgumentsConverter.cs @@ -104,7 +104,7 @@ JsonSerializer serializer foreach (var (key, value) in source) { writer.WritePropertyName(key); - serializer.Serialize(writer, value); + serializer.Serialize(writer, value, typeof(object)); } } diff --git a/src/Common/Serialization/SafeSerializationBinder.cs b/src/Common/Serialization/SafeSerializationBinder.cs new file mode 100644 index 0000000..73b9442 --- /dev/null +++ b/src/Common/Serialization/SafeSerializationBinder.cs @@ -0,0 +1,47 @@ +using Newtonsoft.Json; +using Newtonsoft.Json.Serialization; + +namespace ActionCache.Common.Serialization; + +/// +/// A serialization binder that restricts type resolution to assemblies already loaded +/// in the current AppDomain and explicitly blocks known deserialization gadget-chain namespaces. +/// +internal class SafeSerializationBinder : ISerializationBinder +{ + // Known prefixes used in published Newtonsoft.Json gadget chains. + private static readonly string[] BlockedTypePrefixes = + [ + "System.Windows", + "System.Workflow", + "System.Web.Security", + "Microsoft.Exchange", + "Microsoft.IdentityModel", + ]; + + /// + public Type BindToType(string? assemblyName, string typeName) + { + if (BlockedTypePrefixes.Any(prefix => typeName.StartsWith(prefix, StringComparison.Ordinal))) + throw new JsonSerializationException( + $"Type '{typeName}' is not permitted for deserialization."); + + // Only resolve against assemblies already present in the AppDomain — this prevents + // an attacker from triggering the load of arbitrary assemblies via $type. + var type = AppDomain.CurrentDomain + .GetAssemblies() + .Where(assembly => assembly.GetName().Name == assemblyName) + .Select(assembly => assembly.GetType(typeName)) + .FirstOrDefault(resolved => resolved is not null); + + return type ?? throw new JsonSerializationException( + $"Type '{typeName}' from assembly '{assemblyName}' could not be resolved."); + } + + /// + public void BindToName(Type serializedType, out string? assemblyName, out string? typeName) + { + assemblyName = serializedType.Assembly.GetName().Name; + typeName = serializedType.FullName; + } +} diff --git a/src/Common/Utilities/Namespace.cs b/src/Common/Utilities/Namespace.cs index 26e33ae..5360d14 100644 --- a/src/Common/Utilities/Namespace.cs +++ b/src/Common/Utilities/Namespace.cs @@ -10,11 +10,20 @@ public record class Namespace(string Value) /// internal const string Assembly = nameof(ActionCache); + private static readonly AsyncLocal?> _routeValueStore = new(); + + private static Dictionary GetStore() => + _routeValueStore.Value ??= new Dictionary(); + /// - /// Gets or sets a string value containing route template parameters. - /// This property is used to hold route information that can include parameterized values. + /// Gets or sets a string value containing route template parameters, + /// scoped to the current async execution context to prevent cross-request mutation. /// - public string? ValueWithRouteTemplateParameters { get; set; } + public string? ValueWithRouteTemplateParameters + { + get => GetStore().TryGetValue(Value, out var v) ? v : null; + set => GetStore()[Value] = value; + } /// /// Creates a fully qualified namespace key. diff --git a/src/Memory/MemoryActionCacheFactory.cs b/src/Memory/MemoryActionCacheFactory.cs index cff0c30..7fdbcf3 100644 --- a/src/Memory/MemoryActionCacheFactory.cs +++ b/src/Memory/MemoryActionCacheFactory.cs @@ -73,7 +73,9 @@ IActionCacheRefreshProvider refreshProvider EntryOptions = new ActionCacheEntryOptions { AbsoluteExpiration = absoluteExpiration, - SlidingExpiration = slidingExpiration + SlidingExpiration = slidingExpiration, + LockDuration = EntryOptions.LockDuration, + LockTimeout = EntryOptions.LockTimeout }, RefreshProvider = RefreshProvider, CacheLocker = new NullCacheLocker() diff --git a/src/Redis/RedisActionCache.cs b/src/Redis/RedisActionCache.cs index 5215247..1dbcea1 100644 --- a/src/Redis/RedisActionCache.cs +++ b/src/Redis/RedisActionCache.cs @@ -83,10 +83,9 @@ public override async Task SetAsync(string key, TValue value) if (Assembly.TryGetResourceAsText(LuaResources.SetHash, out var script)) { - await Cache.ScriptEvaluateAsync(script, - [(string)Namespace, (RedisKey)key], - [redisValue, absoluteExpiration, slidingExpiration, ttl], - CommandFlags.FireAndForget + await Cache.ScriptEvaluateAsync(script, + [(string)Namespace, (RedisKey)key], + [redisValue, absoluteExpiration, slidingExpiration, ttl] ); } else @@ -129,7 +128,7 @@ public override async Task GetAsync(string key) return default!; } - if (ActionCacheEntryOptions.HasExpiredSlidingExpiration(hashEntries.GetSlidingExpiration())) + if (ActionCacheEntryOptions.HasSlidingExpiration(hashEntries.GetSlidingExpiration())) { await Cache.KeyExpireAsync(namespaceKey, TimeSpan.FromMilliseconds(hashEntries.GetSlidingExpiration()), CommandFlags.FireAndForget); } diff --git a/src/Redis/RedisActionCacheFactory.cs b/src/Redis/RedisActionCacheFactory.cs index bf57c01..5956e4a 100644 --- a/src/Redis/RedisActionCacheFactory.cs +++ b/src/Redis/RedisActionCacheFactory.cs @@ -56,7 +56,9 @@ IActionCacheRefreshProvider refreshProvider EntryOptions = new ActionCacheEntryOptions { AbsoluteExpiration = absoluteExpiration, - SlidingExpiration = slidingExpiration + SlidingExpiration = slidingExpiration, + LockDuration = EntryOptions.LockDuration, + LockTimeout = EntryOptions.LockTimeout }, RefreshProvider = RefreshProvider, CacheLocker = new NullCacheLocker() diff --git a/src/SqlServer/SqlServerActionCacheFactory.cs b/src/SqlServer/SqlServerActionCacheFactory.cs index 8462c60..1cfccd1 100644 --- a/src/SqlServer/SqlServerActionCacheFactory.cs +++ b/src/SqlServer/SqlServerActionCacheFactory.cs @@ -67,7 +67,9 @@ IActionCacheRefreshProvider refreshProvider EntryOptions = new ActionCacheEntryOptions { AbsoluteExpiration = absoluteExpiration, - SlidingExpiration = slidingExpiration + SlidingExpiration = slidingExpiration, + LockDuration = EntryOptions.LockDuration, + LockTimeout = EntryOptions.LockTimeout }, RefreshProvider = RefreshProvider, CacheLocker = new SqlServerCacheLocker( diff --git a/test/Unit/AzureCosmos/AzureCosmosActionCacheTtlTests.cs b/test/Unit/AzureCosmos/AzureCosmosActionCacheTtlTests.cs new file mode 100644 index 0000000..76bc4da --- /dev/null +++ b/test/Unit/AzureCosmos/AzureCosmosActionCacheTtlTests.cs @@ -0,0 +1,26 @@ +namespace Unit.AzureCosmos; + +[TestFixture] +public class AzureCosmosActionCacheTtlTests +{ + [TestCase(1500L, 2L, Description = "1500 ms rounds up to 2 s")] + [TestCase(999L, 1L, Description = "999 ms rounds up to 1 s")] + [TestCase(500L, 1L, Description = "500 ms rounds up to 1 s")] + [TestCase(2000L, 2L, Description = "2000 ms (exact multiple) stays 2 s")] + public void SetAsync_TtlConversion_RoundsUpToNearestSecond(long ttlMs, long expectedSeconds) + { + long actualTtlSeconds = (long)Math.Ceiling(ttlMs / 1000.0); + + actualTtlSeconds.Should().Be(expectedSeconds); + } + + [Test] + public void SetAsync_TtlConversion_WhenNoExpiration_ReturnsMinusOne() + { + long ttl = 0L; // ActionCacheEntryOptions.NoExpiration + + long cosmosttl = ttl == 0L ? -1 : (long)Math.Ceiling(ttl / 1000.0); + + cosmosttl.Should().Be(-1); + } +} diff --git a/test/Unit/Common/ActionCacheEntryOptions/ActionCacheEntryOptionsTests.cs b/test/Unit/Common/ActionCacheEntryOptions/ActionCacheEntryOptionsTests.cs index 86d6569..3ced393 100644 --- a/test/Unit/Common/ActionCacheEntryOptions/ActionCacheEntryOptionsTests.cs +++ b/test/Unit/Common/ActionCacheEntryOptions/ActionCacheEntryOptionsTests.cs @@ -107,15 +107,15 @@ public void HasExpiredAbsoluteExpiration_WhenPast_ReturnsTrue() } [Test] - public void HasExpiredSlidingExpiration_WhenZero_ReturnsFalse() + public void HasSlidingExpiration_WhenZero_ReturnsFalse() { - ActionCacheEntryOptions.HasExpiredSlidingExpiration(0L).Should().BeFalse(); + ActionCacheEntryOptions.HasSlidingExpiration(0L).Should().BeFalse(); } [Test] - public void HasExpiredSlidingExpiration_WhenPositive_ReturnsTrue() + public void HasSlidingExpiration_WhenPositive_ReturnsTrue() { - ActionCacheEntryOptions.HasExpiredSlidingExpiration(5000L).Should().BeTrue(); + ActionCacheEntryOptions.HasSlidingExpiration(5000L).Should().BeTrue(); } [Test] @@ -164,4 +164,15 @@ public void DefaultLockTimeout_Is10Seconds() { new ActionCacheEntryOptions().LockTimeout.Should().Be(TimeSpan.FromSeconds(10)); } + + [Test] + public void HasSlidingExpiration_IsEquivalentToHasExpirationValue() + { + long value = 5_000L; + + bool hasSliding = ActionCacheEntryOptions.HasSlidingExpiration(value); + bool hasValue = ActionCacheEntryOptions.HasExpirationValue(value); + + hasSliding.Should().Be(hasValue); + } } diff --git a/test/Unit/Common/ActionCacheKeyBuilder/Test_ActionCacheKeyBuilder_WithActionArguments_Hash.cs b/test/Unit/Common/ActionCacheKeyBuilder/Test_ActionCacheKeyBuilder_WithActionArguments_Hash.cs index 6be27a5..6f49e77 100644 --- a/test/Unit/Common/ActionCacheKeyBuilder/Test_ActionCacheKeyBuilder_WithActionArguments_Hash.cs +++ b/test/Unit/Common/ActionCacheKeyBuilder/Test_ActionCacheKeyBuilder_WithActionArguments_Hash.cs @@ -9,6 +9,22 @@ namespace Unit.Common; [TestFixture] public class ActionCacheKeyBuilderTests { + // Bug H2: WithActionArguments calls actionArguments.ToDictionary() without a null guard. + // Passing null throws NullReferenceException. The correct behavior is either an early + // return (treat null as empty arguments) or a clear ArgumentNullException. + + [Test] + public void WithActionArguments_WhenNull_ShouldNotThrow_BugH2() + { + var builder = new ActionCacheKeyBuilder(); + + // BUG: currently throws NullReferenceException + // Fix: add null guard and return early (treat as no arguments) + Action act = () => builder.WithActionArguments(null!); + + act.Should().NotThrow(); + } + [Test] [TestCaseSource(typeof(TestData), nameof(TestData.GetControllerDescriptors))] public void Build_WithActionArguments_ProducesEncodedKey( diff --git a/test/Unit/Common/ActionCacheKeyComponentsBuilder/Test_ActionCacheKeyComponentsBuilder.cs b/test/Unit/Common/ActionCacheKeyComponentsBuilder/Test_ActionCacheKeyComponentsBuilder.cs index db58000..ad8405e 100644 --- a/test/Unit/Common/ActionCacheKeyComponentsBuilder/Test_ActionCacheKeyComponentsBuilder.cs +++ b/test/Unit/Common/ActionCacheKeyComponentsBuilder/Test_ActionCacheKeyComponentsBuilder.cs @@ -6,6 +6,37 @@ namespace Unit.Common; [TestFixture] public class ActionCacheKeyComponentsBuilderTests { + // Bug M1: ActionCacheKeyComponentsBuilder calls KeyEncoder.Decode(value) in its constructor + // with no try-catch. A corrupted or manually crafted cache key that is not valid hex throws + // FormatException with no context about which key was malformed. + // + // Fix: wrap the decode in try-catch, log the offending key, and either return a safe default + // or rethrow with additional context. + + [Test] + public void Constructor_WhenValueIsNotValidHex_ThrowsFormatException_BugM1() + { + const string malformedKey = "not-valid-hex!!"; + + // BUG: throws FormatException with no context about the bad key. + // Fix: catch and rethrow with key info, or return empty/default components. + Action act = () => new ActionCacheKeyComponentsBuilder(malformedKey); + + act.Should().Throw(); + } + + [Test] + public void Constructor_WhenValueIsValidHexButNotQueryString_DoesNotThrow_BugM1() + { + // Valid hex that decodes to non-query-string content — should not throw + // (the query parser treats unrecognised content as a single key with no value). + var validHexOfArbitraryContent = new ActionCache.Common.Keys.KeyEncoder().Encode("not-a-query-string"); + + Action act = () => new ActionCacheKeyComponentsBuilder(validHexOfArbitraryContent); + + act.Should().NotThrow(); + } + [Test] public void Build_Always_ParsesRouteValuesAndActionArguments() { diff --git a/test/Unit/Common/Concurrency/CacheLockerBaseTests.cs b/test/Unit/Common/Concurrency/CacheLockerBaseTests.cs new file mode 100644 index 0000000..7bd407d --- /dev/null +++ b/test/Unit/Common/Concurrency/CacheLockerBaseTests.cs @@ -0,0 +1,57 @@ +using ActionCache.Common.Concurrency; +using ActionCache.Common.Concurrency.Locks; + +namespace Unit.Common.Concurrency; + +[TestFixture] +public class CacheLockerBaseTests +{ + private sealed class AlwaysFailingLocker : CacheLockerBase + { + public AlwaysFailingLocker() : base(TimeSpan.FromSeconds(5), TimeSpan.FromSeconds(10)) { } + + public override Task ReleaseLockAsync(NullCacheLock cacheLock) => Task.CompletedTask; + + public override Task TryAcquireLockAsync(string resource) + { + var unacquired = new NullCacheLock(resource); + unacquired.IsAcquired = false; + return Task.FromResult(unacquired); + } + + public override Task WaitForLockAsync(string resource) => + TryAcquireLockAsync(resource); + } + + private AlwaysFailingLocker _locker = null!; + + [SetUp] + public void SetUp() => _locker = new AlwaysFailingLocker(); + + [Test] + public async Task WaitForLockThenAsync_Action_WhenLockNotAcquired_ThrowsInvalidOperationException() + { + Func act = () => _locker.WaitForLockThenAsync("resource", () => { }); + + await act.Should().ThrowAsync() + .WithMessage("*resource*"); + } + + [Test] + public async Task WaitForLockThenAsync_AsyncAction_WhenLockNotAcquired_ThrowsInvalidOperationException() + { + Func act = () => _locker.WaitForLockThenAsync("resource", () => Task.CompletedTask); + + await act.Should().ThrowAsync() + .WithMessage("*resource*"); + } + + [Test] + public async Task WaitForLockThenAsync_AsyncFunc_WhenLockNotAcquired_ThrowsInvalidOperationException() + { + Func act = () => _locker.WaitForLockThenAsync("resource", () => Task.FromResult(42)); + + await act.Should().ThrowAsync() + .WithMessage("*resource*"); + } +} diff --git a/test/Unit/Common/Keys/KeyEncoderTests.cs b/test/Unit/Common/Keys/KeyEncoderTests.cs index 7f16c2a..da804dc 100644 --- a/test/Unit/Common/Keys/KeyEncoderTests.cs +++ b/test/Unit/Common/Keys/KeyEncoderTests.cs @@ -53,4 +53,45 @@ public void Encode_ThenDecode_PreservesArbitraryStrings(string original) decoded.Should().Be(original); } + + // Bug L1: KeyEncoder is misnamed — it performs hex encoding, not AES or any form of + // encryption. Cache keys containing PII (email, user ID, session token) are stored in + // the backend as plain hex, offering zero confidentiality. The class name "KeyEncoder" + // and any references to "encryption" in documentation are misleading. + // + // Fix: rename to HexEncoder. If confidentiality is required, implement HMAC-SHA256 + // key derivation (opaque + deterministic) or AES-GCM encryption with a configured key. + + [Test] + public void Encode_WithSameInput_AlwaysProducesSameOutput_IsNotEncryption_BugL1() + { + // True encryption randomises output via an IV — the same plaintext produces different + // ciphertext on each call. KeyEncoder is deterministic, proving it is not encryption. + var firstCall = _sut.Encode("sensitive-data"); + var secondCall = _sut.Encode("sensitive-data"); + + firstCall.Should().Be(secondCall); + } + + [Test] + public void Encode_OutputIsOnlyHexCharacters_IsNotEncryption_BugL1() + { + var encoded = _sut.Encode("user_id=42&email=alice@example.com"); + + // Output is uppercase hex (A-F, 0-9) — the signature of Convert.ToHexString, + // not a ciphertext or base64-encoded blob. + encoded.Should().MatchRegex("^[0-9A-F]+$"); + } + + [Test] + public void Decode_RequiresNoKey_IsNotEncryption_BugL1() + { + var encoded = _sut.Encode("secret-value"); + + // Symmetric decryption requires a secret key. KeyEncoder.Decode requires no key at all — + // anyone with the hex string can recover the original value. + var decoded = _sut.Decode(encoded); + + decoded.Should().Be("secret-value"); + } } diff --git a/test/Unit/Common/Serialization/CacheJsonSerializerTests.cs b/test/Unit/Common/Serialization/CacheJsonSerializerTests.cs new file mode 100644 index 0000000..ea843a1 --- /dev/null +++ b/test/Unit/Common/Serialization/CacheJsonSerializerTests.cs @@ -0,0 +1,67 @@ +using ActionCache.Common.Serialization; +using Newtonsoft.Json; + +namespace Unit.Common.Serialization; + +// C1 fix: CacheJsonSerializer previously used TypeNameHandling.All which embedded $type in +// every payload, making every cache value a potential RCE gadget target for anyone with +// cache-write access. Fixed to TypeNameHandling.Auto + SafeSerializationBinder: +// - Auto: $type is only emitted when the runtime type differs from the declared type +// (i.e., only for genuine polymorphic scenarios such as IActionResult → OkObjectResult). +// - SafeSerializationBinder: restricts which types may be instantiated from $type, +// blocking known gadget-chain namespaces and unloaded assemblies. + +[TestFixture] +public class CacheJsonSerializerTests +{ + private sealed record UserDto(string Name, int Age); + + [Test] + public void Serialize_WithConcreteObjectType_DoesNotEmbedTypeMetadata() + { + // Declared type == runtime type → Auto does not emit $type. + var json = CacheJsonSerializer.Serialize(new UserDto("Alice", 30)); + + json.Should().NotContain("$type"); + } + + [Test] + public void Serialize_WithCollectionType_DoesNotEmbedTypeMetadata() + { + var json = CacheJsonSerializer.Serialize(new List { "a", "b" }); + + json.Should().NotContain("$type"); + } + + [Test] + public void Serialize_WithConcreteType_DoesNotLeakTypeName() + { + var json = CacheJsonSerializer.Serialize(new UserDto("Alice", 30)); + + json.Should().NotContain(nameof(UserDto)); + } + + [Test] + public void Deserialize_BlockedGadgetChainType_ThrowsJsonSerializationException() + { + // An attacker who writes directly to the cache backend might plant a $type that + // references a known gadget-chain namespace. SafeSerializationBinder must reject it. + var maliciousPayload = """{"$type":"System.Windows.Data.ObjectDataProvider, PresentationFramework","MethodName":"Start"}"""; + + Action act = () => CacheJsonSerializer.Deserialize(maliciousPayload); + + act.Should().Throw() + .WithMessage("*Error resolving type*System.Windows*"); + } + + [Test] + public void Deserialize_ConcreteType_RoundTripsCorrectly() + { + var original = new UserDto("Alice", 30); + + var json = CacheJsonSerializer.Serialize(original); + var result = CacheJsonSerializer.Deserialize(json); + + result.Should().Be(original); + } +} diff --git a/test/Unit/Common/Utilities/NamespaceTests.cs b/test/Unit/Common/Utilities/NamespaceTests.cs index ae9bf20..2cec207 100644 --- a/test/Unit/Common/Utilities/NamespaceTests.cs +++ b/test/Unit/Common/Utilities/NamespaceTests.cs @@ -52,4 +52,40 @@ public void Create_WhenRouteTemplateParametersSet_UsesThemInsteadOfValue() result.Should().Be($"{Namespace.Assembly}:MyNamespace/42:SomeKey"); } + + [Test] + public async Task ValueWithRouteTemplateParameters_ConcurrentRequests_AreIsolatedPerAsyncContext() + { + Namespace sharedNamespace = "Account:{id}"; + var results = new System.Collections.Concurrent.ConcurrentDictionary(); + + await Task.WhenAll( + Task.Run(() => + { + sharedNamespace.ValueWithRouteTemplateParameters = "Account/1"; + results["A"] = sharedNamespace.ValueWithRouteTemplateParameters; + }), + Task.Run(() => + { + sharedNamespace.ValueWithRouteTemplateParameters = "Account/2"; + results["B"] = sharedNamespace.ValueWithRouteTemplateParameters; + }) + ); + + results["A"].Should().Be("Account/1"); + results["B"].Should().Be("Account/2"); + } + + [Test] + public void ValueWithRouteTemplateParameters_WhenSet_ChangesImplicitStringConversion() + { + Namespace ns = "Resource:{id}"; + + string keyBeforeSet = ns; + ns.ValueWithRouteTemplateParameters = "Resource/99"; + string keyAfterSet = ns; + + keyBeforeSet.Should().Be($"{Namespace.Assembly}:Resource:{{id}}"); + keyAfterSet.Should().Be($"{Namespace.Assembly}:Resource/99"); + } } diff --git a/test/Unit/Redis/RedisActionCacheFactoryTests.cs b/test/Unit/Redis/RedisActionCacheFactoryTests.cs index 5c05a9e..e9ac124 100644 --- a/test/Unit/Redis/RedisActionCacheFactoryTests.cs +++ b/test/Unit/Redis/RedisActionCacheFactoryTests.cs @@ -1,3 +1,4 @@ +using System.Reflection; using ActionCache; using ActionCache.Common; using ActionCache.Common.Caching; @@ -72,4 +73,67 @@ public void Create_WithNullExpirations_StillReturnsCache() result.Should().NotBeNull(); } + + // Bug M7: Create(namespace, absoluteExpiration, slidingExpiration) constructs a fresh + // ActionCacheEntryOptions with only the expiration fields set. LockDuration and LockTimeout + // from the configured options are not copied, so they silently reset to their defaults + // (5 s and 10 s respectively). Consumers who configure custom lock durations via IOptions + // will find them ignored whenever a per-namespace expiration is also specified. + // + // Fix: copy LockDuration and LockTimeout from the existing EntryOptions when constructing + // the replacement, or accept the full ActionCacheEntryOptions and override only expiration. + + [Test] + public void Create_WithExpirations_DropsConfiguredLockDuration_BugM7() + { + var customLockDuration = TimeSpan.FromSeconds(30); + var configuredOptions = Options.Create(new ActionCacheEntryOptions + { + LockDuration = customLockDuration + }); + var factory = new RedisActionCacheFactory( + _multiplexerMock.Object, + configuredOptions, + _refreshProviderMock.Object); + + var cache = factory.Create((Namespace)"TestNs", TimeSpan.FromMinutes(5), null); + + var entryOptions = GetEntryOptions(cache!); + + // BUG: LockDuration is reset to the default 5 s instead of the configured 30 s. + entryOptions.LockDuration.Should().Be(customLockDuration); + } + + [Test] + public void Create_WithExpirations_DropsConfiguredLockTimeout_BugM7() + { + var customLockTimeout = TimeSpan.FromSeconds(60); + var configuredOptions = Options.Create(new ActionCacheEntryOptions + { + LockTimeout = customLockTimeout + }); + var factory = new RedisActionCacheFactory( + _multiplexerMock.Object, + configuredOptions, + _refreshProviderMock.Object); + + var cache = factory.Create((Namespace)"TestNs", TimeSpan.FromMinutes(5), null); + + var entryOptions = GetEntryOptions(cache!); + + // BUG: LockTimeout is reset to the default 10 s instead of the configured 60 s. + entryOptions.LockTimeout.Should().Be(customLockTimeout); + } + + private static ActionCacheEntryOptions GetEntryOptions(IActionCache cache) + { + var type = cache.GetType(); + FieldInfo? field = null; + while (type != null && field == null) + { + field = type.GetField("EntryOptions", BindingFlags.NonPublic | BindingFlags.Instance); + type = type.BaseType; + } + return (ActionCacheEntryOptions)field!.GetValue(cache)!; + } } diff --git a/test/Unit/Redis/RedisActionCacheTests.cs b/test/Unit/Redis/RedisActionCacheTests.cs index baf670c..8016ff4 100644 --- a/test/Unit/Redis/RedisActionCacheTests.cs +++ b/test/Unit/Redis/RedisActionCacheTests.cs @@ -140,4 +140,25 @@ public async Task SetAsync_Always_UsesLuaScriptWhenAvailable() db => db.ScriptEvaluateAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Once); } + + // Bug H4: the primary SetAsync path passes CommandFlags.FireAndForget to ScriptEvaluateAsync. + // The result (including errors) is discarded entirely — the caller believes the value was + // cached when it was not. A network error, OOM, or Lua error is silently swallowed. + // Fix: remove CommandFlags.FireAndForget and await the result so failures are surfaced. + + [Test] + public async Task SetAsync_WhenLuaScriptLoads_ShouldNotUseFireAndForget_BugH4() + { + CommandFlags? capturedFlags = null; + _databaseMock + .Setup(db => db.ScriptEvaluateAsync( + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) + .Callback((_, _, _, flags) => capturedFlags = flags) + .ReturnsAsync(RedisResult.Create(RedisValue.Null)); + + await _sut.SetAsync("key", "value"); + + // BUG: capturedFlags is CommandFlags.FireAndForget — write failures are invisible to the caller. + capturedFlags.Should().NotBe(CommandFlags.FireAndForget); + } } diff --git a/test/Unit/Redis/RedisExpiryServiceTests.cs b/test/Unit/Redis/RedisExpiryServiceTests.cs index 76f09bc..e32f071 100644 --- a/test/Unit/Redis/RedisExpiryServiceTests.cs +++ b/test/Unit/Redis/RedisExpiryServiceTests.cs @@ -1,3 +1,4 @@ +using System.Text.RegularExpressions; using ActionCache.Redis; using Moq; using StackExchange.Redis; @@ -113,6 +114,97 @@ public async Task ExpiryCallback_WhenMessageHasNoColon_DoesNotCallSortedSetRemov It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); } + // Bug H6: The regex ^(.*):([^:]+)$ splits on the last colon in the full Redis key. + // The plan flags this as broken for namespaces containing colons. The tests below + // verify the actual behavior: because (.*) is greedy, it always captures everything + // up to the last colon, which IS the correct namespace/key boundary — provided the + // cache key itself (hex-encoded) never contains a colon. These tests document the + // behavior under multi-colon namespaces so that any future key-format change does + // not silently break namespace extraction. + + [Test] + public async Task ExpiryCallback_WhenNamespaceContainsSingleColon_CorrectlySplitsKey_H6() + { + Action? capturedHandler = null; + var handlerCaptured = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + _subscriberMock + .Setup(subscriber => subscriber.SubscribeAsync( + It.IsAny(), + It.IsAny>(), + It.IsAny())) + .Callback, CommandFlags>( + (_, handler, _) => { capturedHandler = handler; handlerCaptured.SetResult(); }) + .Returns(Task.CompletedTask); + + RedisKey? capturedSortedSetKey = null; + RedisValue? capturedMember = null; + _databaseMock + .Setup(db => db.SortedSetRemoveAsync( + It.IsAny(), It.IsAny(), It.IsAny())) + .Callback((key, member, _) => + { + capturedSortedSetKey = key; + capturedMember = member; + }) + .ReturnsAsync(true); + + using var cts = new CancellationTokenSource(); + await _sut.StartAsync(cts.Token); + await handlerCaptured.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // Namespace "ActionCache:Users" with cache key "abc123" + capturedHandler!.Invoke( + RedisChannel.Literal("__keyevent@0__:expired"), + new RedisValue("ActionCache:Users:abc123")); + + await Task.Delay(100); + + capturedSortedSetKey.Should().Be((RedisKey)"ActionCache:Users"); + capturedMember.Should().Be((RedisValue)"abc123"); + } + + [Test] + public async Task ExpiryCallback_WhenNamespaceContainsMultipleColons_SplitsAtLastColon_H6() + { + Action? capturedHandler = null; + var handlerCaptured = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + _subscriberMock + .Setup(subscriber => subscriber.SubscribeAsync( + It.IsAny(), + It.IsAny>(), + It.IsAny())) + .Callback, CommandFlags>( + (_, handler, _) => { capturedHandler = handler; handlerCaptured.SetResult(); }) + .Returns(Task.CompletedTask); + + RedisKey? capturedSortedSetKey = null; + RedisValue? capturedMember = null; + _databaseMock + .Setup(db => db.SortedSetRemoveAsync( + It.IsAny(), It.IsAny(), It.IsAny())) + .Callback((key, member, _) => + { + capturedSortedSetKey = key; + capturedMember = member; + }) + .ReturnsAsync(true); + + using var cts = new CancellationTokenSource(); + await _sut.StartAsync(cts.Token); + await handlerCaptured.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // Namespace "ActionCache:Area:Controller:Action" with cache key "abc123" + // The greedy (.*) captures everything up to the last colon, so the split IS correct. + capturedHandler!.Invoke( + RedisChannel.Literal("__keyevent@0__:expired"), + new RedisValue("ActionCache:Area:Controller:Action:abc123")); + + await Task.Delay(100); + + capturedSortedSetKey.Should().Be((RedisKey)"ActionCache:Area:Controller:Action"); + capturedMember.Should().Be((RedisValue)"abc123"); + } + [Test] public async Task ExpiryCallback_WhenMessageMatchesNamespaceKeyPattern_RemovesMemberFromSortedSet() { diff --git a/test/Unit/SqlServer/SqlServerActionCacheFactoryTests.cs b/test/Unit/SqlServer/SqlServerActionCacheFactoryTests.cs index 3a9342d..8905ba3 100644 --- a/test/Unit/SqlServer/SqlServerActionCacheFactoryTests.cs +++ b/test/Unit/SqlServer/SqlServerActionCacheFactoryTests.cs @@ -1,3 +1,4 @@ +using System.Reflection; using ActionCache; using ActionCache.Common; using ActionCache.Common.Caching; @@ -77,4 +78,44 @@ public void Create_WithNullExpirations_StillReturnsCache() result.Should().NotBeNull(); } + + // Bug M7: same as RedisActionCacheFactory — LockDuration and LockTimeout are not copied + // into the per-call ActionCacheEntryOptions when expiration overrides are provided. + // Note: SqlServerCacheLocker is initialised with the GLOBAL EntryOptions lock settings + // (correct for the locker itself), but the EntryOptions stored inside the cache context + // is missing the custom lock values, affecting any future code that reads them from context. + + [Test] + public void Create_WithExpirations_DropsConfiguredLockDurationInContext_BugM7() + { + var customLockDuration = TimeSpan.FromSeconds(30); + var configuredOptions = Options.Create(new ActionCacheEntryOptions + { + LockDuration = customLockDuration + }); + var factory = new SqlServerActionCacheFactory( + _cacheMock.Object, + _sqlServerOptions, + configuredOptions, + _refreshProviderMock.Object); + + var cache = factory.Create((Namespace)"TestNs", TimeSpan.FromMinutes(5), null); + + var entryOptions = GetEntryOptions(cache!); + + // BUG: LockDuration is reset to the default 5 s inside the cache context. + entryOptions.LockDuration.Should().Be(customLockDuration); + } + + private static ActionCacheEntryOptions GetEntryOptions(IActionCache cache) + { + var type = cache.GetType(); + FieldInfo? field = null; + while (type != null && field == null) + { + field = type.GetField("EntryOptions", BindingFlags.NonPublic | BindingFlags.Instance); + type = type.BaseType; + } + return (ActionCacheEntryOptions)field!.GetValue(cache)!; + } }