Can I further secure this blog post loading script?

Question

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.

 <?php include $_SERVER['DOCUMENT_ROOT']."/resources/config.php"; 


try {

    $dbh = new PDO($dsn, $username, $password, $options);

    $dbh->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 "<h1>".$title."<sup>".$new_date_vis."</sup></h1><br />";
       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?

Answer 1

• 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.

Answer 2

Security

Is this procedural code basically secure?

This is in addition to the points @ratchet freak mentioned.

XSS

As you are echoing $title without sanitizing it, any user that can create posts can execute arbitrary JavaScript on the clients computer (via persistent XSS). Right now, this may or may not be only you, but in the future, you might allow other people to post as well.

XSS is often underestimated, but among other, it allows the following:

• steal (session) cookies
• track user actions (logging entered text, passwords, etc)
• make actions on users behalf (like sending messages via forms)
• injecting ads
• defacement (changing the complete content of your website)

You can easily prevent this by passing $title to htmlspecialchars before echoing it.

If content.txt also contains (HTML) content by non-root users, sanitizing this will be a lot more difficult, but I am assuming it's only ever created by you.

Config

Your config.php file seems to be located inside the web directory, you should move it outside of the web root.

Having a password in a PHP file inside the web root could leak your password in some instances.

Handling Exceptions

Right now, you are echoing the exception (not a good idea for production code as @ratchet freak mentioned), but are not handling it, you call orderBlogPost anyways (and it will fail, uncontrolled). You should either try to recover, or redirect to a custom error page.

General Code Comments

Style

• your indentation is off, which makes your code harder to read
• class names should start with an uppercase letter

OOP

Has separating this class and defining functions improved security of the script?

No (why would it?), but it hasn't made it weaker either.

Have I improved the usability of this post loader?

Not really. You basically wrapped your function up in a class. It doesn't hurt, but it doesn't provide any benefits either.

have I committed any OOP class/function sins?

It looks like your class is in the same file as the code above and below it (the include and the call to the class). This isn't how it's normally done. You put a class in its own, separate PHP file. The way you are doing it, you cannot use your class anywhere else, because as soon as you include it, all the other code gets executed.

General Approach

Is there a reason that the content of a post is inside a textfile instead of the database? If you didn't want/could use a database, I would understand, but since some important information is stored in the database, why not all? Your current approach seems a bit inflexible.