Skip to content

Commit

Permalink
Enforce HTTP request Content-Length correctness (#62541)
Browse files Browse the repository at this point in the history
* Enforce HTTP request Content-Length correctness

* Skip test on Browser

* Test that subsequent HTTP/1.1 requests can still succeed on a new connection

* Be consistent about exception nesting
  • Loading branch information
MihaZupan authored Dec 10, 2021
1 parent 8965454 commit 7b2925b
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 6 deletions.
3 changes: 3 additions & 0 deletions src/libraries/System.Net.Http/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,9 @@
<data name="net_http_invalid_response" xml:space="preserve">
<value>The server returned an invalid or unrecognized response.</value>
</data>
<data name="net_http_request_content_length_mismatch" xml:space="preserve">
<value>Sent {0} request content bytes, but Content-Length promised {1}.</value>
</data>
<data name="net_http_invalid_response_premature_eof" xml:space="preserve">
<value>The response ended prematurely.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,23 @@ internal sealed partial class HttpConnection : IDisposable
{
private sealed class ContentLengthWriteStream : HttpContentWriteStream
{
public ContentLengthWriteStream(HttpConnection connection) : base(connection)
private readonly long _contentLength;

public ContentLengthWriteStream(HttpConnection connection, long contentLength)
: base(connection)
{
_contentLength = contentLength;
}

public override void Write(ReadOnlySpan<byte> buffer)
{
BytesWritten += buffer.Length;

if (BytesWritten > _contentLength)
{
throw new HttpRequestException(SR.net_http_content_write_larger_than_content_length);
}

// Have the connection write the data, skipping the buffer. Importantly, this will
// force a flush of anything already in the buffer, i.e. any remaining request headers
// that are still buffered.
Expand All @@ -31,6 +40,11 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo
{
BytesWritten += buffer.Length;

if (BytesWritten > _contentLength)
{
return ValueTask.FromException(new HttpRequestException(SR.net_http_content_write_larger_than_content_length));
}

// Have the connection write the data, skipping the buffer. Importantly, this will
// force a flush of anything already in the buffer, i.e. any remaining request headers
// that are still buffered.
Expand All @@ -41,6 +55,11 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo

public override Task FinishAsync(bool async)
{
if (BytesWritten != _contentLength)
{
return Task.FromException(new HttpRequestException(SR.Format(SR.net_http_request_content_length_mismatch, BytesWritten, _contentLength)));
}

_connection = null;
return Task.CompletedTask;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ public async Task SendRequestBodyAsync(CancellationToken cancellationToken)

if (sendRequestContent)
{
using var writeStream = new Http2WriteStream(this);
using var writeStream = new Http2WriteStream(this, _request.Content.Headers.ContentLength.GetValueOrDefault(-1));

if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.RequestContentStart();

Expand All @@ -214,6 +214,12 @@ public async Task SendRequestBodyAsync(CancellationToken cancellationToken)
await vt.ConfigureAwait(false);
}

if (writeStream.BytesWritten < writeStream.ContentLength)
{
// The number of bytes we actually sent doesn't match the advertised Content-Length
throw new HttpRequestException(SR.Format(SR.net_http_request_content_length_mismatch, writeStream.BytesWritten, writeStream.ContentLength));
}

if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.RequestContentStop(writeStream.BytesWritten);
}

Expand Down Expand Up @@ -1506,10 +1512,14 @@ private sealed class Http2WriteStream : HttpBaseStream

public long BytesWritten { get; private set; }

public Http2WriteStream(Http2Stream http2Stream)
public long ContentLength { get; private set; }

public Http2WriteStream(Http2Stream http2Stream, long contentLength)
{
Debug.Assert(http2Stream != null);
Debug.Assert(contentLength >= -1);
_http2Stream = http2Stream;
ContentLength = contentLength;
}

protected override void Dispose(bool disposing)
Expand All @@ -1534,6 +1544,11 @@ public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationTo
{
BytesWritten += buffer.Length;

if ((ulong)BytesWritten > (ulong)ContentLength) // If ContentLength == -1, this will always be false
{
return ValueTask.FromException(new HttpRequestException(SR.net_http_content_write_larger_than_content_length));
}

Http2Stream? http2Stream = _http2Stream;

if (http2Stream == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,14 @@ private async Task SendContentAsync(HttpContent content, CancellationToken cance
await content.CopyToAsync(writeStream, null, cancellationToken).ConfigureAwait(false);
}

if (_requestContentLengthRemaining > 0)
{
// The number of bytes we actually sent doesn't match the advertised Content-Length
long contentLength = content.Headers.ContentLength.GetValueOrDefault();
long sent = contentLength - _requestContentLengthRemaining;
throw new HttpRequestException(SR.Format(SR.net_http_request_content_length_mismatch, sent, contentLength));
}

// Set to 0 to recognize that the whole request body has been sent and therefore there's no need to abort write side in case of a premature disposal.
_requestContentLengthRemaining = 0;

Expand Down Expand Up @@ -414,8 +422,7 @@ private async ValueTask WriteRequestContentAsync(ReadOnlyMemory<byte> buffer, Ca

if (buffer.Length > _requestContentLengthRemaining)
{
string msg = SR.net_http_content_write_larger_than_content_length;
throw new IOException(msg, new HttpRequestException(msg));
throw new HttpRequestException(SR.net_http_content_write_larger_than_content_length);
}
_requestContentLengthRemaining -= buffer.Length;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -860,10 +860,11 @@ private bool MapSendException(Exception exception, CancellationToken cancellatio

private HttpContentWriteStream CreateRequestContentStream(HttpRequestMessage request)
{
Debug.Assert(request.Content is not null);
bool requestTransferEncodingChunked = request.HasHeaders && request.Headers.TransferEncodingChunked == true;
HttpContentWriteStream requestContentStream = requestTransferEncodingChunked ? (HttpContentWriteStream)
new ChunkedEncodingWriteStream(this) :
new ContentLengthWriteStream(this);
new ContentLengthWriteStream(this, request.Content.Headers.ContentLength.GetValueOrDefault());
return requestContentStream;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3361,4 +3361,70 @@ public sealed class SocketsHttpHandler_RequestValidationTest_Sync : SocketsHttpH
{
protected override bool TestAsync => false;
}

[ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))]
public abstract class SocketsHttpHandler_RequestContentLengthMismatchTest : HttpClientHandlerTestBase
{
public SocketsHttpHandler_RequestContentLengthMismatchTest(ITestOutputHelper output) : base(output) { }

[Theory]
[InlineData(0, 1)]
[InlineData(1, 0)]
[InlineData(1, 2)]
[InlineData(2, 1)]
public async Task ContentLength_DoesNotMatchRequestContentLength_Throws(int contentLength, int bytesSent)
{
await LoopbackServerFactory.CreateClientAndServerAsync(async uri =>
{
using var client = CreateHttpClient();

var content = new ByteArrayContent(new byte[bytesSent]);
content.Headers.ContentLength = contentLength;

Exception ex = await Assert.ThrowsAsync<HttpRequestException>(() => client.PostAsync(uri, content));
Assert.Contains("Content-Length", ex.Message);

if (UseVersion.Major == 1)
{
await client.GetStringAsync(uri).WaitAsync(TestHelper.PassingTestTimeout);
}
},
async server =>
{
try
{
await server.HandleRequestAsync();
}
catch { }

// On HTTP/1.x, an exception being thrown while sending the request content will result in the connection being closed.
// This test is ensuring that a subsequent request can succeed on a new connection.
if (UseVersion.Major == 1)
{
await server.HandleRequestAsync().WaitAsync(TestHelper.PassingTestTimeout);
}
});
}
}

public sealed class SocketsHttpHandler_RequestContentLengthMismatchTest_Http11 : SocketsHttpHandler_RequestContentLengthMismatchTest
{
public SocketsHttpHandler_RequestContentLengthMismatchTest_Http11(ITestOutputHelper output) : base(output) { }
protected override Version UseVersion => HttpVersion.Version11;
}

[ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.SupportsAlpn))]
public sealed class SocketsHttpHandler_RequestContentLengthMismatchTest_Http2 : SocketsHttpHandler_RequestContentLengthMismatchTest
{
public SocketsHttpHandler_RequestContentLengthMismatchTest_Http2(ITestOutputHelper output) : base(output) { }
protected override Version UseVersion => HttpVersion.Version20;
}

[ConditionalClass(typeof(HttpClientHandlerTestBase), nameof(IsMsQuicSupported))]
public sealed class SocketsHttpHandler_RequestContentLengthMismatchTest_Http3 : SocketsHttpHandler_RequestContentLengthMismatchTest
{
public SocketsHttpHandler_RequestContentLengthMismatchTest_Http3(ITestOutputHelper output) : base(output) { }
protected override Version UseVersion => HttpVersion.Version30;
protected override QuicImplementationProvider UseQuicImplementationProvider => QuicImplementationProviders.MsQuic;
}
}

0 comments on commit 7b2925b

Please sign in to comment.