froger-me / wp-packages-update-server

WP Packages Update Server - Run Your Own Update Server for Plugins and Themes
GNU General Public License v3.0
141 stars 39 forks source link

Access_Token will be removed by July 1st, 2020 #18

Closed sekra24 closed 4 years ago

sekra24 commented 4 years ago

Hey @froger-me

first of all thank you for your great job with the wp-plugin-update-server repo. Github sent me an email where they announce that they will not support the access_token by July 1st, 2020.

This is the email from GitHub:

Hi @sekra24,

On February 14th, 2020 at 07:02 (UTC) your personal access token (reponame) using WordPress/5.3.2; https://www.domain.com was used as part of a query parameter to access an endpoint through the GitHub API:

https://api.github.com/repositories/XXXXXXXXXX/releases/latest

Please use the Authorization HTTP header instead, as using the access_token query parameter is deprecated and will be removed July 1st, 2020.

Depending on your API usage, we'll be sending you this email reminder once every 3 days for each token and User-Agent used in API calls made on your behalf. Just one URL that was accessed with a token and User-Agent combination will be listed in the email reminder, not all.

Visit https://developer.github.com/changes/2019-11-05-deprecated-passwords-and-authorizations-api/#authenticating-using-query-parameters for more information.

Thanks, The GitHub Team

froger-me commented 4 years ago

Hello @sekra24 ! Thanks for reaching out - I have uploaded a new version (1.4.14) to fix that.

As noted in the latest version of the readme.txt file:

Upgrading from 1.4.13 to 1.4.14 requires to upgrade the lib directory of all the packages delivered by WP Plugin Update Server due to a change in the Github API (deprecation of query parameters authentication). You can find the updated lib content in the integration examples in the wp-plugin-update-server/integration-examples directory.

For given settings:

=> the api calls are now done with: 'https://[user]:[token]@api.github.com/[user]/...

sekra24 commented 4 years ago

Hey @froger-me thank you! Great work! Sebastian

reynoldspaul commented 4 years ago

Hi @froger-me , great plugin thanks for your hard work.

I've installed latest today, with github package and get a deprecation email from github regarding URL query params with the access_token:

Github Email

Hi

On August 24th, 2020 at 09:48 (UTC) your personal access token (token_name) using WordPress/5.5; http://raiser-wp-website.localhost was used as part of a query parameter to access an endpoint through the GitHub API:

https://api.github.com/repositories/219022507/zipball/2.0.3

Please use the Authorization HTTP header instead, as using the access_token query parameter is deprecated. If this token is being used by an app you don't have control over, be aware that it may stop working as a result of this deprecation.

Depending on your API usage, we'll be sending you this email reminder on a monthly basis for each token and User-Agent used in API calls made on your behalf. Just one URL that was accessed with a token and User-Agent combination will be listed in the email reminder, not all.

Visit https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param for more information about suggested workarounds and removal dates.

Details from Github docs:

Using access_token as a query param

If you're currently making an API call similar to

curl "https://api.github.com/user/repos?access_token=my_access_token"

Instead, you should send the token in the header:

curl -H 'Authorization: token my_access_token' https://api.github.com/user/repos

froger-me commented 4 years ago

Hi @RaiserWeb ! Github have discontinued the use of access_token ; as stated in the main README.MD, I do not plan to actively maintain this project anymore. I do hope the community will be able to submit pull requests and keep it alive. Please feel free to fork the repo, adapt the code, and possibly send a pull request to share with everyone else.

Best regards, Alex

reynoldspaul commented 4 years ago

Hi @froger-me , thanks for the info.

I know you're not maintaining anymore, but I'm curious about a security related aspect, since the project feels well designed, but I feel there's a security hole -

When a plugin using the update checker hits the endpoint

website.com/wppus-update-api?action=get_metadata&package_id=plugin-slug&update_license_key=xxxx

the api returns a valid download link, and does not do validation on allowed domains/key expiry. The allowed domains validation is only done on a request to the wppus-license-api endpoint. Thought I'd flag this.

Thanks for the work you've done, it's a well rounded project.

froger-me commented 4 years ago

Hi @RaiserWeb !

I just tested what you mentioned - that's some distant memory right there! Makes me feel young again haha (yes, in dev time).

So, regarding the security bit and licensing, this is not exactly accurate:

Now, of course, a user can get hold of that signature in the database, and fiddle with metadata, adapt the code base to circumvent some checks (inject the domain in the code base, stuff like that), and do some manual work at each update.

