User Profile page - code quality , security & Performance feedback

Question

once user login, we are displaying all the information of user in profile/account page. please check code quality , security & Performance.

<?php
include 'home.php';
// session_start();
require_once 'class.user.php';
$user_home = new USER();

if (!$user_home->is_logged_in()) {
  header("Location: index.php");
  die();
}
?>

<?php

if (isset($_POST['submit'])) {
// new data
  $name = $_POST['name'];
  $email = $_POST['email'];
  $phone = $_POST['phone'];
  $street_address = $_POST['street_address'];
  $street_address_2 = trim($_POST['street_address_2']);
  $city = trim($_POST['city']);
  $state = trim($_POST['state']);
  $zip_code = trim($_POST['zip_code']);
  $country = trim($_POST['country']);
  $sold_by = trim($_POST['sold_by']);
  $portfolio = trim($_POST['portfolio']);
  $paypal_email_id = trim($_POST['paypal_email_id']);
  $account_holder_name = trim($_POST['account_holder_name']);
  $account_number = trim($_POST['account_number']);
  $branch_name = trim($_POST['branch_name']);
  $bank_name = trim($_POST['bank_name']);
  $ifsc_code = trim($_POST['ifsc_code']);
  $tax = trim($_POST['tax']);
  $photo = trim($_POST['photo']);


// $uid = (isset($_GET['userID']) ? intval($_GET['userID']) : -1);
  $uid = (isset($_SESSION['userSession']) ? intval($_SESSION['userSession']) : 0);


// query
  if ($uid > 0) {
    $user_home->update($name, $email, $phone, $street_address, $street_address_2, $city, $state, $zip_code, $country, $sold_by,
        $portfolio, $paypal_email_id, $account_holder_name, $account_number, $branch_name, $bank_name, $ifsc_code, $tax, $uid);
    if($_FILES['photo'])
      $user_home->add_photo($_FILES['photo']);
  }
}


$stmt = $user_home->runQuery("SELECT * FROM tbl_users WHERE userID=:uid");
$stmt->execute(array(":uid" => $_SESSION['userSession']));
$row = $stmt->fetch(PDO::FETCH_ASSOC);

/* php codde end*/
?>

<!DOCTYPE html>
<html class="no-js">
<h4> Welcome to profile page</h4>

<head>
  <title><?php echo $row['email']; ?></title>
  <a href="user.php?<?php print($userRow['name']); ?>"></a>  <title>  <?php echo $row['name']; ?> Profile page </title>
  <!-- Bootstrap -->
  <link href="bootstrap/css/bootstrap.min.css" rel="stylesheet" media="screen">
  <link href="bootstrap/css/bootstrap-responsive.min.css" rel="stylesheet" media="screen">
  <link href="assets/styles.css" rel="stylesheet" media="screen">
  <link href="assets/style.css" rel="stylesheet" media="screen">
  <!-- HTML5 shim, for IE6-8 support of HTML5 elements -->
  <!--[if lt IE 9]>
  <script src="http://html5shim.googlecode.com/svn/trunk/html5.js"></script>
  <![endif]-->

<body>
  <form action="profile1.php" method="POST">
    <h3 class = "h3">Personal details</h3>

    Name :
    <?php echo $row['name'] ?> <br/>

    Email :
    <?php echo $row['email'] ?> <br>

    Sold by :
    <?php echo $row['sold_by'] ?>   <br/>

    Portfolio :
    <?php echo $row['portfolio'] ?> <br/>

    <a class = "change1" href="fpass1.php">Click here to Change Password </a>

  </form>

  <a href="profile1.php">
    <button class="pedit" name="submit">Edit</button>
  </a>

  <form action="profile1.php" method="POST">

    <h3 class = "h3">ADDRESS</h3>

    Phone :
    <?php echo $row['phone'] ?> <br>
    street address :
    <?php echo $row['street_address'] ?> <br>
    street address 2 :
    <?php echo $row['street_address_2'] ?> <br>
    city :
    <?php echo $row['city'] ?> <br>
    state :
    <?php echo $row['state'] ?> <br>
    country :
    <?php echo $row['country'] ?> <br>
    zip :
    <?php echo $row['zip_code'] ?> <br>


  </form>
  <a href="profile1.php">
    <button class="pedit" name="submit">Edit</button>
  </a>
  <form action="profile1.php" method="POST">


    <h3 class = "h3">Payment details</h3>

    Paypal email id :
    <?php echo $row['paypal_email_id'] ?>    <br/>

    <h4> Bank Details [ Wiretransfer ] </h4>

    account holder name :
    <?php echo $row['account_holder_name'] ?> <br/>
    account number :
    <?php echo $row['account_number'] ?> <br/>
    branch name :
    <?php echo $row['branch_name'] ?> <br/>
    bank name :
    <?php echo $row['bank_name'] ?> <br/>
    ifsc code :
    <?php echo $row['ifsc_code'] ?> <br/>


  </form>
  <a href="profile1.php">
    <button class="pedit" name="submit">Edit</button>
  </a>

  <form action="profile1.php" method="POST">

    <h3 class = "h3">Other information</h3>
    Tax id scan copy :
    <?php if(file_exists($user_home->get_photo())): ?>
        <img src="<?php echo $user_home->get_photo_url() ?>" class="avatar" />
    <?php endif; ?>


    <input id="sub" type="submit" name="submit" value="Save" style="display:none"/>
  </form>
  <a href="profile1.php">
    <button class="pedit" name="submit">Edit</button>
  </a>

