5
\$\begingroup\$

I'm currently learning PHP, SQL/MySQL and HTML to develop a web application project (I have very little practical experience in all of these).

To get started, I spent far too long and many questions on Stack Overflow writing a web application which inserts records into my database for Game of Thrones characters. The web app uses a form, which POSTs to a PHP script, which uses PDO to interact with MySQL. I've done nothing to prevent SQL injection - although because this is a private project, so I'm not concerned about security in this instance.

I'd appreciate input on whether I'm using any bad practices, or if there are better/more efficient ways of doing what I'm doing. I plan to use a similar construct to get me started in my other web app project, so - apart from the SQL security weakness that I've acknowledged - if there are any other ways in which code like this would be unsuitable for a public domain, I'd like to know that as well.

asoiaf.php

<html>
<body>
    <form action="addchar.php" method="post">
        <select name="bookIntroduced">
            <option value="1">A Game of Thrones</option>
            <option value="2">A Clash of Kings</option>
            <option value="3">A Storm of Swords</option>
            <option value="4">A Feast for Crows</option>
            <option value="5">A Dance with Dragons</option>
        </select>
        <p>
            Page introduced:<br>
            <input type="text" name="pageIntroduced" tabindex=1 autofocus>
            <br>Title:<br>
            <input type="text" name="title" tabindex=2>
            <br>First name<br>
            <input type="text" name="forename" tabindex=3>
            <br>Surname<br>
            <input type="text" name="surname" tabindex=4>
            <br>Old surname<br>
            <input type="text" name="oldSurname" tabindex=5>
            <br>Alias or nickname<br>
            <input type="text" name="alias" tabindex=6>
            <br>Regnal number<br>
            <input type="text" name="regnalNumber" tabindex=7>
            <br>
            <input type="submit" value="Add character" tabindex=8>
        </p>
    </form>
    <?php
        // Displays the message returned from the PHP script.
        if ($_GET['msg']) {
            echo "<br>".$_GET['msg'];
        }

    ?>
</body>
</html>

addchar.php

<?php

if ($_POST) {
    // Configuration.
    $username = "root";
    $password = "root";
    $hostname = "localhost";
    $dbname = "asoiaf";
    $tablename = "charlist";

    // Opens a connection to the database.
    try {
        $conn = new PDO("mysql:host=$hostname;dbname=$dbname", $username, $password);
        // I don't know what this does; a
        $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    } catch(PDOException $e) {
        echo $e->getmessage();
    }

    // Gets the next available primary key from the table.      
    $qry = $conn->query("SELECT Auto_Increment FROM information_schema.tables WHERE table_name='$tablename'");
    // Fetches the result of the query, stores it in $result.       
    $result = $qry->fetch();
    // Puts the resulting primary key into $id.
    $id = $result['Auto_Increment'];
    // Fetches all the other information from the form.
    $bookIntroduced = $_POST['bookIntroduced'];
    $pageIntroduced = $_POST['pageIntroduced'];
    $forename = $_POST['forename'];
    $surname = $_POST['surname'];
    $oldSurname = $_POST['oldSurname'];
    $alias = $_POST['alias'];
    $title = $_POST['title'];
    $regnalNumber = $_POST['regnalNumber'];

    // Queries the table to see if a record exists with the same forename and surname values.
    $qry = $conn->query("SELECT forename, surname FROM charlist WHERE forename='$forename' AND surname='$surname'");
    $result = $qry->fetch();
    // If a record with the same forename/surname exists, the entry is a duplicate entry and should be disallowed.
    if ($result[0]==$forename && $result[1]=$surname) {
        // Return to the original page reporting a duplicate error.
        header('Location: asoiaf.php?msg=duplicate error');
    } else {
        // Prepare the SQL statement.
        $sql = "INSERT INTO $tablename (id, bookIntroduced, pageIntroduced, forename, surname, oldSurname, alias, title, regnalNumber)
                VALUES (:id, :bookIntroduced, :pageIntroduced, :forename, :surname, :oldSurname, :alias, :title, :regnalNumber)";
        $q = $conn -> prepare($sql);
        // Executes the SQL.
        $q -> execute(array(':id' => $id, ':bookIntroduced' => $bookIntroduced, ':pageIntroduced' => $pageIntroduced, ':forename' => $forename, ':surname' => $surname, ':oldSurname' => $oldSurname, ':alias' => $alias, ':title' => $title, ':regnalNumber' => $regnalNumber));

        // Closes the PDO connection.
        $conn = null;

        // Returns to the original HTML page.
        header('Location: asoiaf.php?msg=success');
    }
}
?>

These are two separate screenshots of the web form spliced together:

enter image description here

\$\endgroup\$
  • 1
    \$\begingroup\$ Hello there and welcome to Code Review! Your quesion is IMO fully on-topic. Don't worry about comments on to-be-closed questions. I've yet to see a post that was closed without comments ;) btw. Concerning on-topicness you can check the help center or search a little on Code Review Meta. You can also drop by in Code Review Chat ;) \$\endgroup\$ – Vogel612 Jul 25 '14 at 10:36
6
\$\begingroup\$

Security - XSS

In addition to the SQL injection you mentioned, your code is also prone to XSS attacks as you are not sanitising echo "<br>".$_GET['msg'];

Try this:

http://localhost/asoiaf.php?msg=%3Cscript%3Ealert(%22bad%22)%3C/script%3E

The argument is URL encoded and looks like this in plain text:

<script>alert("bad");</script>

So you can see that arbitrary JavaScript can be executed. This can for example be used to steal someones cookies. You can read more about XSS here.

Security - Information Leak

echo $e->getmessage();

It is never a good idea to display direct error messages to a user. You might want to set a boolean flag somewhere and check if you are in production mode or not (that way, you only have to change you code in one location).