Actually, let me give you an example: it is actually possible to get 1 subscription to, say WPML, and deploy updates on an unlimited amount of websites manually and hack the plugin to null the license check. But that's the thing: it's a lot of work, and most users will not have the know how. I do know how, I tried it for studying purposes and see how WPPUS could work, if it would be solid enough (I have a lifetime license so I don't need to do that sort of shady/possibly illegal things). But their business model remains sound, because it's a lot more than preventing a few tech-savvy people from downloading updates.

So if:

The only real way of preventing users from circumventing any type of licensing scheme would be to externalize a critical feature as a service executed externally on your own server after license check and then spit back the computed result to their website. Not only for updates, but each time the feature is used at page load. Anything short of that has workarounds (and depending on how the service is built, even that could have workarounds), and it's a trade-off, really.

I hope that clears out some things (I tried my best to explain in a not-too-boring way and hope it didn't feel like lecturing ;) ).

sekra24 commented 4 years ago

Hey @froger-me

wasn't that the solution for the access token deprecation? (see your reply below)

Best, Sebastian

Hello @sekra24 ! Thanks for reaching out - I have uploaded a new version (1.4.14) to fix that.

As noted in the latest version of the readme.txt file:

Upgrading from 1.4.13 to 1.4.14 requires to upgrade the lib directory of all the packages delivered by WP Plugin Update Server due to a change in the Github API (deprecation of query parameters authentication). You can find the updated lib content in the integration examples in the wp-plugin-update-server/integration-examples directory.

For given settings:

* Remote repository service URL `https://github.com/[user]/`

* Remote repository service credentials `token`

=> the api calls are now done with: 'https://[user]:[token]@api.github.com/[user]/...

reynoldspaul commented 4 years ago

Hi @froger-me , thanks for the security information. Yes, that all makes total sense. I didn't realise about the update_license_signature part of the api - I had it disabled for some reason during my testing.

If anyone is here for the GitHub download URL auth being depreciated, my work around was to use http_request_args filter to add in the Authentication header on the wp_safe_remote_get() call to retrieve the zipball.

    add_filter('http_request_args', function($parsed_args, $url)use($api){
        return $api->setUpdateDownloadHeaders($parsed_args, $url);
    }, 2, 10);
froger-me commented 4 years ago

@RaiserWeb @sekra24 Thanks for all the digging! I was doing some myself, and yes, there is another method in Puc_v4p4_Vcs_GitHubApi class that uses the access_token in query parameter: buildArchiveDownloadUrl. It should be built the same way as in buildApiUrl to satisfy the new Github requirements, and it's not... Below is how to rewrite part of the code base with Basic Authentication (using headers as @RaiserWeb demonstrated instead is a possibility too).

The first step is to change download_remote_package in class-wppus-update-server.php, calling wp_remote_get instead of wp_safe_remote_get (because the latter doesn't accept credentials in urls):

protected function download_remote_package( $url, $timeout = 300 ) {

        if ( ! $url ) {
            return new WP_Error( 'http_no_url', __( 'Invalid URL provided.', 'wppus' ) );
        }

        $local_filename = wp_tempnam( $url );

        if ( ! $local_filename ) {

            return new WP_Error( 'http_no_file', __( 'Could not create temporary file.', 'wppus' ) );
        }

        $response = wp_remote_get( $url, array(
            'timeout'  => $timeout,
            'stream'   => true,
            'filename' => $local_filename,
        ) );

        if ( is_wp_error( $response ) ) {
            unlink( $local_filename );
            error_log(  __METHOD__ . ' invalid value for $response: ' .  print_r( $response, true ) ); // @codingStandardsIgnoreLine

            return $response;
        }

        if ( 200 !== absint( wp_remote_retrieve_response_code( $response ) ) ) {
            unlink( $local_filename );

            return new WP_Error( 'http_404', trim( wp_remote_retrieve_response_message( $response ) ) );
        }

        $content_md5 = wp_remote_retrieve_header( $response, 'content-md5' );

        if ( $content_md5 ) {
            $md5_check = verify_file_md5( $local_filename, $content_md5 );

            if ( is_wp_error( $md5_check ) ) {
                unlink( $local_filename );
                error_log(  __METHOD__ . ' invalid value for $md5_check: ' .  print_r( $md5_check, true ) ); // @codingStandardsIgnoreLine

                return $md5_check;
            }
        }

        return $local_filename;
    }

Then, rewriting buildArchiveDownloadUrl like so:

/**
         * Generate a URL to download a ZIP archive of the specified branch/tag/etc.
         *
         * @param string $ref
         * @return string
         */
        public function buildArchiveDownloadUrl($ref = 'master') {

            if ( !empty($this->accessToken) ) {
                $url = sprintf(
                'https://%1$s:%2$s@api.github.com/repos/%3$s/%4$s/zipball/%5$s',
                    urlencode($this->userName),
                    urlencode($this->accessToken),
                    urlencode($this->userName),
                    urlencode($this->repositoryName),
                    urlencode($ref)
                );
            } else {
                $url = sprintf(
                    'https://api.github.com/repos/%1$s/%2$s/zipball/%3$s',
                    urlencode($this->userName),
                    urlencode($this->repositoryName),
                    urlencode($ref)
                );
            }

            return $url;
        }

The original uses the signDownloadUrl that adds the problematic query parameters:

                 /**
         * Generate a URL to download a ZIP archive of the specified branch/tag/etc.
         *
         * @param string $ref
         * @return string
         */
        public function buildArchiveDownloadUrl($ref = 'master') {
            $url = sprintf(
                'https://api.github.com/repos/%1$s/%2$s/zipball/%3$s',
                urlencode($this->userName),
                urlencode($this->repositoryName),
                urlencode($ref)
            );
            if ( !empty($this->accessToken) ) {
                $url = $this->signDownloadUrl($url);
            }
            return $url;
        }

                 /**
         * @param string $url
         * @return string
         */
        public function signDownloadUrl($url) {
            if ( empty($this->credentials) ) {
                return $url;
            }
            return add_query_arg('access_token', $this->credentials, $url);
        }

The thing is, signDownloadUrl is used in some other parts of the code base, so there may be other issues down the line if developers want to extend the plugin and make use of the libraries. These methods are part of the Plugin Update Checker library written by Yahnis Elsts ; the version of the library shipped in WPPUS is way outdated now, and updating to the latest would mean re-adapting the parts of the plugin that depend on it (that's precisely the reason why I dropped the support of this plugin, because it's not trivial).

I hope that helps!

Edit: actually tested the code, found it was not working, fixed above.