Skip to content

Fix PipeReader contract violation in DefaultHttpRequestParser #9761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tobias-tengler merged 2 commits into from
May 21, 2026
Merged

Fix PipeReader contract violation in DefaultHttpRequestParser #9761

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
feb860c
Fix PipeReader contract violation in DefaultHttpRequestParser
tobias-tengler May 21, 2026
dc85205
Cleanup
tobias-tengler May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 66 additions & 58 deletions src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Parsers/DefaultHttpRequestParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,48 +51,40 @@ public async ValueTask<GraphQLRequest[]> ParseRequestAsync(
{
try
{
ReadResult result;
var result = await ReadFullBodyAsync(requestBody, cancellationToken);

do
// After the loop, the final ReadAsync has not yet been advanced. The pipe
// contract requires every successful ReadAsync to be followed by an AdvanceTo,
// otherwise the next ReadAsync (e.g. Kestrel draining the request body)
// throws "Reading is already in progress".
try
{
result = await requestBody.ReadAsync(cancellationToken);

if (result.Buffer.Length > _maxRequestSize)
if (result.IsCanceled)
{
requestBody.AdvanceTo(result.Buffer.End);
throw new GraphQLRequestException("Request size exceeds maximum allowed size.");
throw new OperationCanceledException();
}

if (!result.IsCompleted && !result.IsCanceled)
{
// We tell the pipe that we've examined everything but consumed nothing yet.
requestBody.AdvanceTo(result.Buffer.Start, result.Buffer.End);
}
}
while (result is { IsCompleted: false, IsCanceled: false });
var requestParser = new Utf8GraphQLRequestParser(
_parserOptions,
_documentCache,
_documentHashProvider,
skipDocumentBody);

if (result.IsCanceled)
return requestParser.Parse(result.Buffer);
}
finally
{
throw new OperationCanceledException();
requestBody.AdvanceTo(result.Buffer.End);
}

var requestParser = new Utf8GraphQLRequestParser(
_parserOptions,
_documentCache,
_documentHashProvider,
skipDocumentBody);

var requests = requestParser.Parse(result.Buffer);

// Mark all data as consumed
requestBody.AdvanceTo(result.Buffer.End);

return requests;
}
catch (GraphQLRequestException)
{
throw;
}
catch (OperationCanceledException)
{
throw;
}
catch (SyntaxException ex)
{
throw DefaultHttpRequestParser_SyntaxError(ex);
Expand All @@ -118,48 +110,40 @@ public async ValueTask<GraphQLRequest> ParsePersistedOperationRequestAsync(

try
{
ReadResult result;
var result = await ReadFullBodyAsync(requestBody, cancellationToken);

do
// After the loop, the final ReadAsync has not yet been advanced. The pipe
// contract requires every successful ReadAsync to be followed by an AdvanceTo,
// otherwise the next ReadAsync (e.g. Kestrel draining the request body)
// throws "Reading is already in progress".
try
{
result = await requestBody.ReadAsync(cancellationToken);

if (result.Buffer.Length > _maxRequestSize)
if (result.IsCanceled)
{
requestBody.AdvanceTo(result.Buffer.End);
throw new GraphQLRequestException("Request size exceeds maximum allowed size.");
throw new OperationCanceledException();
}

if (!result.IsCompleted && !result.IsCanceled)
{
// We tell the pipe that we've examined everything but consumed nothing yet.
requestBody.AdvanceTo(result.Buffer.Start, result.Buffer.End);
}
}
while (result is { IsCompleted: false, IsCanceled: false });
var requestParser = new Utf8GraphQLRequestParser(
_parserOptions,
_documentCache,
_documentHashProvider,
skipDocumentBody);

if (result.IsCanceled)
return requestParser.ParsePersistedOperation(parsedDocumentId, operationName, result.Buffer);
}
finally
{
throw new OperationCanceledException();
requestBody.AdvanceTo(result.Buffer.End);
}

var requestParser = new Utf8GraphQLRequestParser(
_parserOptions,
_documentCache,
_documentHashProvider,
skipDocumentBody);

var request = requestParser.ParsePersistedOperation(parsedDocumentId, operationName, result.Buffer);

// Mark all data as consumed
requestBody.AdvanceTo(result.Buffer.End);

return request;
}
catch (GraphQLRequestException)
{
throw;
}
catch (OperationCanceledException)
{
throw;
}
catch (SyntaxException ex)
{
throw DefaultHttpRequestParser_SyntaxError(ex);
Expand All @@ -170,6 +154,30 @@ public async ValueTask<GraphQLRequest> ParsePersistedOperationRequestAsync(
}
}

private async ValueTask<ReadResult> ReadFullBodyAsync(
PipeReader requestBody,
CancellationToken cancellationToken)
{
while (true)
{
var result = await requestBody.ReadAsync(cancellationToken);

if (result.Buffer.Length > _maxRequestSize)
{
requestBody.AdvanceTo(result.Buffer.End);
throw new GraphQLRequestException("Request size exceeds maximum allowed size.");
}

if (result.IsCompleted || result.IsCanceled)
{
return result;
}

// We tell the pipe that we've examined everything but consumed nothing yet.
requestBody.AdvanceTo(result.Buffer.Start, result.Buffer.End);
}
}

public GraphQLRequest ParseRequestFromParams(IQueryCollection parameters, bool skipDocumentBody = false)
{
// next, we deserialize the GET request with the query request builder ...
Expand Down
85 changes: 85 additions & 0 deletions ...Chocolate/AspNetCore/test/AspNetCore.Tests/Serialization/DefaultHttpRequestParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,91 @@ void Parse()
Assert.Equal("The operation id has an invalid format.", ex.Message);
}

[Fact]
public async Task ParseRequestAsync_Should_AdvancePipe_When_SyntaxError()
{
// arrange
var pipe = new Pipe();
await pipe.Writer.WriteAsync(Encoding.UTF8.GetBytes("{ invalid json"));
await pipe.Writer.CompleteAsync();

var parser = new DefaultHttpRequestParser(
new DefaultDocumentCache(),
new Sha256DocumentHashProvider(),
256,
ParserOptions.Default);

// act
await Assert.ThrowsAsync<GraphQLRequestException>(async () =>
await parser.ParseRequestAsync(
pipe.Reader,
skipDocumentBody: false,
CancellationToken.None));

// assert
// The pipe must be left in a state where a subsequent ReadAsync
// does not throw "Reading is already in progress".
var result = await pipe.Reader.ReadAsync();
Assert.True(result.IsCompleted);
}
Comment thread
tobias-tengler marked this conversation as resolved.
Resolved

[Fact]
public async Task ParseRequestAsync_Should_AdvancePipe_When_PipeCanceled()
{
// arrange
var pipe = new Pipe();
await pipe.Writer.WriteAsync(Encoding.UTF8.GetBytes("{ \"id\": \"abc\" }"));
pipe.Reader.CancelPendingRead();

var parser = new DefaultHttpRequestParser(
new DefaultDocumentCache(),
new Sha256DocumentHashProvider(),
256,
ParserOptions.Default);

// act
await Assert.ThrowsAsync<OperationCanceledException>(async () =>
await parser.ParseRequestAsync(
pipe.Reader,
skipDocumentBody: false,
CancellationToken.None));

// assert
// The pipe must be left in a state where a subsequent ReadAsync
// does not throw "Reading is already in progress".
await pipe.Writer.CompleteAsync();
var result = await pipe.Reader.ReadAsync();
Assert.True(result.IsCompleted);
}
Comment thread
tobias-tengler marked this conversation as resolved.
Resolved

[Fact]
public async Task ParsePersistedOperationRequestAsync_Should_AdvancePipe_When_SyntaxError()
{
// arrange
var pipe = new Pipe();
await pipe.Writer.WriteAsync(Encoding.UTF8.GetBytes("{ invalid json"));
await pipe.Writer.CompleteAsync();

var parser = new DefaultHttpRequestParser(
new DefaultDocumentCache(),
new Sha256DocumentHashProvider(),
256,
ParserOptions.Default);

// act
await Assert.ThrowsAsync<GraphQLRequestException>(async () =>
await parser.ParsePersistedOperationRequestAsync(
"abc1213_5164-ABC-123",
operationName: null,
pipe.Reader,
skipDocumentBody: false,
CancellationToken.None));

// assert
var result = await pipe.Reader.ReadAsync();
Assert.True(result.IsCompleted);
}
Comment thread
tobias-tengler marked this conversation as resolved.
Resolved

public sealed class MockQueryParams(string id) : IQueryCollection
{
public IEnumerator<KeyValuePair<string, StringValues>> GetEnumerator()
Expand Down
Loading