4
\$\begingroup\$

I have made a calculator in PHP and I need some feedback in terms of security and performance or any other general improvements.

You can download a zip file with all the contents from this link if you wish to try it out for yourself! I rewrote the source and comments from Norwegian to English, except the .css file. I also put in a export of the MySQL database I used, so it's possible to test out the database if necessary.

The calculator also contains some "easter eggs". This is a school project of mine, and the teacher said that those would add a bonus. There are currently two:

  1. Expand the log 5 times in a row (decreased for less clickyness)
  2. Make a calculation that results in 1337

enter image description here

My questions:

  1. Can something be improved with the filter function (security-wise)?
  2. Is there other practices for making a PHP calculator with support for cos,sin and tan?
  3. Are there any security flaws with the MySQL connection?
  4. Is there any major performance issues?

index.php

<?php
session_start();  //start use of sessions on this document

//This is a function that gets the result of the Resultval session, if not set it returns 0
function getResIfSet() {
    if (isset($_SESSION["Resultval"])) {
        echo $_SESSION["Resultval"];
    } else {
        echo 0;
    }
}
?> 

<!DOCTYPE html>
<html>
    <head>

        <meta charset="utf-8">                                      <!--Set charset to UTF-8-->
        <title>Net-Calculator</title>                               <!--Set title of window-->
        <link rel="stylesheet" type="text/css" href="style_eng.css">    <!--Link to external stylesheet: style.css-->

        <!--Import JQuery-->
        <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>    <!--Imorter JQuery biblotek for funksjonar.. etc-->

        <!--Start javascript with JQuery-->
        <script>

            //This functions just fades in the .box class when the page loads
            $(function() {
                $(".box").hide().fadeIn(500);
            });

            //Once document is ready..
            $(document).ready(function() {

                var position = false;   //Determines if the log is expanded or not
                var clicks = 0;         //Keeps a hold of how many times the #log_box is clicked (Easter egg)
                var res = "<?php getResIfSet(); ?>";    //The result value if user has calculated something (gets from php session *see top of document*)

                //if the result equals 1337 (Easter egg2)
                if (res == "1337") {
                    $(".mainwindow").animate({top: '-2000px'}, 2000);       //Make .mainwindow float far up
                    $("#elite").fadeIn(1000);                               //Fade in the #elite (img)
                    $("#elite_text").fadeIn(3000);                         //Fade in the #elite_text (paragaph)
                }

                //If the #log_box div is clicked..
                $("#log_box").click(function() {

                    //check the position value and expand/retract based on that
                    if (position === false) {

                        //make #log_box to auto height with animate.. i found this on the net, not that sure how it works tbh :D
                        $(this).animate({height: $("#log_box").get(0).scrollHeight});

                        position = true;    //invert the position variable

                        //This is the "OM NOM NOM" easter egg.. if the #log_box is expanded 5 times it turns it into a face!
                        if (clicks === 5) {
                            $(".mainwindow").fadeOut(5000);
                            $("h1").html("0&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;0");
                            $("#log_box").html("'OM NOM NOM!'<br><br><br><br><br><br><br>");
                            $("form").html("&nbsp;");
                            $(".box").css("display", "none");
                        } else {
                            //if clicks isnt 5.. increase it each time the #log_box expands
                            clicks++;
                        }

                    } else {
                        //make the #log_box to a single line only (compact)
                        $(this).animate({height: '20px'});

                        position = false;   //invert the position variable
                    }
                });
            });
        </script>
    </head>


    <body>

        <div id="mainwrap">                                 <!--MainWrap just centers the content on the webpage-->

            <div id="wrap2">                                <!--This is an additional div around the main wrap the 1337 image gets put in (Easter egg)-->

                <img src="1337.png" id="elite">             <!--Image gets loaded in, but is set to display:none in css by default unless 1337 is the result (Above)-->

                <p id="elite_text">Click <a href="index_eng.php">here</a> to go back!</p>  <!--Just a simple paragraph for the user to reload the page-->

                <div class="mainwindow">                    <!--This is the div where the "calculator" part is-->

                    <h1>Net Calculator</h1>                     <!--Create a text logo--> 

                    <form action="calculator.php" method="post">                            <!--Create the form to send data to calculator.php with the method post-->
                        <input id="input_bar" type="text" name="Input" autofocus>           <!--Make an text field with autofocus on, so the user dont need to click it-->         
                        <input id="submit_btn" type="submit" value="Rekn ut!">        <!--Finally a submit button-->
                    </form>

                    <?php

                    //This section prints out the "boxes" with errors, answers or warnings

                    //Check if Result session and Type session is set.. if they are, print out a box with the Result.. and Type defines the color of the border.
                    if (isset($_SESSION["Result"], $_SESSION["Type"]) && !empty($_SESSION["Result"]) && !empty($_SESSION["Type"])) {

                        //Example: <div class="box answer"><h3> 3 + 3 = 6 </h3></div>
                        //Note: Type defines the border of the box
                        echo '<div class="box ' . $_SESSION["Type"] . '"><h3>' . $_SESSION["Result"] . '</h3></div>';

                        //Clear the used sessions (also Resultval for 1337 easter egg) so it wont carry over if the phage is refreshed
                        unset($_SESSION["Result"]);
                        unset($_SESSION["Resultval"]);
                        unset($_SESSION["Type"]); 
                    }

                    //This next if prints out a warning to the user, used to notify mysql database errors, but the calculator still works.. so it needs an extra box.

                    //Check if the warning session is set
                    if (isset($_SESSION["Warn"]) && !empty($_SESSION["Warn"])) {

                        //This box doesen't have changing borders, so its set to yellow by default, but the warning message changes
                        echo '<div class="box warn"><h3>' . $_SESSION["Warn"] . '</h3></div>'; 

                        //then clear the used session once done.
                        unset($_SESSION["Warn"]);                                               
                    }
                    ?>

                    <!--This is a work in progress (WIP)-->
                    <div id="log_box">
                        <h3>Log (Click me!)</h3>

                        Just static text for now, but i plan to add this in the future.<br>
                        3*3 = 15<br>
                        15-3 = 13<br>
                        20/5 = 4<br>
                        (30*2)/3 = 20<br>
                        10+10 = 20<br>
                        <br>
                        The log will only be local based on sessions, since the database is just to log whats happening..<br>
                        Imagine if everyone saw what other users were doing.. Thats why this is gonna be local only.

                    </div>

                </div>
            </div>
        </div>
    </body>
