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

Fix forwarding of headers starting with http_ in the proxy #12

Merged
merged 2 commits into from
Aug 31, 2018

Conversation

ejennings-mdsol
Copy link
Contributor

Having talked with @johngluckmdsol offline about #11, this PR is inspired by his fix and from his discussion with @cabbott. It seems like when the proxy was originally created in https://github.com/mdsol/mauth-client/pull/45 that the intent was to forward headers that begin with HTTP_ but to remove the leading HTTP_ (but to not forward HTTP_HOST). This PR restores that behavior that was changed in https://github.com/mdsol/mauth-client/pull/106.

@jfeltesse-mdsol

Copy link

@johngluckmdsol johngluckmdsol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This spec is a great start but it's not a substitute for an integration test that would have actually caught the original problem. If we add request headers to mauth and the proxy mangles them again, this won't catch that. Since we don't modify mauth that often, it's fine, so LGTM

@jfeltesse-mdsol
Copy link
Contributor

Ahhhh it all makes sense now.

When @johngluckmdsol first brought it up I was confused by the lack of any match or =~ call that would be needed for $' to do anything useful.
As you pointed out, this is the offending refactoring that took place many moons ago:

- request_env.each do |k,v|
-  if k =~ /\AHTTP_/ && !%w(HTTP_HOST).include?(k)
+ request_env.each do |k, v|
+  if k.start_with?('HTTP_') && !%w(HTTP_HOST).include?(k)
    name = $'

I guess this serves as another lesson that ruby global variables are evil.

@jfeltesse-mdsol jfeltesse-mdsol merged commit 9cc8398 into master Aug 31, 2018
@jfeltesse-mdsol jfeltesse-mdsol deleted the fix/proxy_header_fwding branch August 31, 2018 23:59
@jfeltesse-mdsol
Copy link
Contributor

merged and tagged but I don't have access to rubygems.org to publish a new release...

@cabbott cabbott restored the fix/proxy_header_fwding branch September 26, 2018 02:25
@cabbott cabbott deleted the fix/proxy_header_fwding branch September 26, 2018 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants