microsoft / reverse-proxy

A toolkit for developing high-performance HTTP reverse proxy applications.
https://microsoft.github.io/reverse-proxy
MIT License
8.53k stars 838 forks source link

Error when Proxying Websocket with Shiny Server #1073

Closed marcelo-g-simas closed 3 years ago

marcelo-g-simas commented 3 years ago

Describe the bug

I am exploring using YARP to proxy requests to a Shiny server (https://shiny.rstudio.com/). From an existing ASP.NET Core 3.1 web app. However, the page fails to load when it attempts to setup a websocket with Shiny. Here is the error that appears on the logs:

[15:08:31 INF] Request starting HTTP/1.1 GET http://localhost:58471/shiny/websocket/  
[15:08:31 INF] Executing endpoint 'shiny'
[15:08:31 INF] Proxying to http://localhost:7368/websocket/
[15:08:31 INF] UpgradeResponseClient: The client reported an error when copying the upgraded response body.
System.InvalidOperationException: Response Content-Length mismatch: too many bytes written (87 of 0).
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ThrowTooManyBytesWritten(Int32 count)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.VerifyAndUpdateWrite(Int32 count)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.WritePipeAsync(ReadOnlyMemory`1 data, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpResponseStream.WriteAsync(ReadOnlyMemory`1 source, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpUpgradeStream.WriteAsync(ReadOnlyMemory`1 source, CancellationToken cancellationToken)
   at Yarp.ReverseProxy.Service.Proxy.StreamCopier.CopyAsync(Boolean isRequest, Stream input, Stream output, IClock clock, CancellationToken cancellation)
[15:08:31 INF] Executed endpoint 'shiny'

To Reproduce

I followed the sample of how to get YARP added to an existing web app and here is the proxy server section of my appsettings.config:

"ReverseProxy": {
    "Routes": {
      "shiny": {
        "ClusterId": "shiny",
        "Match": {
          "Path": "/shiny/{*any}"
        },
        "Transforms": [
          { "PathRemovePrefix": "/shiny" }
        ]
      }
    },
    "Clusters": {
      "shiny": {
        "Destinations": {
          "shiny": {
            "Address": "http://localhost:7368/"
          }
        }
      }
    }
  }

To get a running Shiny server on localhost you will need to install R (https://www.r-project.org/) and start an R interactive session. Once you get the R prompt type in the following commands to install the Shiny package and get a sample app started:

install.packages("shiny")
shiny::runExample("01_hello", port = 7368) 

Further technical details

Tratcher commented 3 years ago

It sounds like the 101 Switching Protocols response from Shinny includes a Content-Length: 0 header and that kestrel is then enforcing that length on the upgraded stream.

There are a few ways to confirm that. A) Enable Kestrel's connection logs to see a raw trace of the response: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/servers/kestrel/endpoints?view=aspnetcore-5.0#connection-logging B) Put in a response transform and then setting a debugger break point there to inspect the 101 response.

If this is what's happening then you should be able to mitigate it by removing the Content-Length response header.

{
  "ResponseHeader": "Content-Length",
  "Set": "",
  "When": "Always"
}

If this is confirmed then we may want to fix it in Kestrel.

samsp-msft commented 3 years ago

Is this a bug in shiny, or our upgrade handling?

marcelo-g-simas commented 3 years ago

Thanks! I am going to try the config change first.

marcelo-g-simas commented 3 years ago

Yep, the suggested mitigation made it so the page loads successfully and the Shiny app is working as expected while being proxied. Going to put in a code transform and breakpoint to get you confirmation.

marcelo-g-simas commented 3 years ago

Confirmed: Screen Shot 2021-06-10 at 4 17 21 PM

Tratcher commented 3 years ago

Is this a bug in shiny, or our upgrade handling?

I'll have to check the spec, but it's certainly unusual. A) Most WebSocket 101 responses do not have this header or we'd have seen this by now. B) Any value other than 0 would certainly be a bug in that server. 0 is ambiguous. C) Kestrel shouldn't enforce length limits on upgrades, that's a bug that should be fixed. D) YARP could mitigate this by removing the header for upgrades.

Tratcher commented 3 years ago

https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2

A server MUST NOT send a Content-Length header field in any response with a status code of 1xx (Informational) or 204 (No Content).

Ok, Shiny's directly violating the spec here.

marcelo-g-simas commented 3 years ago

https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2

A server MUST NOT send a Content-Length header field in any response with a status code of 1xx (Informational) or 204 (No Content).

Ok, Shiny's directly violating the spec here.

Do you want me to log an issue on their GitHub?

For the record I have successfully proxied Shiny apps using both Apache and ngnix. So this may be a well-known misbehavior.

Tratcher commented 3 years ago

Do you want me to log an issue on their GitHub?

Yes please. We'll see what they say about fixing it.

For now at least the mitigation is easy.

For the record I have successfully proxied Shiny apps using both Apache and ngnix. So this may be a well-known misbehavior

This is the kind of misbehavior that could normally be overlooked, functionally you end up with the same result, it just happened to trigger an alternate validation code path in Kestrel.

samsp-msft commented 3 years ago

https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2

A server MUST NOT send a Content-Length header field in any response with a status code of 1xx (Informational) or 204 (No Content).

Ok, Shiny's directly violating the spec here.

Isn't the problem that we are writing data along with the content-length:0. What are we writing?

Tratcher commented 3 years ago

Isn't the problem that we are writing data along with the content-length:0. What are we writing?

101 says you've switched protocols after the headers are sent, any subsequent data is WebSocket data.

Kestrel's bug is that it saw you set a Content-Length 0 and it's still enforcing that after the upgrade when you try to send WebSocket data.

marcelo-g-simas commented 3 years ago

Today I tested against a real Shiny Server (https://www.rstudio.com/products/shiny/) and the problem does not happen. So this issue only appears when one is testing using an interactive R session. I tested this against the latest Shiny Server (shiny-server-1.5.16.958-amd64.deb).

karelz commented 3 years ago

Triage: Seems to be external problem. It is spec violation. Closing.