Creating an App from Scratch: Part 9

Creating a Web App from Scratch Part 9

Bugs, Security, and Other Tweaks

There were supposed to be only eight parts to this series, but as we started releasing them, Chris and I realized that there was going to need to be a follow-up post to address some of the bug fixes, security patches, and a few other minor changes.

NOTE: All the changes we’re going to cover in this article are already included in the source code.

Bug Fixes

After releasing the live app, a handful of bugs showed up in the comments. We tried to address these as quickly as possible to keep the app from causing unnecessary grief for our users. We’ll go over the major bugs here.

Account Created, List Failed Error

The first thing we saw was that when there were more than just one or two people trying to create accounts, the app started failing to create user lists after an account was created. Upon reviewing the code, I found that the error seemed to be coming from the following line:

$userID = $this->_db->lastInsertId();

$userID seemed to be unreliable, and therefore the query to insert a new list into the database was failing regularly. To fix this, we implemented a complex query that worked around the use of lastInsertId():

            /*
             * If the UserID was successfully
             * retrieved, create a default list.
             */
            $sql = "INSERT INTO lists (UserID, ListURL) VALUES
                    (
                        (
                            SELECT UserID
                            FROM users
                            WHERE Username=:email
                        ),
                        (
                            SELECT MD5(UserID)
                            FROM users
                            WHERE Username=:email
                        )
                    )";

Performance-wise, this is going to be slower than the original post, but it’s incredibly more reliable (since implementing this fix, we’ve had no reports of this error). Any MySQL supergeeks who may have a better solution, please post it in the comments!

Double-Clicking “Add” Sometimes Added Duplicate Entries

One little user interface quirk that was discovered was that you could click multiple times in succession on the “Add” button. In our original JavaScript, we only cleared the value of the input field upon a successful AJAX result. That is ideal, since when that text disappears that is your visual queue that it’s been added to your list successfully. Plus, generally that happens quickly enough it feels pretty instant. However, if you straight up “double-click” on that add button (which people absolutely still do that), you might get two or more requests off before the success comes back and clears the fields (when the field is clear, the submit button will do nothing).

One method to fix this could have been to clear the field as soon as a click happens, but the problem there is that if the save is unsuccessful you’ll lose your text. Instead, we just add a little more smarts. When the submit button is clicked and there is text ready to add, the AJAX request is made and the button is disabled. Upon success, the field is cleared and the button is re-enabled. This ensures only one submission is possible.

