From 40804778d8597cf2bcf2d530588a62dcd25ad28a Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 13 Jul 2026 10:12:32 +0100 Subject: [PATCH 1/2] fix #3127 fix #3128 allow reasonable CLIENT commands to proceed without admin mode enabled --- src/StackExchange.Redis/Message.cs | 77 ++++++++++++++++++- src/StackExchange.Redis/RedisDatabase.cs | 18 +++++ .../LibraryNameSuffixAdminTests.cs | 54 +++++++++++++ 3 files changed, 148 insertions(+), 1 deletion(-) create mode 100644 tests/StackExchange.Redis.Tests/LibraryNameSuffixAdminTests.cs diff --git a/src/StackExchange.Redis/Message.cs b/src/StackExchange.Redis/Message.cs index 17ded6c20..f6c209000 100644 --- a/src/StackExchange.Redis/Message.cs +++ b/src/StackExchange.Redis/Message.cs @@ -7,6 +7,7 @@ using System.Text; using System.Threading; using Microsoft.Extensions.Logging; +using RESPite; using RESPite.Internal; using RESPite.Messages; using StackExchange.Redis.Profiling; @@ -171,6 +172,29 @@ internal void PrepareToResend(ServerEndPoint resendTo, bool isMoved) public RedisCommand Command => command; public virtual string CommandAndKey => Command.ToString(); + [AsciiHash(nameof(SubCommandMetadata))] + internal enum SubCommand + { + [AsciiHash("")] + Unknown = 0, + [AsciiHash("GETNAME")] + GetName, + [AsciiHash("ID")] + Id, + [AsciiHash("INFO")] + Info, + [AsciiHash("SETINFO")] + SetInfo, + [AsciiHash("SETNAME")] + SetName, + } + + protected virtual bool TryGetSubCommand(out SubCommand subCommand) + { + subCommand = SubCommand.Unknown; + return false; + } + /// /// Things with the potential to cause harm, or to reveal configuration information. /// @@ -180,6 +204,21 @@ public bool IsAdmin { switch (Command) { + case RedisCommand.CLIENT when TryGetSubCommand(out var subCommand): + switch (subCommand) + { + case SubCommand.GetName: + case SubCommand.SetName: + case SubCommand.Id: + case SubCommand.Info: + case SubCommand.SetInfo: + return false; + } + return true; + /* possible? reasonable? + case RedisCommand.CONFIG when TryGetSubCommand(out var subCommand): + // allow .Get? + */ case RedisCommand.BGREWRITEAOF: case RedisCommand.BGSAVE: case RedisCommand.CLIENT: @@ -691,7 +730,7 @@ internal bool ComputeResult(PhysicalConnection connection, ref RespReader reader } catch (Exception ex) { - connection.OnDetailLog($"{ex.GetType().Name}: {ex.Message}"); + connection?.OnDetailLog($"{ex.GetType().Name}: {ex.Message}"); ex.Data.Add("got", prefix.ToString()); connection?.BridgeCouldBeNull?.Multiplexer?.OnMessageFaulted(this, ex); box?.SetException(ex); @@ -2017,5 +2056,41 @@ private UnknownMessage() : base(0, CommandFlags.None, RedisCommand.UNKNOWN) { } } public void SetNoFlush() => Flags |= NoFlushFlag; + + internal static partial class SubCommandMetadata + { + [AsciiHash(CaseSensitive = false)] + internal static partial bool TryParse(ReadOnlySpan value, out SubCommand subCommand); + + [AsciiHash(CaseSensitive = false)] + internal static partial bool TryParse(ReadOnlySpan value, out SubCommand subCommand); + + internal static bool TryGetSubCommand(in RedisValue value, out SubCommand subCommand) + { + // the inline short-blob is unpacked into this stack slot; keep it alive (a genuine + // named local) for as long as the span returned by UnsafeRawSpan is in use + switch (value.Type) + { + case RedisValue.StorageType.ByteArray: + case RedisValue.StorageType.MemoryManager: + case RedisValue.StorageType.ShortBlob: + // all three contiguous byte-blob kinds expose their bytes directly + return TryParse(value.UnsafeRawSpan(out _), out subCommand); + case RedisValue.StorageType.String: + // char-backed: parse the chars directly, no UTF8 round-trip + return TryParse(value.RawString().AsSpan(), out subCommand); + case RedisValue.StorageType.Sequence when value.GetByteCount() <= BufferBytes: + // non-contiguous: normalize into a small stack buffer + // (sub-commands are short, so anything longer cannot match) + Span tmp = stackalloc byte[BufferBytes]; + var len = value.CopyTo(tmp); + return TryParse(tmp.Slice(0, len), out subCommand); + // numeric / null / unknown are never a sub-command (e.g. it is never `123`); + // if that ever changes, revisit + } + subCommand = SubCommand.Unknown; + return false; + } + } } } diff --git a/src/StackExchange.Redis/RedisDatabase.cs b/src/StackExchange.Redis/RedisDatabase.cs index 21518eaa7..96390ac1e 100644 --- a/src/StackExchange.Redis/RedisDatabase.cs +++ b/src/StackExchange.Redis/RedisDatabase.cs @@ -5613,6 +5613,24 @@ public override int GetHashSlot(ServerSelectionStrategy serverSelectionStrategy) return slot; } public override int ArgCount => _args.Count; + + protected override bool TryGetSubCommand(out SubCommand subCommand) + { + // the sub-command (if any) is the first argument after the command itself, + // e.g. CLIENT [GETNAME]; ad-hoc Execute args are boxed objects, so normalize + // the first one to a RedisValue before probing it against the known sub-commands + foreach (object arg in _args) + { + var value = RedisValue.TryParse(arg, out var valid); + if (valid) + { + return SubCommandMetadata.TryGetSubCommand(value, out subCommand); + } + break; // only the first argument is a sub-command candidate + } + subCommand = SubCommand.Unknown; + return false; + } } private sealed class ScriptEvalMessage : Message, IMultiMessage diff --git a/tests/StackExchange.Redis.Tests/LibraryNameSuffixAdminTests.cs b/tests/StackExchange.Redis.Tests/LibraryNameSuffixAdminTests.cs new file mode 100644 index 000000000..eafb99acb --- /dev/null +++ b/tests/StackExchange.Redis.Tests/LibraryNameSuffixAdminTests.cs @@ -0,0 +1,54 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Tests; + +public class LibraryNameSuffixAdminTests(InProcServerFixture fixture) +{ + private ConfigurationOptions NoAdminConfig() + { + // start from the shared in-process server config, but explicitly *disable* admin mode + var options = fixture.Config.Clone(); + options.AllowAdmin = false; + Assert.False(options.AllowAdmin); + return options; + } + + [Fact] + public async Task AddLibraryNameSuffixWorksWithoutAdmin() + { + await using var conn = await ConnectionMultiplexer.ConnectAsync(NoAdminConfig()); + + // internally this fixes up connected servers via CLIENT SETINFO (best-effort); the + // CLIENT SETINFO sub-command is not admin; don't report it (telemetry, etc) + conn.AddLibraryNameSuffix("mysuffix"); + } + + [Fact] + public async Task ClientSubCommandsViaExecuteDoNotRequireAdmin() + { + await using var conn = await ConnectionMultiplexer.ConnectAsync(NoAdminConfig()); + var server = conn.GetServer(conn.GetEndPoints()[0]); + + // none of these CLIENT sub-commands are admin, so they must not trip the admin-mode guard + // even though AllowAdmin is disabled (regression: the ad-hoc ExecuteMessage previously did + // not expose its sub-command to Message.IsAdmin, so CLIENT was treated as wholesale-admin) + var id = server.Execute("CLIENT", "ID"); + Assert.True((long)id > 0); + + Assert.Equal("OK", (string?)server.Execute("CLIENT", "SETNAME", "roundtrip")); + Assert.Equal("roundtrip", (string?)server.Execute("CLIENT", "GETNAME")); + } + + [Fact] + public async Task AdminClientSubCommandsStillRequireAdmin() + { + await using var conn = await ConnectionMultiplexer.ConnectAsync(NoAdminConfig()); + var server = conn.GetServer(conn.GetEndPoints()[0]); + + // CLIENT LIST is a genuine admin sub-command (not in the allow-list), so it must still be + // blocked when AllowAdmin is disabled - the fix must not blanket-allow every CLIENT usage + var ex = Assert.Throws(() => server.Execute("CLIENT", "LIST")); + Assert.Contains("admin mode", ex.Message); + } +} From 89f1c67de116d6a66f6dca9adfa97c8406c9ea73 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Mon, 13 Jul 2026 10:28:46 +0100 Subject: [PATCH 2/2] comments --- src/StackExchange.Redis/Message.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/StackExchange.Redis/Message.cs b/src/StackExchange.Redis/Message.cs index f6c209000..8b4470de2 100644 --- a/src/StackExchange.Redis/Message.cs +++ b/src/StackExchange.Redis/Message.cs @@ -2067,14 +2067,13 @@ internal static partial class SubCommandMetadata internal static bool TryGetSubCommand(in RedisValue value, out SubCommand subCommand) { - // the inline short-blob is unpacked into this stack slot; keep it alive (a genuine - // named local) for as long as the span returned by UnsafeRawSpan is in use switch (value.Type) { case RedisValue.StorageType.ByteArray: case RedisValue.StorageType.MemoryManager: case RedisValue.StorageType.ShortBlob: // all three contiguous byte-blob kinds expose their bytes directly + // (the discard here *must* be stack-local; that's the "Unsafe" in this API) return TryParse(value.UnsafeRawSpan(out _), out subCommand); case RedisValue.StorageType.String: // char-backed: parse the chars directly, no UTF8 round-trip