Are there any security risks when submitting data-attribute data through AJAX?

Question

I have written a simple author subscription plugin. Basically, it shows a subscribe button similar to YouTube. When a (logged in) user clicks on it, they subscribe to that author, and will get notified by their posts.

I'm using AJAX to make the button not refresh the page, and a data-attribute to send the author ID to my function, but I'm not sure if this approach causes any risks.

<button class="subscribe_button" data-author-id="352" data-action="subscribe">

Could someone theoretically change this data-attribute's value to cause an SQL injection or something? It seems that posting data through AJAX is very vulnerable to attacks, more so than if it were just a PHP file. Am I justified into thinking this? Are there any risks involved with my code below?

<script>        
    ( function( $ ) {

        var ajaxurl = "<?php echo admin_url('admin-ajax.php'); ?>",
            subscriptions_container = $('.subscriptions_container');

        $(subscriptions_container).on('click', '.subscribe_button', function() {                    

            // Disable button temporarily
            $(this).unbind("click");

            var thisButton = $(this);

            // Define author_id and button action
            var author_id = $(this).data( "author-id"),
                action = $(this).data( "action") + '_callback';

            // Data to be sent to function
            var data = {
                'action': action,
                'security': '<?php echo $ajax_nonce; ?>',
                'author_id': author_id
            };

            // Send data with AJAX
            jQuery.post(ajaxurl, data, function(response) {
                thisButton.closest('.subscriptions').replaceWith(response);
            });
        });
    } )( jQuery );
</script>

And my PHP callback function:

function subscribe_callback() {
    check_ajax_referer( '*****', 'security' );

    if ( is_user_logged_in() ) {                
        global $wpdb;
        $table_name = $wpdb->prefix . "subscriptions";

        // Check if author_id is posted and if it's a number
        if ( isset($_POST['author_id']) && is_numeric($_POST['author_id'])  ) {
            $author_id = $_POST['author_id'];
            $subscriber_id = get_current_user_id(); 

            // check if author is not subscribing to himself, if not then add database query
            if ( $author_id != $subscriber_id ) {
                if ( $wpdb->insert(
                    $table_name,
                    array(
                        'subscriber_user_id' => $subscriber_id,
                        'author_user_id' => $author_id,
                        'email_notification' => 'yes',
                        'subscription_date' => current_time( 'mysql' )
                    ),
                    array(
                        '%d',
                        '%d',
                        '%s',
                        '%s'
                    )
                ) !== FALSE ) {
                    // add subscriber to usermeta
                    $author_subscriber_count = get_user_meta($author_id, 'subscribers', true);
                    $author_subscriber_count++; 
                    update_user_meta($author_id, 'subscribers', $author_subscriber_count);  

                    echo subscribe_button($author_id);          
                }
            }
        }
    }

    wp_die();
} add_action( 'wp_ajax_subscribe_callback', 'subscribe_callback' );

Answer 1

When dealing with submit forms, even if they are sent with AJAX, you must play by the Never trust user's input rule.

Every data-attribute can be changed, edited via Inspector. Your only trusted validation should be on the server side, as you did with:

if ( isset($_POST['author_id']) || is_numeric($_POST['author_id']) )

Personally, I would inverse the logic and first check all the attributes before starting any action.

check_ajax_referer( '*****', 'security' );

if ( ! isset($_POST['author_id']) && ! is_numeric($_POST['author_id']) ) {
      // tell the villain that this tower is watched
      wp_send_json_error( 'Wrong author ID!' );
}

// now is safe to cache
$author_id = $_POST['author_id'];

Stolen from Codex.

The parameters are passed through a URI encoding so I wouldn't worry too much about data type or casting.

Answer 2

No, it is not more or less risky than excepting data from the user via URL parameter or a POST request sent by an HTML form. For all of these methods applies the rule Andrei Lupu already mentioned: Consider each input as possibly harmful.

To protect your system you should start by validating the input against your expectation, if possible. E.g. you know that the user ID will always be numeric. After validation you have to escape special characters on context change. If you want to use a string as part of a SQL query, it's a context change and thus you must escape characters with a special meaning in an SQL context. In your case, wpdb::insert() doest that for you.

Another possible context change would be, if you want to use user input directly in a HTML context (print it to your template).

Another important step in a client-server-application is to verify the command was actually intended by the user. Otherwise you open your application to cross-side request forgery (CSRF) attacks. As you (obviously) use WordPress' nonce functions and the feature is only available for logged in users, you're command is fairly authenticated.

So from a today perspective, your code example is considered to be secure.

Answer 3

Data attributes can be easly overriden, for example by jQuery function data. I would do this with attributes using generated md5 from user id and login, for example $user_id . '-' . md5($user_id . $user_login). You php script should check user by:

• exploding attribute by - sign

• get user by id (before - sign)

• generate md5 and compare it with value after - sign