0
\$\begingroup\$

In a project I've started, I decided to implement some new code to handle AJAX requests.

The requirements are that it can ONLY access the /ajax/ folder, load translations from another folder (/lang/) and that you MUST be authentificated previously.

This code is currently in use in a CMS.

Here is the ajax.php file (placed on the root of your server's public_html folder):

<?php
    define( 'ROOT', $_SERVER['DOCUMENT_ROOT'] ); //define the root beforehand, to avoid repetition

    //if you have the session cookie and the file exists and has the proper format (to avoid stuff like ../../../../../killall$\.php)
    if( isset( $_COOKIE[session_name()] ) && @is_file($file = ROOT.'/ajax/'.preg_replace('@^/([a-z_]+)\.php@', '$1', $_SERVER['ORIG_PATH_INFO']).'.php') )
    {
        ob_start(); //to avoid to 'leak' non-JSON garbage
        session_start();

        //if you are an administrator and have a level higher or equal than 2
        if(isset($_SESSION['admin']) && $_SESSION['admin']['level']>=2)
        {
            //database file, returns a mysqli_connection() result or false
            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 $out = array('error '=> true, 'type' => 'link'); //indicates which error was it (in this case, a communication error)

            if( isset($_REQUEST['debug']) && $_SESSION['admin']['level']>=5)
            {
                //debug informations sent when the parameter ?debug is added
                //only allowed for administrators with level higher than 5
                $out['__debug'] = array(
                    'output' => ob_get_clean(),
                    'mysql' => array(
                        'n' => @mysqli_errno( $link ),
                        'desc' => @mysqli_error( $link )
                    ),
                    '$_POST' => print_r( $_POST, true ),
                    '$_SERVER' => print_r( $_SERVER, true ),
                    '$_SESSION' => print_r( $_SESSION, true ),
                    'last_error' => @error_get_last()
                );
            }
            else ob_end_clean(); //or throw the output away, no need if you aren't level 5

            //outputs the nice JSON code to the browser, browsers LOVE this!
            echo json_encode( $out );
        }
        else
        {
            //if you aren't, you aren't allowed here
            header( 'HTTP/1.1 403 Forbidden' );
            echo '{"error":403}';
        }
    }
    else
    {
        //if none of the conditions was met, we (lie, if you don't have the cookie, and) send the error 404 
        header( 'HTTP/1.1 404 Not Found' );
        echo '{"error":404}';
    }

And the .htaccess file, in the same directory:

RewriteEngine on

RewriteRule ^/?ajax/([a-z_]+)\.php ajax.php/$1.php [L]

The files in the lang folder only have something like this:

return array();

Nothing fancy.

The $_SESSION has a defined structure, like this:

array(
    ['admin'] => array
    (
        ['id'] => 0,
        ['level'] => 5, //maximum is 5, for now
        ['active'] => 1, //0 or 1
        ['username'] => 'username',
        ['name'] => 'Full name',
        ['email'] => 'email@address.com',
        ['lang'] => 'en' //it can be 'pt', 'de' or other
    )
)

I'm using mysqli_* on this one, since the way it is used is very similar to the mysql_* functions.

To access a file, you simply access to /ajax/my_cool_file.php. If you want to get a hold on the debug informations, you append ?debug to the url like this: /ajax/my_cool_file.php?debug.

In terms of security, versitility, re-usability and readability; what do you think?

What can I do to save me a bunch of time to debug and solve other issues?

(A little outside the scope) What other 'cool' features can I implement?

\$\endgroup\$
  • \$\begingroup\$ Ouch, please don't use the @ operator of death. It supresses warnings/notices, that might warn you about issues that you should fix. If you drop something, pick it up. Don't sweep it under the rug. PS: the mysqli_* might look similar to mysql_* API at first, but spend 5 minutes reading through the docs, and you'll soon find out it's very different indeed. \$\endgroup\$ – Elias Van Ootegem Feb 11 '15 at 13:45
  • \$\begingroup\$ @EliasVanOotegem I know those and I'm fully aware that the API is different. I know they accept different parameters and that they return different things. I'm fully aware of that. I was just explaining why I did what I did. And regarding the death operator, I've removed most of it on part 2. \$\endgroup\$ – Ismael Miguel Feb 11 '15 at 13:50
1
\$\begingroup\$

Early Return

You say:

    if( isset( $_COOKIE[session_name()] ) && @is_file($file = ROOT.'/ajax/'.preg_replace('@^/([a-z_]+)\.php@', '$1', $_SERVER['ORIG_PATH_INFO']).'.php') )
    else
    {
        //if none of the conditions was met, we (lie, if you don't have the cookie, and) send the error 404 
        header( 'HTTP/1.1 404 Not Found' );
        echo '{"error":404}';
    }

Why lie? Why not replace this with

    if ( ! isset( $_COOKIE[session_name()] ) )
    {
        //if you aren't, you aren't allowed here
        header( 'HTTP/1.1 403 Forbidden' );
        echo '{"error":403}';
        exit();
    }

    if ( ! @is_file($file = ROOT.'/ajax/'.preg_replace('@^/([a-z_]+)\.php@', '$1', $_SERVER['ORIG_PATH_INFO']).'.php') )
    {
        //if the condition isn't met, we send the error 404 
        header( 'HTTP/1.1 404 Not Found' );
        echo '{"error":404}';
        exit();
    }

Now we give a reasonably correct error in either case.

This also exits and locks in the header. Your original code would keep trying to show the page.

This is called early return. It has the side benefit of putting the error message near to the error condition. Your original version had the normal flow close and the error message as far away as it could be.

You could then continue with

    ob_start(); //to avoid to 'leak' non-JSON garbage
    session_start();

    //if you aren't an administrator with a level higher or equal than 2
    if ( ! isset($_SESSION['admin']) || $_SESSION['admin']['level'] < 2)
    {
        header( 'HTTP/1.1 403 Forbidden' );
        echo '{"error":403}';
        exit();
    }

Avoid the Single Statement Form

You use the single statement version of an else twice, both times with the block then clause:

        }
        else $out = array('error '=> true, 'type' => 'link'); //indicates which error was it (in this case, a communication error)
        }
        else ob_end_clean(); //or throw the output away, no need if you aren't level 5

