`MessageHttpContext` does not handle unreadable/unseekable `Stream`s well
dougbu opened this issue · 1 comments
As mentioned in hidden comments in #384 (because I changed the relevant lines slightly), HttpMessageContent.ValidateStreamForReading(...)
uses CanRead
where CanSeek
would be more appropriate. This causes problems when the inner HttpContent.ReadAsStreamAsync()
returns a Stream
that is readable but not seekable. EmptyContent
provides such a Stream
. Specifically, error messages are unintelligible.
- If the inner
HttpContent
provides an unreadableStream
,HttpMessageContent.ValidateStreamForReading(...)
ignores the problem entirely. This allows execution to continue past where problems should have been detected.
System.NotSupportedException: Stream does not support reading.
at System.IO.StreamHelpers.ValidateCopyToArgs(Stream source, Stream destination, Int32 bufferSize)
at System.IO.Stream.CopyToAsync(Stream destination, Int32 bufferSize, CancellationToken cancellationToken)
at System.IO.Stream.CopyToAsync(Stream destination, CancellationToken cancellationToken)
at System.Net.Http.StreamToStreamCopy.CopyAsync(Stream source, Stream destination, Int32 bufferSize, Boolean disposeSource, CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
at System.Net.Http.HttpContent.CopyToAsyncCore(ValueTask copyTask)
at System.Net.Http.HttpMessageContent.SerializeToStreamAsync(Stream stream, TransportContext context) in ..\src\System.Net.Http.Formatting\HttpMessageContent.cs:line 194
at System.Net.Http.HttpContent.CopyToAsyncCore(ValueTask copyTask)
...
- If the inner
HttpContent
provides a readable but unseekableStream
,HttpMessageContent.ValidateStreamForReading(...)
again misses the issue and attempts to usestream.Position
instead of providing a useful message.
System.NotImplementedException: The method or operation is not implemented.
at System.Net.Http.HttpMessageContentTests.ReadOnlyStream.set_Position(Int64 value) in ..\test\System.Net.Http.Formatting.Test\HttpMessageContentTests.cs:line 496
at System.Net.Http.DelegatingStream.set_Position(Int64 value)
at System.Net.Http.HttpMessageContent.ValidateStreamForReading(Stream stream) in ..\src\System.Net.Http.Formatting\HttpMessageContent.cs:line 369
at System.Net.Http.HttpMessageContent.SerializeToStreamAsync(Stream stream, TransportContext context) in ..\src\System.Net.Http.Formatting\HttpMessageContent.cs:line 193
at System.Net.Http.HttpContent.CopyToAsyncCore(ValueTask copyTask)
...
/fyi @Tratcher, @stephentoub, @mkArtakMSFT, @javiercn, @MackinnonBuck, @wtgodbe
Note I already have a fix ready for this. It's not normally something we'd change in this repo (because I can't see a security angle). But HttpMessageContent
has received some attention lately and this came up as part of that work. I also confirmed things w/ a bunch of tests.
(You can tell the above came from unit testing by looking closely at the second stack trace in the description 😉)