0

I am new to PDO and am creating a class. I want to make the code as clean as possible and have a few pages as possible. I am planning on sending the column and table names in as a parameter: 

   function get($column, $table, $where) {
        global $db;
        $query = 'SELECT '.$column.' FROM '.$table.' WHERE '.$where.'';
        try {
            $statement = $db->prepare($query);
            $statement->execute();
            $result = $statement->fetchAll();
            $statement->closeCursor();
            return $result;
        } catch (PDOException $e) {
            $error_message = $e->getMessage();
            display_db_error($error_message);
        }
   }

It works fine, but I am wondering how safe it would be considered.

Sam J.
  • 353
  • 1
  • 3
  • 12
  • 2
    Your code is unsafe. Unless you're testing $column, $table and $where some place else in your code, this is ripe for sql injection. I wouldn't do what you plan on doing. It's a path to crazy bugs and errors. Allowing the user to control what table is accessed is obviously a major flaw in your application design. – Halfstop Feb 12 '15 at 19:35
  • Thank you. That was what I was wondering. I appreciate your help. – Sam J. Feb 12 '15 at 19:36
  • No problem, glad I could help. If you want write less code, I recommend using a framework like Symfony. It gets you our of the nitty gritty coding and working at a higher level. – Halfstop Feb 12 '15 at 19:41
  • @gnarly is right, and that should be an answer. PDO is safe when you use [bind parameters](http://php.net/manual/en/pdostatement.bindparam.php), but not when you concatenate strings. – Mike Sherrill 'Cat Recall' Feb 12 '15 at 21:10
  • I added my comment as an answer. – Halfstop Feb 12 '15 at 21:11
  • My other issue is that I want this where statement: WHERE id = :id AND otherid = :otherid but I might not always have an "id" or "otherid". So my plan was to pass "id = :id AND otherid = :otherid" in as a parameter, so I could change it if I had, too. If I can't do this, how else would you suggest doing it? Without making a separate function.... – Sam J. Feb 12 '15 at 21:27
  • Relating to an earlier point: Symfony itself doesn't handle databases, as far as I know. However it uses Doctrine, an ORM, which simplifies access to databases - so whilst web frameworks are in general a good idea, it's the ORM that will help here, not the web framework. – halfer Feb 12 '15 at 21:53
  • possible duplicate of [How to select from a dynamic column through a variable with PDO?](http://stackoverflow.com/questions/28283141/how-to-select-from-a-dynamic-column-through-a-variable-with-pdo) – david strachan Feb 13 '15 at 12:35
  • @davidstrachan, no it is not the same. I was asking about the security issues with my code, unlike the other post. – Sam J. Feb 13 '15 at 21:36
  • It shows you how to sanitize table & column names to stop your query being vulnerabe to SQL injection. – david strachan Feb 14 '15 at 08:40

1 Answers1

3

Your code is unsafe. Unless you're testing $column, $table and $where some place else in your code, this is ripe for sql injection. I wouldn't do what you plan on doing. It's a path to crazy bugs and errors. Allowing the user to control what table is accessed is obviously a major flaw in your application design.

Halfstop
  • 1,710
  • 17
  • 34