validating a "key" - Githubissues

mihaijoldis commented 9 years ago

Hey. Got a small issue. I`m using addQueryArgFilter to add a license_key param in the plugin file.

I can see that the param hits the update server when checking for updates but when clicking Update Now for a plugin the license field is not sent in the request.

Is there something else that needs to run to add the param to the download action ?

mihaijoldis commented 9 years ago

I think i`m getting a bit confused here. If you have a plugin that requires users to enter a license on their websites. Which action do you actually have to "validate" and allow or not to update ?

Just the get_metadata ?

YahnisElsts commented 9 years ago

addQueryArgFilter only adds parameters to update information requests. If you want to include it when the user clicks "Update Now", you need to also add it to the download_url field.

There are multiple ways to do that. The quickest approach would be to use the puc_pre_inject_update-$slug filter. It takes one argument - an instance of PluginUpdate_x_y. Modify the download_url property of this instance and return the (modified) instance. If you're curious, the relevant code is around line 505 of plugin-update-checker.php.

If you have a plugin that requires users to enter a license on their websites. Which action do you actually have to "validate" and allow or not to update ?

The only action you have to validate in the update server is download. However, it might also be a good idea to check the key in the get_metadata action and return a different response to users who don't have a valid key.

Personally, I let all users see update details, but I remove the download_url field from data sent to unauthorized users. This way people who haven't entered a key (or have an invalid/expired key) can still see there is an update available, but they don't get an "Update Now" link.

mihaijoldis commented 9 years ago

I like your approach and i think thats what i'd like to achieve too. So at the moment addQueryArgFilter is adding the license in my plugins.

In my server extended class i'd have a checkAuthorization function. I should have 2 conditions one for download action and one for get_metadata.

Or if i understood from your approach validate the key in the get_metadata action and if the key is invalid just remove the download_url ?

YahnisElsts commented 9 years ago

My checkAuthorization looks something like this (sensitive stuff redacted):

protected function checkAuthorization($request) {
    parent::checkAuthorization($request);

    //Prevent download if the user doesn't have a valid license.
    $license = somehowLoadLicense();
    if ( $request->action === 'download' && ! ($license && $license->isValid()) ) {
        if ( !isset($license) ) {
            $message = 'You must provide a license key to download this plugin.';
        } else {
            $error = $license->get('error');
            $message = $error ? $error : 'Sorry, your license is not valid.';
        }
        $this->exitWithError($message, 403);
    }
}

I remove the download_url in the filterMetadata method:

protected function filterMetadata($meta, $request) {
    $meta = parent::filterMetadata($meta, $request);

    //Only include the download URL if the license is valid.
    $license = somehowLoadLicense();
    if ( $license && $license->isValid() ) {
        //Append the license key the automatically generated download URL.
        $args = array(
            'license_key' => $request->param('license_key'),
            'license_site_url' => $request->param('license_site_url'),
        );
        $meta['download_url'] = self::addQueryArg($args, $meta['download_url']);
    } else {
        unset($meta['download_url']);
    }

    return $meta;
}

I hope that helps.

mihaijoldis commented 8 years ago

sorry to bump an old issue but i need some assistance. i'm adding the license key using the filterMetadata and but when i try to update a plugin i keep getting Download failed. Service Unavailable

this is the checkAuthorization function

    protected function checkAuthorization( $request ) {
        parent::checkAuthorization($request);

        $license_key = stripslashes( $request->param('license_key') );
        //Prevent download if the user doesn't have a valid license.
        if( $request->action === 'download' && ! is_valid_license( $license_key ) ) {
                $this->exitWithError( 'Sorry, your license is not valid', 403);
        }
    }

is_valid_license is a function that returns false or true and the download_url is appended correctly if the license is valid. On the updates page Wordpress is showing the url its downloading the package from and that url works fine if i copy paste it in browser the validation works correctly

YahnisElsts commented 8 years ago

Interesting. Some ideas:

Look at the PHP error log on the server that hosts the package. "Service unavailable" could indicate that there was a PHP error.
Use a traffic analyzer (e.g. Wireshark) to see what request(s) WordPress sends and what response it gets. Seeing the full response details could help track down the problem. Compare the requests headers used by WordPress with the headers used by your browser. Maybe there's some discrepancy there that's causing the problem.
Is the package hosted on the same server as your test site? WordPress has an opt-in security feature where it will block requests to certain local domains. That can lead to updates failing for no apparent reason. The latest master version includes a workaround for this problem.

mihaijoldis commented 8 years ago

the update server is inside an updates/ directory on the same main public_html/ where the WordPress is. But the thing is that if my entire custom class is just

class SecureUpdateServer extends Wpup_UpdateServer {
}

then the updating works just fine.

mihaijoldis commented 8 years ago

BTW i'm using just this inside my plugin: https://github.com/YahnisElsts/plugin-update-checker/blob/master/plugin-update-checker.php without the other files

jacobhenke commented 8 years ago

ini_set("display_errors", 1);

And I would recommend Chrome Dev Tools before Wireshark. I think Wireshark is a little more low level than is needed here. (IMHO. It's a little overwhelming if you haven't played with it before.)

jacobhenke commented 8 years ago

Also, on the WordPress side: define( 'WP_DEBUG', true ); near the bottom of your wp-config file. https://codex.wordpress.org/Debugging_in_WordPress

mihaijoldis commented 8 years ago

Yeah i got debug enabled. No errors at all.

YahnisElsts commented 8 years ago

Correct me if I'm wrong, but Chrome Dev tools isn't going to work here. It only catches AJAX requests. It won't show requests sent by PHP scripts via the WordPress HTTP API or something like Curl.

Yeah i got debug enabled. No errors at all.

Hmm, are you 100% sure you're looking at the right file? What happens if you intentionally trigger an error?

jacobhenke commented 8 years ago

@YahnisElsts, Sorry, I re-read his comment after posting my own and realized it was happening on the WordPress side. I guess I have to actually read things before I can try to be helpful!

mihaijoldis commented 8 years ago

this is really erally weird..tested the same plugin code on a different plugin and its working just fine. Could be that specific plugin with issues ? More on this while testing locally using MAMP i get this error Could not create directory. gold-cart/merchants/wpec_auth_net/classes/anet_php_sdk/lib/net/authorize/api/contract/v1/TransactionResponseType/ErrorsAType

possibly the "updater" or the server side are having issues when the plugin has a pretty huge directory structure ?

YahnisElsts commented 8 years ago

possibly the "updater" or the server side are having issues when the plugin has a pretty huge directory structure ?

Maybe, but that's a separate issue. It probably wouldn't cause a "download failed" error.

Does the first plugin use any hooks that could affect how HTTP requests are handled? Anything that would add or change query parameters, or modify URLs, or anything else the other plugin doesn't do? How about other plugins that are active on the same site? Could one of them be interfering with the download?

mihaijoldis commented 8 years ago

I think its something in my code. I left my extended class empty without any functions and its working correctly.

Any way i can send u my extended server to take a look at it ? Its really small and easy but there might be something i`m missing there.

YahnisElsts commented 8 years ago

Sure. My email is whiteshadow@w-shadow.com

Edit: However, it's pretty late, so I probably won't get a chance to look at the code today.

SwiftThemes commented 6 years ago

Is there a filter similar to puc_pre_inject_update-$slug for themes.

I can see that the param hits the update server when checking for updates but when clicking Update Now for a plugin the license field is not sent in the request.

Same issue, but with a theme.

YahnisElsts commented 6 years ago

Yes, it's something like puc_pre_inject_update_theme-$slug. I recommend using the addFilter utility method for this - it will automatically add the necessary prefix and suffix to the filter tag. For example:

$updateChecker->addFilter('pre_inject_update', 'my_callback')

SwiftThemes commented 6 years ago

Thank you. This filter is running 5-6 times? Is that expected?

YahnisElsts commented 6 years ago

I've never counted, but that seems plausible. It would run every time some code requests the update_plugins transient.

YahnisElsts commented 6 years ago

I did some testing and it looks like it would run at least 4 times on every admin page. This is because WordPress shows the number of available updates in the admin menu and the Toolbar (a.k.a Admin Bar). To get that information, WP core calls the wp_get_update_data function which gets the update_plugins transient once and also calls wp_get_translation_updates, which loads the same transient again. This happens twice (admin menu + Toolbar) for a total of 4 filter runs.

Pages that show update-related information in more places - like "Plugins -> Installed Plugins" or "Dashboard -> Updates" - will trigger the filter several more times. So seeing it go off 5-6 times is not that surprising.

SwiftThemes commented 6 years ago

Here is my hacky solution to add license key to the download url. Is there a better way?

<?php
$theme_update_checker->addFilter( 'pre_inject_update', 'my_filter_download_link' );

function my_filter_download_link( $query_args ) {
    $url_components = parse_url( $query_args->download_url );
        //Skip the filter if the get params are already added.
    if ( strpos( $url_components['query'], 'license_key' ) ) {
        return $query_args;
    }
    if ( $url_components['query'] ) {
        $query_args->download_url .= '&license_key=' . get_theme_mod( 'license_key' ) . '&url=' . urlencode( get_site_url() );
    } else {
        $query_args->download_url .= '?license_key=' . get_theme_mod( 'license_key' ) . '&url=' . urlencode( get_site_url() );
    }

    return $query_args;
}

YahnisElsts commented 6 years ago

You can use the add_query_arg function to significantly simplify code.

YahnisElsts / wp-update-server

validating a "key" #30