Skip to content

Fix feed location check and force HTTPS over HTTP.#1115

Draft
message-dimke wants to merge 7 commits intodevelopfrom
fix/feed-location-url-match
Draft

Fix feed location check and force HTTPS over HTTP.#1115
message-dimke wants to merge 7 commits intodevelopfrom
fix/feed-location-url-match

Conversation

@message-dimke
Copy link
Copy Markdown
Contributor

@message-dimke message-dimke commented Mar 24, 2025

Changes proposed in this Pull Request:

  • Replacing get_site_url with wp_get_upload_dir for the feed location check.
  • Forcing HTTPS over HTTP for the feed location URI.

Additional details:

Changelog entry

Fix - Feed location URL matching.

…reating the initial feed URL when posting it to Pinterest.
@message-dimke message-dimke requested a review from a team March 24, 2025 11:13
@message-dimke message-dimke self-assigned this Mar 24, 2025
@github-actions github-actions Bot added changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug. labels Mar 24, 2025
@message-dimke message-dimke changed the title Replacing get_site_url with wp_get_upload_dir as we use it when c… Fix feed location check and force HTTPS over HTTP. Mar 25, 2025
@layoutd
Copy link
Copy Markdown
Collaborator

layoutd commented Mar 25, 2025

If a site doesn't use HTTPS, what will happen? Would it make sense to check is_ssl() to determine whether to force http or https? If the site doesn't have a valid certificate, would unconditionally forcing https still work?

@layoutd
Copy link
Copy Markdown
Collaborator

layoutd commented Mar 25, 2025

Also is this a draft still?

@message-dimke
Copy link
Copy Markdown
Contributor Author

Hey, @layoutd !

I believe that for any e-commerce store SSL is a must. Most payment gateways require it, etc. To my understanding is_ssl won't give you any information if the website has SSL configured, it will tell you if the current page is served by HTTPS or not. I would always force HTTP to HTTPS.

@layoutd layoutd removed the request for review from a team October 15, 2025 23:01
@simplysaru simplysaru requested a review from Copilot March 27, 2026 05:37
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates how the plugin detects/matches feed “location” URLs by basing comparisons on the uploads base URL (via wp_get_upload_dir()) instead of the site URL, and normalizes the feed URL scheme to HTTPS to avoid mismatches from HTTP upload URLs.

Changes:

  • Update feed location matching logic to use uploads baseurl + PINTEREST_FOR_WOOCOMMERCE_LOG_PREFIX prefix (instead of get_site_url()).
  • Force HTTPS scheme when building local feed URLs and when matching feed locations.
  • Update unit tests to mock uploads baseurl via the upload_dir filter.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
tests/Unit/FeedsTest.php Adjusts tests to stub uploads base URL via upload_dir filter for feed-location matching.
src/LocalFeedConfigs.php Forces HTTPS when generating the feed URL base derived from uploads baseurl.
src/Feeds.php Switches feed location matching from get_site_url() to uploads baseurl + prefix, forcing HTTPS.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/Unit/FeedsTest.php
Comment thread tests/Unit/FeedsTest.php
Comment thread src/LocalFeedConfigs.php Outdated
Comment thread src/Feeds.php Outdated
}

return 0 === strpos( $feed['location'] ?? '', get_site_url() );
// Some sites may be misconfigured and return HTTP schema for the feed location URL. We force it to become HTTPS.
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

The comment says "HTTP schema", but for URLs the correct term is "scheme". Please update to avoid confusion.

Suggested change
// Some sites may be misconfigured and return HTTP schema for the feed location URL. We force it to become HTTPS.
// Some sites may be misconfigured and return HTTP scheme for the feed location URL. We force it to become HTTPS.

