-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Fix HttpLoggingMiddleware Request/Response bodies logging in case of stream being closed by a subsequent middleware #61490
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ internal sealed class ResponseBufferingStream : BufferingStream, IHttpResponseBo | |
private readonly IHttpLoggingInterceptor[] _interceptors; | ||
private bool _logBody; | ||
private Encoding? _encoding; | ||
private string? _bodyBeforeClose; | ||
|
||
private static readonly StreamPipeWriterOptions _pipeWriterOptions = new StreamPipeWriterOptions(leaveOpen: true); | ||
|
||
|
@@ -179,7 +180,7 @@ public void LogResponseBody() | |
{ | ||
if (_logBody) | ||
{ | ||
var responseBody = GetString(_encoding!); | ||
var responseBody = GetStringInternal(); | ||
_logger.ResponseBody(responseBody); | ||
} | ||
} | ||
|
@@ -188,7 +189,27 @@ public void LogResponseBody(HttpLoggingInterceptorContext logContext) | |
{ | ||
if (_logBody) | ||
{ | ||
logContext.AddParameter("ResponseBody", GetString(_encoding!)); | ||
logContext.AddParameter("ResponseBody", GetStringInternal()); | ||
} | ||
} | ||
|
||
private string GetStringInternal() | ||
{ | ||
var result = _bodyBeforeClose ?? GetString(_encoding!); | ||
// Reset the value after its consumption to preserve GetString(encoding) behavior | ||
_bodyBeforeClose = null; | ||
return result; | ||
} | ||
|
||
public override void Close() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the request stream impl is fine, but we should double-check and maybe add a comment about the assumption. |
||
{ | ||
if (_logBody) | ||
{ | ||
// Subsequent middleware can close the response stream after writing its body | ||
// Preserving the body for the final GetStringInternal() call. | ||
_bodyBeforeClose = GetString(_encoding!); | ||
} | ||
|
||
base.Close(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -390,6 +390,46 @@ public async Task RequestBodyCopyToAsyncWorks(string expected) | |||||
Assert.Contains(TestSink.Writes, w => w.Message.Contains(expected)); | ||||||
} | ||||||
|
||||||
[Theory] | ||||||
[MemberData(nameof(BodyData))] | ||||||
public async Task RequestBodyWithStreamCloseWorks(string expected) | ||||||
{ | ||||||
var options = CreateOptionsAccessor(); | ||||||
options.CurrentValue.LoggingFields = HttpLoggingFields.RequestBody; | ||||||
|
||||||
var middleware = CreateMiddleware( | ||||||
async c => | ||||||
{ | ||||||
var arr = new byte[4096]; | ||||||
var contentLengthBytesLeft = c.Request.Body.Length; | ||||||
|
||||||
// (1) The subsequent middleware reads right up to the buffer size (guided by the ContentLength header) | ||||||
while (contentLengthBytesLeft > 0) | ||||||
{ | ||||||
var res = await c.Request.Body.ReadAsync(arr, 0, (int)Math.Min(arr.Length, contentLengthBytesLeft)); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit?
Suggested change
|
||||||
contentLengthBytesLeft -= res; | ||||||
if (res == 0) | ||||||
{ | ||||||
break; | ||||||
} | ||||||
} | ||||||
|
||||||
// (2) The subsequent middleware closes the request stream after its consumption | ||||||
c.Request.Body.Close(); | ||||||
}, | ||||||
options); | ||||||
|
||||||
var httpContext = new DefaultHttpContext(); | ||||||
httpContext.Request.ContentType = "text/plain"; | ||||||
var buffer = Encoding.UTF8.GetBytes(expected); | ||||||
httpContext.Request.Body = new MemoryStream(buffer); | ||||||
httpContext.Request.ContentLength = buffer.Length; | ||||||
|
||||||
await middleware.Invoke(httpContext); | ||||||
|
||||||
Assert.Contains(TestSink.Writes, w => w.Message.Contains(expected)); | ||||||
} | ||||||
|
||||||
[Fact] | ||||||
public async Task RequestBodyReadingLimitLongCharactersWorks() | ||||||
{ | ||||||
|
@@ -1155,6 +1195,32 @@ public async Task StartAsyncResponseHeadersLogged() | |||||
await middlewareTask; | ||||||
} | ||||||
|
||||||
[Theory] | ||||||
[MemberData(nameof(BodyData))] | ||||||
public async Task ResponseBodyWithStreamCloseWorks(string expected) | ||||||
{ | ||||||
var options = CreateOptionsAccessor(); | ||||||
options.CurrentValue.LoggingFields = HttpLoggingFields.ResponseBody; | ||||||
var middleware = CreateMiddleware( | ||||||
async c => | ||||||
{ | ||||||
c.Response.ContentType = "text/plain"; | ||||||
|
||||||
// (1) The subsequent middleware writes its response | ||||||
await c.Response.WriteAsync(expected); | ||||||
|
||||||
// (2) The subsequent middleware closes the response stream after it has completed writing to it | ||||||
c.Response.Body.Close(); | ||||||
}, | ||||||
options); | ||||||
|
||||||
var httpContext = new DefaultHttpContext(); | ||||||
|
||||||
await middleware.Invoke(httpContext); | ||||||
|
||||||
Assert.Contains(TestSink.Writes, w => w.Message.Contains(expected)); | ||||||
} | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add a test for |
||||||
[Fact] | ||||||
public async Task UnrecognizedMediaType() | ||||||
{ | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass
HttpLoggingInterceptorContext
into the ctor like we do for the response stream and then check if body logging is enabled before getting the string?