In general, it is probably better to use the same format for both the then and the else.

        }
        else
        {
            //indicate which error it was (in this case, a communication error)
            $out = array('error '=> true, 'type' => 'link');
        }

and

        }
        else
        {
            //or throw the output away, no need if you aren't level 5
            ob_end_clean();
        }

This also allows you to be more consistent with the location of your comments, always starting them on a new line.

A final reason is that there is a class of bug that comes from modifying a single statement else clause thinking that you are adding to the else when you are really doing something in the general case. I.e. forgetting to switch from the single statement version to the block version when the block version is needed. When it happens, this can be hard enough to debug to justify always using the block form.

It's also worth noting that you can get rid of the else entirely if you switch from ob_get_clean to ob_get_contents. Then you can simply always do an ob_end_clean. This would also simplify the logic if you modify the if with another clause later. Thus it is more robust in the face of modification.

\$\endgroup\$
  • \$\begingroup\$ Well, I really appreciate your review (probably a few misspellings here). I agree with most of what you said. I disagree with the idea of using the early return. This is to avoid a useless lookup on the filesystem and a regex match. That is really the only reason. I didn't though about the last part. That is one heck of an advice! I will use the ob_get_contents() as you suggested. It's faster and easier to write. Thank you! \$\endgroup\$ – Ismael Miguel Feb 11 '15 at 12:10
  • \$\begingroup\$ I've followed your advice regarding the early return. I've changed it to dry my code. Thank you for the tip about ob_get_contents(), once again. \$\endgroup\$ – Ismael Miguel Feb 11 '15 at 12:42
  • \$\begingroup\$ I've changed the code and made a new question. If you want, you can check it on codereview.stackexchange.com/questions/80257/…. Feel free to drop a comment or another answer. \$\endgroup\$ – Ismael Miguel Feb 11 '15 at 12:52
2
\$\begingroup\$

There are some thing which caught my eye. Im not the best on security related questions though (you can ask Security forum on the StackExchange network to check expert answers). But there are something related to readability and maintainability.