</html>

calculator.php

<?php

//sets result session to the input of this function
function setResult($resultString) {
    $_SESSION["Result"] = $resultString;
}

//sets the warn session which indicates a warning (see index.php)
function setWarning($toWarn) {
    $_SESSION["Warn"] = $toWarn;
}

//sets the type of box to display (basicly color of the border)
function setBox($boxType) {

    //two things work here; answer and error
    $_SESSION['Type'] = $boxType;
}

//returns the user to the starts and ends the document
function returnToStart() {
    header("Location: index_eng.php");  //navigates the user back to index.php
    die();                          //end the document so it wont continue to load..
}

//get the input from the form index.php sent to this document.. if its empty.. set an error (bellow)
function getInput() {

    //if input session isnt set to anything...
    if (!empty($_POST["Input"]) && isset($_POST["Input"])) {

        return $_POST['Input']; //return the value if it is set

    } else {
        //if not..
        setResult("Error: Input is empty!");    //set the result to the error message
        setBox("error");                        //set the box type to error (red border)
        returnToStart();                        //and finally return the user back to index.php
    }
}

//filter the user input and error out if it doesent contain what it is supposed to contain
function filter($toFilter) {

    //first, remove all semicolons and whitespaces in the user input
    $toFilter = preg_replace('/[; ]+/', '', $toFilter);

    //then error out if input contains any other characters except those needed for sin(), cos() and tan()
    //this is to minimize harmful functions that you can write into the eval() function
    if (preg_match('/[bdefghjklmpqruvwxyzBDEFGHJKLMPQRUVWXYZ]+/', $toFilter)) {

        setResult("Error: Calculation contains unknown text. ");    //set the result to the error message
        setBox("error");                                            //set the box type to error (red border)
        returnToStart();                                            //and finally return the user back to index.php
    }

    return $toFilter;   //return the filtered input
}

//checks if the result string still contains text of any kind.. it it does thats an error..
function checkResult($toCheck) {
    if (preg_match('/[a-zA-Z]+/', $toCheck)) {
        return true;
    } else {
        return false;
    }
}

//log the result to the database
function logResult($input_filter, $result) {

    $servername = "127.0.0.1";      //server adress
    $username = "usernamehere";     //database username
    $password = "passwordhere";     //and password


    //create connection
    $conn = new mysqli($servername, $username, $password);

    //check if the connection was sucsessfull..
    if ($conn->connect_error) {

        setWarning("Warning: Could not connect to database.");    //write a warning if it couldnt connect to the database

    } else {

        //create a query string that inserts values into the database kalk_log at table log
        $query = "INSERT INTO `calc_log`.`log` (`Calculation`, `Result`, `FromIP`) "
                . "VALUES ('$input_filter', '$result', '" . $_SERVER['REMOTE_ADDR'] . "');";

        //run the query against the conenction..if it fails, display a message indicating it failed
        if ($conn->query($query) === FALSE) {
            setWarning("Warning: Couldn't put calculation into database.");  //set warning message, no need for box type since its always yellow by default
            $conn->close(); //finally close the connection!
        }
    }

    $conn->close(); //close connection if it still isnt
}

//this is a unfinished function, supposed to log to a session and display it on the local log (the one with click me)
function localLog($calculation, $result){
    //TODO: do stuff here
}

//The main method for solving a calculation (calls the other methods)
function solve() {

    echo 'Calculating...';                              //just a indication that the process has started

    $input_raw = getInput();                            //get the input and put it into a local variable
    $input_filter = filter($input_raw);                 //filter the input and assign it to new variable
    $result = eval('return ' . $input_filter . ';');    //calculate using eval and input and put it to a variable

    //if the result still contains text.. dont display it and error out
    if (checkResult($result)) {
        setResult("Error: Result contains unknown text.");      //write error message
        setBox("error");                                        //set the box to type error
        returnToStart();                                        //return the user to the start
    }

    if (!empty($result) || $result === 0) {                     //checks if the result is empty (which means error from eval) i use the or (||) to still display even if the actual calculation is 0 (as in 51-51)
        $_SESSION["Resultval"] = $result;
        setResult($input_filter . ' = <u>' . $result . '</u>'); //set result session to what it returns with some html tags to format it correctly
        setBox("answer");

        //You can comment out logResult to supress the warnings on the website
        logResult($input_filter, $result);                      //log the result to the database using the input and result, rest it figures out itself.
        localLog($input_filter, $result);                       //log to a session to display on the webpage (Work In Progress)
    } else {
        setResult('Error: Something went wrong when solving.'); //if the result is empty.. set an error message as the result
        setBox("error");                                        //set box type to error..
    }

    returnToStart();                                            //and return to the start..
}

session_start();    //start use of sessions in the document
solve();            //call the main method (solve) and figure out the calculation from the user

style.css

#mainwrap{
    min-width: 300px;
    max-width: 500px;
    margin-left: auto;
    margin-right: auto;
    margin-top: 50px;

}

body{
    background: url(bakgrunn.jpg) no-repeat center center fixed;
    -webkit-background-size: cover;
    -moz-background-size: cover;
    -o-background-size: cover;
    background-size: cover;
}

.mainwindow{
    position: relative;
    background-color: #dfdfdf;
    border: gray solid 3px;
    border-radius: 15px;
    padding: 15px;
    text-align: center;
    margin-bottom: 10px;
    font-family: cursive;
}

.box{
    background-color: whitesmoke;
    border-radius: 15px;
    padding: 15px;
    text-align: center;
    min-height: 30px;
    font-family: sans-serif;
    margin-bottom: 20px;

}

#log_box{
    background-color: whitesmoke;
    border: gray solid 3px;
    border-radius: 15px;
    padding: 15px;
    text-align: center;
    font-family: sans-serif;
    overflow: hidden;
    height: 20px;
}

.error{
    border: red solid 3px;
}
.warn{
    border: yellow solid 3px;
}
.answer{
    border: greenyellow solid 3px;
}

#input_bar{
    width: 75%;
    border: black solid 1px;
    border-radius: 5px;
}

#submit_btn{
    width: 75px;
    margin-top: 10px;
    margin-bottom: 25px;
    border: black solid 1px;
    background-color: white;
    border-radius: 5px;
}

#submit_btn:hover{
    background-color: lightcoral;
}

h1{
    color: black;
}

h3{
    padding:0;
    margin:0;
    margin-bottom: 15px;
}

table{
    width: 100%;
    border-collapse: collapse;
    border: grey solid 2px;
    border-radius: 15px;
    margin-top: 15px;
}

th,td{
    border: gray solid 2px;
    width: 50%;
    padding: 7px;
}

#elite{
    display:none;
    position: absolute;
    width: 100%;
    height: 300px;
    left: 0px;
    z-index: -1;
}

#elite_text{
    display:none;
    position: relative;
    top: 300px;
    color: red;
    text-align: center;
    width: 100%;
}

#wrap2{
    position:absolute;
    width: 500px;
    height: auto;
}
\$\endgroup\$
  • \$\begingroup\$ Your 5 clicks easter egg actually takes 6 clicks to activate \$\endgroup\$ – bumperbox Nov 13 '14 at 23:50
2
\$\begingroup\$

Search for ### and you will find my comments in your code

<?php

//sets result session to the input of this function
function setResult($resultString) {
    $_SESSION["Result"] = $resultString;
}

//sets the warn session which indicates a warning (see index.php)
function setWarning($toWarn) {
    $_SESSION["Warn"] = $toWarn;
}

//sets the type of box to display (basicly color of the border)
function setBox($boxType) {

    //two things work here; answer and error
    $_SESSION['Type'] = $boxType;
}

//returns the user to the starts and ends the document
function returnToStart() {
    header("Location: index_eng.php");  //navigates the user back to index.php

    // ### die indicates failure, use exit instead
    // ### die();                          //end the document so it wont continue to load..
    exit;
}

//get the input from the form index.php sent to this document.. if its empty.. set an error (bellow)
// ### think about your function names, a function called getInput that redirects to the start isn't really getting input is it?
// ### perhaps a better name, getInputOrReturnToStart()
function getInput() {

    //if input session isnt set to anything...

    // ### if (!empty($_POST["Input"]) && isset($_POST["Input"])) {
    // ### empty tests for isset as well
    if (!empty($_POST["Input"])) {

        return $_POST['Input']; //return the value if it is set

    } else {
        //if not..
        setResult("Error: Input is empty!");    //set the result to the error message
        setBox("error");                        //set the box type to error (red border)
        returnToStart();                        //and finally return the user back to index.php
    }
}

//filter the user input and error out if it doesent contain what it is supposed to contain
function filter($toFilter) {

    //first, remove all semicolons and whitespaces in the user input
    $toFilter = preg_replace('/[; ]+/', '', $toFilter);

    //then error out if input contains any other characters except those needed for sin(), cos() and tan()
    //this is to minimize harmful functions that you can write into the eval() function
    if (preg_match('/[bdefghjklmpqruvwxyzBDEFGHJKLMPQRUVWXYZ]+/', $toFilter)) {

        setResult("Error: Calculation contains unknown text. ");    //set the result to the error message
        setBox("error");                                            //set the box type to error (red border)
        returnToStart();                                            //and finally return the user back to index.php
    }

    return $toFilter;   //return the filtered input
}

//checks if the result string still contains text of any kind.. it it does thats an error..
function checkResult($toCheck) {

    // ### simplify code
//    if (preg_match('/[a-zA-Z]+/', $toCheck)) {
//        return true;
//    } else {
//        return false;
//    }

    return preg_match('/[a-zA-Z]+/', $toCheck);
}

//log the result to the database
function logResult($input_filter, $result) {

    // ### i realise this is a simple project, but it is best to store config data in constants at start of script
    // ### or in a separate file, not in the middle of your code
    $servername = "127.0.0.1";      //server adress
    $username = "usernamehere";     //database username
    $password = "passwordhere";     //and password


    //create connection
    $conn = new mysqli($servername, $username, $password);

    //check if the connection was sucsessfull..
    if ($conn->connect_error) {

        setWarning("Warning: Could not connect to database.");    //write a warning if it couldnt connect to the database

    } else {

        // ### always escape sql parameters, as external parameters could have been interfered with
        // ### what if $_SERVER['REMOTE_ADDR'] is not set?
        $remote_addr = mysqli_real_escape_string($_SERVER['REMOTE_ADDR']);

        //create a query string that inserts values into the database kalk_log at table log
        $query = "INSERT INTO `calc_log`.`log` (`Calculation`, `Result`, `FromIP`) "
            . "VALUES ('$input_filter', '$result', '" . $remote_addr . "');";

        //run the query against the conenction..if it fails, display a message indicating it failed
        if ($conn->query($query) === FALSE) {
            setWarning("Warning: Couldn't put calculation into database.");  //set warning message, no need for box type since its always yellow by default
            $conn->close(); //finally close the connection!
        }
    }

    // ### if there is a conn error you are trying to close a connection that was never opened?
    // ### Warning: mysqli::close(): Couldn't fetch mysqli
    $conn->close(); //close connection if it still isnt
}

//this is a unfinished function, supposed to log to a session and display it on the local log (the one with click me)
function localLog($calculation, $result){
    //TODO: do stuff here
}