Copilot uses AI. Check for mistakes.
Comment thread src/Feeds.php Outdated
Comment on lines +339 to +343
// Some sites may be misconfigured and return HTTP schema for the feed location URL. We force it to become HTTPS.
$force_https = str_replace( 'http:', 'https:', wp_get_upload_dir()['baseurl'] );
$feed_location = trailingslashit( $force_https ) . PINTEREST_FOR_WOOCOMMERCE_LOG_PREFIX . '-';

return 0 === strpos( $feed['location'] ?? '', $feed_location );
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

does_feed_match() is called inside loops over remote feeds; calling wp_get_upload_dir() each time can be unnecessarily expensive and may behave inconsistently if upload_dir is filtered dynamically. Consider computing the normalized base URL/prefix once (e.g., in the caller or via a cached/static value) and reusing it for all comparisons.

Suggested change
// Some sites may be misconfigured and return HTTP schema for the feed location URL. We force it to become HTTPS.
$force_https = str_replace( 'http:', 'https:', wp_get_upload_dir()['baseurl'] );
$feed_location = trailingslashit( $force_https ) . PINTEREST_FOR_WOOCOMMERCE_LOG_PREFIX . '-';
return 0 === strpos( $feed['location'] ?? '', $feed_location );
static $feed_location_prefix = null;
if ( null === $feed_location_prefix ) {
// Some sites may be misconfigured and return HTTP schema for the feed location URL. We force it to become HTTPS.
$force_https = str_replace( 'http:', 'https:', wp_get_upload_dir()['baseurl'] );
$feed_location_prefix = trailingslashit( $force_https ) . PINTEREST_FOR_WOOCOMMERCE_LOG_PREFIX . '-';
}
return 0 === strpos( $feed['location'] ?? '', $feed_location_prefix );

Copilot uses AI. Check for mistakes.
Comment thread src/LocalFeedConfigs.php Outdated
Comment on lines +93 to +96
$file_name_base = trailingslashit( wp_get_upload_dir()['basedir'] ) . PINTEREST_FOR_WOOCOMMERCE_LOG_PREFIX . '-';
$url_base = trailingslashit( wp_get_upload_dir()['baseurl'] ) . PINTEREST_FOR_WOOCOMMERCE_LOG_PREFIX . '-';
// Some sites may be misconfigured and return HTTP schema for the feed location URL. We force it to become HTTPS.
$force_https = str_replace( 'http:', 'https:', wp_get_upload_dir()['baseurl'] );
$url_base = trailingslashit( $force_https ) . PINTEREST_FOR_WOOCOMMERCE_LOG_PREFIX . '-';
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

wp_get_upload_dir() is called multiple times here; it’s cleaner and avoids repeated work to store the result in a local variable and use it for both basedir and baseurl. Also, WordPress provides set_url_scheme( $url, 'https' ) which is more robust than a raw str_replace when forcing HTTPS.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

…figs to avoid double wp_get_upload_dir() call

Agent-Logs-Url: https://github.com/woocommerce/pinterest-for-woocommerce/sessions/1ddf8e31-dbd9-469f-8c34-d9d7ff6f9dbc

Co-authored-by: simplysaru <2734132+simplysaru@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI commented Apr 1, 2026

If a site doesn't use HTTPS, what will happen? Would it make sense to check is_ssl() to determine whether to force http or https? If the site doesn't have a valid certificate, would unconditiona...

The HTTPS forcing is applied consistently in both places — when the feed URL is first created/registered with Pinterest (LocalFeedConfigs.php) and when matching against it (Feeds.php). Since both sides normalize to HTTPS, the comparison stays consistent regardless of the site's SSL status.

For sites without a valid certificate, the feed URL would be HTTPS in Pinterest's records but the file might not be reachable over HTTPS — that's a broader hosting configuration issue separate from URL matching. Checking is_ssl() would actually cause a mismatch: if the site later enables SSL, the newly forced-HTTPS URL in LocalFeedConfigs wouldn't match the previously HTTP-registered URL at Pinterest. The current unconditional normalization avoids that inconsistency.

Copilot AI requested a review from simplysaru April 1, 2026 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants