HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Can I further secure this blog post loading script?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisscriptcansecurefurtherpostloadingblog

Problem

The following code is developed around a mysql database where the posts table has 3 columns: post_ID, date and title.

config.php holds the values for $dsn, $username, $password, $options.

The content for each post is held in a directory format:

year/month/day/(database column)title/content.txt


By querying the database, I retrieve the date, which lets me order the posts (newest first).

$new_date_dir formats the date to serve as the directory for the include link $link.

$new_date_vis formats the date to serve as the date for the post.

The file is called using include within another page which uses exterior formatting.

setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

} catch (PDOException $e) {

    echo 'Connection failed: ' . $e->getMessage();

}

function orderBlogPost($dbh) {
$statement = $dbh->prepare("
SELECT date, title
FROM post 
ORDER BY UNIX_TIMESTAMP(date) DESC
LIMIT 0,5
");

$statement->execute();

    $statement->bindColumn('date', $date);

    $statement->bindColumn('title', $title);

    while ($row = $statement->fetch(PDO::FETCH_BOUND)) {  

        $new_date_dir = date("Y-m-d",strtotime($date));

        $new_date_vis = date("d-m-Y",strtotime($date));

        $new_row = preg_replace("[-]", "/", $new_date_dir);

        $link = $_SERVER['DOCUMENT_ROOT']."/content/".$new_row."/".$title."/content.txt";

       echo "".$title."".$new_date_vis."";
       include "$link";
    }

}
orderBlogPost($dbh);   

$dbh = null

?>


Whilst, this is outputting my desired result, I would like to know the following:

  • Is this procedural code basically secure/usable?



  • Can you suggest further reading on refactoring?



  • How would I benefit from using OOP in this scenario?

Solution

-
secure?

-
not entirely: you emit a debug message to the user when something goes wrong when connecting to the database which may leak information about the server (consider a dumb PDO implementation that emits the $dsn, $options and $username in the error message).

Replace the echo with a forward to a 502 page and log the error message internally.

-
you also include a file from a text file repository and assume it doesn't contain any php code that may do Bad Things™ (while the $dbh is still in scope) or any XSS shenanigans.

You can use readFile to avoid the php code but not the XSS.

-
usable?

-
Is is that just emits a fixed number of titles in a fixed way. to do anything else you need to write a new function most of it will be copy and paste.

Instead just return an array with date-title pairs and let the calling code figure out how it should be displayed.

also add a parameter to allow the calling code to pass in a parameter for the number of records requested and at what point it should start to allow paging?

Besides that you store a date as a string in the database, let dates be a dates.

In fact the function itself is not necessary with how the code is used.

Make OOP? well you could create a querryable object that will return the aforementioned array and keeps the object handle private.

Context

StackExchange Code Review Q#67451, answer score: 4

Revisions (0)

No revisions yet.