suggestions on cleaning up my coding

serverman · February 16, 2017

I'm working on my first real project that I coded (other than the user system). I could use some help to know what I could and should clean up in my coding. I am self taught. I know I need to add sanitize codes to the field inputs and that is in the works. Do you see something that would make more sense to be a function in the functions.php include?

Index.php

<?php
include_once 'includes/db_connect.php';
include_once 'includes/functions.php';

sec_session_start();
$PageTitle="Open Managment";
function customPageHeader(){?>
<script type="text/JavaScript" src="js/sha512.js"></script> 
<script type="text/JavaScript" src="js/forms.js"></script> 
  <!--Arbitrary HTML Tags-->
<?php }
include_once('includes/header.php');
if (isset($_GET['error'])) {
	echo '<p class="error">Error Logging In!</p>';
}
 if (login_check($mysqli) == true) : ?>
        <p>Welcome <?php echo htmlentities($_SESSION['username']); ?>! If you are done, please <a href="includes/logout.php">log out</a>.</p>
          <!--Arbitrary HTML Tags-->

<?php 
$results = $mysqli->query("SELECT id, title, address, occupied FROM property");
?>
<div>
	<div class="propr">
	<div class="propc"><h2>Property Title</h2></div>
	<div class="propc"><h2>Address</h2></div>
	<div class="propc"><h2>Occupied</h2></div>
</div>
<?php 
while($row = $results->fetch_array()) { ?>
<div class="propr">
	<div class="propc"><a href="list_prop.php?parent=<?= $row['id'] ?>"><?= $row["title"] ?></a></div>
	<div class="propc"><?= $row["address"] ?></div>
	<div class="propc"><?php if ($row["occupied"] == 1) {echo 'yes';} else { echo 'no'; }?></div>
</div>
<?php 
}  
print '</div>';

?>
  <p> Add a new property:</p>
  
   <form action="newprop.php" method="post">
        Property Name<input type="text" name="title">
        Address<input type="text" name="address">
        <input type="submit" value="Submit">   
        </form>  
<div>
<?php 
$results = $mysqli->query("SELECT id, first_name, last_name, email, phone FROM tenant");

while($row = $results->fetch_array()) { ?>
<div class="propr">
	<div class="propc"><a href="list_tenant.php?parent=<?= $row['id'] ?>"><?= $row["first_name"] ?></a></div>
	<div class="propc"><?= $row["last_name"] ?></div>
	<div class="propc"><?= $row["email"] ?></div>
	<div class="propc"><?= $row["phone"] ?></div>
</div>
<?php 
}  


$mysqli->close();
?></div>
<p>Add tenant</p>

 <form action="new_tenant.php" method="post">
        First Name<input type="text" name="fname">
        Last Name<input type="text" name="lname">
        Email<input type="text" name="email">
        Phone Number<input type="text" name="phone">    
        <input type="submit" value="Submit">   
        </form>   
        <?php
              else :
        	echo '<p> You are currently logged out. Please log in.</p>
        <form action="includes/process_login.php" method="post" name="login_form"> 			
            Email: <input type="text" name="email" />
            Password: <input type="password" 
                             name="password" 
                             id="password"/>
            <input type="button" 
                   value="Login" 
                   onclick="formhash(this.form, this.form.password);" /> 
        </form>';
        endif;
   ?>
    </body>
</html>

List_tenant.php

<?php
include_once 'includes/db_connect.php';
include_once 'includes/functions.php';

sec_session_start();
$PageTitle="Open Managment";
function customPageHeader(){?>

  <!--Arbitrary HTML Tags-->
<?php }
include_once('includes/header.php');

?>

<?php if (login_check($mysqli) == true) :     ?>
        
<p>Welcome <?php echo htmlentities($_SESSION['username']); ?>! If you are done, please <a href="includes/logout.php">log out</a>.</p>
<span> You are veiwing:</span>
 <?php 
