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

Hourofcode redirect to original IP-address location #19272

Merged
merged 2 commits into from
Nov 17, 2017

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented Nov 17, 2017

This PR is an attempt to make the redirect logic on hourofcode.com more stable by ignoring intermediate/untrusted proxy-server IP addresses.

The original implementation of hoc_detect_country used Geocoder.search(request.ip).first (equivalent to request.safe_location), which uses Rack::Request#ip to determine the source IP address. #ip tries to determine a 'secure' (unspoofable) IP address using the REMOTE_ADDR or HTTP_X_FORWARDED_FOR request header, backtracking through the forwarded-ip-address chain until it reaches the first address that's not 'trusted' IP-address range (either private range or in a trusted-ip whitelist, which for us includes a list of CloudFront IP-address ranges maintained in trusted_proxies.json).

However, for a simple country redirect we don't care if the address is spoofable, so we can use request.location (which uses #geocoder_spoofable_ip for the source-IP address) instead.

Added a test (test_redirect_untrusted_proxy) that fails with the old implementation to ensure the updated implementation works as described.

Redirect to location of (spoofable) source ip-address,
rather than the (secure) location of the first untrusted proxy in the
forwarded IP-address chain.
@wjordan wjordan merged commit 264ac7b into staging Nov 17, 2017
@wjordan wjordan deleted the hoc_detect_country_fix branch February 12, 2018 19:15
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.

None yet

1 participant