//The main method for solving a calculation (calls the other methods)
function solve() {

    echo 'Calculating...';                              //just a indication that the process has started

    $input_raw = getInput();                            //get the input and put it into a local variable
    $input_filter = filter($input_raw);                 //filter the input and assign it to new variable
    $result = eval('return ' . $input_filter . ';');    //calculate using eval and input and put it to a variable

    //if the result still contains text.. dont display it and error out
    if (checkResult($result)) {
        setResult("Error: Result contains unknown text.");      //write error message
        setBox("error");                                        //set the box to type error
        returnToStart();                                        //return the user to the start
    }

    if (!empty($result) || $result === 0) {                     //checks if the result is empty (which means error from eval) i use the or (||) to still display even if the actual calculation is 0 (as in 51-51)
        // ### you have set functions for every other use of $_SESSION except this one??
        $_SESSION["Resultval"] = $result;
        setResult($input_filter . ' = <u>' . $result . '</u>'); //set result session to what it returns with some html tags to format it correctly
        setBox("answer");

        //You can comment out logResult to supress the warnings on the website
        logResult($input_filter, $result);                      //log the result to the database using the input and result, rest it figures out itself.
        localLog($input_filter, $result);                       //log to a session to display on the webpage (Work In Progress)
    } else {
        setResult('Error: Something went wrong when solving.'); //if the result is empty.. set an error message as the result
        setBox("error");                                        //set box type to error..
    }

    returnToStart();                                            //and return to the start..
}

session_start();    //start use of sessions in the document
solve();            //call the main method (solve) and figure out the calculation from the user
\$\endgroup\$
  • \$\begingroup\$ This peice doesent work.. $remote_addr = mysqli_real_escape_string($_SERVER['REMOTE_ADDR']); it needs to be $remote_addr = $conn->real_escape_string($_SERVER['REMOTE_ADDR']); \$\endgroup\$ – Kad Nov 14 '14 at 8:25
4
\$\begingroup\$

Signal-to-Noise ratio

Take this with a grain of salt, I realize your teacher might actually have made that a requirement. Generally, comments should not be used to describe what the code is doing, as the code itself does a pretty good job of doing that itself. Comments should be used to explain why your code is doing something, in cases where you are doing something unusual or that is not obviously clear.

For example, these comments are not useful:

<?php
session_start();  //start use of sessions on this document
//This is a function that gets the result of the Resultval session, if not set it returns 0
function getResIfSet() {
    if (isset($_SESSION["Resultval"])) {
        echo $_SESSION["Resultval"];
    } else {
        echo 0;
    }
}
?> 

You could simply instead write function getResultIfSet() and the code would be self-explaining. Here is another example where the comments are not useful, and just make it harder to read:

//get the input from the form index.php sent to this document.. if its empty.. set an error (bellow)
function getInput() {
    //if input session isnt set to anything...
    if (!empty($_POST["Input"]) && isset($_POST["Input"])) {
        return $_POST['Input']; //return the value if it is set
    } else {
        //if not..
        setResult("Error: Input is empty!");    //set the result to the error message
        setBox("error");                        //set the box type to error (red border)
        returnToStart();                        //and finally return the user back to index.php
    }
}

Instead this would read better:

function getInput() {
    if (!empty($_POST["Input"]) && isset($_POST["Input"])) {
        return $_POST['Input'];
    } else {
        setResult("Error: Input is empty!");
        setBox("error");                    
        returnToStart();                    
    }
}

You did make some comments that were helpful, where the code itself was not clear why you are doing something unusual looking:

            //if the result equals 1337 (Easter egg2)
            if (res == "1337") {
                $(".mainwindow").animate({top: '-2000px'}, 2000);       //Make .mainwindow float far up
                $("#elite").fadeIn(1000);                               //Fade in the #elite (img)
                $("#elite_text").fadeIn(3000);                         //Fade in the #elite_text (paragaph)
            }

And here also:

                    //This is the "OM NOM NOM" easter egg.. if the #log_box is expanded 5 times it turns it into a face!
                    if (clicks === 5) {
                        $(".mainwindow").fadeOut(5000);
                        $("h1").html("0&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;0");
                        $("#log_box").html("'OM NOM NOM!'<br><br><br><br><br><br><br>");
                        $("form").html("&nbsp;");
                        $(".box").css("display", "none");
                    } else {
                        //if clicks isnt 5.. increase it each time the #log_box expands
                        clicks++;
                    }

As for the code itself, I think it looks good and well written, hopefully someone more experienced will find other things to say about it.

\$\endgroup\$
  • \$\begingroup\$ Yeah, the teacher demanded that we commented and explained everything we did. When i program for myself i don't write comments on self explaining prices of code. But i appreciate your feedback! \$\endgroup\$ – Kad Nov 19 '14 at 15:00
  • \$\begingroup\$ That's what I figured, not sure why some teachers ask that, I find it counter-productive and counter-intuitive. Glad you accepted the other answer as it is more constructive! \$\endgroup\$ – Phrancis Nov 19 '14 at 16:40

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.