YahnisElsts / plugin-update-checker

A custom update checker for WordPress plugins. Useful if you don't want to host your project in the official WP repository, but would still like it to support automatic updates. Despite the name, it also works with themes.
MIT License
2.26k stars 410 forks source link

Using REST API endpoint for download_url value #405

Closed MichelleBlanchette closed 3 years ago

MichelleBlanchette commented 3 years ago

Hi, @YahnisElsts. Thank you for this great resource. It's working great!

I'm building a custom REST API to serve the plugin update zip files. After numerous tests and backtrace investigations, I believe to have found the root of my issue.

Since you built your own wp-update-server, I was wondering if you're aware of this issue. When saving the download_url param returned in my json response, it seems WordPress actually strips all query string data. This causes the "Update Plugin" download request to break, returning an error like Plugin package not available. since my REST API requires the authentication token provided in the returned download_url.

Putting a debug log at /wp-admin/includes/class-plugin-upgrader.php#L286 shows the $current->response['my-plugin/my-plugin.php']->package does not contain the query params:

$current = get_site_transient( 'update_plugins' );
error_log(print_r($current, true));

For example, my server's returned download_url value may be

http://localhost:8000/wp-content/releases/my-plugin-2.0.0.zip?user=yourdomain&auth=d10532f0e9

but then the debug output on the client side just shows

http://localhost:8000/wp-content/releases/my-plugin-2.0.0.zip

In summary: Do you know if the download_url return value can support GET params? Thanks!

MichelleBlanchette commented 3 years ago

I've now found that WordPress in fact does keep GET params. (In my original post, I had gotten my tests mixed up and confused.)

So the problem actually seems to be that WordPress disregards the package URL if it doesn't contain .zip (I'm assuming).

When using a .zip path with query params, it does keep the full package URL all the way through and successfully installs the update.

However, using my REST API URL seems to be rejected because $current->response['my-plugin/my-plugin.php']->package is blank in all error log cases that I have being logged. For example, if download_url is:

http://localhost:8000/wp-json/my-server/v1/releases/my-plugin/latest?user=yourdomain&auth=d10532f0e9

Also, my REST endpoint never receives a download request. This leads me to think WordPress validates the package URL and it fails (perhaps if no .zip is matched), causing WordPress to not send a request at all.

YahnisElsts commented 3 years ago

wp-update-server also uses query parameters and its download URLs do not contain the .zip extension by default, so I don't think that could be the problem. However, your assumption that WordPress validates URLs is correct. It's been a while since I looked at that part of the WP core; could it be that it doesn't like the custom port 8000?

Also, I recommend installing the Debug Bar plugin. Open the "PUC (your-slug)" section and see if it shows the correct download URL there. If it does not, the URL is getting messed up somehow before it even gets to WordPress core.

MichelleBlanchette commented 3 years ago

Thank you for confirming, @YahnisElsts! That helped me solve my issue much more quickly!

So it seems the WordPress REST API library only supports JSON responses..? Whatever the matter, it seems the responses were getting transformed into Unicode rather than staying in binary. I've yet to find how to send binary via a WP_REST_Reponse object, which is what is expected to be returned when registering a WP REST endpoint. Maybe it only supports UTF JSON..? šŸ¤·

Sigh... Regardless, I'm now back in business! The missing URL was in fact due to having a variable get set in the wrong place (that I just so happen to notice 2mins in to doing a code review). Thanks to your advice, Debug Bar helped rapidly confirm that fix!

Then I was getting a new installation error:

The package could not be installed. PCLZIP_ERR_BAD_FORMAT (-10) : Unable to find End of Central Dir Record signature

That's when I realized my hunch about the curl displaying Unicode rather than warning about binary output was an actual issue. So I had my function in the REST API endpoint's callback do the typical header() calls with a readfile() and all was solved. It'd be cool to return binary in the REST API "the WordPress way", but I'm probably just using it in a way that wasn't intended. Who knows. šŸ˜…

This has been a 3-day shenanigan, after a 3-day Docker shenanigan. It's been a long week! šŸ˜©

I hope this helps someone else, though, that comes across this thread.

Thanks again, @YahnisElsts. You're awesome! šŸ†

MichelleBlanchette commented 3 years ago

By the way, one last gotcha is the localhost ports, like @YahnisElsts mentioned.

I didn't mention in my original post, but, yes, that was the issue causing this error:

An error occurred while updating {Your Plugin}: Download failed. A valid URL was not provided.

In all my error logging and backtrace prints in WP Core, I found a call to wp_http_validate_url() which caused the localhost:8000 URL to be rejected. Simply commenting out those lines caused it to pass validation.

However, editing WP Core obviously isn't a long-living solution, so I've now implemented the following filter hook to have my localhost URLs bypass validation:

add_filter( 'http_request_args', function( $parsed_args ) {
    $parsed_args['reject_unsafe_urls'] = false;
    return $parsed_args;
}, 999 );

Please don't implement this in a production environment as it is a security risk!

YahnisElsts commented 3 years ago

Looking at WP_REST_Server::serve_request(), it appears that there's no way to make it output anything other than JSON. The method always calls wp_json_encode() on the response data. However, in theory, you could use the rest_pre_serve_request filter to bypass that part of server code and send the response yourself. Not sure if that's worth the effort, though.

I think you could also make the URL validation workaround safe(r) by:

MichelleBlanchette commented 3 years ago

That does sound like the solution, but you're right, it totally isn't worth it. Besides, this is more efficient, I imagine, since I don't need to buffer the entire contents of the zip into a string. I can just let readfile() operate as intended. The resulting code looks much cleaner in comparison, anyways.

Oh, yeah, I totally could. It's no worry for me, though, since I'm just doing this locally in my Docker containers. Ultimately, my REST API will be on my real server that has an actual domain pointing to it. So it shouldn't be any issue once deployed.

And by the way, 'http_request_reject_unsafe_urls' is not the right hook to use. I tested it, looked at WP Core, and quickly noticed the results of that hook get replaced with a call to wp_parse_args. That original filter only affects the default value, so it gets overridden when an actual argument is provided. Using the hook right after that is what does the trick, so I updated my previous reply. šŸ‘

YahnisElsts commented 3 years ago

Sounds good. It seems that the issue has been resolved (at least as much as it can be with a local site), so I'll close this now.