Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I was wondering if someone could take some time to look over my portfolio script and see let me know if it's secure enough to be uploaded live and connected to my database. My previous mySQL script was VERY insecure...

The Database Connection:

    public function  __construct($server, $databaseName, $username, $password)
    {
        $this->server = $server;
        $this->databaseName = $databaseName;
        $this->username = $username;
        $this->password = $password;
        $this->openConnection();
    }

    protected function openConnection()
    {
        $this->databaseLink = new PDO("mysql:host=$this->server;dbname=$this->databaseName", $this->username, $this->password);
    }

    public function getLink()
    {
        return $this->databaseLink;
    }
}

$database = new Database("localhost", "XXXXXXX", "XXXXXXX", "XXXXXXX");
$databaseLink = $database->getLink();
?>

The Client Detail Page

$client = $_GET["client"];

$getClientDataSql = "SELECT * FROM `client` WHERE `client`=:client";
$statement = $databaseLink->prepare($getClientDataSql);
$statement->bindParam(":client", $client, PDO::PARAM_STR);
$statement->execute();

if(!$statement->rowCount())
{
    echo "No Clients Found";
}
else
{
    $row = $statement->fetch(PDO::FETCH_ASSOC);

    // Title
    echo "<div id='portfolio_detail'>";
    echo "<h3>".$row["title"]."</h3>";
    echo "<div class='portfolio_block'>".$row["description"]."</div>";

    // Tags
    $tags = explode(",", $row["tags"]);
    if (count($tags))
    {
        echo "<div class='portfolio_block'>";
    foreach ($tags as $tag)
        {
            echo "<span class='tag'>".$tag."</span>";
    }
        echo "</div>";
    }

    // Colours
    $colours = explode(",", $row["colours"]);
    if (count($colours))
    {
        echo "<div class='portfolio_block'><h3>Colours</h3>";
    foreach ($colours as $colour)
    {
            echo "<div class='colour_block' id='".$colour."' alt='".$colour."' title='".$colour."'></div>";
    }
    echo "</div>";
    }

    // Services
    $services = explode(",", $row["services"]);
    if (count($services))
    {
        echo "<div class='portfolio_block'><h3>Services</h3><ul>";
    foreach ($services as $service)
    {
            echo "<li id='".$service."'>".$service."</li>";
    }
    echo "</ul></div>";
    }
    echo "<div class='portfolio_visit'><a href='".$row["visit"]."'>Visit Website</a></div>";
    echo "</div>";
    echo "<div id='portfolio_detail_img'><img src='"."/client/".$row["client"].".png"."' alt='".$row["client"]."' title='".$row["client"]."' /></div>";
    echo "<div id='portfolio_detail_slide' class='rsMinB'>";
    echo "<div><img src='"."/client/".$row["client"]."_1.png"."' /></div>";
    echo "<div><img src='"."/client/".$row["client"]."_2.png"."' /></div>";
    echo "<div><img src='"."/client/".$row["client"]."_3.png"."' /></div>";
    echo "</div>";
}

?>

The Client List Page

$getPortofolioDataSql = "SELECT `id`, `title`, `description`, `client` FROM `client`";
$statement = $databaseLink->prepare($getPortofolioDataSql);
$statement->execute();
if($statement->rowCount() == 0)
{
    echo "No Portfolio Found";
}
else
{
    while($portofolioDetails = $statement->fetch(PDO::FETCH_ASSOC))
    {
        echo "<li class='portfolio'>";
    echo "<a href='portfolio/".$portofolioDetails["client"]."'><img src='http://www.creativewebgroup.co.uk/client/".$portofolioDetails["client"].".png' /></a>";
        echo "</li>";
    }
}
?>
share|improve this question
2  
Please do not edit the question in a way which makes existing answers meaningless. –  palacsint Mar 10 '13 at 16:32
1  
In response to your flag: We don't delete questions just because they're answered and probably not helpful to anyone else (to some degree that applies to most questions here). That would just take away reputation from the answerers for no good reason. –  sepp2k Mar 10 '13 at 18:01

1 Answer 1

up vote 2 down vote accepted
  • Your database queries are using bound parameters with prepared statements. Good.

  • You need to escape your output.

Other observations:

  • Your Database class adds nothing (unless you've abbreviated it here for simplicity?). Consider simply using PDO.
  • You have no error handling. Try loading your page with incorrect db credentials and see what I mean.
  • Consider using markup with php instead of string-ified markup in php. It is much easier to read and maintain.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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