Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am creating a site that is very heavily relying on ajax. To stop major spamming on my site, I decided to implement a php system that checks how many actions have been made in the last 5 minutes. I wrote this fairly quickly and I was wondering if there are any major issues with my code. Read the footnote as to why I can't test this. If you see any major errors here, please let me know. Also, spelling errors would be the fault of autocorrect. Thank you sooo much!

<?php
    session_start();
    require_once(".conf.php");
    //Main config
    $verification = $_GET['verification_token'];
    $requestactions = $_GET['request_actions'];
    $username = $_SESSION['username'];
    $login = $_SESSION['login'];
    $ip = $_SERVER['SERVER_ADDR'];
    //Set up config
    $maxactions = 40;
    $maxactionserror = 'Hmm, it seems you exceded the max actions allowed here. Try again in a few, Thank you!';
    $actiontable = 'actions'
    $currentactions = 0;
    $timeout = 1;
    $cookieexpire = 600 //Time in MS
    $blockreason = 'exceeded action requests';
    //Query config
    $firstquery = mysql_query("SELECT * FROM '$actiontable' WHERE username = '$username'");
    $rows = mysql_num_rows($firstquery);

    function check() {  
        if ($rows) {
            if ($rows >= $maxactions) {
                setcookie['reasonblocked', $blockreason, time() + $cookieexpire);
                setcookie['ipblocked', $ip, time() + $cookieexpire);
                setcookie['blockfinished', 'timestamp', time() + $cookieexpire);       
                $blockquery = mysql_query("INSERT INTO '$actiontable'('eligible') VALUES ('0') WHERE 'username' = '$username'");
                echo $maxactionserror;
                unset($login);
                unset($username);
                session_destroy();
            } else {
                while ($get = mysql_fetch_array($firstquery)) {
                    $currentactions = $get['cactions'];
                    $newactions = $currentactions++;
                    $secondquery = mysql_query("INSERT INTO '$actiontable'('username', 'auth_token', 'cactions', 'userip') VALUES ('$username', '$verification', '$newactions', '$ip')");
                    echo 'success';
                    check();
                }   
            }
        } else {
            $secondquery = mysql_query("INSERT INTO '$actiontable'('username', 'auth_token', 'cactions') VALUES ('$username', '$verification', '1')");
            echo 'success';
        }   
    }
?>

PS. I am on vacation so I am unable to test this so that is why I posted on here.

share|improve this question

1 Answer

up vote 0 down vote accepted

Okay, so the first thing here: prevent yourself from sql injections. Of course you can escape your params but parametrized queries with PDO or so would be much better. You can also use mysqli - here's an example: http://www.php.net/manual/de/mysqli.prepare.php

Also this won't execute since you have syntax errors in there (ie a missing ; here: $cookieexpire = 600 //Time in MS).

Also, don't rely on cookies for this. If someone really wants to spam, it is really easy just to delete your cookies or just return them in a valid state... store the ipaddress (or ipaddr and the browser-string) and then save that in a database (or something like redis).

There is much more to talk about here, but this would be a good start.

share|improve this answer
Ok thanks. I noticed those few errors specifically the missing ; after I posted and I never fixed here. I am inserting the ip into a database. I don't know why I didn't post that here. Hmm. What else is wrong here? – Joe Torraca Mar 13 '12 at 21:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.