Skip to content

Commit 70c74e7

Browse files
authored
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.
1 parent 2d55126 commit 70c74e7

4 files changed

Lines changed: 55 additions & 8 deletions

File tree

src/Opc.Ua.Server/Server/RequestManager.cs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public void RequestCompleted(OperationContext context)
171171
/// <summary>
172172
/// Called when the client wishes to cancel one or more requests.
173173
/// </summary>
174-
public void CancelRequests(uint requestHandle, out uint cancelCount)
174+
public void CancelRequests(NodeId sessionId, uint requestHandle, out uint cancelCount)
175175
{
176176
var cancelledRequests = new List<uint>();
177177

@@ -180,7 +180,8 @@ public void CancelRequests(uint requestHandle, out uint cancelCount)
180180
{
181181
foreach (OperationContext request in m_requests.Values)
182182
{
183-
if (request.ClientHandle == requestHandle)
183+
if (request.SessionId == sessionId &&
184+
request.ClientHandle == requestHandle)
184185
{
185186
request.RequestLifetime.TryCancel(StatusCodes.BadRequestCancelledByRequest);
186187
cancelledRequests.Add(request.RequestId);

src/Opc.Ua.Server/Server/StandardServer.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1101,7 +1101,10 @@ public override async ValueTask<CancelResponse> CancelAsync(
11011101
CancelResponse response;
11021102
try
11031103
{
1104-
ServerInternal.RequestManager.CancelRequests(requestHandle, out uint cancelCount);
1104+
ServerInternal.RequestManager.CancelRequests(
1105+
context.SessionId,
1106+
requestHandle,
1107+
out uint cancelCount);
11051108

11061109
response = new CancelResponse
11071110
{

tests/Opc.Ua.Server.Tests/RequestManagerTests.cs

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ public void CancelRequestsCancelsMatchingRequestsAndFiresEvent()
9999
};
100100

101101
// Act
102-
m_requestManager.CancelRequests(42, out uint cancelCount);
102+
m_requestManager.CancelRequests(context.SessionId, 42, out uint cancelCount);
103103

104104
// Assert
105105
Assert.That(cancelCount, Is.EqualTo(1));
@@ -123,14 +123,53 @@ public void CancelRequestsShouldCancelActivateSessionRequestWithoutSession()
123123

124124
uint cancelCount = 0;
125125
Assert.DoesNotThrow(
126-
() => m_requestManager.CancelRequests(requestHandle, out cancelCount));
126+
() => m_requestManager.CancelRequests(context.SessionId, requestHandle, out cancelCount));
127127

128128
Assert.That(cancelCount, Is.EqualTo(1));
129129
Assert.That(
130130
context.OperationStatus.Code,
131131
Is.EqualTo(StatusCodes.BadRequestCancelledByRequest));
132132
}
133133

134+
[Test]
135+
public void CancelRequestsDoesNotCancelMatchingHandleFromDifferentSession()
136+
{
137+
var cancellingSession = new Mock<ISession>();
138+
cancellingSession.Setup(s => s.Id).Returns(new NodeId(1));
139+
140+
var otherSession = new Mock<ISession>();
141+
otherSession.Setup(s => s.Id).Returns(new NodeId(2));
142+
143+
const uint requestHandle = 42;
144+
using var ownRequestLifetime = new RequestLifetime();
145+
using var otherRequestLifetime = new RequestLifetime();
146+
147+
var ownContext = new OperationContext(
148+
new RequestHeader { RequestHandle = requestHandle },
149+
null,
150+
RequestType.Read,
151+
ownRequestLifetime,
152+
cancellingSession.Object);
153+
var otherContext = new OperationContext(
154+
new RequestHeader { RequestHandle = requestHandle },
155+
null,
156+
RequestType.Read,
157+
otherRequestLifetime,
158+
otherSession.Object);
159+
160+
m_requestManager.RequestReceived(ownContext);
161+
m_requestManager.RequestReceived(otherContext);
162+
163+
m_requestManager.CancelRequests(cancellingSession.Object.Id, requestHandle, out uint cancelCount);
164+
165+
Assert.That(cancelCount, Is.EqualTo(1));
166+
Assert.That(ownRequestLifetime.CancellationToken.IsCancellationRequested, Is.True);
167+
Assert.That(otherRequestLifetime.CancellationToken.IsCancellationRequested, Is.False);
168+
Assert.That(
169+
otherContext.OperationStatus.Code,
170+
Is.EqualTo(StatusCodes.Good));
171+
}
172+
134173
[Test]
135174
public void RequestCompletedRemovesRequestAndCompletesLifetime()
136175
{
@@ -154,7 +193,7 @@ public void RequestCompletedRemovesRequestAndCompletesLifetime()
154193

155194
// Assert
156195
// To ensure it is removed, cancelling it will yield 0 count
157-
m_requestManager.CancelRequests(42, out uint cancelCount);
196+
m_requestManager.CancelRequests(context.SessionId, 42, out uint cancelCount);
158197
Assert.That(cancelCount, Is.Zero);
159198
// Assert that lifetime is completed (disposed), which means TryCancel returns false
160199
Assert.That(requestLifetime.TryCancel(StatusCodes.BadTimeout), Is.False);

tests/Opc.Ua.Server.Tests/SessionPublishQueueTests.cs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,13 +161,17 @@ public void PublishAsync_WhenParked_CancelServiceCancelsRequest()
161161
subMock.Setup(s => s.Id).Returns(1);
162162
queue.Add(subMock.Object); // added but not ready, so the request must park
163163

164+
var sessionMock = new Mock<ISession>();
165+
sessionMock.Setup(s => s.Id).Returns(new NodeId(1));
166+
164167
const uint requestHandle = 77;
165168
using var requestLifetime = new RequestLifetime();
166169
var context = new OperationContext(
167170
new RequestHeader { RequestHandle = requestHandle },
168171
null,
169172
RequestType.Publish,
170-
requestLifetime);
173+
requestLifetime,
174+
sessionMock.Object);
171175
requestManager.RequestReceived(context);
172176

173177
var sink = new TestParkSink();
@@ -181,7 +185,7 @@ public void PublishAsync_WhenParked_CancelServiceCancelsRequest()
181185
"A parked request releases its processing worker at the park point.");
182186

183187
// A Cancel service call carrying the Publish request handle.
184-
requestManager.CancelRequests(requestHandle, out uint cancelCount);
188+
requestManager.CancelRequests(context.SessionId, requestHandle, out uint cancelCount);
185189

186190
Assert.That(cancelCount, Is.EqualTo(1), "The Cancel service should match the parked Publish request.");
187191
Assert.CatchAsync<OperationCanceledException>(() => task);

0 commit comments

Comments
 (0)