</body>
</html>

Where is $user_home defined? This might be pertinent code to include in this review as, in looking at how it is being used, I think there may be some questionable design decisions within that class. — Mike Brant, Nov 15, 2016 at 15:01
@MikeBrant Thanks for suggestion, i will post new question for that..... — USer345738380, Nov 16, 2016 at 10:15

tim · Accepted Answer · 2016-11-16 09:16:27Z

Security

You didn't post the really interesting code regarding security (the database update and the file upload), but concerning the code you do have:

XSS

It's never a good idea to just echo user input without encoding, as it leads to XSS. In the case of XSS in a user area, this may be exploited by creating a new account, placing an XSS payload into that profile, and forcefully logging a user into that account (eg via Login CSRF, Session Fixation, etc).

The impact may or may not be serious, but it is a sign of bad design. If you have to think about which user inputs to encode where, you will eventually make a mistake. It's better to automatically encode everything.

CSRF

The code you posted doesn't have any CSRF protection. If it doesn't exist elsewhere, you should add it.

Relative Path Overwrite

You shouldn't include CSS files relatively, but absolutely (same goes for JS files).

Otherwise, an attacker could place CSS code into the page, and possibly use a specifically crafted URL to execute that CSS code (eg profile.php////).

CSS injection isn't as serious as XSS, but it can lead to the reading of sensitive data, to defacement, and possibly phishing.

the doctype declaration means that RPO will not work in all browsers, but it should still be fixed.

Commented out Code

This line:

// $uid = (isset($_GET['userID']) ? intval($_GET['userID']) : -1);

is obviously dangerous, as an attacker could update arbitrary profiles.

It should be remove to avoid that someone uncomments it later for some reason.

Input Validation

While input validation shouldn't be a primary solution to any vulnerabilities, it is very good to have it as defense in depth.

You validate the uid (although as @Ron mentioned, you later use the unclean version), but nothing else. It would be good to also validate other values, such as the email id (probably an integer?), etc.

Thanks, i will try to follow your answer & get back to you soon.... — USer345738380, Nov 16, 2016 at 10:14

Ron Beyer · Accepted Answer · 2016-11-15 16:05:37Z

I can see a couple things:

At the top, you have this:

$user_home = new USER();

I assume that USER is a user model, so why is it named $user_home? As somebody said it looks like this class has some design issues, but since you didn't post it I really can't comment more than that on it.

Near the top, you set a $uid variable:

$uid = (isset($_SESSION['userSession']) ? intval($_SESSION['userSession']) : 0);

Which is a little confusing. Why is the uid (user ID) called userSession in the session object? Is this really a "user id" or is it a "session id"?

A little while later you execute an SQL statement:

$stmt = $user_home->runQuery("SELECT * FROM tbl_users WHERE userID=:uid");
$stmt->execute(array(":uid" => $_SESSION['userSession']));

Which, for some reason, instead of using the existing $uid it gets it from the session again. I would expect that you would want:

$stmt = $user_home->runQuery("SELECT * FROM tbl_users WHERE userID=:uid");
$stmt->execute(array(":uid" => $uid));

Because it allows you to change where you get the $uid from in a single place.

The next thing I see is this confusing piece of code:

<title><?php echo $row['email']; ?></title>
  <a href="user.php?<?php print($userRow['name']); ?>"></a>  <title>  <?php echo $row['name']; ?> Profile page </title>

Why are you inserting a link between two <title></title> elements?
Why do you have two <title> elements? One is the email, the other is the name?
Why are you using print() in one part and echo in the other?
Where did $userRow come from? Shouldn't it be $row?

The last thing I see is this:

<h3 class = "h3">Other information</h3>
    Tax id scan copy :
    <?php if(file_exists($user_home->get_photo())): ?>
        <img src="<?php echo $user_home->get_photo_url() ?>" class="avatar" />
    <?php endif; ?>

You are saying its a "tax id scan copy" but then you are loading a "photo" and displaying it as an avatar. What is the difference between get_photo() and get_photo_url()? I'm assuming one is a physical file path and the other is a web URL. If that is the case, I would suffix the first with file_path, otherwise a casual observer would expect it to return the binary image data.

Thanks, i will try to follow your answer & get back to you soon.... — USer345738380, Nov 16, 2016 at 10:14

Community · Accepted Answer · 2020-06-10 13:24:26Z

This is an addition to other reviews.

Validity of html

The following snippet is invalid. You must move this under the body tag. Keep in mind that a <h4> element is a level below <h3>. Even if your html page is properly parsed, semantic parsers will not understand the structure of the document. In fact "Welcome to (the) profile page" is not a title at all. Consider using a properly styled <header> perhaps.

<html class="no-js">
<h4> Welcome to profile page</h4>

<head>

The a tag in the following snippet can not occur in the head. Furthermore you have a duplicate title tag.

  <title><?php echo $row['email']; ?></title>
  <a href="user.php?<?php print($userRow['name']); ?>"></a>  <title>  <?php echo $row['name']; ?> Profile page </title>

You forget to close the head tag in the following snippet.

  <!-- HTML5 shim, for IE6-8 support of HTML5 elements -->
  <!--[if lt IE 9]>
  <script src="http://html5shim.googlecode.com/svn/trunk/html5.js"></script>
  <![endif]-->

<body>

You are echoing variables directly, while they may contain invalid characters. This can produce invalid output. Instead, encode html entities for your chosen doctype, in this case html 5.

Odd html parts

I see you use the following html several times.

  <a href="profile1.php">
    <button class="pedit" name="submit">Edit</button>
  </a>

It makes little sense to use a button inside an <a> tag. Users do not expect a navigation event when clicking a button. Furthermore, since the buttons appear near a form, the user might expect the button to belong to that form. Just use a plain "Edit" text instead.

Classes

You are giving <h3> tags the class "h3". This is counter-productive. Either rename the class if you mean to style a specific group of h3 elements and name the class after what they have in common, or remove the class and style the elements with:

h3 {
    color: pink;
}

Thanks, i will try to follow your answer & get back to you soon.... — USer345738380, Nov 16, 2016 at 10:13

Alexander · Accepted Answer · 2016-11-23 12:49:10Z

This is an addition to other reviews.

Code Quality:

include 'home.php';:

In the context of your shared script it's not obvious what it does. I recommend to rename it to e.g. home.config.php. Further depending on the content there are other more beautiful ways to handle "that".

require_once 'class.user.php':

Including classes manually shouldn't be practiced anymore. Okay, maybe in an application which has like 3 files in total. However, in larger applications you should use autoload feature provided by PHP using spl_autoload_register. So, when you try to instantiate a class PHP automatically tries to include the class once if it does not exists in the current context.

View & Business Logic are not separated:

Nowadays view and business logic is separated to provide a better overview. Further it grants a better maintenance if implemented correctly. Check patterns like MVC which stands for Model View Controller.

Other

header("Location: index.php");

You perform a redirect without specifying the reason like 401 unauthorized.

Thanks, i will try to follow your answer & get back to you soon.... — USer345738380, Nov 23, 2016 at 12:48

Stack Exchange Network

User Profile page - code quality , security & Performance feedback

4 Answers 4

Security

Validity of html

Odd html parts

Classes

Your Answer

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

Hot Network Questions

User Profile page - code quality , security & Performance feedback

4 Answers 4

Security

Validity of html

Odd html parts

Classes

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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