Skip to content

MessageHttpContext does not handle unreadable/unseekable Streams well #389

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

Closed
dougbu opened this issue Feb 23, 2023 · 1 comment · Fixed by #390
Closed

MessageHttpContext does not handle unreadable/unseekable Streams well #389

dougbu opened this issue Feb 23, 2023 · 1 comment · Fixed by #390
Assignees
Labels

Comments

@dougbu
Copy link
Member

dougbu commented Feb 23, 2023

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)
...
  1. 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 dougbu added the bug label Feb 23, 2023
@dougbu dougbu self-assigned this Feb 23, 2023
@dougbu
Copy link
Member Author

dougbu commented Feb 23, 2023

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

dougbu added a commit to dougbu/AspNetWebStack that referenced this issue Feb 23, 2023
- fix aspnet#389
- check `Stream` before first read attempt
- check `CanRead` (not `CanSeek`) before second or later read attempts
- add lots of tests of related scenarios
- nit: correct a recent `HttpContentMessageExtensions` comment
dougbu added a commit to dougbu/AspNetWebStack that referenced this issue Feb 23, 2023
- fix aspnet#389
- check `Stream` before first read attempt
- check `CanRead` (not `CanSeek`) before second or later read attempts
- add lots of tests of related scenarios
- nit: correct a recent `HttpContentMessageExtensions` comment
dougbu added a commit to dougbu/AspNetWebStack that referenced this issue Feb 24, 2023
- fix aspnet#389
- check `Stream` before first read attempt
- check `CanRead` (not `CanSeek`) before second or later read attempts
- add lots of tests of related scenarios
- nit: correct a recent `HttpContentMessageExtensions` comment
dougbu added a commit that referenced this issue Feb 24, 2023
- fix #389
- check `Stream` before first read attempt
- check `CanRead` (not `CanSeek`) before second or later read attempts
- add lots of tests of related scenarios
- nit: correct a recent `HttpContentMessageExtensions` comment

Apply suggestions from code review
- specifically, correct an `HttpMessageContent` comment

Co-authored-by: Chris Ross <Tratcher@Outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant