From ea7e53cab007ded127e0e4b91bd0642f40d78dea Mon Sep 17 00:00:00 2001 From: "Chris Ross (ASP.NET)" Date: Thu, 2 Aug 2018 16:04:21 -0700 Subject: [PATCH] Send Resets for unhandled app exceptions #2733 --- src/Kestrel.Core/CoreStrings.resx | 3 + .../Internal/Http/HttpProtocol.cs | 9 +- .../Internal/Http2/Http2Stream.cs | 10 +- .../Properties/CoreStrings.Designer.cs | 14 ++ test/Kestrel.Core.Tests/Http2StreamTests.cs | 232 +++++++++++++++++- 5 files changed, 263 insertions(+), 5 deletions(-) diff --git a/src/Kestrel.Core/CoreStrings.resx b/src/Kestrel.Core/CoreStrings.resx index 66048b1b15..eb30b393f4 100644 --- a/src/Kestrel.Core/CoreStrings.resx +++ b/src/Kestrel.Core/CoreStrings.resx @@ -563,4 +563,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l More data received than specified in the Content-Length header. + + An error occured after the response headers were sent, a reset is being sent. + \ No newline at end of file diff --git a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs index 62aa16ef64..6acbc0e378 100644 --- a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs +++ b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs @@ -1032,8 +1032,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { if (HasResponseStarted) { - // We can no longer change the response, so we simply close the connection. - _keepAlive = false; + ErrorAfterResponseStarted(); return Task.CompletedTask; } @@ -1058,6 +1057,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http return WriteSuffix(); } + protected virtual void ErrorAfterResponseStarted() + { + // We can no longer change the response, so we simply close the connection. + _keepAlive = false; + } + [MethodImpl(MethodImplOptions.NoInlining)] private async Task ProduceEndAwaited() { diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index 48699e84ae..464075cbd2 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -362,10 +362,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 AbortCore(abortReason); } + protected override void ErrorAfterResponseStarted() + { + // We can no longer change the response, send a Reset instead. + base.ErrorAfterResponseStarted(); + var abortReason = new ConnectionAbortedException(CoreStrings.Http2StreamErrorAfterHeaders); + ResetAndAbort(abortReason, Http2ErrorCode.INTERNAL_ERROR); + } + protected override void ApplicationAbort() { var abortReason = new ConnectionAbortedException(CoreStrings.ConnectionAbortedByApplication); - ResetAndAbort(abortReason, Http2ErrorCode.CANCEL); + ResetAndAbort(abortReason, Http2ErrorCode.INTERNAL_ERROR); } private void ResetAndAbort(ConnectionAbortedException abortReason, Http2ErrorCode error) diff --git a/src/Kestrel.Core/Properties/CoreStrings.Designer.cs b/src/Kestrel.Core/Properties/CoreStrings.Designer.cs index 8b977316ec..fdc5d3fd3a 100644 --- a/src/Kestrel.Core/Properties/CoreStrings.Designer.cs +++ b/src/Kestrel.Core/Properties/CoreStrings.Designer.cs @@ -2086,6 +2086,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core internal static string FormatHttp2StreamErrorMoreDataThanLength() => GetString("Http2StreamErrorMoreDataThanLength"); + /// + /// An error occured after the response headers were sent, a reset is being sent. + /// + internal static string Http2StreamErrorAfterHeaders + { + get => GetString("Http2StreamErrorAfterHeaders"); + } + + /// + /// An error occured after the response headers were sent, a reset is being sent. + /// + internal static string FormatHttp2StreamErrorAfterHeaders() + => GetString("Http2StreamErrorAfterHeaders"); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/test/Kestrel.Core.Tests/Http2StreamTests.cs b/test/Kestrel.Core.Tests/Http2StreamTests.cs index c908ee8031..39a926cf81 100644 --- a/test/Kestrel.Core.Tests/Http2StreamTests.cs +++ b/test/Kestrel.Core.Tests/Http2StreamTests.cs @@ -20,6 +20,7 @@ using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions.Internal; using Microsoft.AspNetCore.Testing; +using Microsoft.Extensions.Logging; using Microsoft.Net.Http.Headers; using Xunit; @@ -1063,6 +1064,233 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); } + [Fact] + public async Task ContentLength_Response_FirstWriteMoreBytesWritten_Throws_Sends500() + { + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(async context => + { + context.Response.ContentLength = 11; + await context.Response.WriteAsync("hello, world"); // 12 + }); + + await StartStreamAsync(1, headers, endStream: true); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 0, + withFlags: (byte)Http2DataFrameFlags.END_STREAM, + withStreamId: 1); + + Assert.Contains(_logger.Messages, m => m.Exception?.Message.Contains("Response Content-Length mismatch: too many bytes written (12 of 11).") ?? false); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + _hpackDecoder.Decode(headersFrame.HeadersPayload, endHeaders: false, handler: this); + + Assert.Equal(3, _decodedHeaders.Count); + Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); + Assert.Equal("500", _decodedHeaders[HeaderNames.Status]); + Assert.Equal("0", _decodedHeaders[HeaderNames.ContentLength]); + } + + [Fact] + public async Task ContentLength_Response_MoreBytesWritten_ThrowsAndResetsStream() + { + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(async context => + { + context.Response.ContentLength = 11; + await context.Response.WriteAsync("hello,"); + await context.Response.WriteAsync(" world"); + }); + + await StartStreamAsync(1, headers, endStream: true); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 56, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 6, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + await WaitForStreamErrorAsync(1, Http2ErrorCode.INTERNAL_ERROR, "Response Content-Length mismatch: too many bytes written (12 of 11)."); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + _hpackDecoder.Decode(headersFrame.HeadersPayload, endHeaders: false, handler: this); + + Assert.Equal(3, _decodedHeaders.Count); + Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); + Assert.Equal("200", _decodedHeaders[HeaderNames.Status]); + Assert.Equal("11", _decodedHeaders[HeaderNames.ContentLength]); + } + + [Fact] + public async Task ContentLength_Response_NoBytesWritten_Sends500() + { + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(context => + { + context.Response.ContentLength = 11; + return Task.CompletedTask; + }); + + await StartStreamAsync(1, headers, endStream: true); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 0, + withFlags: (byte)Http2DataFrameFlags.END_STREAM, + withStreamId: 1); + + Assert.Contains(_logger.Messages, m => m.Exception?.Message.Contains("Response Content-Length mismatch: too few bytes written (0 of 11).") ?? false); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + _hpackDecoder.Decode(headersFrame.HeadersPayload, endHeaders: false, handler: this); + + Assert.Equal(3, _decodedHeaders.Count); + Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); + Assert.Equal("500", _decodedHeaders[HeaderNames.Status]); + Assert.Equal("0", _decodedHeaders[HeaderNames.ContentLength]); + } + + [Fact] + public async Task ContentLength_Response_TooFewBytesWritten_Resets() + { + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(context => + { + context.Response.ContentLength = 11; + return context.Response.WriteAsync("hello,"); + }); + + await StartStreamAsync(1, headers, endStream: true); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 56, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 6, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + await WaitForStreamErrorAsync(1, Http2ErrorCode.INTERNAL_ERROR, "Response Content-Length mismatch: too few bytes written (6 of 11)."); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + _hpackDecoder.Decode(headersFrame.HeadersPayload, endHeaders: false, handler: this); + + Assert.Equal(3, _decodedHeaders.Count); + Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); + Assert.Equal("200", _decodedHeaders[HeaderNames.Status]); + Assert.Equal("11", _decodedHeaders[HeaderNames.ContentLength]); + } + + [Fact] + public async Task ApplicationExeption_BeforeFirstWrite_Sends500() + { + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(context => + { + throw new Exception("App Faulted"); + }); + + await StartStreamAsync(1, headers, endStream: true); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 0, + withFlags: (byte)Http2DataFrameFlags.END_STREAM, + withStreamId: 1); + + Assert.Contains(_logger.Messages, m => (m.Exception?.Message.Contains("App Faulted") ?? false) && m.LogLevel == LogLevel.Error); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + _hpackDecoder.Decode(headersFrame.HeadersPayload, endHeaders: false, handler: this); + + Assert.Equal(3, _decodedHeaders.Count); + Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); + Assert.Equal("500", _decodedHeaders[HeaderNames.Status]); + Assert.Equal("0", _decodedHeaders[HeaderNames.ContentLength]); + } + + [Fact] + public async Task ApplicationExeption_AfterFirstWrite_Resets() + { + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(async context => + { + await context.Response.WriteAsync("hello,"); + throw new Exception("App Faulted"); + }); + + await StartStreamAsync(1, headers, endStream: true); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 37, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + await ExpectAsync(Http2FrameType.DATA, + withLength: 6, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + await WaitForStreamErrorAsync(1, Http2ErrorCode.INTERNAL_ERROR, "App Faulted"); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + _hpackDecoder.Decode(headersFrame.HeadersPayload, endHeaders: false, handler: this); + + Assert.Equal(2, _decodedHeaders.Count); + Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); + Assert.Equal("200", _decodedHeaders[HeaderNames.Status]); + } + [Fact] public async Task RST_STREAM_Received_AbortsStream() { @@ -1221,7 +1449,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests }); await StartStreamAsync(1, _browserRequestHeaders, endStream: true); - await WaitForStreamErrorAsync(expectedStreamId: 1, Http2ErrorCode.CANCEL, CoreStrings.ConnectionAbortedByApplication); + await WaitForStreamErrorAsync(expectedStreamId: 1, Http2ErrorCode.INTERNAL_ERROR, CoreStrings.ConnectionAbortedByApplication); await WaitForAllStreamsAsync(); Assert.Contains(1, _abortedStreamIds); @@ -1273,7 +1501,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests withFlags: 0, withStreamId: 1); - await WaitForStreamErrorAsync(expectedStreamId: 1, Http2ErrorCode.CANCEL, CoreStrings.ConnectionAbortedByApplication); + await WaitForStreamErrorAsync(expectedStreamId: 1, Http2ErrorCode.INTERNAL_ERROR, CoreStrings.ConnectionAbortedByApplication); await WaitForAllStreamsAsync(); Assert.Contains(1, _abortedStreamIds);