dotnet / dotnet/aspnetcore

[SignalR] Negative SequenceMessage.SequenceId permanently silences a connection

Open Beginner friendly
#69,283 1 comment 0 reactions 0 assignees View on GitHub
area-signalr
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 5h
Merged PRs (30d)
276

Description

**Observed on:** `release/10.0` @ 10.0.13, commit `1307f36b2990bf5c81aace7c6a1535a2be46d7ef`. I have not checked the 8.0/9.0 branches.

### Summary

`MessageBuffer.ShouldProcessMessage` validates only the upper bound of a
client-supplied `SequenceMessage.SequenceId`. A negative value is accepted and
permanently wedges the connection into dropping every subsequent message.

### Detail

`src/SignalR/common/Shared/MessageBuffer.cs:218`

```csharp
if (sequenceMessage.SequenceId > _currentReceivingSequenceId)
{
throw new InvalidOperationException("Sequence ID greater than amount of messages we've received.");
}
_currentReceivingSequenceId = sequenceMessage.SequenceId;
```

There is no lower bound. `SequenceMessage.SequenceId` is a plain `long` with no
validation in its constructor, and both protocols parse it raw — `ReadInt64` in
`MessagePackHubProtocolWorker`, `ReadAsInt64` in `JsonHubProtocol`.

A client that negotiated stateful reconnect can send `SequenceId = long.MinValue`.
`_currentReceivingSequenceId` becomes negative, and every subsequent
`HubInvocationMessage` then hits the duplicate check a few lines below:

```csharp
var currentId = _currentReceivingSequenceId;
_currentReceivingSequenceId++;
if (currentId <= _latestReceivedSequenceId)
{
return false; // dropped as a duplicate
}
```

`_latestReceivedSequenceId` is initialised to `long.MinValue`, so the very first
comparison is true and stays true. Recovery would require ~2^63 increments. The
connection remains open, passes keep-alives, and silently discards everything the
client sends (`DefaultHubDispatcher.cs:161` -> `Log.DroppingMessage`).

### Impact

Self-inflicted only. `_currentReceivingSequenceId` and `_latestReceivedSequenceId`
live in the per-connection `MessageBuffer`; there is no cross-connection or
cross-user state, so a client can only silence itself. Filing as a robustness /
input-validation gap, not a security issue.

### Suggested fix

Reject non-positive sequence IDs alongside the existing upper-bound check —
message IDs start at 1 and increment by 1, per the comment at MessageBuffer.cs:46:

```csharp
if (sequenceMessage.SequenceId <= 0)
{
throw new InvalidOperationException("Sequence ID must be positive.");
}
```

### Repro sketch

Connect with `WithStatefulReconnect()`, send a `SequenceMessage` with
`SequenceId = long.MinValue`, then invoke any hub method and observe it is never
dispatched while the connection stays alive.

### Verification status

This is a source-level trace, not an executed repro. I wrote the test above against
`MessageBufferTests` but could not run it: building this servicing branch requires
the runtime toolset `10.0.13-servicing.26454.107`, which is not available from the
public feed, so `restore.sh` cannot complete outside Microsoft-internal
infrastructure. Flagging that explicitly rather than implying I observed it running.

Contributor guide

Open the contributing guide

Research direction

Start with src/SignalR/common/Shared/MessageBuffer.cs around ShouldProcessMessage and the MessageBufferTests regression test mentioned in the issue. Trace the SequenceMessage handling and verify that invalid non-positive IDs are rejected while valid messages continue to dispatch; use the issue's stateful reconnect repro sketch for behavioral verification if the required runtime toolset is available.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
api, backend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.