Boost: Fix skip proxy origins to load css during critical CSS generation

[haqadn]

While generating critical CSS, the CSS files need to be proxied through the plugin to avoid CORS restrictions. Previously the proxy was skipped if the server hostname matched the request, or if there was no hostname provided in the URL. Now, it has a filter hook to allow more control.

Correction: As @davidlonjon later pointed out, we don't really need the filter to make this work. The real issue is, the port was not taking into consideration while comparing hostname.

Changes proposed in this Pull Request:

Improved hostname comparison to take port into account.

Reason behind:

While developing locally through docker, the docker container internally would understand the hostname as localhost. However, we can also change the external port mapping through PORT_WORDPRESS env variable.
This leads to an issue. The external hostname and internal hostname are different so they are proxied while loading assets for critical CSS generation. Since the external URLs of the asset would contain the port in the request, it would be unreachable internally by the plugin.

Does this pull request change what data or activity we track or use?

No

Testing instructions:

• Specify a value for PORT_WORDPRESS in tools/docker/.env. The value should be something other than 80(as that would previously break). Read more.
• Restart your jetpack development environment and make sure you can now access the the dev site at the new port i.e. localhost:
• Try to regenerate CSS.
• Critical CSS should now generate without any issue.

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ⚠️ All commits were linted before commit.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• ✅ Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

---

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

---

Boost plugin:

• Next scheduled release: September 7, 2021.
• Scheduled code freeze: August 30, 2021.

[davidlonjon]

@eAdnan007,

This works well but I am wondering if instead/additionally to add the filter we could compare $_SERVER['HTTP_HOST'] (which includes the port) with a build parsed host that contains the host and the port (if not empty) as wp_parse_url gives us the host and port too. So a user having a different port does not have to have the filter in a MU plugin.

What do you think? Also Mark?
Also if we decide to keep the filter, we might want to document it in the developer docs as extra setup step for user using WordPress on a different port?

[thingalon]

@eAdnan007,

This works well but I am wondering if instead/additionally to add the filter we could compare $_SERVER['HTTP_HOST'] (which includes the port) with a build parsed host that contains the host and the port (if not empty) as wp_parse_url gives us the host and port too. So a user having a different port does not have to have the filter in a MU plugin.

What do you think? Also Mark?
Also if we decide to keep the filter, we might want to document it in the developer docs as extra setup step for user using WordPress on a different port?

Hmm. I was under the (incorrect) impression that the webserver was running inside docker at port 80, and that docker's external port was just mapping. But... you're right. $_SERVER['HTTP_HOST'] would do the job here. I can't believe I didn't check that.

@eAdnan007 - can you verify that using $_SERVER['HTTP_HOST'] in the check fixes it for you?

[haqadn]

@eAdnan007,

This works well but I am wondering if instead/additionally to add the filter we could compare $_SERVER['HTTP_HOST'] (which includes the port) with a build parsed host that contains the host and the port (if not empty) as wp_parse_url gives us the host and port too. So a user having a different port does not have to have the filter in a MU plugin.

What do you think? Also Mark?
Also if we decide to keep the filter, we might want to document it in the developer docs as extra setup step for user using WordPress on a different port?

Thank you @davidlonjon for keeping the focus on the right thing. Even though the proxy was failing due to docker not being able to external port internally, the actual issue was that the hostname comparison was not fully correct.

I suppose we don't need the filter anymore since I cannot really think of many use cases someone would need to allow cross-origin requests to skip proxy.

[davidlonjon]

Nice, this looks great to me now 👍

[davidlonjon]

👍

[thingalon]

This does not proxy files on external URLs with no port specified.

e.g.:

add_action( 'wp_enqueue_scripts', function () {
    wp_enqueue_style( 'my-plugin-custom-styles', 'https://wordpress.com/calypso/evergreen/10175.fbadd59c9ac7774351b4.min.css' );
} );

--> no proxy. I suspect the issue is the if statement highlighted inline:

[thingalon]

I suspect this if statement shouldn't be here. Shouldn't $resource_origin always start with $parsed['host']? Otherwise, $resource_origin will be empty for any URL without a port specified.

[haqadn]

That is a silly mistake 🤦🏽‍♂️

[thingalon]

👍 Nice!