A few quick things:
$del
is never defined when you delete based on it
- Use prepared statements
- Not technically necessary here in your script (depending on
$del
)
- a good habit to have even when data is safe
- If you remove the filter later, then the escaping will already be there
- Don't silently change user's input
- If the user provides invalid input, tell him; don't silently change it.
- Not a hard rule by any means, but if a user provides a name and you change it for him, that might be a bit confusing.
- Strive for valid HTML
- Probably just meant to be a toy-esque script, but it's CodeReview, so might as well be pedantic :-).
- You do have a stray
</p>
though after the list
input
- It's a good habit to separate business logic and presentation
- MVC is basically what I'm hinting at, but there's no need to go all the way
- New data should never be fetched or generated inside of HTML
- The best description of what I'm getting at that I've ever heard is probably (summarized):
- If you can't reskin your site without repeating PHP code other than simple output, something is wrong.
- A bit hard to explain if you've never seen it, so if you want, I can do a little example.
- Escape HTML
- Any time you put a variable into html, it should be escaped with htmlspecialchars or htmlentities.
echo '<tr><td>' . htmlspecialchars($row['name']) . '</td>';
- Just because one input is set does not mean that another is
$_POST['submit']
and $_POST['name']
are not magically linked
- A user could exploit this to cause a notice
- Never directly access anything a user can control the existence or content of
- Use
filter_input
like showerhead suggested
- Or, if you want to do it more directly:
$name = (isset($_POST['name']) && is_string($_POST['name'])) ? $_POST['name'] : null;
- Note: everything comes in as either a string or an array even if it's an integer.
$id = (isset($_POST['id']) && is_string($_POST['id'])) ? (int) $_POST['id'] : null
;'
- (Yes, I am 100% crazy-level paranoid)
Edit: This is a response to the comment below
if you could elaborate on a couple of your points that would be great.
Particularly: "Just because one input is set does not mean that another is $_POST['submit'] and $_POST['name'] are not magically linked A user could exploit this to cause a notice"
Consider this code:
if (isset($_POST['submit'])) {
$name = $_POST['name'];
...
}
As discussed above, you should never access a key that your not sure exists. I'm assuming the logic here is, "well if the form was submitted, then the name field must exist."
That's not the case though. Consider this HTML:
<form action="http://yoursite.tld/yourpage.php" method="POST">
<input type="submit" name="submit" value="Submit">
</form>
Now imagine that someone submits this form.
Reading $_POST['name']
will trigger an "undefined index" notice.
So, in short, what I was getting at is that there is no link between the submit
and name
indexes. It's completely possible for one to be set and not the other.
Would this actually ever matter? Probably not. But still, it can have bad effects like a malicious user seeing file path or trying to fill up a log file.
The code should look like this:
if (isset($_POST['submit'])) {
$name = (isset($_POST['name']) && is_string($_POST['name'])) ? $_POST['name'] : null;
...
}
(Or you could use filter_input
.)
And: The $del issue. The script works great even though you say it isn't defined?
The first time $del
appears is on line 15 ($db->query("DELETE FROM name WHERE name='$del'");
). This means that it is either defined in db.php, or it's not defined.
If it's defined in db.php, it shouldn't be.
If it's not defined, that means that it's issuing a notice and being treated as null
.
(There's a third, very unlikely possibility that register globals is enabled. If that abomination is enabled, immediately disable it, or if your hosting company has enabled it without you requesting it, promptly switch companies.)
When null
is casted to a string, it becomes an empty string, so the query is probably running like:
$db->query("DELETE FROM name WHERE name=''");
Also, this query falls under my "Use prepared statements" point above.
Imagine if someone submits the following form:
<form action="http://yoursite.tld/page.php" method="POST">
<input type="submit" name="del_submit" value="Delete">
<input type="hidden" name="del" value="' OR 1">
</form>
That means that this query would be executed:
DELETE FROM name WHERE name='' OR 1
1
is always true, thus the entire table would be cleared.