//get tenant name and current var
 $a = intval($_GET['parent']);
 $results = $mysqli->query("SELECT id, first_name, last_name, current FROM tenant WHERE id = '$a'");
while($row = $results->fetch_array()) { 
echo $row["first_name"], '&nbsp', $row["last_name"];
$current = $row["current"];
}
$results->free();

 
if(isset($_POST['submit'])) {
 
 $prop_id = $_POST["prop_id"];
 $tent_id = $_GET['parent'];
 $rent = $_POST["rent"];
 $late = $_POST["late"];
 $start_date = $_POST["start_d"];

 $mysqli->query("
		INSERT INTO tenancy (property_id, tent_id, rent, late, start_date, current)
		VALUES ('$prop_id', '$tent_id', '$rent', '$late', '$start_date', '1' );
		");
 $mysqli->query("		
		UPDATE tenant SET current='1' WHERE id=' $tent_id';
		");
 $mysqli->query("
		UPDATE property SET occupied='1' WHERE id=' $prop_id';
		");
 $mysqli->close();
} elseif ($current == 0) {
?>
  
  <form action="list_tenant.php?parent=<?=$a?>" method="post">
  <select name="prop_id">
  <?php //get props for drop down 
  $results = $mysqli->query("SELECT id, address FROM property");
  while($row = $results->fetch_array()) { ?>
  	<option value="<?= $row['id'] ?>"><?= $row["address"] ?></option>
  <?php 
  }  
  ?>
  </select>
  Rent Amount<input type="text" name="rent">
  Late Fee<input type="text" name="late">
  Move in date<input type="text" name="start_d">
 <input type="submit" name="submit" value="submit"> 
 </form>
 
 <?php
 $mysqli->close();
 }
 elseif ($current == 1){
 	echo 'they live somewhere';
 }

 ?>

 <p>tenancy info:</p> 
  
 
 
<?php else : ?>
            <p>
                <span class="error">You are not authorized to access this page.</span> Please <a href="index.php">login</a>.
            </p>
        <?php endif; ?>
    </body>
</html>

functions.php

<?php


include_once 'psl-config.php';

function sec_session_start() {
    $session_name = 'sec_session_id';   // Set a custom session name 
    $secure = SECURE;

    // This stops JavaScript being able to access the session id.
    $httponly = true;

    // Forces sessions to only use cookies.
    if (ini_set('session.use_only_cookies', 1) === FALSE) {
        header("Location: ../error.php?err=Could not initiate a safe session (ini_set)");
        exit();
    }

    // Gets current cookies params.
    $cookieParams = session_get_cookie_params();
    session_set_cookie_params($cookieParams["lifetime"], $cookieParams["path"], $cookieParams["domain"], $secure, $httponly);

    // Sets the session name to the one set above.
    session_name($session_name);

    session_start();            // Start the PHP session 
    session_regenerate_id();    // regenerated the session, delete the old one. 
}

function login($email, $password, $mysqli) {
    // Using prepared statements means that SQL injection is not possible. 
    if ($stmt = $mysqli->prepare("SELECT id, username, password, salt 
				  FROM members 
                                  WHERE email = ? LIMIT 1")) {
        $stmt->bind_param('s', $email);  // Bind "$email" to parameter.
        $stmt->execute();    // Execute the prepared query.
        $stmt->store_result();

        // get variables from result.
        $stmt->bind_result($user_id, $username, $db_password, $salt);
        $stmt->fetch();

        // hash the password with the unique salt.
        $password = hash('sha512', $password . $salt);
        if ($stmt->num_rows == 1) {
            // If the user exists we check if the account is locked
            // from too many login attempts 
            if (checkbrute($user_id, $mysqli) == true) {
                // Account is locked 
                // Send an email to user saying their account is locked 
                return false;
            } else {
                // Check if the password in the database matches 
                // the password the user submitted.
                if ($db_password == $password) {
                    // Password is correct!
                    // Get the user-agent string of the user.
                    $user_browser = $_SERVER['HTTP_USER_AGENT'];

                    // XSS protection as we might print this value
                    $user_id = preg_replace("/[^0-9]+/", "", $user_id);
                    $_SESSION['user_id'] = $user_id;

                    // XSS protection as we might print this value
                    $username = preg_replace("/[^a-zA-Z0-9_\-]+/", "", $username);

                    $_SESSION['username'] = $username;
                    $_SESSION['login_string'] = hash('sha512', $password . $user_browser);

                    // Login successful. 
                    return true;
                } else {
                    // Password is not correct 
                    // We record this attempt in the database 
                    $now = time();
                    if (!$mysqli->query("INSERT INTO login_attempts(user_id, time) 
                                    VALUES ('$user_id', '$now')")) {
                        header("Location: ../error.php?err=Database error: login_attempts");
                        exit();
                    }

                    return false;
                }
            }
        } else {
            // No user exists. 
            return false;
        }
    } else {
        // Could not create a prepared statement
        header("Location: ../error.php?err=Database error: cannot prepare statement");
        exit();
    }
}

function checkbrute($user_id, $mysqli) {
    // Get timestamp of current time 
    $now = time();

    // All login attempts are counted from the past 2 hours. 
    $valid_attempts = $now - (2 * 60 * 60);

    if ($stmt = $mysqli->prepare("SELECT time 
                                  FROM login_attempts 
                                  WHERE user_id = ? AND time > '$valid_attempts'")) {
        $stmt->bind_param('i', $user_id);

        // Execute the prepared query. 
        $stmt->execute();
        $stmt->store_result();

        // If there have been more than 5 failed logins 
        if ($stmt->num_rows > 5) {
            return true;
        } else {
            return false;
        }
    } else {
        // Could not create a prepared statement
        header("Location: ../error.php?err=Database error: cannot prepare statement");
        exit();
    }
}

function login_check($mysqli) {
    // Check if all session variables are set 
    if (isset($_SESSION['user_id'], $_SESSION['username'], $_SESSION['login_string'])) {
        $user_id = $_SESSION['user_id'];
        $login_string = $_SESSION['login_string'];
        $username = $_SESSION['username'];

        // Get the user-agent string of the user.
        $user_browser = $_SERVER['HTTP_USER_AGENT'];

        if ($stmt = $mysqli->prepare("SELECT password 
				      FROM members 
				      WHERE id = ? LIMIT 1")) {
            // Bind "$user_id" to parameter. 
            $stmt->bind_param('i', $user_id);
            $stmt->execute();   // Execute the prepared query.
            $stmt->store_result();

            if ($stmt->num_rows == 1) {
                // If the user exists get variables from result.
                $stmt->bind_result($password);
                $stmt->fetch();
                $login_check = hash('sha512', $password . $user_browser);

                if ($login_check == $login_string) {
                    // Logged In!!!! 
                    return true;
                } else {
                    // Not logged in 
                    return false;
                }
            } else {
                // Not logged in 
                return false;
            }
        } else {
            // Could not prepare statement
            header("Location: ../error.php?err=Database error: cannot prepare statement");
            exit();
        }
    } else {
        // Not logged in 
        return false;
    }
}

function esc_url($url) {

    if ('' == $url) {
        return $url;
    }

    $url = preg_replace('|[^a-z0-9-~+_.?#=!&;,/:%@$\|*\'()\\x80-\\xff]|i', '', $url);
    
    $strip = array('%0d', '%0a', '%0D', '%0A');
    $url = (string) $url;
    
    $count = 1;
    while ($count) {
        $url = str_replace($strip, '', $url, $count);
    }
    
    $url = str_replace(';//', '://', $url);

    $url = htmlentities($url);
    
    $url = str_replace('&', '&', $url);
    $url = str_replace("'", ''', $url);

    if ($url[0] !== '/') {
        // We're only interested in relative links from $_SERVER['PHP_SELF']
        return '';
    } else {
        return $url;
    }
}

once I fix security issues I will upload my project to github. Im doing this as an open source property management app.

Edited February 16, 2017 by serverman

ginerjm · February 16, 2017

The first thing I would change is to separate your html from your php. Keep them apart; use php vars only in the html to pop in dymanic data where you want it. Don't build your html as you gather the data. A key indicator of this is not having to switch in and out of php mode all the time. My scripts use one php tag - a begin at the top of the script.

Don't bury functions in the mainline code. They are functions so that you can move that logic away from the main thought process.

Jacques1 · February 17, 2017

Trying to remove security vulnerabilities afterwards is a bad strategy which almost never works. Sooner or later, you're going to overlook a vulnerability or simply give up this tedious task. Instead, use secure programming techniques from the beginning. Don't ever let unsafe values near your SQL queries or HTML markup, not even for a quick prototype.

As to the HTML, I strongly recommend you use a template engine like Twig. While it's theoretically possible to generate HTML with plain PHP like people did in the 90s, this almost always ends up in an ugly mess of spaghetti code and XSS vulnerabilities. Twig prevents this by strictly separating the code from the markup, by auto-escaping all input and by providing a much nicer syntax.

Stuffing all your functions into one giant functions.php script is also a bad idea, because nobody wants to read through that. Instead, modularize your application. Put related functions into separate scripts.

The session and security stuff is copied and pasted from wikiHow, right? Don't do that. A lot of people publishing code on random websites know less about PHP than you do. Copypasta also means the code will never receive any updates and just keep rotting in your application. If you want to use existing tools, pick a trustworthy source (active GitHub projects with multiple contributors are good candidates) and use a mechanism which supports updates (e. g. Composer).

In your case, you can simply remove the wikiHow stuff and use the Password Hash API instead.

serverman · February 17, 2017

OK. Yeah my plan was to clean it up to make it more read able but maybe you are right and I should first attack the security then clean it. I got the login system off of github but yeah its a dead project on it. So I guess now I will dump the login system that I have, then fix all the XSS issues in my code. Then work in or write a new login system. I think removing the current member system will make it way easier to clean up the code.

Jacques1 · February 17, 2017

Forget about the XSS vulnerabilities and just use Twig. It will be a major improvement of both the quality and security of your application.

Learning the basics of Twig is simple. It's just HTML with a few tags for variable insertion, control flow statements etc.

Edited February 17, 2017 by Jacques1

serverman · February 17, 2017

I will think about it. Honestly the whole point of this project was really learn a lot more about php. I've just always used it to just get by when needed and I stopped doing web design work a few years ago.(Joined the military) but php is just a fun hobby I am trying to pick back up. For me I don't see the challenge in a framework. I need a challenge or I get bored.

Jacques1 · February 17, 2017

I'm not sure if doing low-level grunt work is really a challenge and a good way to learn PHP. Seems more like a waste of time.

I understand the scepticism towards libraries/tools/frameworks (not having full control, feeling that the programming part is taken away from you etc.). But this is mostly unfounded. To the contrary, getting rid of routine stuff lets you concentrate on the real programming challenges. When you no longer have to write the same PHPSQLHTML form handling code over and over again, you can think more about the actual features.

This also isn't a decision between plain old PHP and "a framework". You can use libraries within your application, you can use framework components, you can use microframeworks. There's a wide range of possibilities.

Sign In

suggestions on cleaning up my coding

Recommended Posts

serverman

Link to comment

Share on other sites

ginerjm

Link to comment

Share on other sites

Jacques1

Link to comment

Share on other sites

serverman

Link to comment

Share on other sites

Jacques1

Link to comment

Share on other sites

serverman

Link to comment

Share on other sites

Jacques1

Link to comment

Share on other sites

Join the conversation

Browse

Activity

Important Information