In /js/lists.js, we added the following at line 114:

    $('#add-new').submit(function(){

       //  ... variables and whitelist stuff ...

        if (newListItemText.length > 0) {
            // Button is DISABLED
            $("#add-new-submit").attr("disabled", true);

            $.ajax({

                // Ajax params stuff

                success: function(theResponse){

                    // list adding stuff

                    // field is cleared and button is RE-ENABLED
                    $("#new-list-item-text").val("");
                    $("#add-new-submit").removeAttr("disabled");
     }

NOTE: As you can see, we remove the “disabled” attribute entirely upon a successful response from our query. That is the only way to re-able a submit button. Setting disabled to “false” has no effect.

Changing Email Address with a Blank Email Crippled Account

It was also brought to our attention that clicking the “Change Email” button with a blank field would not only succeed, but would cripple the account and make it unusable. Fixing this was as simple as making sure the email address submitted was valid by inserting the following in the updateEmail() method in /inc/class.users.inc.php:

        if( FALSE === $username = $this->validateUsername($_POST['username']) )
        {
            return FALSE;
        }

Then, instead of binding the $_POST value to the query, we bind the new variable $username, which contains the valid email address if the check didn’t fail. Note the use of the new function validateUserEmail()—we’ll go over that in the next section on security.

Security Issues

After we worked the bugs out of our app, we turned to the security holes that were pointed out by commenters. Some of these were simple oversights on our part, and some of the problems were news to us. With the help of our readers, though, we tried to patch everything up.

JavaScript Could Be Inserted Into Edited Items

When creating new items, we checked for any JavaScript in the input using the cleanHREF() function, then stripped out unwanted tags on the server side using strip_tags() and a whitelist of acceptable tags. However, we had missed that JavaScript could be inserted into existing items when they were edited. To correct this issue, we turned to a preexisting input sanitizing function (lines 00326-00384) posted by Zoran in the comments of Part 8.

We wrapped the code in a method called cleanInput() and placed it in /inc/class.users.inc.php:

    /**
     * Removes dangerous code from the href attribute of a submitted link
     *
     * @param string $input        The string to be cleansed
     * @return string            The clean string
     */
    private function cleanInput($data)
    {
        // http://svn.bitflux.ch/repos/public/popoon/trunk/classes/externalinput.php
        //  ----------------------------------------------------------------------
        // | Copyright (c) 2001-2006 Bitflux GmbH                                 |
        //  ----------------------------------------------------------------------
        // | Licensed under the Apache License, Version 2.0 (the "License");      |
        // | you may not use this file except in compliance with the License.     |
        // | You may obtain a copy of the License at                              |
        // | http://www.apache.org/licenses/LICENSE-2.0                           |
        // | Unless required by applicable law or agreed to in writing, software  |
        // | distributed under the License is distributed on an "AS IS" BASIS,    |
        // | WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or      |
        // | implied. See the License for the specific language governing         |
        // | permissions and limitations under the License.                       |
        //  ----------------------------------------------------------------------
        // | Author: Christian Stocker <chregu@bitflux.ch>                        |
        //  ----------------------------------------------------------------------
        //
        // Kohana Modifications:
        // * Changed double quotes to single quotes, changed indenting and spacing
        // * Removed magic_quotes stuff
        // * Increased regex readability:
        //   * Used delimeters that aren't found in the pattern
        //   * Removed all unneeded escapes
        //   * Deleted U modifiers and swapped greediness where needed
        // * Increased regex speed:
        //   * Made capturing parentheses non-capturing where possible
        //   * Removed parentheses where possible
        //   * Split up alternation alternatives
        //   * Made some quantifiers possessive

        // Fix &entityn;
        $data = str_replace(array('&amp;','&lt;','&gt;'), array('&amp;amp;','&amp;lt;','&amp;gt;'), $data);
        $data = preg_replace('/(&#*w )[x00-x20] ;/u', '$1;', $data);
        $data = preg_replace('/(&#x*[0-9A-F] );*/iu', '$1;', $data);
        $data = html_entity_decode($data, ENT_COMPAT, 'UTF-8');

        // Remove any attribute starting with "on" or xmlns
        $data = preg_replace('#(<[^>] ?[x00-x20"'])(?:on|xmlns)[^>]* >#iu', '$1>', $data);

        // Remove javascript: and vbscript: protocols
        $data = preg_replace('#([a-z]*)[x00-x20]*=[x00-x20]*([`'"]*)[x00-x20]*j[x00-x20]*a[x00-x20]*v[x00-x20]*a[x00-x20]*s[x00-x20]*c[x00-x20]*r[x00-x20]*i[x00-x20]*p[x00-x20]*t[x00-x20]*:#iu', '$1=$2nojavascript...', $data);
        $data = preg_replace('#([a-z]*)[x00-x20]*=(['"]*)[x00-x20]*v[x00-x20]*b[x00-x20]*s[x00-x20]*c[x00-x20]*r[x00-x20]*i[x00-x20]*p[x00-x20]*t[x00-x20]*:#iu', '$1=$2novbscript...', $data);
        $data = preg_replace('#([a-z]*)[x00-x20]*=(['"]*)[x00-x20]*-moz-binding[x00-x20]*:#u', '$1=$2nomozbinding...', $data);

        // Only works in IE: <span style="width: expression(alert('Ping!'));"></span>
        $data = preg_replace('#(<[^>] ?)style[x00-x20]*=[x00-x20]*[`'"]*.*?expression[x00-x20]*([^>]* >#i', '$1>', $data);
        $data = preg_replace('#(<[^>] ?)style[x00-x20]*=[x00-x20]*[`'"]*.*?behaviour[x00-x20]*([^>]* >#i', '$1>', $data);
        $data = preg_replace('#(<[^>] ?)style[x00-x20]*=[x00-x20]*[`'"]*.*?s[x00-x20]*c[x00-x20]*r[x00-x20]*i[x00-x20]*p[x00-x20]*t[x00-x20]*:*[^>]* >#iu', '$1>', $data);

        // Remove namespaced elements (we do not need them)
        $data = preg_replace('#</*w :w[^>]* >#i', '', $data);

        do
        {
            // Remove really unwanted tags
            $old_data = $data;
            $data = preg_replace('#</*(?:applet|b(?:ase|gsound|link)|embed|frame(?:set)?|i(?:frame|layer)|l(?:ayer|ink)|meta|object|s(?:cript|tyle)|title|xml)[^>]* >#i', '', $data);
        }
        while ($old_data !== $data);

        return $data;
    }

Then, we modified updateListItem() on line 239 to call the new method:

        $newValue = $this->cleanInput(strip_tags(urldecode(trim($_POST["value"])), WHITELIST));

CATCH: This function appears to encode any non-English characters. Foreign language users may see some unexpected behavior.

Cross-Site Request Forgeries

Another risk we hadn’t considered when building this app was the possibility that a malicious user could send bogus requests to our app by piggybacking on a Colored Lists user’s session in a form of attack called Cross-Site Request Forgeries (CSRF). The snippet of JavaScript below, placed on any site, would be executed if a user that was logged in to our app were to visit the page. (Huge thanks to Dan at Sketchpad for pointing this out and providing the above sample attack.)

    <script type="text/javascript">

        var form = document.createElement("form");
        form.setAttribute("method", "post");
        form.setAttribute("action", "http://coloredlists.com/db-interaction/users.php");

        var fields = new Array();
        fields["user-id"] = "158";
        fields["action"] = "deleteaccount";

        for(var key in fields)
        {
            var hiddenField = document.createElement("input");
            hiddenField.setAttribute("type", "hidden");
            hiddenField.setAttribute("name", key);
            hiddenField.setAttribute("value", fields[key]);

            form.appendChild(hiddenField);
        }

        document.body.appendChild(form);
        form.submit();

    </script>

To remedy this, we need to generate a token to include with each form submission that is also stored in the session. That way we can make sure the two match before executing any requests. In doing this, CSRF is virtually eliminated.

In /common/base.php, we added following at line 19:

    if ( !isset($_SESSION['token']) )
    {
        $_SESSION['token'] = md5(uniqid(rand(), TRUE));
    }

This creates a unique token for the user’s session. Then, on every form on our site, we added the following hidden input:

                <input type="hidden" name="token"
                    value="<?php echo $_SESSION['token']; ?>" />

And updated both /db-interaction/lists.inc.php and /db-interactions/users.inc.php with the following starting at line 15:

if ( $_POST['token'] == $_SESSION['token']
    && !empty($_POST['action'])
    && isset($_SESSION['LoggedIn'])
    && $_SESSION['LoggedIn']==1 )

Now any request made without a valid token will fail. For more on CSRF, visit Chris Shiflett’s blog.

Some Input Was Improperly Sanitized

Above, we talked about the problem with blank email change requests breaking accounts, and we created a method called validateUsername() that made sure only valid email addresses were allowed to change an existing user email. That method looks like this:

    /**
     * Verifies that a valid email address was passed
     *
     * @param string $email    The email address to check
     * @return mixed        The email address on success, FALSE on failure
     */
    private function validateUsername($email)
    {
        $pattern = "/^([ a-zA-Z0-9]) ([ a-zA-Z0-9._-])*@([a-zA-Z0-9_-]) ([a-zA-Z0-9._-] ) $/";
        $username = htmlentities(trim($email), ENT_QUOTES);
        return preg_match($pattern, $username) ? $username : FALSE;
    }

Essentially, it uses a regular expression to match the pattern of a valid email address, and either returns the validated email address of boolean FALSE.

Other Changes

Aside from bugs and security patches, there were a couple parts of the site that we just felt should have been better.

Made Public URLs Tougher to Guess

First, the original public list URLs were determined using dechex(), and they were short and easy to guess. We modified them to use MD5 instead to create longer, much more difficult to guess public URLs. This happens right at the list’s creation when the query calls SELECT MD5(UserID) in createAccount() on line 100.

Allowed “Safe” Links in List Items

Some links are acceptable, and we felt that our app would be much more useful if safe links were allowed in list items. To allow this, we simply removed the call to strip_tags() in formatListItems() (found in /inc/lists.inc.php on line 173):

        return "tttt<li id="$row[ListItemID]" rel="$order" "
            . "class="$c" color="$row[ListItemColor]">$ss"
            . $row['ListText'].$d
            . "$se</li>n";

The items are now sanitized on the way in, so we don’t need to worry about them on the way out.

Summary

The steps we took above helped make our app more secure and dependable. However, we know that nothing is ever perfect, so if you’ve got other bugs, security holes, or suggestions, let us know in the comments!

Series Authors

Jason Lengstorf is a software developer based in Missoula, MT. He is the author of PHP for Absolute Beginners and regularly blogs about programming. When not glued to his keyboard, he’s likely standing in line for coffee, brewing his own beer, or daydreaming about being a Mythbuster.
Chris Coyier is a designer currently living in Chicago, IL. He is the co-author of Digging Into WordPress, as well as blogger and speaker on all things design. Away from the computer, he is likely to be found yelling at Football coaches on TV or picking a banjo.

About Jason Lengstorf

Jason Lengstorf lives in Portland, OR. He loves design, code, business, and marketing. He also writes and speaks about design, code, business, and marketing. View all posts by Jason Lengstorf →
  • http://matt@matt-bridges.com Matt B

    Great job, you guys! Security is always an issue, and when you put ideas and theories to practical use, it makes it easier for the rest of us to understand.

    Keep it up!

  • http://dalairen.ru Dalairen

    I'm going to translate the whole series to Russian and publish it at habrahabr.ru, the IT community blog portal, with all supportive matherial and all proper credits to you and Chris. Are there any restrictions and recomendations? Maybe I must leave something alone?) No app copies will be done though, only links.

  • http://www.ennuidesign.com Jason Lengstorf

    @Dalairen:

    That's great! Drop me a link when it goes up (even though I won't be able to read it) :) — I'll post a link about the translation.

  • http://thinsoldier thinsoldier

    if you use characters like é and á and î in the list text, when it auto-saves and you refresh the page you get a symbol of a question mark in a diamond in place of the facy letters.

    This is an issue that's plagued me on my own projects. All my clients love writing their stuff in MS word and then pasting it into the web forms. Word apparently loves replacing simple things like dashes (-) and ampersands (&) and apostrophies (') with these special characters.

    You really want to write some super useful code that tons of people will love you for? Show us how to deal with this characters going into and coming out of the database once and for all.

  • http://montana@complimedia.com Montana Flynn

    Big ups from the westside.

  • mikemc

    I know this is a bit late, but hopefully someone will come across this. I'm new to PHP. I was looking at your source code I notice you have an 'inc' folder and a 'common' folder. What differentiates the two? I have just been using one 'includes' folder. Are the two folders better practice or just more a matter of preference.

    Thanks for a great tut!

    Mike

  • http://www.ennuidesign.com Jason Lengstorf

    @Mike

    In this app it was more a matter of preference. If I had it to do over again, the PHP includes that handle processing would be in the /inc/ folder, and the ones that generate formatting for the header/footer/etc. would be in the /common/ folder.

  • mikemc

    Thanks Jason for the speedy reply. Will keep that in mind in the future.

  • thinsoldier

    CATCH: This function appears to encode any non-English characters. Foreign language users may see some unexpected behavior.

    I don't know any language other than english but I want to mention my neighbor François in an entry I get 2 weird letters in place of the c. There are a lot of English speaking people with accented Ees and Aas in their name.

    Please please please show us how to deal with the non-English charactrers! please. It's a problem that's plagued me for almost 4 years now.

  • Felipe

    when you click the X button to delete an item the sure? tab slides out, but it sticks there. so if you decide not to delete it, even if you click out of it, i just stays there.

    awesome series by the way.

    and how would i go about doing mods to the design?

    i cant seem to the the source code to run in localhost

    thank you

  • http://www.ennuidesign.com Jason Lengstorf

    @thinsoldier:

    Foreign characters get mangled by certain character-encodings. A lot of databases will default to latin1_swedish_ci, which has caused character issues for me in the past. I now make sure all my databases are in utf8 (I use utf8_general_ci on this site), and that seems to handle special characters properly.

    The security function we borrowed in this app seems to encode special characters, such as Japanese characters, when it's run. I'm not sure, but I believe it's because the ASCII character codes fall out of the allowed range in that function. By adjusting the range, that would potentially solve the problem. Sorry if that's not the answer you were looking for.

    @Felipe:

    Are you referring to the actual design? You'll have to geek around with Chris's Photoshop file for that. He handled all the design and HTML/CSS for this app.

    As far as getting it running in localhost, make sure you've got the database credentials set up properly for your environment. Is it giving you an error? If so, what does it say?

  • Felipe

    @thinsoldier: thanks for the reply man. i must warn im a noob. at php and database that is.

    the error i get is this:

    Connection failed: SQLSTATE[28000] [1045] Access denied for user 'db_user'@'localhost' (using password: YES)

    not sure what i need to do.

    thank you again.

  • http://ennuidesign.com Jason Lengstorf

    @Felipe

    You need to replace the information in constants.inc.PHP with your local installation info.

    The defaults are user name root and a blank password.

    Good luck!

  • jupiter

    I just want to thank you for a great post and for making the code available. I used part of your code to create a login interface, in my server, it works beautifully it does not yet serve any purpose but it may some time in the future. [littleprince.dyndns.org/homejupiter]. I wonder I if might make a suggestion for something further maybe for this app or a separate project, just something I would like to learn – A two level login system – ?

  • Michael

    Hi,

    Absolutely brill tutorial. Fancy using it for future PHP work I do.

    Only slight issue I get is:

    Warning: require_once(inc/FirePHPCore/FirePHP.class.php) [function.require-once]: failed to open stream: No such file or directory in /lists/common/base.php on line 13

    Any ideas whats happening?

    Do I need to install this library?

  • Michael

    A quick Google search (what else?) found the issue:

    http://code.google.com/p/firephp/source/browse/branches/Library-FirePHPCore-0.2/lib/FirePHPCore/?r=579

    Thanks again for taking the time to make such a useful tutorial.

  • http://steadystatesolutions.org Kurtis Dinelle

    I realize this is relatively old, but I discovered another minor security flaw. Although you disallow javascript in the href attribute of anchor tags, you didn't disallow adding other attributes to allowed tags. For example, I posted this link:

    [a href=”#” onclick=”javascript:alert('XSS')”>Click Me[/a]

  • http://www.ennuidesign.com Jason Lengstorf

    @Kurtis Dinelle:

    Thanks for pointing this out! Are you posting that as a new item, or when you edit an item? The function cleanInput() should be removing any onclick or other attributes; let me know and I'll check it out.

    Thanks!

  • http://steadystatesolutions.org Kurtis Dinelle

    Yes it's a new item. When editing an item, your right, it gets filtered out.

  • http://www.ennuidesign.com Jason Lengstorf

    @Kurtis Dinelle:

    Hmmm… Sounds like we forgot to apply cleanInput() to new items. Thanks for pointing this out!

  • AndiFox

    what should I do guys?

    I have php 5

    I'm sorry I'm a little newbie

  • http://www.ennuidesign.com Jason Lengstorf

    @AndiFox:

    Is there any additional information included with that error? Or is that it?

  • AndiFox

    hehe sorry for my bad englissh

    other error in my browser

    Fatal error: Class 'PDO' not found in C:xampphtdocsColoredLists_v1.0commonbase.php on line 27

    please helpme!!

  • AndiFox

    i dont know..i paste the sourcecode in the root folder change the parameters of the constant variables of the database and all but I can not make it work

    Thank!

  • http://www.ennuidesign.com Jason Lengstorf

    @AndiFox:

    It looks like you need to get PDO running on your installation of PHP. Check your php.ini file and follow the instructions here: http://ca3.php.net/manual/en/pdo.installation.php

    Good luck!

  • Prakash

    This application is not working in Opera.,

    Try this in opera.,

  • http://www.miamiwebdesignpro.com Miami Web Design

    Wow chris, this is a very long and detailed tutorial. You definitely have some great stuff here. Keep up the good work.

  • http://www.webdesignplanet.us webdesign-planet

    Very practical tut and thanks for these article.by the way, do you have any book?

  • http://www.ennuidesign.com Jason Lengstorf

    @webdesign-planet

    Glad you enjoyed it! I have two books, actually:

    PHP for Absolute Beginners – http://amzn.to/doVW7h

    Pro PHP and jQuery – http://amzn.to/aa0ZJO

    Thanks!

  • Mark

    Hey,

    I'm not 100% sure on this without writing a test-case, but I believe I know the cause of your “Account Created, List Failed” error.

    Short answer:

    You need to create user accounts inside a transaction. I.e. Begin transaction, Insert new user, Get the userid, Insert new list, Commit transaction.

    Longer answer:

    The MySQLi extension does this differently to PDO, afaik, the docs aren't terribly clear.

    MySQLi's insert_id function works on a per-connection basis, so calling insert_id in auto-commit mode will always return the id of the record you've just created – as expected.

    However PDO's lastInsertId function seems to return the id of the last record to be created via any connection. Obviously this breaks everything when more than one person tries to sign up at once:

    User #1: Insert user // id: 1

    User #2: Insert user // id: 2

    User #2: Userid = lastInsertId() // id: 2 OK

    User #1: Userid = lastInsertId() // id: 2 UHOH! Expected 1

    User #2: Insert List // id: 2 OK

    User #1: Insert List // id: 2 Throws some sort of db error, duplicate primary key!

    As mentioned, if you wrap the account create process in a transaction only one connection will be able to update the database at a time, and lastInsertId should work as expected.

    References:

    http://www.php.net/manual/en/mysqli.insert-id.php

    http://www.php.net/manual/en/pdo.lastinsertid.php

  • http://www.ennuidesign.com Jason Lengstorf

    @Mark:

    Thanks! I think you're dead-on with that.

  • http://www.copterlabs.com/ Jason Lengstorf

    It's linked in the header. I just tested it and the link works. Let me know if you can't find it!

  • Rolf

    I must be daft, i can't find either in the PHP source or in the javascripts how you create a users public html file.

    Where and how is it done?

  • http://www.copterlabs.com Jason Lengstorf

    The HTML file doesn't actually exist for the users. We're using .htaccess to pass the name of the HTML file as the value of $_GET['list'], which we use in index.php when the user is logged out.

    Check out .htaccess and index.php to see where it's happening.

    Good luck!

  • Hunter

    First, I just wanted to say that this set of tutorials is the most comprehensive example on web development that I have ever come across and it has helped me a ton.

    Next, I wanted to share with you a bug that I've found. If you set up a new account, click on the validation email to set up your password, and proceed to type in mis-matched passwords it returns you to the password page (notice the $_GET fields are now empty). Then if you enter in matching passwords you become logged in. The problem with this is that the account was never verified and the password hash was never stored so the next time you try to login you have no success.

    I'm searching for a simple work-around. I'll post when I have something.

    Again, thanks for such awesome work.

  • Hunter

    OK, I think I've found a solution to the bug from above.

    I think that the $_GET value for 'v' needs to be maintained during every call to accountverify.php. In order to do this I simply changed the form action from

    action=”./accountverify.php”>

    to

    action=”./accountverify.php?v=”>

    on accountverify.php and resetpassword.php.

    This should take care of the problem, but it might not be the best solution. If there is a more secure/fireproof way, I'm open to your suggestions.

  • Hunter

    That last post was supposed to have a $_GET [ 'v' ] after the v= part.

  • http://www.copterlabs.com Jason Lengstorf

    Good catch, Hunter!

    Probably an even better solution would be to either store the contents of $_GET['v'] in a session or cookie, but your solution works just fine.

    Thanks!

  • Ryan

    Part 7's link is broken? Can you please post a new link if available?

  • http://www.jurgelenas.lt/ Julius

    Hey,

    I found error in inc/class.users.inc.php lines 59-63 :)

    if($row['theCount']!=0) {

    return ” Error ”

    . ” Sorry, that email is already in use. ”

    . “Please try again. “;

    }

    There should be:

    if($row['theCount']!==0) {

We’re Ready When You Are

Ready to build something awesome?

Start Here