1
\$\begingroup\$

Can someone tell me if this code is safe? Can be sql-injected or something else hacked? Code get some rows from db and show in pages with pagination... if i can improve let me know and show me how, thanks.

$conn = new PDO('mysql:host=localhost;dbname=admin_admin', 'admin_admin', 'password');
     $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); 
     $num_rows = $conn->query('SELECT COUNT(*) FROM a_topics')->fetchColumn(); 
     $pages = new Paginator($num_rows,25,array(25,50,100,250,'All'));
     $stmt = $conn->prepare('SELECT a_topics.pid, a_topics.title, a_topics.forum_id, b_forums.id, b_forums.name 
        FROM a_topics INNER JOIN b_forums ON a_topics.forum_id = b_forums.id
        ORDER BY a_topics.pid DESC LIMIT :start,:end');
     $stmt->bindParam(':start', $pages->limit_start, PDO::PARAM_INT);
     $stmt->bindParam(':end', $pages->limit_end, PDO::PARAM_INT);
     $stmt->execute();
     $result = $stmt->fetchAll();
     echo $pages->display_jump_menu().$pages->display_items_per_page();
     echo $pages->display_pages();
        foreach($result as $row) {
        echo "$row[0] - $row[1] - $row[2]";
     } catch(PDOException $e) {
     echo 'ERROR: ' . $e->getMessage();
     }
\$\endgroup\$
4
\$\begingroup\$

This code is safe from the SQL injection standpoint but it is likely prone to XSS because of the untreated output, and also to possibly leak the sensitive information due to the error message unconditionally spat out.

Regarding other improvements I would suggest a more robust connection code and to avoid bindParam calls thanks to disabled emulation mode.

The rewritten code would be

include 'pdo.php';
$num_rows = $conn->query('SELECT COUNT(*) FROM a_topics')->fetchColumn(); 
$pages = new Paginator($num_rows,25,array(25,50,100,250,'All'));
$stmt = $conn->prepare('SELECT a_topics.pid, a_topics.title, a_topics.forum_id, b_forums.id, b_forums.name 
    FROM a_topics INNER JOIN b_forums ON a_topics.forum_id = b_forums.id
    ORDER BY a_topics.pid DESC LIMIT :start,:end');
$stmt->execute(['start' => $pages->limit_start, 'end' => $pages->limit_end]);
$result = $stmt->fetchAll();

echo $pages->display_jump_menu().$pages->display_items_per_page();
echo $pages->display_pages();
foreach($result as $row) {
    echo htmlspecialchars($row[0]),
         " - ", 
         htmlspecialchars($row[1]),
         " - ", 
         htmlspecialchars($row[2])";
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I undestand thank you. One more question and i'm done, what if i use --> if ($_GET["id"] == "$row[0]" { echo "page1"; } <-- inside foreach, is safe? \$\endgroup\$ – Tatu Eugen Jun 11 '19 at 8:07

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