patternphpMinor
Web application to insert into MySQL database
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
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'
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
Try this:
The argument is URL encoded and looks like this in plain text:
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
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
It just sets the error mode. In this case to the most verbose option available (exceptions). In production code, this could be changed to
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.
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.