Skip to content

Enforce HTTP request Content-Length correctness #62541

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
merged 4 commits into from
Dec 10, 2021
Merged

Enforce HTTP request Content-Length correctness #62541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9cd239a
Enforce HTTP request Content-Length correctness
MihaZupan Dec 8, 2021
f75a004
Skip test on Browser
MihaZupan Dec 8, 2021
0217e34
Test that subsequent HTTP/1.1 requests can still succeed on a new con…
MihaZupan Dec 8, 2021
eb8d21e
Be consistent about exception nesting
MihaZupan Dec 9, 2021
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
3 changes: 3 additions & 0 deletions src/libraries/System.Net.Http/src/Resources/Strings.resx
View file
Open in desktop
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
21 changes: 20 additions & 1 deletion ...raries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthWriteStream.cs
View file
Open in desktop
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
19 changes: 17 additions & 2 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
View file
Open in desktop
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
11 changes: 9 additions & 2 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
View file
Open in desktop
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);
Copy link
Member Author

@MihaZupan MihaZupan Dec 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care about this being an IOException?
It's technically a combination of IO + wrong headers.

Currently you would get

new HttpRequestException(
    net_http_content_stream_copy_error,
    new IOException(
        net_http_content_write_larger_than_content_length,
        new HttpRequestException(
            net_http_content_write_larger_than_content_length)))

after this PR

new HttpRequestException(
    net_http_content_write_larger_than_content_length)

(this extends to the rest of the exceptions we are throwing in this PR)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the change here is to provide the same exception chain as with H/2 and HTTP/1.1? If so, I don't see a reason against.

Copy link
Member

@halter73 halter73 Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand not wanting to change the outer exception type at this point, but why wouldn't this cause an InvalidOperationException? That's what gets thrown by Kestrel if you try to write too much to the response stream after setting a Content-Length header. To me, it indicates a bug in the program.

Edit: On second thought, why not change the outer exception type? I hope nothing is relying on HttpClient throwing an HttpRequestException if they give it a too-large request body.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not change the outer exception type? I hope nothing is relying on HttpClient throwing an HttpRequestException

HttpClient will always try to throw either a TaskCanceledException or an HttpRequestException.
Aside from initial argument validation, If something else gets thrown somewhere, we'll wrap it in HttpRequestException.

Sending too much or too little is the same kind of issue - the header you gave us doesn't match the content. Ideally, the user should see a similar kind of error.

why wouldn't this cause an InvalidOperationException? That's what gets thrown by Kestrel if you try to write too much to the response stream

Most HttpClient users won't actually do the WriteAsync that goes over the limit themselves. They will use the built-in HttpContent types.

var content = new StreamContent(myStream);
content.Headers.ContentLength = someContentLength;

await client.PostAsync("foo", content);

I wouldn't expect this to throw InvalidOperationException as a user. It's not something I would consider an invalid operation, but more an argument/input exception.
Similarly, I wouldn't expect this to throw something different if my content length calculation was off in the other direction.

}
_requestContentLengthRemaining -= buffer.Length;

Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
View file
Open in desktop
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
66 changes: 66 additions & 0 deletions src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3365,4 +3365,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;
}
}