Prevent Updating AU via API

[namithj]

Pull Request

What changed?

Prevent Updating AU via API.
This PR prevents showing Aspire Update in the plugin search results as well as remove any update notifications for Aspire Update plugin to prevent the plugin from getting updated via the APIs.

Why did it change?

This is a special case consideration for AspireUpdate plugin considering its importance in the supply chain and to prevent Supply Chain Attacks.

Did you fix any specific issues?

Fixes #325
Fixes #324

CERTIFICATION

By opening this pull request, I do agree to abide by the Code of Conduct and be bound by the terms of the Contribution Guidelines in effect on the date and time of my contribution as proven by the revision information in GitHub.

[namithj]

This update was tested by changing the plugin slug to that of classic-editor since AU is not available in search results.

[afragen]

Where is it that you would expect AU to otherwise receive an update? This can quite possibly prevent any updates to AU.

I don't think it's needed and may be harmful.

[namithj]

This just hides AU from plugin search results and also from the updates section in dashboard. I have deliberately left the install action unaltered. This is needed as any plugin with the au slug can potentially hijack the plugin is served from an API or w.org. All it takes is for someone to snatch the slug.

[afragen]

You cannot stop someone from adding a plugin to dot org. The code you added will stop any update to AU. Even that from Git Updater Lite.

[afragen]

For the record, neither of those hooks are used to Install plugins.

[afragen]

This code doesn't do what you think it does.

[afragen]

Using the Update URI header effectively means that dot org would never be able to supply updates.

https://make.wordpress.org/core/2021/06/29/introducing-update-uri-plugin-header-in-wordpress-5-8/

[namithj]

Which of these functions might interfere with Git Updater Lite?

One is removing the plugin with the same slug from search results (Don't think GUL uses the plugins api). The other is removing it from the transient with list of plugins having an update available.

Again the purpose is to prevent other APIs configured in AU from overwriting AU with their own versions of AU (the reason for adding GUL). Additionally this only filters the search results when done from Wordpress admin.

[costdev]

@namithj Can you post a proof of concept code sample which filters the results to include the aspireupdate slug to demonstrate the problem?

At the moment, I'm not sure that the hypothetical problem applies in practice with some of WP core's checks etc internally.

Sample filter code would mean we can test without needing to change the plugin slug to something else and therefore make the test closer to real-world.

AspireUpdate should be installed, activated, and the version unbumped locally.

The filter code in a small test plugin will allow us to verify whether AspireUpdate can be found in search results and whether it can be updated from those search results even though it has the UpdateURI header.

[costdev]

The code you added will stop any update to AU. Even that from Git Updater Lite.

The sample code I requested above can also check this.

[namithj]

We can just ask @chuckadams to add AU back in into AC

[namithj]

That would also help us test this out in real world, given that AU slug is a bit different from the convention and knowing if this is even required.

The only way I see this might interfere with GUL is if it uses the Wordpress default transient to mark an available update, which I don't think is the case as it has its own scheduler

[afragen]

Git Updater Lite uses WP core functionality for updating. This means it adds an appropriate API response to both the update transient and the plugins_api() response.

aspireupdate/vendor/afragen/git-updater-lite/Lite.php

Lines 156 to 157 in a8e008e

	add_filter( "{$type}s_api", array( $this, 'repo_api_details' ), 99, 3 );
	add_filter( "site_transient_update_{$type}s", array( $this, 'update_site_transient' ), 15, 1 );

[namithj]

There is an is_admin() and $_REQUEST['s'] checks to get around that for plugins_api. Our filter works only for searched from WP admin.

For transients filter perhaps adding a is_admin() check can resolve that.

If that won't work, we will probably have to keep showing AU in search results and risk overwriting.

Or is there another solution?

[costdev]

(A) Showing AspireUpdate in search results and (B) overwriting are two different things though.

If AspireUpdate is already installed, the "Install" button won't be shown. It will either be "Activate" or "Active", depending on status. The "Update" button shouldn't appear unless GU is reporting a higher version because we have the UpdateURI header.

The concern hasn't yet been confirmed as an issue. That should be the focus for now, IMO.

[costdev]

I mentioned in Slack that I'd follow up with more info, so here's the situation.

The presence of the Update URI header should prevent a plugin or theme from being updated by the API.

However, this prevention is enforced by the API. Yes, you read that right. The API (i.e. WordPress.org by default) decides whether to honour a plugin's clear instruction, or whether to supply its own. Yes, this is a clear supply chain issue.

So... while this hasn't yet been resolved in WordPress Core, here's my recommended approach to get us started:

1. Add a new method to API_Rewrite; remove_non_api_assets( $assets );
2. Call this method when (the URL is rewritten) && (URL contains '/plugins/update-check/' || URL contains '/themes/update-check/' ). Pass the appropriate data (plugins or themes) to the new method.
3. The new method should, at least for this initial test, array_filter() -> return empty( $asset['UpdateURI'] );
4. Replace the original assets array with the modified assets array and send it to the API.
5. In the response, exclude any assets that weren't in the original request (e.g. If an API tries to force an update for a plugin it wasn't asked for).

The new method is just to keep the context contained and more easily testable.

This approach means the skipping of such assets is done client-side as it should be. However, we should also skip assets in the AspireCloud API when UpdateURI is present. This approach also should not impact GUL either.

When I can, I'll open a PR with the above and we can examine and test the approach.

[namithj]

This need to also be added to the search endpoint as the plugin will show up with an update button if it comes up in a search IF the current approach can won't work with GUL

[costdev]

The Update button won't show in the search result if the above approach is used, unless of course one is available from GUL.

The request that will be handled is the one that sets the transient, and that's used to determine whether to show the Update button.

[namithj]

We need to cover user searches too as the plugin can't turn up there, this would only handle update checks right?

[costdev]

We may indeed need to add to other endpoints, but I'd like to focus on the supply chain issue first and ensure that's robustly handled.

[namithj]

I think we can remove the transients filter from this PR and keep the results filter to cover searches