HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Web application to insert into MySQL database

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
insertapplicationintodatabasemysqlweb

Problem

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


    
        
            A Game of Thrones
            A Clash of Kings
            A Storm of Swords
            A Feast for Crows
            A Dance with Dragons
        
        
            Page introduced:
            
            Title:
            
            First name
            
            Surname
            
            Old surname
            
            Alias or nickname
            
            Regnal number
            
            
            
        
    
    ".$_GET['msg'];
        }

    ?>


addchar.php

```
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'

Solution

Security - XSS

In addition to the SQL injection you mentioned, your code is also prone to XSS attacks as you are not sanitising echo "
".$_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:

alert("bad");


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.

Context

StackExchange Code Review Q#58029, answer score: 6

Revisions (0)

No revisions yet.