2
\$\begingroup\$

I am a programming novice. I've written this simple PHP script to run a very basic comment form and would appreciate any feedback, especially on these three topics:

  1. Efficiency (not sure the right word for this): basically, am I doing anything in a clearly awkward/inferior/inelegant way, in terms of using significantly more lines of code or blocks to do what I could have done in less, poorly organizing the code, or bypassing functions or techniques that would provide equivalent functionality with simpler code?
  2. Security: this script (and the page that calls it) are currently protected by htpasswd and will only be accessed by trusted individuals. In light of that, does my script have any security weaknesses?
  3. Readability + conventionality: are there any awkward practices that would look strange or confusing to an experienced PHP programmer?

To be clear, the code is working exactly as I intend now, which is to display existing comments and to provide two fields (name and comment) for submission, which submit successfully only if both are nonempty and otherwise indicate with CSS which is blank and preserve the POST data. (I do not receive any errors when using ini_set('display_errors', 'On'); error_reporting(E_ALL | E_STRICT);.)

My code, which follows, is included via include from another PHP page that has the surrounding HTML and CSS. (I should also note that I realize that this script does not follow the PRG model.)

<?php 

/** Function read_comments() takes no arguments. It reads from what it presumes is a correctly formatted (in HTML) comments.txt file. It then prints based on whether the comments file is empty—a default message if so, or otherwise the preformmated contents. */

function read_comments() {

    $comments = file_get_contents('comments.txt');

    if(empty($comments)) {

        echo '<p><i>There are no comments at this time.</i></p>';

    } else {

        echo $comments;

    }

}

/** Function print_form() prints the user submission form, including inline CSS and POST data if a field is left blank. It takes one integer argument that does the following:
    * 0 = print blank form
    * 1 = mark the textarea border red (to indicate invalid information, defined later as null); also prints POST data
    * 2 = mark the text input field border red (to indicate invalid information, defined later as null); also prints POST data
    * 3 = mark both fields red */

function print_form($val) {

    /* This line is purely stylistic. */
    echo '<p><i>Leave a question/comment:</i></p>';

    $form = '<form method="post" action="#"><textarea name="comment" id="comment" style="';

    /* toggle CSS on textarea based on validation */
    if (($val == 1)||($val == 3)) {

        $form .= 'border: 1px solid #912; ';

    }

    $form .= 'width: 80%; height: 10em;">';

    /* display post data if any part of the form is invalid */
    if ($val != 0) {

        $form .= strip_tags($_POST['comment']);

    }

    $form .= '</textarea><p><i>Your name:</i><br><input type="text" name="name" id="name"';

    /* display post data if any part of the form is invalid */
    if ($val != 0) {

        $form .= 'value="' . strip_tags($_POST['name']) . '"';

    }

    /* toggle CSS on input block based on validation */
    if (($val == 2)||($val == 3)) {

        $form .= ' style="border: 1px solid #912;"';

    }

    $form .= '></p><p><input type="Submit" value="Post comment"></p></form>';

    echo $form;
}

/** The role of process_form() is to evaluate whether there is any information to be written; if so, validate it; then call read_comments() and print_form() appropriately */
function process_form() {

    $err = 0;

    if ($_POST) {

        /* validate the comment box */
        if (empty($_POST['comment'])) {

            $err++;

        }

        /* validate the name box */
        if (empty($_POST['name'])) {

            $err = $err + 2;

        }

        /* if valid, process the form */
        if ($err == 0) {

            /* create full HTML comment string */
            $comment = '<p>“' . strip_tags($_POST['comment']) . '”<br><span style="text-align: right; font-size: 0.75em;">—' . strip_tags($_POST['name']) . ', ' . date('F j\, g\:i A') . '</span></p>';

            /* write file to comments page */
            file_put_contents('comments.txt', $comment, FILE_APPEND | LOCK_EX);

        }

    }

    /* Run read_comments() to retrieve the comments, including anything new, then print the form with the appropriate validation value */
    read_comments();
    print_form($err);

}

process_form();

