3
\$\begingroup\$

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?

\$\endgroup\$
2
  • 3
    \$\begingroup\$ How did you decide on the list of allowable characters? See Falsehoods Programmers Believe Abut Names \$\endgroup\$ Commented Jun 7, 2015 at 9:00
  • \$\begingroup\$ Its a website for polish so i don't expect any other characters but thats not the question \$\endgroup\$
    – rafal235
    Commented Jun 7, 2015 at 9:09

2 Answers 2

4
\$\begingroup\$

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]);
}
\$\endgroup\$
1
1
\$\begingroup\$

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.

\$\endgroup\$

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