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

X-Forwarded-Proto being set to upstream protocol #41

Merged
merged 2 commits into from Sep 28, 2016

Conversation

@jmm
Copy link
Contributor

jmm commented Aug 31, 2016

Hi, xforward isn't working the way I expect with respect to X-Forwarded-Proto (h2o2@5.1.1 / hapi@14.2.0 / node@4.5.0). Say h2o2 is running on a server that's listening for https requests and does an upstream http request. In the upstream request X-Forwarded-Proto should be https, right? I'm getting http.

I checked out the source and it looks like it's setting it to the protocol configured for the upstream request. Example:

handler: {
  proxy: {
    host,
    port,
    // This will be the value of X-Forwarded-Proto
    protocol: "http",
    xforward: true,
  }
}

I modified one of the tests to illustrate the problem. In the next commit I changed the implementation to work the way I expect and the other tests continue to pass.

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Aug 31, 2016

What your saying sounds right to me. As for the other usage of the protocol of the upstream request in that function (determining the agent, etc.), I haven't taken time to fully evaluate, but it seems correct as-is.

@jmm

This comment has been minimized.

Copy link
Contributor Author

jmm commented Aug 31, 2016

Ok, thanks for the feedback.

As for the other usage of the protocol of the upstream request in that function (determining the agent, etc.), I haven't taken time to fully evaluate, but it seems correct as-is.

Yeah, I hadn't looked into any of that. It'd definitely be good for this to be reviewed by yourself or someone else more familiar than me for:

  • correctness of the change in X-Forwarded-Proto setting
  • correctness of the revised test
  • to confirm that the test changes don't regress coverage for something else (the report says 100% before and after)
  • in case it suggests something else that might need attention.

I just now took a quick look and using the upstream protocol for setting up the agent seems right to me (though I don't actually know that much about agents).

Thanks!

@osukaa osukaa added the bug label Aug 31, 2016
@osukaa osukaa added this to the 5.2.0 milestone Aug 31, 2016
@osukaa

This comment has been minimized.

Copy link
Contributor

osukaa commented Aug 31, 2016

The "x-forwarded-proto" should keep the origin protocol, the agent should be created based on the configuration you set for h2o2

@osukaa

This comment has been minimized.

Copy link
Contributor

osukaa commented Sep 27, 2016

Hey! Would you mind updating your branch so I can publish both x-forwarded headers issues together? Sorry for being so late :(

@jmm jmm force-pushed the jmm:x-forwarded-proto branch from d6c5541 to 8c53fcb Sep 28, 2016
@jmm

This comment has been minimized.

Copy link
Contributor Author

jmm commented Sep 28, 2016

Hey @osukaa! Should be all set. Don't worry about it, I know how it is.

@osukaa
osukaa approved these changes Sep 28, 2016
@osukaa osukaa merged commit d715678 into hapijs:master Sep 28, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@osukaa

This comment has been minimized.

Copy link
Contributor

osukaa commented Sep 28, 2016

@jmm the 5.2.0 version has been published! :)

@jmm

This comment has been minimized.

Copy link
Contributor Author

jmm commented Sep 28, 2016

Thanks @osukaa and @devinivy :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.