Can you find 3 most severe defects in this code?

frozen_space · 2010-01-04 12:35:50

I have been asked with this question, not sure if i get the right answer

<?php
class Document {

   public $user;

   public $name;

   public function init($name, User $user) {
       assert(strlen($name) > 5);
       $this->user = $user;
       $this->name = $name;
   }

   public function getTitle() {
       $db = Database::getInstance();
       $row = $db->query('SELECT * FROM document WHERE name = "' .
$this->name . '" LIMIT 1');
       return $row[3]; // third column in a row
   }

   public function getContent() {
       $db = Database::getInstance();
       $row = $db->query('SELECT * FROM document WHERE name = "' .
$this->name . '" LIMIT 1');
       return $row[6]; // sixth column in a row
   }

   public static function getAllDocuments() {
       // to be implemented later
   }

}

class User {

   public function makeNewDocument($name) {
       $doc = new Document();
       $doc->init($name, $this);
       return $doc;
   }

   public function getMyDocuments() {
       $list = array();
       foreach (Document::getAllDocuments() as $doc) {
           if ($doc->user == $this)
               $list[] = $doc;
       }
       return $list;
   }

}
?>

1. Possible SQL injection
2. No constructor defined
3. Should not use assert in the production env ????

Last edited by frozen_space (2010-01-05 03:57:49)

Daniel03155 · 2010-01-06 01:36:27

Hey,

I wouldn't say that having no constructor makes it a severe mistake. Constructors are optional, but you can surely benefit from having them. One severe defect would be not sanitizing the variables before confronting them with the database.
Another one would be

return $row[6]; // sixth column in a row

mysql_query returns a resource, it seems like you forgot to to add mysql_fetch_row() to get a result row array, then display the column you wish. (The same goes with the others).
Lookup mysql_query().

Do you really need all these methods to be public?

frozen_space · 2010-01-06 02:30:17

Thanks, Daniel,
I found another one, we should not write like this foreach (Document::getAllDocuments() as $doc) , the getAllDocuments() should not be in the loop

Forums

#1 2010-01-04 12:35:50

Can you find 3 most severe defects in this code?

#2 2010-01-06 01:36:27

Re: Can you find 3 most severe defects in this code?

#3 2010-01-06 02:30:17

Re: Can you find 3 most severe defects in this code?

Board footer