Skip to content

wp_remote_get now sends the site url as referer#13664

Merged
jeherve merged 4 commits intomasterfrom
fix/maps-referer
Oct 14, 2019
Merged

wp_remote_get now sends the site url as referer#13664
jeherve merged 4 commits intomasterfrom
fix/maps-referer

Conversation

@rakmob42
Copy link
Copy Markdown

@rakmob42 rakmob42 commented Oct 5, 2019

Changes proposed in this Pull Request:

Testing instructions (Before the fix):

  • Go to widgets page and add Contact Info & Map widget by Jetpack
  • Enter Google's API key (without HTTP referrer restrictions) as explained in Jetpack docs. Embed works and we can see a preview after saving the widget
  • Remove the API key, save the widget (basically reset the widget and start fresh)
  • Go to Google API page and enable HTTP referrer restrictions to your site URL. Now on the widgets page, enter Google's API key again and save the widget. It returns an error Google Maps Platform rejected your request. This IP, site or mobile application is not authorized to use this API key.

Testing instructions (After the fix):

  • Go to Google API page and enable HTTP referrer restrictions to your site URL
  • Go to widgets page and add Contact Info & Map widget by Jetpack
  • Enter Google's API key and save the widget
  • It works as expected (displays a preview, no error).

Proposed changelog entry for your changes:

  • Fixes Google Maps referrer restrictions issue for "Contact Info & Map" widget.

@rakmob42 rakmob42 added Bug When a feature is broken and / or not performing as intended [Feature] Extra Sidebar Widgets [Status] Needs Review This PR is ready for review. labels Oct 5, 2019
@rakmob42 rakmob42 requested review from a team and kbrown9 October 5, 2019 02:33
@rakmob42 rakmob42 self-assigned this Oct 5, 2019
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Oct 5, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against 9524dce

@jeherve jeherve added this to the 7.9 milestone Oct 7, 2019
jeherve
jeherve requested changes Oct 7, 2019
View reviewed changes
Copy link
Copy Markdown
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.

In fd3c20d I fixed all PHPCS warnings in that file, including spacing and a trailing comma in the changes you've introduced.

This was a minor change though. Other than that, this seems to work well. 👍 I only have one minor remark.

I'll let @kbrown9 jump in and review as well. 👍

jeherve
jeherve reviewed Oct 7, 2019
View reviewed changes
modules/widgets/contact-info.php
Comment on lines +444 to 448
$wp_remote_get_args = array(
'headers' => array( 'Referer' => site_url() ),
);
$response = wp_remote_get( esc_url_raw( $path ), $wp_remote_get_args );

Copy link
Copy Markdown
Member

@jeherve jeherve Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The actual changes are here.

Copy link
Copy Markdown
Author

@rakmob42 rakmob42 Oct 11, 2019

Choose a reason for hiding this comment

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

Thanks Jer! :)

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Oct 7, 2019
@mdawaffe
Copy link
Copy Markdown
Member

mdawaffe commented Oct 7, 2019

@kbrown9 (you asked about referrers elsewhere). I looked a bit more, and the URL-as-Referer logic has been there since the beginning of the Requests library we use (since WordPress 4.6 in 2016):

WordPress/Requests@78d4f3c#diff-52f160e1af3ff131e027808a6c511447R50

Apparently it was taken from the previous library, SimplePie, which has included this code since 2006 (see the get_file() method further down the page there) as a means to get around anti-hotlinking.

https://github.com/simplepie/simplepie/blob/1.0_b1/simplepie.inc#L1406-L1410

So all HTTP requests sent by WordPress set the destination URL as the Referrer because SimplePie needed a way to bypass hotlinking prevention for some image requests thirteen years ago :)

Some related issues:

kbrown9
kbrown9 previously approved these changes Oct 7, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@kbrown9 kbrown9 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good to me! I tested the branch using an API key with an HTTP restriction, and it works as expected.

Thanks for fixing this @codestor4!

@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Oct 7, 2019

Caution: This PR has changes that must be merged to WordPress.com
Hello codestor4! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33679-code before merging this PR. Thank you!

jeherve
jeherve requested changes Oct 11, 2019
View reviewed changes
@jeherve
changed site_url function to home_url
9524dce 
Co-Authored-By: Jeremy Herve <jeremy@jeremy.hu>
@rakmob42
Copy link
Copy Markdown
Author

rakmob42 commented Oct 11, 2019 •
edited
Loading

Thanks everyone for reviewing this PR and sharing your thoughts. And thanks Jer for suggesting that commit. Sorry I am late to the party, was too occupied this week.

I think this should be good to go :)
Changing status now..

@rakmob42 rakmob42 added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Oct 11, 2019
@rakmob42 rakmob42 requested a review from jeherve October 14, 2019 08:33
jeherve
jeherve approved these changes Oct 14, 2019
View reviewed changes
Copy link
Copy Markdown
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.

This looks good to me. Merging.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 14, 2019
@jeherve jeherve merged commit 6aefc26 into master Oct 14, 2019
@jeherve jeherve deleted the fix/maps-referer branch October 14, 2019 08:44
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 14, 2019
@jeherve jeherve mentioned this pull request Oct 14, 2019
Closed
jeherve added a commit that referenced this pull request Oct 23, 2019
@jeherve
Changelog: add #13664
abdaedb
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

@kbrown9 kbrown9 kbrown9 left review comments

Assignees

@rakmob42 rakmob42

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Extra Sidebar Widgets Touches WP.com Files

Projects

None yet

Milestone

7.9

Development

Successfully merging this pull request may close these issues.

6 participants

@rakmob42 @jetpackbot @mdawaffe @matticbot @jeherve @kbrown9