Skip to content
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

version 2.5 breaks the OWIN interop layer for setting request Schemes #717

Closed
baronfel opened this issue Nov 7, 2018 · 6 comments
Closed
Assignees

Comments

@baronfel
Copy link

baronfel commented Nov 7, 2018

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.

@ademar ademar self-assigned this Nov 7, 2018
@ademar
Copy link
Member

ademar commented Nov 8, 2018

@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 ?

@baronfel
Copy link
Author

baronfel commented Nov 8, 2018

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.

@ademar
Copy link
Member

ademar commented Nov 8, 2018

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)})

@baronfel
Copy link
Author

baronfel commented Nov 8, 2018

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

ademar added a commit that referenced this issue Nov 10, 2018
@ademar
Copy link
Member

ademar commented Nov 10, 2018

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

@ademar ademar closed this as completed Nov 10, 2018
@baronfel
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants