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\$

closed as off-topic by yuri, Toby Speight, esote, t3chb0t, Mast Jun 14 at 8:47

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – yuri, Toby Speight, esote, t3chb0t, Mast
If this question can be reworded to fit the rules in the help center, please edit the question.

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])";
}
\$\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 at 8:07

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