Maintainability often comes with good readability and use of proper functions. The very first thing that caught my eye was your extensive use of the silence operator @. This is usually a code smell as you cannot control errors. If you some day encounters a nasty bug you would have to remove them all (and insert them again when you are finished) to check every aspect of the code. To silence the script you can set the error reporting level using the following code.

/*
 * You can make sure errors doesn't get displayed, but still logged.
 *
 * display_errors should be set in the php.ini configuration file. If the value is
 * set during execution time fatal errors cannot be stopped.
 */
ini_set('display_errors', 0);
ini_set('log_errors', 1);

/*
 * Customize error reporting to avoid logging notices. 
 */
error_reporting(E_ALL & ~E_NOTICE);

/*
 * Or disable error reporting all together. This will also stop error logging.
 */
error_reporting(0);

This also has the nice side effect that you can still log errors and you now have a way to detect (and correct them).

Then there are some code structural related things. On the first real line of code you check if the file exists and use the silence operator unnecessarily since file_exists returns FALSE on missing file. But before you can check the file you run a regular expression. Consider extracting this regular expression into a variable. You also assign a variable inside a function. This can lead to some serious bugs later, be careful.

You then include a lot of files where you once more use the silence operator. If those files doesn't exist you have a more serious problem that cannot be ignored at runtime, since your code now isn't functioning. It is your responsibility as the developer to ensure all files exist.

You also have some inconsistencies around your if and else statements. Some of the time you surround them with curly brackets, sometimes not. It is considered bad practice to skip the curly brackets as you may in future add code to that code block without realizing the curly brackets are missing. This will then create an error. Always use them.

Then some minor things.

  1. To me the constant ROOT is redundant, since the value already exists in the $_SERVER superglobal.
  2. I also hope these comments are for the sake of this post and does not exist in your source code. You should only use comments when the code doesn't immediately reveal its true purpose. Use comments to help readability and make code-flow more visible.

Its nice to see you keep indentation levels, keep up the good work on that.

To me it seem a little overkill to return the values of the $_SERVER superglobal in the debug array. This superglobal contains much more information than the script uses and not all information are relevant for the purpose of debugging.

Hope this can guide you, happy coding!

\$\endgroup\$
  • \$\begingroup\$ Honestly, I MUST have the errors showing. Therefore, I use the ob_start(); to catch them and analise using the parameter ?debug. The errors are already being logged. Those comments DO exists in the production code. They have to. I'm not the only programmer. I have to keep track of what I did and leave explanations of EVERYTHING. The $_SERVER variable isn't overkill because I may need some informations from that variable. And each access may have different values. About the ROOT, it simplifies readability. And makes it easy to change the root in the future. \$\endgroup\$ – Ismael Miguel Feb 11 '15 at 12:17
  • \$\begingroup\$ The reason why I used the error supression was faulty logic which I changed now. It does return false on top, but also throws a big ugly warning if you check for a file that doesn't exist. Which at that point, the warning isn't important. About the regular expression, I know it isn't the best place there but I want to abuse the lazy evaluation and only run that when the session cookie is set. \$\endgroup\$ – Ismael Miguel Feb 11 '15 at 12:21
  • \$\begingroup\$ @IsmaelMiguel - You must have errors showing? This is bad practice as a potential malicious user can do harm with the information he might get. I would disable errors like I have shown and restore the previous configurations before ending/continuing the script. Regarding the $_SERVER debugging thing. How can different accesses change what information that script relies on? \$\endgroup\$ – AnotherGuy Feb 11 '15 at 12:23
  • \$\begingroup\$ The errors show, but they are being caught with ob_start(). I need those, sadly. Also, the $_SERVER has some information that might be useful on debug. Also, only level 5 users have access to that. Yes, it will hurt a lot but it is there only for debugging and we won't have the parameter ?debug set when we finish the CMS. \$\endgroup\$ – Ismael Miguel Feb 11 '15 at 12:26
  • \$\begingroup\$ @IsmaelMiguel - You said there are other developers. Then is is extremely important not to abuse lazy evaluation and things alike as it hinders readability and further increases the chances of errors. Regarding comments. If the other developers cannot understand the simple parts of the code you have written there might be bigger problems. If you want to make sure you can identify code you have written, I recommend using a version-system. \$\endgroup\$ – AnotherGuy Feb 11 '15 at 12:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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