?>
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$
  1. Am I doing anything in a clearly awkward/inferior/inelegant way?

    Yes, coding html as php strings is extremely difficult to read and maintain. Do this (preferably in a separate view file):

    <span><?php echo htmlspecialchars($foo); ?></span>
    

    instead of:

    echo "<span>" . htmlspecialchars($foo) . "</span>"
    
  2. In light of that, does my script have any security weaknesses?

    Two that I can see right off:

    • Your forms are vulnerable to Cross Site Request Forgery. It does not matter that the area is protected by login/password. CSRF does not require a would be attacker to be a registered member of your site. Use a token in your form to guard against csrf.

    • Your markup is vulnerable to Cross Site Scripting. Again, protected by password does not protect you from "innocent" mistakes by your users. Always escape (htmlspecialchars) your output.

  3. Are there any awkward practices that would look strange or confusing to an experienced PHP programmer?

    • I know you mentioned being aware of PRG, you should absolutely use it. You'll thank yourself later.
    • You may eventually experience performance issues if your comments.txt file gets very large. Typically, data like this would be stored in a db and then the results paginated.
\$\endgroup\$
1
\$\begingroup\$
  • separate the HTML/layout from the PHP
  • (don't store the resulting HTML of the comments, a CSV/DB with name, comment and date should be more suitable. This is more effort in first instance, but otherwise you will not be able to change the layout in the future. Also see Robs comment in relation to Filter Input - Escape Output)
  • rename read_comments() to load_comments_and_display(), but see item 1
  • what is val? use descriptive names and don't use any binary encoding
  • ?> can be skipped as best practise
  • maybe use htmlentities instead of strip_tags if you ever what do add some html in a comment, which should be displayed (see also PHP: Prevent XSS with strip_tags()?)

With separating the layout from the logic there is even no need for additional methods.

comments.tpl.php

 <?php
    //$comments - HTML of all current comments
    //$invalidName
    //$invalidComment
    //$comment
    //$name
    <style type="text/css"><!-- move to a css file //-->
        .error {border: 1px solid #912;}
        .comment {text-align: right; font-size: 0.75em;}
    </style>
    <?if empty($comments):?>
        <p><i>There are no comments at this time.</i></p>
    <?else:?>
        <?=$comments?>
    <?endif?>

    <p><i>Leave a question/comment:</i></p>
    <form method="post" action="#">
        <textarea 
            <?if($invalidComment):?>class="error"<?endif?> 
            name="comment" id="comment" style="width: 80%; height: 10em;">
            <?=$comment?>
        </textarea>
        <p><i>Your name:</i><br>
        <input 
            <?if($invalidName):?>class="error"<?endif?> 
            type="text" name="name" id="name" value="<?=$name?>">
        </p>
        <p><input type="Submit" value="Post comment"></p>
    </form>

comments.php you can include in your other file

<?php
$invalidName=false;
$invalidComment=false;
if ($_POST) {
    $invalidComment=empty($_POST['comment']);
    $comment=$invalidComment?"":strip_tags($_POST['comment']);
    $invalidName=empty($_POST['name']);
    $name=$invalidName?"":strip_tags($_POST['name']);

    $valid=!$invalidName && !$invalidComment;
    if ($valid)
    {
        $newComment='<p>“' . $comment . '”<br><span class="comment">—' . $name . ', ' . date('F j\, g\:i A') . '</span></p>'
        file_put_contents('comments.txt', $comment, FILE_APPEND | LOCK_EX);
        // clear input, so user won't press submit again
        $comment=""; 
        $name=""
    }
}

$comments = file_get_contents('comments.txt');
require "comments.tpl.php";
\$\endgroup\$
2
  • \$\begingroup\$ please read about filter input escape output \$\endgroup\$ Jan 26, 2013 at 20:55
  • \$\begingroup\$ Of course you're right, but as long as he stores the final html in the comment file (assuming he has no database or csv), this is not really applicable here, as the output is stored in the file and storing a filtered input is not possible. \$\endgroup\$ Jan 27, 2013 at 9:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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