[SignalR] Negative SequenceMessage.SequenceId permanently silences a connection
- 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
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