GET call is Made to wp.org from health check

ipajen commented 4 weeks ago

On wp-admin/site-health.php?tab=debug it’s making a GET to http://Wp.org but not on AP for WP_Debug_Data::debug_data() wp-admin/includes/class-wp-debug-data.php:430 WP_Site_Health->show_site_health_tab() wp-admin/includes/class-wp-site-health.php:68 do_action('site_health_tab_content') wp-includes/plugin.php:517

namithj commented 3 weeks ago

Found 3 Calls on this page and all three are being rerouted

From Debug Log for this page.

[2024-10-22 17:15:05] [STRING]: Default API Found :https://api.wordpress.org

[2024-10-22 17:15:05] [STRING]: API Rerouted to :https://api.aspirecloud.org

[2024-10-22 17:15:07] [STRING]: Default API Found :https://api.wordpress.org/core/checksums/1.0/?version=6.6.2&locale=en_US

[2024-10-22 17:15:07] [STRING]: API Rerouted to :https://api.aspirecloud.org/core/checksums/1.0/?version=6.6.2&locale=en_US

namithj commented 3 weeks ago

Can you share the exact GET url to investigate further

ipajen commented 3 weeks ago

The GET call is going to https://wordpress.org and not https://api.wordpress.org The debug is not logging the GET call in AP.

Not its in the wp-admin/site-health.php?tab=debug (Go to Health Check and click "Info")

its correctly rerouting the POST on https://api.wordpress.org/core/version-check/1.7/ to https://api.aspirecloud.org/core/version-check/1.7/

I wonder if it could be the GET is to provide the information is the health check: Communication with WordPress.org | WordPress.org is reachable

costdev commented 2 weeks ago

@ipajen I wonder if it could be the GET is to provide the information is the health check: Communication with WordPress.org | WordPress.org is reachable

That's exactly what it is. Ref

asirota commented 2 weeks ago

So is this a bug? @namithj @costdev

ipajen commented 2 weeks ago

If I use AspireUpdate, is there any case when want we communications should go to WP and not AP? Hopefully not but if so it’s important it’s include in the docs.

namithj commented 2 weeks ago

It’s not a bug but something we should look at later. I won’t consider it relevant for V1. Wordpress strangely checks if the api is available by making a get call to Wordpress.org instead of api.wordpress.org.

It probability bad programming or an attempt to figure out if someone is bypassing the actual API.

we need to decide on how important it is, and whether we need to do anything to handle this call.

costdev commented 2 weeks ago

Agreed. Not a bug and not necessary for 1.0.0.

This is a callout to dotorg essentially as a basic check that, in theory, the site will be able to connect to the dotorg APIs, and to the RSS feeds for the dashboard widget, for example. It doesn't pass any query arguments and it's just a check for 200 response.

For completeness, I do think this is worth considering so that Site Health can report if the API's host can be connected to, rather than the user only finding out on certain screens.

Similar to the other requests, this can be filtered using 'pre_http_request', and checking the $url argument for https://wordpress.org (possibly also checking $args['method'] for 'GET' for a more specific check), then replacing with the host.

Since the host may respond with an API error code, it may be worth checking for a specific range of response codes, rather than simply 200. We can work that out though.

Note also that a 'gettext_default' filter may need to be added in the 'pre_http_request' callback to replace 'WordPress.org' references with the host in the test result string. This can then be unhooked inside its own callback to minimize performance impact.

We can take some time to investigate and determine the best and simplest approach.

I'm happy to put together a proof of concept for this early this week to get us started, unless someone else wants to jump on in.

costdev commented 2 weeks ago

If I use AspireUpdate, is there any case when want we communications should go to WP and not AP? Hopefully not but if so it’s important it’s include in the docs.

Maybe for the Dashboard RSS widget for news and events if there's no AspireUpdate mechanism to provide an alternative, or no alternative is provided.

While this may suggest adding a separate Site Health test for the API, I think it's okay to replace the existing dotorg test rather, since whether the dashboard widget can contact dotorg isn't particularly important.

namithj commented 2 weeks ago

This is taken care of, we just proxy the request to w.org when it’s not relevant.

costdev commented 2 weeks ago

@namithj Thanks! Can you link to where this is being handled?

namithj commented 2 weeks ago

@costdev its part of AspireCloud behaviour, not handled in the plugin side of things

costdev commented 2 weeks ago

Ah gotcha, thanks!

ipajen commented 4 days ago

What about change in health check : Communication with WordPress.org | WordPress.org is reachable to: Communication with aspirepress.org | aspirepress.org is reachable

and make a setting in AU where its possible to disable the news/events widget. Like https://github.com/dimadin/disable-wordpress-events-and-news-dashboard-widget

namithj commented 4 days ago

We can easily change that API call to aspirepress but it's just a check whether the API is accessible strangely done to w.org. The real health check happens in api.w.org itself. When we implement the endpoint the health check will work as usual (not sure if we need to do that) until then it will be mirrored and work as usual.

The question is whether we should ping aspirepress.org like w.org is doing. It's silly, it should be hitting api.w.org and reading the response to check (check itself is not required as we will get the status from the actual api call). Changing it might be problematic as we won't have context in the empty api call. This needs thought.

Regarding removing the news widget, that would be a policy decision and not a technical decision and requires triage. Let's move it to a separate ticket.

aspirepress / aspireupdate

GET call is Made to wp.org from health check #14