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.

Sign up for GitHub

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

Jump to bottom

Plans in Plugin: correctly forward the client IP for currency localization #8930

Merged
merged 2 commits into from
Feb 26, 2018
Merged

Plans in Plugin: correctly forward the client IP for currency localization #8930

merged 2 commits into from
Feb 26, 2018

Conversation

mattwiebe
Copy link
Contributor

@mattwiebe mattwiebe commented Feb 23, 2018 •
edited by oskosk
Loading

Follows on from #8834

Changes proposed in this Pull Request:

  • use the correct $_SERVER var from the client so that the wpcom plans endpoint can guess the client's locale and localize the currencty
  • correctly bypass caching when the jetpack_cache_plans filter returns false

Changelog entry

Plans: correctly forward the client IP for currency localization.

@mattwiebe mattwiebe added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] WP REST API labels Feb 23, 2018
@mattwiebe mattwiebe requested a review from a team as a code owner February 23, 2018 21:58
withinboredom
withinboredom suggested changes Feb 23, 2018
View reviewed changes
Copy link
Contributor

@withinboredom withinboredom left a comment

Choose a reason for hiding this comment

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

We should keep X_FORWARDED_FOR and CLIENT_IP headers because a lot of hosts probably have load balancers in front of their servers. Especially places like GC, AWS, etc.

_inc/lib/class.core-rest-api-endpoints.php Outdated
// no ip list to forward, so create one:
$ip = $_SERVER['HTTP_CLIENT_IP'];
}
// passing along from client to help geolocate currency
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to start with X_FORWARDED_FOR in case there's a load balancer in front of the site (like cloudflare, AWS)

_inc/lib/class.core-rest-api-endpoints.php Outdated
$ip = $_SERVER['HTTP_CLIENT_IP'];
}
// passing along from client to help geolocate currency
$ip = $_SERVER['REMOTE_ADDR'];
Copy link
Contributor

Choose a reason for hiding this comment

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

HTTP_CLIENT_IP is another proxy var that might be set ... I completely forget about REMOTE_ADDR (it's not applicable to my test site, so overlooked it)

jeherve
jeherve reviewed Feb 24, 2018
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Should we rely on the data we get and store for Protect in here? It may provide us with at reliable IP address?
See the trusted_ip_header option for example.

@mattwiebe
Copy link
Contributor Author

haha, I was just working on the same thing, thanks

@mattwiebe mattwiebe merged commit 9446769 into master Feb 26, 2018
@mattwiebe mattwiebe deleted the fix/plans-endpoint-ip-forwarding branch February 26, 2018 20:59
@oskosk oskosk added this to the 5.9 milestone Feb 27, 2018
@oskosk oskosk added [Status] Has Changelog and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 27, 2018
oskosk added a commit that referenced this pull request Feb 27, 2018
@oskosk
Add changelog entry for #8930
136abaa
oskosk added a commit that referenced this pull request Feb 27, 2018
@oskosk
Changelog 5.9: running changelog (#8819)
1174ef5 
* update changelog.txt

* Update readme.txt with scaffolding for 5.9 changelog and release draft shortlink

* Add changelog entry for #8243

* Add changelog entry for #8296

* Add changelog entry for #8367

* Add changelog entry for #8686

* Add changelog entry for #8707

* Add changelog entry for #8709 and #8714

* Add changelog entry for #8729

* Add changelog entry for #8777

* Add changelog entry for #8780

* Add changelog entry for #8786

* Add changelog entry for #8787

* Add changelog entry for #8801 #8805 #8832 #8865 and #8804

* Add changelog entry for #8817

* Add changelog entry for #8822

* Add changelog entry for #8823

* Add changelog entry for #8829

* Add changelog entry for #8834

* move some items to major enhancements

* Add changelog entry for #8836

* Add changelog entry for #8839

* Add changelog entry for #8861

* Add changelog entry for #8862

* Add changelog entry for #8863

* Add changelog entry for #8866

* Add changelog entry for #8870

* Add changelog entry for #8874

* Add changelog entry for #8875

* Add changelog entry for #8881

* Add changelog entry for #8890

* Add changelog entry for #8911

* Add changelog entry for #8927

* Add changelog entry for #8931

* Add changelog entry for #8933

* Add changelog entry for #8930

* fix wording

* typo

* minor fixes

* replace partner scripts for Jetpack Start in changelog entry

* Update to-test.md

* Update to-test.md

* minor style fixes to to-test.md

* minor style fixes to to-test.md

* minor fixes on to-test.md

* Add changelog entry for #8868

* Add changelog entry for #8844

* Add changelog entry for #8664

* Add changelog entry for #8935

* Add changelog entry for #8425

* Add changelog entry for #8625
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve left review comments

@withinboredom withinboredom withinboredom approved these changes

Assignees
No one assigned
Labels
[Feature] WP REST API
Projects
None yet
Milestone
5.9
Development

Successfully merging this pull request may close these issues.
5 participants
@mattwiebe @jeherve @withinboredom @oskosk @matticbot