Botbuilder-dotnet: NotSupportedException in BotMessageHandler at request.Body.Position = 0

[EricDahlvang]

Version

Pulled this branch: CLM/CosmosDBKeyLength to test against a CosmosDb instance for:
https://github.com/Microsoft/botbuilder-dotnet/pull/1370

Describe the bug

System.NotSupportedException: Specified method is not supported.
in Microsoft.Bot.Builder.Integration.AspNet.Core.Handlers.BotMessageHandler
on

request.Body.Position = 0;

System.NotSupportedException: Specified method is not supported.
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpRequestStream.set_Position(Int64 value)
   at Microsoft.Bot.Builder.Integration.AspNet.Core.Handlers.BotMessageHandler.ProcessMessageRequestAsync(HttpRequest request, IAdapterIntegration adapter, BotCallbackHandler botCallbackHandler, CancellationToken cancellationToken) in C:\work\v4\2.8.2019\botbuilder-dotnet\libraries\integration\Microsoft.Bot.Builder.Integration.AspNet.Core\BotMessageHandler.cs:line 23
   at Microsoft.Bot.Builder.Integration.AspNet.Core.Handlers.BotMessageHandlerBase.HandleAsync(HttpContext httpContext) in C:\work\v4\2.8.2019\botbuilder-dotnet\libraries\integration\Microsoft.Bot.Builder.Integration.AspNet.Core\BotMessageHandlerBase.cs:line 60
   at Microsoft.AspNetCore.Builder.Extensions.MapMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

To Reproduce

Steps to reproduce the behavior:

1. Run Microsoft.Bot.Builder.TestBot
2. Connect the emulator
3. Send a message to the bot
4. See error

Expected behavior

No exception

Screenshots

If applicable, add screenshots to help explain your problem.

Additional context

I'm not sure if this is due to: https://github.com/Microsoft/botbuilder-dotnet/commit/4a4ccd74fe650ddd97570156e2ca964d6155db69 ... or if the Microsoft.Bot.Builder.TestBot is using the wrong version of some library?

[bug]

[drub0y]

Ah, so clearly there wasn't a test done on a non-buffered Stream. We can't let the code blindly reposition like this because, if buffering wasn't enabled, the Stream is forward only 'cause it's literally streaming the bytes off the network, so a call to set the Position is not supported.

A check to Stream::CanSeek should fix this problem. Testing it out now and will PR.

[drub0y]

Confirmed the above. Working on full fix and want to make sure a buffering stream still works as well. PR incoming soon.

[EricDahlvang]

@drub0y I don't know enough about how this all works. Is this same issue also going to be a problem here:

https://github.com/Microsoft/botbuilder-dotnet/blob/master/libraries/integration/Microsoft.Bot.Builder.Integration.ApplicationInsights.Core/TelemetrySaveBodyASPMiddleware.cs#L49

Or does this .EnableBuffering call handle it?
https://github.com/Microsoft/botbuilder-dotnet/blob/master/libraries/integration/Microsoft.Bot.Builder.Integration.ApplicationInsights.Core/TelemetrySaveBodyASPMiddleware.cs#L30

It seems like we should be setting a higher than default bufferThreshold when calling EnableBuffering.

[drub0y]

@EricDahlvang that's right, the EnableBuffering call happens 100% of the time there so that makes it "safe" for multiple reads and, unlike the Sentry implementation discussed in this issue, the TelemetrySaveBodyASPMiddleware was never trying to read the body _after_ the BotMessageHandler, so it didn't run into the same issues due to BotMessageHandler closing the Stream.