From 70c74e7cb87f57b503d9c809960546db88264777 Mon Sep 17 00:00:00 2001 From: MrAlaskan <61153944+MrAlaskan@users.noreply.github.com> Date: Sun, 19 Jul 2026 12:23:52 +0800 Subject: [PATCH] fix: scope Cancel request matching to the current session (#4023) ### Summary This PR scopes `Cancel` request matching to the caller's session and adds regression tests that verify a request handle can no longer cancel work that belongs to another session. ### Problem According to OPC UA Part 4, `Cancel` applies to a request handle from the same session that issued the original request. The server should not use a client-supplied `requestHandle` to match outstanding requests across all sessions. Previously, `StandardServer.CancelAsync()` validated the caller's session but then forwarded only the raw `requestHandle` to `RequestManager.CancelRequests()`. `RequestManager` iterated the global outstanding-request table and canceled every request whose `ClientHandle` matched, without checking the owning session. Since request handles are only session-scoped and different sessions can legitimately reuse the same value, one session could cancel another session's in-flight request. ### Changes - Pass the validated caller session id from `StandardServer.CancelAsync()` into `RequestManager.CancelRequests()`. - Restrict `RequestManager.CancelRequests()` to requests whose `SessionId` and `ClientHandle` both match the `Cancel` request. - Update existing request-manager and publish-queue tests to pass the session id and add a regression test that proves matching request handles in different sessions no longer interfere with each other. --- src/Opc.Ua.Server/Server/RequestManager.cs | 5 ++- src/Opc.Ua.Server/Server/StandardServer.cs | 5 ++- .../RequestManagerTests.cs | 45 +++++++++++++++++-- .../SessionPublishQueueTests.cs | 8 +++- 4 files changed, 55 insertions(+), 8 deletions(-) diff --git a/src/Opc.Ua.Server/Server/RequestManager.cs b/src/Opc.Ua.Server/Server/RequestManager.cs index 297a506fe5..ce8a74b843 100644 --- a/src/Opc.Ua.Server/Server/RequestManager.cs +++ b/src/Opc.Ua.Server/Server/RequestManager.cs @@ -171,7 +171,7 @@ namespace Opc.Ua.Server /// /// Called when the client wishes to cancel one or more requests. /// - public void CancelRequests(uint requestHandle, out uint cancelCount) + public void CancelRequests(NodeId sessionId, uint requestHandle, out uint cancelCount) { var cancelledRequests = new List(); @@ -180,7 +180,8 @@ namespace Opc.Ua.Server { foreach (OperationContext request in m_requests.Values) { - if (request.ClientHandle == requestHandle) + if (request.SessionId == sessionId && + request.ClientHandle == requestHandle) { request.RequestLifetime.TryCancel(StatusCodes.BadRequestCancelledByRequest); cancelledRequests.Add(request.RequestId); diff --git a/src/Opc.Ua.Server/Server/StandardServer.cs b/src/Opc.Ua.Server/Server/StandardServer.cs index e63b7e32a9..89ad0396f0 100644 --- a/src/Opc.Ua.Server/Server/StandardServer.cs +++ b/src/Opc.Ua.Server/Server/StandardServer.cs @@ -1101,7 +1101,10 @@ namespace Opc.Ua.Server CancelResponse response; try { - ServerInternal.RequestManager.CancelRequests(requestHandle, out uint cancelCount); + ServerInternal.RequestManager.CancelRequests( + context.SessionId, + requestHandle, + out uint cancelCount); response = new CancelResponse { diff --git a/tests/Opc.Ua.Server.Tests/RequestManagerTests.cs b/tests/Opc.Ua.Server.Tests/RequestManagerTests.cs index dc6196a575..a5979720a6 100644 --- a/tests/Opc.Ua.Server.Tests/RequestManagerTests.cs +++ b/tests/Opc.Ua.Server.Tests/RequestManagerTests.cs @@ -99,7 +99,7 @@ namespace Opc.Ua.Server.Tests }; // Act - m_requestManager.CancelRequests(42, out uint cancelCount); + m_requestManager.CancelRequests(context.SessionId, 42, out uint cancelCount); // Assert Assert.That(cancelCount, Is.EqualTo(1)); @@ -123,7 +123,7 @@ namespace Opc.Ua.Server.Tests uint cancelCount = 0; Assert.DoesNotThrow( - () => m_requestManager.CancelRequests(requestHandle, out cancelCount)); + () => m_requestManager.CancelRequests(context.SessionId, requestHandle, out cancelCount)); Assert.That(cancelCount, Is.EqualTo(1)); Assert.That( @@ -131,6 +131,45 @@ namespace Opc.Ua.Server.Tests Is.EqualTo(StatusCodes.BadRequestCancelledByRequest)); } + [Test] + public void CancelRequestsDoesNotCancelMatchingHandleFromDifferentSession() + { + var cancellingSession = new Mock(); + cancellingSession.Setup(s => s.Id).Returns(new NodeId(1)); + + var otherSession = new Mock(); + otherSession.Setup(s => s.Id).Returns(new NodeId(2)); + + const uint requestHandle = 42; + using var ownRequestLifetime = new RequestLifetime(); + using var otherRequestLifetime = new RequestLifetime(); + + var ownContext = new OperationContext( + new RequestHeader { RequestHandle = requestHandle }, + null, + RequestType.Read, + ownRequestLifetime, + cancellingSession.Object); + var otherContext = new OperationContext( + new RequestHeader { RequestHandle = requestHandle }, + null, + RequestType.Read, + otherRequestLifetime, + otherSession.Object); + + m_requestManager.RequestReceived(ownContext); + m_requestManager.RequestReceived(otherContext); + + m_requestManager.CancelRequests(cancellingSession.Object.Id, requestHandle, out uint cancelCount); + + Assert.That(cancelCount, Is.EqualTo(1)); + Assert.That(ownRequestLifetime.CancellationToken.IsCancellationRequested, Is.True); + Assert.That(otherRequestLifetime.CancellationToken.IsCancellationRequested, Is.False); + Assert.That( + otherContext.OperationStatus.Code, + Is.EqualTo(StatusCodes.Good)); + } + [Test] public void RequestCompletedRemovesRequestAndCompletesLifetime() { @@ -154,7 +193,7 @@ namespace Opc.Ua.Server.Tests // Assert // To ensure it is removed, cancelling it will yield 0 count - m_requestManager.CancelRequests(42, out uint cancelCount); + m_requestManager.CancelRequests(context.SessionId, 42, out uint cancelCount); Assert.That(cancelCount, Is.Zero); // Assert that lifetime is completed (disposed), which means TryCancel returns false Assert.That(requestLifetime.TryCancel(StatusCodes.BadTimeout), Is.False); diff --git a/tests/Opc.Ua.Server.Tests/SessionPublishQueueTests.cs b/tests/Opc.Ua.Server.Tests/SessionPublishQueueTests.cs index dcfe960f77..1b03b8fee1 100644 --- a/tests/Opc.Ua.Server.Tests/SessionPublishQueueTests.cs +++ b/tests/Opc.Ua.Server.Tests/SessionPublishQueueTests.cs @@ -161,13 +161,17 @@ namespace Opc.Ua.Server.Tests subMock.Setup(s => s.Id).Returns(1); queue.Add(subMock.Object); // added but not ready, so the request must park + var sessionMock = new Mock(); + sessionMock.Setup(s => s.Id).Returns(new NodeId(1)); + const uint requestHandle = 77; using var requestLifetime = new RequestLifetime(); var context = new OperationContext( new RequestHeader { RequestHandle = requestHandle }, null, RequestType.Publish, - requestLifetime); + requestLifetime, + sessionMock.Object); requestManager.RequestReceived(context); var sink = new TestParkSink(); @@ -181,7 +185,7 @@ namespace Opc.Ua.Server.Tests "A parked request releases its processing worker at the park point."); // A Cancel service call carrying the Publish request handle. - requestManager.CancelRequests(requestHandle, out uint cancelCount); + requestManager.CancelRequests(context.SessionId, requestHandle, out uint cancelCount); Assert.That(cancelCount, Is.EqualTo(1), "The Cancel service should match the parked Publish request."); Assert.CatchAsync(() => task);