PDO - setAttribute

// I don't know what this does; a
$conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

It just sets the error mode. In this case to the most verbose option available (exceptions). In production code, this could be changed to PDO::ERRMODE_SILENT.

Auto-Increment

Why are you manually incrementing the primary ID? The database should take care of this for you. Doing it manually adds a lot of unneeded code, and also an additional database query.

Other than these issues, you code seems quite good to me.

\$\endgroup\$
  • \$\begingroup\$ Awesome. A few points: firstly, could you please explain the code after the URL? It doesn't make any sense to me. Also, I have the echos in for debugging purposes, thank you for reminding me. And I didn't think I was manually incrementing the primary id, I'm fetching the auto-increment. How else would I do it? I need a value to put in the INSERT statement for the primary key, surely? \$\endgroup\$ – Lou Jul 25 '14 at 14:56
  • \$\begingroup\$ @LeoKing updated my answer. Concerning the id: If you specified AUTO_INCREMENT for the id field, you do not need to insert anything. Just leave that parameter out. \$\endgroup\$ – tim Jul 25 '14 at 15:08
4
\$\begingroup\$

About your HTML

You are missing the DOCTYPE. If you want to use HTML5, it would be:

<!DOCTYPE html>

While you have an opening html tag (<html>), the closing one is missing (</html>).

You are missing the required title element (it’s part of the head element, which is not required to be added explicitly, but there is no good reason to omit it in this case).

If you don’t set the character encoding on the HTTP level, you might want to add it as meta element in the head. In case of UTF-8:

<meta charset="utf-8">

So the basic structure would look like:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title>…</title>
  </head>
  <body>
  </body>
</html>

It doesn’t seem to make sense to have a p element that contains all your fields, as p is for paragraphs. Use p (or maybe better, div) for each field-label pair instead.

The br elements are not used according to their definition in HTML. You must only use them for "meaningful" (i.e., being a necessary part of the content) line breaks, for example for poems or addresses. If you use a block element (like p/div, see above) for each field-label pair, the line break comes for free (i.e., from the browser’s default CSS).

You should use label elements for the field labels. If you connect the field to its label (e.g., with the for attribute), clicking the label will focus its field; and it’s of course very important for accessibility.

There is no need to have explicit tabindex attributes if you don’t want to change the tabbing order. In your code, the first tab on the page will focus the first text input, although the select list is displayed above this field (so keyboard navigating users might be confused as it’s not what they would expect).

I wouldn’t use autofocus on the first text field; besides that it’s not necessarily the first field users want to interact with (as the select list comes before), it can be a problem for accessibility. autofocus should only be used when the auto-focussed field is very likely something that almost all users want to interact with right from the start (common example: the input field on the front page of web search engines).

For the field "Page introduced", you could use the Number state (type="number) instead of Text.

So your form could look like:

<form action="addchar.php" method="post">

  <select name="bookIntroduced">
    <option value="1">A Game of Thrones</option>
    <option value="2">A Clash of Kings</option>
    <option value="3">A Storm of Swords</option>
    <option value="4">A Feast for Crows</option>
    <option value="5">A Dance with Dragons</option>
  </select>

  <div>
    <label for="pageIntroduced">Page introduced</label>
    <input type="number" min="1" name="pageIntroduced" id="pageIntroduced">
  </div>

  <div>
    <label for="title">Title</label>
    <input type="text" name="title" id="title">
  </div>

  <!-- … -->

  <div>
    <input type="submit" value="Add character">
  </div>

</form>
\$\endgroup\$
0
\$\begingroup\$

I would just add some points I use.

From my point of view, you can skip this part, ie, using variables as they are posted. It is kind of redundant and more error prone assingning new variables. I assign new variables only if I need some further processing.

$bookIntroduced = $_POST['bookIntroduced'];
$pageIntroduced = $_POST['pageIntroduced'];
$forename = $_POST['forename'];
$surname = $_POST['surname'];
$oldSurname = $_POST['oldSurname'];
$alias = $_POST['alias'];
$title = $_POST['title'];
$regnalNumber = $_POST['regnalNumber'];

Using them as they are: $_POST['bookIntroduced'], $_POST['forename'], etc.

In this part:

// Queries the table to see if a record exists with the same forename and surname values.
$qry = $conn->query("SELECT forename, surname FROM charlist WHERE forename='$forename' AND surname='$surname'");

I prefer to escape variables, it is much safer and cleaner:

$qry = $conn->query("SELECT forename, surname FROM charlist WHERE forename='" . $forename . "' AND surname='" . $surname . "'");

or including my first point:

$qry = $conn->query("SELECT forename, surname FROM charlist WHERE forename='" . $_POST['forename'] . "' AND surname='" . $_POST['surname'] . "'");

I hope this will help you somehow.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you. Why do you prefer to write variables outside of the quotes, though? I'm not sure I understand what advantage it provides. \$\endgroup\$ – Lou Jul 27 '14 at 7:45
  • \$\begingroup\$ safety reasons: for example if your forename would be O'neal - and you are using single quotes like this WHERE forename='$forename' which would produce an error in the query. \$\endgroup\$ – Angel M. Jul 27 '14 at 8:16
  • \$\begingroup\$ Oh, okay, that makes sense. \$\endgroup\$ – Lou Jul 27 '14 at 8:18
  • \$\begingroup\$ of course - best way is to provide escaping characters, but good coding practice is: 'Never trust the user's input' :) \$\endgroup\$ – Angel M. Jul 27 '14 at 8:19
  • \$\begingroup\$ Or do you mean 'Never trust the user\'s input'? \$\endgroup\$ – Lou Jul 27 '14 at 8:21

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.