search

aspnet / AspNetWebStack

ASP.NET MVC 5.x, Web API 2.x, and Web Pages 3.x (not ASP.NET Core)
Other
858 stars 354 forks source link

`MessageHttpContext` does not handle unreadable/unseekable `Stream`s well #389

Closed dougbu closed 1 year ago

dougbu commented 1 year ago

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.

  1. If the inner HttpContent provides an unreadable Stream, 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)
...
  2. If the inner HttpContent provides a readable but unseekable Stream, HttpMessageContent.ValidateStreamForReading(...) again misses the issue and attempts to use stream.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)
...
dougbu commented 1 year ago

/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 😉)