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?

• xDaevax: Please consider revising your question title to be more specific to your actual code. "This code" is pretty generic, and doesn't do much to attract attention to your question. Perhaps something like: "How can I improve this CMS style content loader (using OOP or other methodologies)"?
• tim: What data is user supplied? Does the server need to be protected from people who can create posts? If so, you should really escape anything before outputting it (eg $title could be used for an XSS attack; it also looks like it should be possible to use it in an lfi attack, but I did not get that to work).
• devtoform: No data is user supplied currently. This script gathers records from the database, compiles the date and title to navigate filesystem on the server and includes the content file to output the 'post'.
• 200_success: What you may and may not do after receiving answers. I've rolled back Rev 5 → 4.

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 ThingsTM (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.

• devtoform: Thank you for your answer. I will investigate the 502, and readfile. under 'usable?' could you elaborate on how i can omit the date string in the table to let 'dates be dates'.
• ratchet freak: @devtoform Databases allow you to store dates directly without needing to transform them in a string
• devtoform: The $date comes from the database. it is used twice: Firstly, to allow me to order the posts by creation. Secondly, the $new_date_dir is reformatted to allow me to use it as the directory for the relevant content. The reason, to allow me to create a CMS which will create the directories and contents.txt files as posts are created. is there a better way?
• ratchet freak: Yeah don't store the date as a string in the database but as a date (or timestamp) then if you need the date as a string you can use the date("", $date) function where you need to. But everywhere else they will be dates
• devtoform: Maybe I didn't explain enough. the Date in the database is a datetime value currently, which will be moved to timestamp (for post creation). The reason being so i can post twice on the same day, order by the time part of $date but use the date part for the directory, all from a single column using $new_date_dir = date("Y-m-d",strtotime($date))

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.