SuaveIO/suave

version 2.5 breaks the OWIN interop layer for setting request Schemes

baronfel opened this issue · 6 comments

The changes in commit 6303eb04ceff9fdd9d0ea7aab8070d5c512c0b8c around httpBindings in the HttpRequest type introduced a regression in the Owin interop layer.

Previously, in an owin pipeline, one could change the request scheme via OwinContext.Request.Scheme's setter, but the changes to the uriScheme lens setter made it so that any changes to the scheme do not get propagated to the underlying request.

This is fixable by reverting to 2.4.3, which has the old property behavior.

I looked at sending a patch, but I wasn't sure what the best way to construct a HTTPS protocol DU case was, because it requires an obj that is the ssl certificate.

@baronfel Whats the purpose of changing the scheme, it does not seem like something that should be writable? Is this a case of fooling the middleware into thinking is running under HTTPS ?

Specifically it's for using proxy headers like X-Forwarded-Proto to tell a non-https endpoint hosted behind an ssl-termination point like nginx what the original route scheme was. This is often used by those applications to give meaningful routes to client-side applications. In my case I was hosting Identity Server's Identity Manager component at a subroute of a suave application, which itself was behind nginx. Nginx would terminate ssl and set proxy headers, and an owin moddleware would use those headers to update the request before it was sent to the Identity manager component. That component constructs urls based off of the incoming request, and that request suddenly got the wrong scheme with this change.

Sorry, I apologize for breaking your app.

I have a fix, just pondering the consequences.

let uriScheme : Property<_, string> =
      (fun (uri : HttpBinding) -> uri.scheme.ToString()),
      (fun v uri ->
        { uri with
            scheme =
              match v.ToLowerInvariant() with
              | "http" ->
                HTTP
              | "https" ->
                HTTPS null
              | _ ->
                invalidOp (sprintf "Invalid scheme: '%s'" v)})

No problem, it was an easy temporary fix to pin to an earlier version. Your fix is essentially what I was looking to do, and I think it's safe-deposit, but hadn't completed an audit of the uses of the requests' httpBinding

@baronfel I've pushed a new version with this fix. Let us know if you uncover any other issues.

Thanks, I'll be sure to try it out and report back Monday.