All-purposes AJAX handler made with PHP and .htaccess that speaks JSON - Part 2 Earlier, I've posted a question about a file used to handle AJAX requests.

I've followed some of the advices given and made some changes on the code itself.

You can go on "All-purposes AJAX handler made with PHP and .htaccess that speaks JSON" to read more information about it's working.

This is simply a follow-up, and I will only present here the PHP code:

&lt;?php
    define( 'ROOT', $_SERVER['DOCUMENT_ROOT'] ); //define the root beforehand

    //if the cookie isn't set or the session failed or you aren't an admin or your level is inferior to 2
    if( !isset( $_COOKIE[session_name()] ) || !@session_start() || !isset($_SESSION['admin']) || $_SESSION['admin']['level']&lt;2 )
    {
        //you aren't allowed here
        header( 'HTTP/1.1 403 Forbidden' );
        echo '{"error":403}';
    }
    //if the file exists and the name has the proper format (to avoid stuff like ../../../../../killall$\.php)
    else if( @is_file($file = ROOT.'/ajax/'.preg_replace('@^/([a-z_]+)\.php@', '$1', $_SERVER['ORIG_PATH_INFO']).'.php') )
    {
        ob_start(); //avoid 'leaked' non-JSON garbage
        session_start();

        //database file, returns the result of mysqli_connection()
        if( $link = include ROOT.'/bd/bd.php' )
        {
            //returns a massive array with the translations
            $text = include ROOT.'/lang/'.$_SESSION['admin']['lang'].'.php';

            //the included files return an array or nothing (requiring the (array) there)
            $out = (array)include $file;
        }
        else
        {
            //indicates which error was it (in this case, a communication error)
            $out = array('error' =&gt; true, 'type' =&gt; 'link');
        }

        if( isset($_REQUEST['debug']) &amp;&amp; $_SESSION['admin']['level']&gt;=5)
        {
            //debug informations sent when the parameter ?debug is added
            //only allowed for administrators with level higher than 5
            $out['__debug'] = array(
                'mysql' =&gt; array(
                    'n' =&gt; mysqli_errno( $link ),
                    'desc' =&gt; mysqli_error( $link )
                ),
                '$_POST' =&gt; print_r( $_POST, true ),
                '$_SERVER' =&gt; print_r( $_SERVER, true ),
                '$_SESSION' =&gt; print_r( $_SESSION, true ),
                'last_error' =&gt; error_get_last(),

                'output' =&gt; ob_get_contents()
            );
        }

        ob_end_clean(); //clears the errors

        //outputs the JSON code to the browser
        echo json_encode( $out );
    }
    else
    {
        //if none of the conditions was met, send the error 404 
        header( 'HTTP/1.1 404 Not Found' );
        echo '{"error":404}';
    }


Since the code looks a little more difference, I've made a new question instead of an edit in the older one.

Considering the new code, What else can I change?

What else can I improve?
There are two things I would refactor.

The first if statement (now) checks if the session has been initialized. I think this can be done in a more readable way by using the native PHP function session_status(). You can also write the admin check into a variable. This small refactor will make the if-statement more readable.

$session_name = session_name();

/*
 * Checks if a session exists and that the session was initialized
 * properly.
 */
$session_active = (session_status() == PHP_SESSION_ACTIVE);

/*
 * Checks if an administrator is logged in and that his
 * access level is greater than 2.
 */
$admin_authorized = (isset($_SESSION['admin'] &amp;&amp; $_SESSION['admin']['level'] &gt; 2)

if(!isset($_COOKIE[$session_name]) || !$session_active || !$admin_authorized) {

    header('HTTP/1.1 403 Forbidden');
    echo '{"error":403}';

    /*
     * Remember to exit to ensure execution is stopped.
     */
    exit; 

}


It doesn't hurt to write an extra variable if it helps readability. A too long or complex if-statement-body can hurt readability, because it can be hard to keep track of what conditions has been checked or what the condition actually does. By extracting the logic into variables you can also better document the specific piece of logic. Consider writing all documentation before the whole statement or before each variable definition.

Then there are the part where you check if the requested file exists. I think this should be refactored as well. You should also remove the silence operator. According to documentation is_file also checks if the file actually exists.

I would refactor this to:

/*
 * This may seem redundant, but experience has taught me that you should only
 * perform one manipulation on a variable per line. Its easier to read what is
 * actually happening and to modify it later.
 */
$file = preg_replace('@^/([a-z_]+)\.php@', '$1', $_SERVER['ORIG_PATH_INFO']);
$file = ROOT . '/ajax/' . $file . '.php';

if(!is_file($file)) {

    /*
     * The requested file does not exist.
     */
    header('HTTP/1.1 404 Not Found');
    echo '{"error":404}';
    exit;

}

// Continue execution as the file exists.


In the last post you also mentioned security. As said in that post I'm not security expert, but one thing that I have made a habit is to extract values from superglobals using the filter_input() function. It helps you filter out the input to better match your exact requirements. Imagine an input that was expected to provide a string, but was in fact an integer, due to a malicious user. This would break your code. As an example when extracting the ORIG_PATH_INFO you would write.

/*
 * There might be more/better filters. To add more filters delimit them
 * using the pipe-character: |
 */
$original_path_info = filter_input(INPUT_SERVER, 'ORIG_PATH_INFO', FILTER_SANITIZE_STRING);


This helps make sure the variable is actually a string. Its not bulletproof (no where near actually), but it could remove malicious code from a potential attacker.

Hope this helps, happy coding!