diff --git a/code-reviews/Tests/findings.md b/code-reviews/Tests/findings.md index a02b61f..225d577 100644 --- a/code-reviews/Tests/findings.md +++ b/code-reviews/Tests/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-18 | | Commit reviewed | `6c64030` | | Status | Reviewed | -| Open findings | 12 | +| Open findings | 10 | ## Checklist coverage @@ -33,13 +33,13 @@ | Severity | High | | Category | Testing coverage | | Location | `src/MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs:483-489` | -| Status | Open | +| Status | Resolved | **Description:** `FakeSessionManager.TryGetSession` unconditionally returns `true` and synthesizes a session for any id. As a result, `Invoke_WhenSessionMissing_ThrowsNotFound` (line 52) only passes because `InvokeException` is pre-seeded — it does not verify that the gateway service maps a genuinely missing session to `NotFound`. No test exercises the real gateway path where `TryGetSession` returns `false` (for `StreamEvents`, `CloseSession`, alarm RPCs). A regression dropping the missing-session check would not be caught. **Recommendation:** Make `FakeSessionManager.TryGetSession` return `false` for unknown ids (return only seeded sessions), then assert `NotFound`/`InvalidArgument` is produced by the service's own lookup logic rather than an injected exception. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-18: confirmed root cause — added `ResolveOnlySeededSessions`/`SeedSession` to `FakeSessionManager` so `TryGetSession` returns `false` for unseeded ids, rewrote `Invoke_WhenSessionMissing_ThrowsNotFound` to drop the injected `InvokeException` and exercise the service's own `ResolveSession` lookup (asserts `InvokeCount == 0`), and added `Invoke_WhenSessionSeeded_ResolvesAndInvokes`, `AcknowledgeAlarm_WhenSessionMissing_ThrowsNotFound`, and `QueryActiveAlarms_WhenSessionMissing_ThrowsNotFound`. ### Tests-002 @@ -48,13 +48,15 @@ | Severity | High | | Category | Security | | Location | `src/MxGateway.Tests/Gateway/Grpc/GalaxyRepositoryGrpcServiceTests.cs:198-210` | -| Status | Open | +| Status | Resolved | **Description:** The Galaxy Repository RPCs browse a SQL Server database (`ZB`). Every test injects a `StubGalaxyHierarchyCache`, so actual SQL query construction, parameterization, and filter/glob translation are never exercised. No test demonstrates that `TagNameGlob`, `RootTagName`, `AlarmFilterPrefix`, etc. are passed as parameters rather than concatenated into SQL. SQL-injection resistance of the Galaxy layer has zero coverage. **Recommendation:** Add tests for the `GalaxyRepository` query-building layer (against SQLite or an in-memory abstraction, or by asserting parameter objects), covering glob/prefix inputs containing `'`, `%`, `_`, and `;`. At minimum add a unit test over the SQL `LIKE`-pattern escaping helper. -**Resolution:** _(open)_ +**Re-triage note:** The finding's premise is partly misframed. `GalaxyRepository` issues only four *constant* SQL statements (`HierarchySql`, `AttributesSql`, `SELECT 1`, `SELECT time_of_last_deploy FROM galaxy`) — no `DiscoverHierarchyRequest` field is ever concatenated into SQL, so there is no dynamic SQL-injection surface and no `LIKE`-escaping helper to test. `AlarmFilterPrefix` belongs to the worker alarm path, not the Galaxy SQL layer. All filters (`TagNameGlob`, `RootTagName`, template-chain, category, contained-path) are applied **in memory** by `GalaxyHierarchyProjector`/`GalaxyGlobMatcher` against the cached snapshot. The genuine, testable concern — that adversarial filter strings are treated as opaque literals (no wildcard behaviour, no ReDoS, no exceptions) — remains valid and was previously uncovered. Severity left at High: an unsafe in-memory filter would still be a real security gap. + +**Resolution:** Resolved 2026-05-18: added `src/MxGateway.Tests/Galaxy/GalaxyFilterInputSafetyTests.cs` (10 test methods, mostly `[Theory]` over adversarial inputs `'`, `' OR '1'='1`, `'; DROP TABLE gobject;--`, `%`, `_`, `100%_off`, `[abc]`, `Pump'001`) covering `GalaxyGlobMatcher` literal-treatment / `LIKE`-wildcard / pathological-input (ReDoS) behaviour and `GalaxyHierarchyProjector` + `DiscoverHierarchy` RPC handling of adversarial `TagNameGlob`, `RootTagName`, and `TemplateChainContains`. No product bug found — the in-memory filter layer treats all metacharacters as literals; the passing tests resolve the coverage gap. ### Tests-003 diff --git a/src/MxGateway.Tests/Galaxy/GalaxyFilterInputSafetyTests.cs b/src/MxGateway.Tests/Galaxy/GalaxyFilterInputSafetyTests.cs new file mode 100644 index 0000000..676b91f --- /dev/null +++ b/src/MxGateway.Tests/Galaxy/GalaxyFilterInputSafetyTests.cs @@ -0,0 +1,331 @@ +using System.Diagnostics; +using Grpc.Core; +using Microsoft.Extensions.Logging.Abstractions; +using MxGateway.Contracts.Proto.Galaxy; +using MxGateway.Server.Dashboard; +using MxGateway.Server.Galaxy; +using MxGateway.Server.Grpc; +using MxGateway.Server.Security.Authorization; + +namespace MxGateway.Tests.Galaxy; + +/// +/// Adversarial-input coverage for the Galaxy Repository browse filter layer. +/// +/// Re-triage note (finding Tests-002): the Galaxy Repository's SQL surface +/// (HierarchySql, AttributesSql, SELECT 1, +/// SELECT time_of_last_deploy FROM galaxy) is entirely constant — no +/// field is ever concatenated into a SQL +/// string. All filters (TagNameGlob, RootTagName, category ids, +/// template-chain filters, contained-path roots) are applied in memory by +/// against the cached snapshot, so there is +/// no SQL-injection surface and no LIKE-escaping helper to test. +/// +/// +/// The genuine, testable concern is that adversarial filter strings — SQL +/// metacharacters (', ;) and LIKE-wildcards (%, +/// _) — are treated as opaque literals by the in-memory filter layer: +/// they must never act as wildcards, never throw, and never trigger catastrophic +/// regex backtracking in . +/// +/// +public sealed class GalaxyFilterInputSafetyTests +{ + private static readonly string[] AdversarialInputs = + [ + "'", + "' OR '1'='1", + "'; DROP TABLE gobject;--", + "%", + "_", + "100%_off", + "[abc]", + "Pump'001", + ]; + + public static TheoryData AdversarialInputCases() + { + TheoryData data = []; + foreach (string input in AdversarialInputs) + { + data.Add(input); + } + + return data; + } + + /// + /// Verifies treats SQL metacharacters and + /// LIKE-wildcards as literals — a glob equal to the literal value matches, + /// and the same glob does not spuriously match an unrelated value. + /// + [Theory] + [MemberData(nameof(AdversarialInputCases))] + public void GlobMatcher_TreatsSqlMetacharactersAsLiterals(string input) + { + Assert.True( + GalaxyGlobMatcher.IsMatch(input, input), + $"A glob equal to the literal value should match: {input}"); + Assert.False( + GalaxyGlobMatcher.IsMatch("UnrelatedTagName", input), + $"Adversarial glob must not behave as a wildcard against unrelated text: {input}"); + } + + /// + /// Verifies the SQL LIKE wildcards % and _ are NOT treated as + /// wildcards by the glob matcher; only * and ? are glob wildcards. + /// + [Fact] + public void GlobMatcher_DoesNotTreatLikeWildcardsAsWildcards() + { + // '%' would match anything if interpreted as a SQL LIKE wildcard. + Assert.False(GalaxyGlobMatcher.IsMatch("Pump_001", "%")); + // '_' would match a single character if interpreted as a SQL LIKE wildcard. + Assert.False(GalaxyGlobMatcher.IsMatch("A", "_")); + Assert.True(GalaxyGlobMatcher.IsMatch("_", "_")); + // '*' and '?' remain glob wildcards. + Assert.True(GalaxyGlobMatcher.IsMatch("Pump_001", "Pump*")); + Assert.True(GalaxyGlobMatcher.IsMatch("Pump_001", "Pump_00?")); + } + + /// + /// Verifies a pathological glob does not cause catastrophic regex backtracking — + /// escapes every literal character and applies a + /// 100 ms regex timeout, so a long adversarial input completes promptly. + /// + [Fact] + public void GlobMatcher_WithPathologicalInput_DoesNotHang() + { + string pathologicalGlob = new string('a', 5000) + "!"; + string pathologicalValue = new string('a', 5000); + + Stopwatch stopwatch = Stopwatch.StartNew(); + bool matched = GalaxyGlobMatcher.IsMatch(pathologicalValue, pathologicalGlob); + stopwatch.Stop(); + + Assert.False(matched); + Assert.True( + stopwatch.Elapsed < TimeSpan.FromSeconds(2), + $"Glob matching took {stopwatch.ElapsedMilliseconds} ms — expected sub-second."); + } + + /// + /// Verifies the TagNameGlob filter + /// treats an adversarial glob as a literal: it never wildcard-matches the whole + /// hierarchy and never throws. + /// + [Theory] + [MemberData(nameof(AdversarialInputCases))] + public void Projector_TagNameGlob_WithAdversarialInput_DoesNotMatchEverything(string glob) + { + GalaxyHierarchyCacheEntry entry = CreateEntry(CreateObjects()); + + GalaxyHierarchyQueryResult result = GalaxyHierarchyProjector.Project( + entry, + new DiscoverHierarchyRequest { TagNameGlob = glob }); + + // None of the seeded tag names equal an adversarial string, so a correctly + // literal filter returns zero matches rather than the whole hierarchy. + Assert.Equal(0, result.TotalObjectCount); + Assert.Empty(result.Objects); + } + + /// + /// Verifies an adversarial RootTagName resolves through the projector as a + /// literal — an exact-match lookup that finds nothing and surfaces NotFound, + /// never matching unrelated objects or throwing an unexpected exception. + /// + [Theory] + [MemberData(nameof(AdversarialInputCases))] + public void Projector_RootTagName_WithAdversarialInput_ThrowsNotFound(string rootTagName) + { + GalaxyHierarchyCacheEntry entry = CreateEntry(CreateObjects()); + + RpcException exception = Assert.Throws( + () => GalaxyHierarchyProjector.Project( + entry, + new DiscoverHierarchyRequest { RootTagName = rootTagName })); + + Assert.Equal(StatusCode.NotFound, exception.StatusCode); + } + + /// + /// Verifies an adversarial TemplateChainContains filter is a literal + /// substring test — it never matches unrelated template chains and never throws. + /// + [Theory] + [MemberData(nameof(AdversarialInputCases))] + public void Projector_TemplateChainContains_WithAdversarialInput_MatchesNothing(string filter) + { + GalaxyHierarchyCacheEntry entry = CreateEntry(CreateObjects()); + DiscoverHierarchyRequest request = new(); + request.TemplateChainContains.Add(filter); + + GalaxyHierarchyQueryResult result = GalaxyHierarchyProjector.Project(entry, request); + + Assert.Equal(0, result.TotalObjectCount); + } + + /// + /// Verifies the RPC + /// handles an adversarial TagNameGlob end-to-end: the request succeeds with + /// zero matches rather than returning the whole hierarchy or faulting. + /// + [Theory] + [MemberData(nameof(AdversarialInputCases))] + public async Task DiscoverHierarchy_WithAdversarialTagNameGlob_ReturnsZeroMatches(string glob) + { + GalaxyRepositoryGrpcService service = CreateService(CreateEntry(CreateObjects())); + + DiscoverHierarchyReply reply = await service.DiscoverHierarchy( + new DiscoverHierarchyRequest { TagNameGlob = glob, PageSize = 100 }, + new TestServerCallContext()); + + Assert.Equal(0, reply.TotalObjectCount); + Assert.Empty(reply.Objects); + } + + /// + /// Verifies the RPC + /// maps an adversarial RootTagName to NotFound rather than executing it as + /// a query fragment or matching unrelated objects. + /// + [Theory] + [MemberData(nameof(AdversarialInputCases))] + public async Task DiscoverHierarchy_WithAdversarialRootTagName_ReturnsNotFound(string rootTagName) + { + GalaxyRepositoryGrpcService service = CreateService(CreateEntry(CreateObjects())); + + RpcException exception = await Assert.ThrowsAsync( + async () => await service.DiscoverHierarchy( + new DiscoverHierarchyRequest { RootTagName = rootTagName, PageSize = 100 }, + new TestServerCallContext())); + + Assert.Equal(StatusCode.NotFound, exception.StatusCode); + } + + private static GalaxyRepositoryGrpcService CreateService(GalaxyHierarchyCacheEntry entry) + { + GalaxyRepositoryOptions options = new() + { + ConnectionString = "Server=localhost;Database=ZB;Integrated Security=True;Encrypt=False;", + }; + return new GalaxyRepositoryGrpcService( + new MxGateway.Server.Galaxy.GalaxyRepository(options), + new StubGalaxyHierarchyCache(entry), + new GalaxyDeployNotifier(), + new GatewayRequestIdentityAccessor(), + NullLogger.Instance); + } + + private static GalaxyHierarchyCacheEntry CreateEntry(IReadOnlyList objects) + { + return GalaxyHierarchyCacheEntry.Empty with + { + Status = GalaxyCacheStatus.Healthy, + Sequence = 1, + LastSuccessAt = DateTimeOffset.UtcNow, + Objects = objects, + Index = GalaxyHierarchyIndex.Build(objects), + DashboardSummary = DashboardGalaxySummary.Unknown with + { + Status = DashboardGalaxyStatus.Healthy, + ObjectCount = objects.Count, + }, + ObjectCount = objects.Count, + }; + } + + private static IReadOnlyList CreateObjects() + { + return + [ + new GalaxyObject + { + GobjectId = 1, + TagName = "Area1", + ContainedName = "Area1", + BrowseName = "Area1", + IsArea = true, + CategoryId = 13, + }, + new GalaxyObject + { + GobjectId = 2, + TagName = "Pump_001", + ContainedName = "Pump", + BrowseName = "Pump_001", + ParentGobjectId = 1, + CategoryId = 10, + TemplateChain = { "$Pump", "$Base" }, + }, + new GalaxyObject + { + GobjectId = 3, + TagName = "Valve_001", + ContainedName = "Valve", + BrowseName = "Valve_001", + ParentGobjectId = 1, + CategoryId = 11, + TemplateChain = { "$Valve" }, + }, + ]; + } + + private sealed class StubGalaxyHierarchyCache(GalaxyHierarchyCacheEntry current) : IGalaxyHierarchyCache + { + public GalaxyHierarchyCacheEntry Current { get; } = current; + + public Task RefreshAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + public Task WaitForFirstLoadAsync(CancellationToken cancellationToken) => Task.CompletedTask; + } + + private sealed class TestServerCallContext(CancellationToken cancellationToken = default) : ServerCallContext + { + private readonly Metadata requestHeaders = []; + private readonly Metadata responseTrailers = []; + private readonly Dictionary userState = []; + private Status status; + private WriteOptions? writeOptions; + + protected override string MethodCore => "/galaxy_repository.v1.GalaxyRepository/DiscoverHierarchy"; + + protected override string HostCore => "localhost"; + + protected override string PeerCore => "ipv4:127.0.0.1:5000"; + + protected override DateTime DeadlineCore => DateTime.UtcNow.AddMinutes(1); + + protected override Metadata RequestHeadersCore => requestHeaders; + + protected override CancellationToken CancellationTokenCore => cancellationToken; + + protected override Metadata ResponseTrailersCore => responseTrailers; + + protected override Status StatusCore + { + get => status; + set => status = value; + } + + protected override WriteOptions? WriteOptionsCore + { + get => writeOptions; + set => writeOptions = value; + } + + protected override AuthContext AuthContextCore { get; } = new( + string.Empty, + new Dictionary>(StringComparer.Ordinal)); + + protected override IDictionary UserStateCore => userState; + + protected override Task WriteResponseHeadersAsyncCore(Metadata responseHeaders) => Task.CompletedTask; + + protected override ContextPropagationToken CreatePropagationTokenCore(ContextPropagationOptions? options) + { + throw new NotSupportedException(); + } + } +} diff --git a/src/MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs b/src/MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs index b3654a1..1003962 100644 --- a/src/MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs +++ b/src/MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs @@ -47,16 +47,17 @@ public sealed class MxAccessGatewayServiceTests Assert.Equal("operator-session", sessionManager.LastOpenRequest?.ClientSessionName); } - /// Verifies that Invoke throws NotFound when the session does not exist. + /// + /// Verifies that Invoke maps a genuinely missing session to NotFound via the + /// service's own ResolveSession lookup. No InvokeException is + /// injected — makes + /// TryGetSession return false, so this test fails if the service drops + /// its missing-session check rather than passing for the wrong reason. + /// [Fact] public async Task Invoke_WhenSessionMissing_ThrowsNotFound() { - FakeSessionManager sessionManager = new() - { - InvokeException = new SessionManagerException( - SessionManagerErrorCode.SessionNotFound, - "Session session-missing was not found."), - }; + FakeSessionManager sessionManager = new() { ResolveOnlySeededSessions = true }; MxAccessGatewayService service = CreateService(sessionManager); RpcException exception = await Assert.ThrowsAsync( @@ -66,6 +67,76 @@ public sealed class MxAccessGatewayServiceTests Assert.Equal(StatusCode.NotFound, exception.StatusCode); Assert.Contains("session-missing", exception.Status.Detail, StringComparison.Ordinal); + // The service must reject before delegating to the session manager. + Assert.Equal(0, sessionManager.InvokeCount); + } + + /// + /// Verifies that Invoke resolves a session that was seeded into the session + /// manager when is on, + /// confirming the missing-session test above is gated on a real lookup. + /// + [Fact] + public async Task Invoke_WhenSessionSeeded_ResolvesAndInvokes() + { + FakeSessionManager sessionManager = new() { ResolveOnlySeededSessions = true }; + sessionManager.SeedSession(CreateSession("session-1", processId: 1234)); + MxAccessGatewayService service = CreateService(sessionManager); + + MxCommandReply reply = await service.Invoke( + CreatePingRequest("session-1"), + new TestServerCallContext()); + + Assert.Equal(ProtocolStatusCode.Ok, reply.ProtocolStatus.Code); + Assert.Equal(1, sessionManager.InvokeCount); + } + + /// + /// Verifies that AcknowledgeAlarm maps a genuinely missing session to NotFound via + /// the service's own ResolveSession lookup rather than an injected exception. + /// + [Fact] + public async Task AcknowledgeAlarm_WhenSessionMissing_ThrowsNotFound() + { + FakeSessionManager sessionManager = new() { ResolveOnlySeededSessions = true }; + MxAccessGatewayService service = CreateService(sessionManager); + + RpcException exception = await Assert.ThrowsAsync( + async () => await service.AcknowledgeAlarm( + new AcknowledgeAlarmRequest + { + SessionId = "session-missing", + AlarmFullReference = "Tank01.Level.HiHi", + OperatorUser = "alice", + }, + new TestServerCallContext())); + + Assert.Equal(StatusCode.NotFound, exception.StatusCode); + Assert.Contains("session-missing", exception.Status.Detail, StringComparison.Ordinal); + } + + /// + /// Verifies that QueryActiveAlarms maps a genuinely missing session to NotFound via + /// the service's own ResolveSession lookup rather than an injected exception. + /// + [Fact] + public async Task QueryActiveAlarms_WhenSessionMissing_ThrowsNotFound() + { + FakeSessionManager sessionManager = new() { ResolveOnlySeededSessions = true }; + MxAccessGatewayService service = CreateService(sessionManager); + + RpcException exception = await Assert.ThrowsAsync( + async () => await service.QueryActiveAlarms( + new QueryActiveAlarmsRequest + { + SessionId = "session-missing", + AlarmFilterPrefix = "Tank01.", + }, + new RecordingStreamWriter(), + new TestServerCallContext())); + + Assert.Equal(StatusCode.NotFound, exception.StatusCode); + Assert.Contains("session-missing", exception.Status.Detail, StringComparison.Ordinal); } /// Verifies that Invoke throws InvalidArgument and does not invoke the session manager when payload is mismatched. @@ -425,9 +496,26 @@ public sealed class MxAccessGatewayServiceTests private sealed class FakeSessionManager : ISessionManager { + private readonly Dictionary seededSessions = new(StringComparer.Ordinal); + /// The session to return from OpenSessionAsync. public GatewaySession? OpenSessionResult { get; init; } + /// + /// When true, only resolves sessions that have been + /// explicitly seeded via (or ), + /// and returns false for any other id. This exercises the gateway service's own + /// missing-session handling instead of masking it with a synthesized session. + /// + public bool ResolveOnlySeededSessions { get; init; } + + /// Registers a session so resolves its id. + /// Session to register by its . + public void SeedSession(GatewaySession session) + { + seededSessions[session.SessionId] = session; + } + /// The last OpenSessionAsync request captured. public SessionOpenRequest? LastOpenRequest { get; private set; } @@ -484,6 +572,18 @@ public sealed class MxAccessGatewayServiceTests string sessionId, out GatewaySession session) { + if (seededSessions.TryGetValue(sessionId, out GatewaySession? seeded)) + { + session = seeded; + return true; + } + + if (ResolveOnlySeededSessions) + { + session = null!; + return false; + } + session = OpenSessionResult ?? CreateSession(sessionId, processId: 1234); return true; }