current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I want to ask you if my PHP code is safe enough. I don't know if I should escape special characters in string after regex validation:

<?php

require_once '../../../wp-load.php';

$errors = [];

if (preg_match('/[^a-ząćęółśżźń ]/i', $_POST['name']) || strlen(trim($_POST['name'])) == 0 || strlen($_POST['name']) > 60) {
    $errors[] = "Invalid name";
}

if (empty($errors)) {
    echo json_encode(['status' => true]);
    $wpdb->query($wpdb->prepare("INSERT INTO people VALUES(null, %s)", $_POST['name']));
} else {
    echo json_encode(['status' => false, 'errors' => $errors]);
}

And what if i use PDO prepare instead of wordpress function?

share|improve this question
3  
How did you decide on the list of allowable characters? See Falsehoods Programmers Believe Abut Names –  200_success♦ yesterday
    
Its a website for polish so i don't expect any other characters but thats not the question –  user2075220 yesterday

2 Answers 2

up vote 3 down vote accepted

In terms of security, you should be safe from SQL injection since you are using parameterized queries as recommended. That's true whether or not you validate the names using the regex. Do not perform any additional escaping — that would only mangle your data.

That regex is for enforcing your business rules (i.e. you want to reject names written in Cyrillic, names with French accents like é, Irish surnames like O'Something), and has nothing to do with database security.

I do not recommend mixing PDO with the WordPress database API.

The WordPress documentation recommends that you use $wpdb->insert() for simple INSERT queries.

In accordance with the WordPress documentation, you should check the return value from $wpdb->query() — a FALSE value indicates failure. You should do that before declaring victory with echo json_encode(['status' => true]);.

if (!empty($errors)) {
    echo json_encode(['status' => false, 'errors' => $errors]);
} elsif (FALSE === $wpdb->insert('people', ['name' => $_POST['name']], '%s')) {
    echo json_encode(['status' => false, 'errors' => ['Database error: ' . $wpdb->last_error]]);
} else {
    echo json_encode(['status' => true]);
}
share|improve this answer
    
I disagree with your final code. It should be more like this: pastebin.com/tUfVirqW –  Ismael Miguel yesterday
up vote 1 down vote

Yes, this should be safe enough. In general, it's reasonable to expect that any decent framework for prepared statements with parameterized queries will correctly prevent SQL injection attacks. No need to escape characters other than those required by your business logic that you define yourself.

On the other hand, it's recommended I to include the column names in the insert statement. And you probably want to trim the input before inserting.

As a side note, the error messages of your valuation logic could be a bit more friendly than just saying "invalid name". It works be friendlier to separate the messages for the different invalid cases: empty input, too long input, invalid characters.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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