SitePoint Sponsor

User Tag List

Page 2 of 3 FirstFirst 123 LastLast
Results 26 to 50 of 60
  1. #26
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Ugh, I'm not even using $username in that sql query, but my questions can be applied to $user_id still, and the fact that I am escaping $username multiple times elsewhere anyway.

    No sleep no brain.

  2. #27
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Here is what I came up with and I think it is right except for one double escape still..

    PHP Code:
    <?php
    session_start
    ();
    if (!isset(
    $_SESSION['username'])) {
            
    header('Location: /website/login.php');
    }
    if (isset(
    $_SESSION['username']) && isset($_SESSION['userid'])) {
        
    $username filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
        
    $userid filter_var($_SESSION['userid'], FILTER_SANITIZE_STRING);
    }
    require_once 
    $_SERVER['DOCUMENT_ROOT'] . '/includes/database_connection.php';
    require_once 
    $_SERVER['DOCUMENT_ROOT'] . '/includes/functions.php';
    if(isset(
    $_POST['item_submit'])) {
        
    $user_id mysqli_real_escape_string($link$_SESSION['user_id']);
        
    $item_name mysqli_real_escape_string($link$_POST['item_name']);
        
    $sql mysqli_query($link"INSERT INTO items (user_id, item_name) VALUES ('$user_id', '$item_name')");
            if (!
    $sql) {
                
    $error 'DATABASE ERROR.';
                include 
    'error.php';
                exit();
            }
            else
            
    header('Location: /website/items.php?user=' htmlspecialchars($username) . '');
    }
    ?>
    <html>
        <ul>
            <li><?php echo '<a href="navigation_link_one.php?user=' htmlspecialchars($username) . '">' ?>navigation_link_one</a></li>
            <li><?php echo '<a href="navigation_link_two.php?user=' htmlspecialchars($username) . '">' ?>navigation_link_two</a></li>
        </ul>
                <form action="add_item.php" method="post">
                
                    <label for="itemname">Item Name:</Label>
                    <input type="textbox" name="item_name"/>
                    
                    <input type="submit" name="item_submit" value="Add Item"/>
                </form>
    </html>
    But I feel like the fact that my navigation escaping $username first, and then my redirect (upon form submission) escaping it again, is double escaping?

    Should have just posted this, much simpler.. hehe

  3. #28
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    Is the page index.php which is the start of your website, also in this directory?

    $_SERVER['DOCUMENT_ROOT']

  4. #29
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Cups View Post
    Is the page index.php which is the start of your website, also in this directory?

    $_SERVER['DOCUMENT_ROOT']
    doc root is /x/

    website files are in /x/website/

    includes are in /x/includes/

    need to sort out protecting my includes someday, am hoping to sort out this escaping issue (assuming there is one) first

    note: when site is tested live doc root is /, main website files are in /, and includes are in /includes. hence the need to protect them.. but that is a job for later.

  5. #30
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by oknow View Post
    I'm assuming, that since the $username variable is automatically htmlspecialchar'd on every page due to my navigation code, that when the user submits a form and $username gets mysqli_special_escape_string put on it - THAT is 'double escaping'?
    Not as such. htmlspecialchars converts a small subset of characters in to their html entity character form:
    • & => &amp;
    • " => &quot;
    • < => &lt;
    • > => &rt;


    Double escaping commonly occurs in three different places.

    First, when the PHP script retrieves values from $_POST and $_GET. If you have magic quotes enabled on your server, O'Brian will be turned in to O\'Brian

    It was soon found though that magic quotes weren't effective enough, so then came mysql_escape_string, which didn't do a proper job, so we now use mysql_real_escape_string

    The second time when people mistakenly escape values is when assign those $_POST and $_GET values to variables. Some people mistakenly perform mysql_real_escape_string at that point, which turns O\'Brian in to O\\\'Brian

    Third is when sending the SQL query, where they may yet again escape the values.

    How do you deal with magic quotes? If they are enabled, turn them off at the server. If you can't turn them off at the server, you can filter them at run-time, but that's at a performance cost.
    See this PHP.net article on disabling magic quotes

    Quote Originally Posted by oknow View Post
    Not to mention that since $username is automatically done on every page load in the navigation, it gets done a THIRD time when the user submits an item and they are redirected:

    PHP Code:
    header('Location: /website/items.php?user=' htmlspecialchars($username) . ''); 
    Is that assumption correct?
    The way that I like to see it is that when the variables are brought in to your code, you should ensure that they end up being naked and unprotected while running around in your code. Why? Because often one value may end up used for three different types of output (database, url, html), and it's not possible to initially escape them so that they're suitably for all types. You could create several copies of the variable, one for each intended usage of it, but that get messy very quickly. So, have the variables unescaped throughout your code and escape them when they're being outputted. That way, any output that is not escaped will instantaneously be a code-smell that you can easily perceive.

    With the header location example above, instead of htmlspecialchars, I think that urlencode() is the more proper one to use instead.

    Using urlencode also helps to remind you about the intended usage of the code. If you had a variable called $location, it would be entirely possible to mistake the intended usage of that variable.

    Code php:
    $location = '/website/items.php?user=' . htmlspecialchars($username);

    When you use urlencode, there's never a miscommunication.

    Code php:
    $location = '/website/items.php?user=' . urlencode($username);

    You will even see the urlencode documentation page using both urlencode and htmlentities at the same time, because they each serve different purposes.

    Quote Originally Posted by oknow View Post
    All the above said, I should (?) be using filter_var on the following instead anyway. But that would still leave me with some 'double escaping' in the rest of the code, if my above assumptions are correct.
    If need be you can var_dump variables while testing to ensure that they are being handled properly. The purpose of escaping a url string is so that it can be safely submitted to load a new page. I don't think that it ends up being received by the page as escaped code. However, if you have magic quotes enabled then that might escape things for you instead, which you need to deal with so that the variable can then be handled more correctly with techniques that do a better job.

    Quote Originally Posted by oknow View Post
    I am also thinking that filter_var is for sanitizing, not escaping for direct output, and I could be wrong there too.
    You're not wrong there.

    Quote Originally Posted by oknow View Post
    PHP Code:
    if (isset($_SESSION['username']) && isset($_SESSION['user_id'])) {
        
    $username filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
        
    $user_id filter_var($_SESSION['user_id'], FILTER_SANITIZE_STRING);

    Although, if user_id is supposed to be a number, or even an integer, you may want to use a different filter for that one, such as FILTER_SANITIZE_NUMBER_INT from the list of sanitize filters

    Basically it seems to come down to two basic fundamentals:
    • Sanity check all inputs, especially if they're potentially unknown (therefore untrusted)
    • Escape all outputs


    It just seems to be common sense.

    Putting it in to practice does have differences depending on where it's coming from, or where it's going to, but that's why there are good standards that can be followed to help reduce the problems that can occur.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  6. #31
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Paul. There are not enough thanks in the world.

    I don't have magic quotes enabled thankfully, so I don't have to deal with that.

    And you know, I never actually thought about htmlspecialchars being unsuitable for my header redirect.. it's not html output, duh!! Thanks for the heads up there.

    Here is the final code I have come up with, I think I finally have it right and can stop bugging you. (Oh please let it be right! )

    PHP Code:
    <?php
    session_start
    ();
    if (!isset(
    $_SESSION['username'])) {
            
    header('Location: /website/login.php');
    }

    if (isset(
    $_SESSION['username']) && isset($_SESSION['userid'])) {
        
    $username filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
        
    $userid filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
    }

    require_once 
    $_SERVER['DOCUMENT_ROOT'] . '/includes/database_connection.php';
    require_once 
    $_SERVER['DOCUMENT_ROOT'] . '/includes/functions.php';

    if (isset(
    $_POST['item_submit'])) {
        
    $user_id mysqli_real_escape_string($link$_SESSION['user_id']);
        
    $item_name mysqli_real_escape_string($link$_POST['item_name']);
            
    $sql mysqli_query($link"INSERT INTO items (user_id, item_name) VALUES ('$user_id', '$item_name')");
                if (!
    $sql) {
                    
    $error 'DATABASE ERROR.';
                    include 
    'error.php';
                    exit();
                }
                else
                
    header('Location: /website/items.php?user=' urlencode($username) . '');
    }
    ?>

    <html>
        <ul>
            <li><?php echo '<a href="navigation_link_one.php?user=' htmlspecialchars($username) . '">' ?>navigation_link_one</a></li>
            <li><?php echo '<a href="navigation_link_two.php?user=' htmlspecialchars($username) . '">' ?>navigation_link_two</a></li>
        </ul>
                <form action="add_item.php" method="post">
                    <label for="itemname">Item Name:</Label>
                    <input type="textbox" name="item_name"/>
                    <input type="submit" name="item_submit" value="Add Item"/>
                </form>
    </html>

  7. #32
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by oknow View Post
    Here is the final code I have come up with, I think I finally have it right and can stop bugging you. (Oh please let it be right! )
    There are several minor things, so what say I do a small code review right here for you.

    If Statements

    Code:
    if (!isset($_SESSION['username'])) {
            header('Location: /website/login.php');
    }
    
    if (isset($_SESSION['username']) && isset($_SESSION['userid'])) {
        $username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
        $userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
    }
    The first significant thing with these two here is that you have the same term that is inverted in one if statement, and not inverted in the other.
    These two if statement can be combined together, so that you don't have to think so hard to realise that they are actually related.

    Code:
    if (!isset($_SESSION['username'])) {
            header('Location: /website/login.php');
    } else if (isset($_SESSION['userid'])) {
        $username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
        $userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
    }
    Even better though is to realise that filter_var gives you an empty value if the variable doesn't exist, so you could do this instead:

    Code php:
    $username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
    if (empty($username)) {
        header('Location: /website/login.php');
    }
    $userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);

    Database Variables

    Next up is the start of the database condition:

    Code:
    if (isset($_POST['item_submit'])) { 
        $user_id = mysqli_real_escape_string($link, $_SESSION['user_id']); 
        $item_name = mysqli_real_escape_string($link, $_POST['item_name']);
    The first problem here is that $user_id already exists, having retrieved the value via filter_var. Now it's replaced with a completely different value.

    Move those variable assignments up top (and remove the mysqli_real_escape_string for now) and that solves several other problems too.

    Code php:
    $item_submit = isset($_POST['item_submit']);
    $userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
    $item_name = filter_input(INPUT_POST, 'item_name', FILTER_SANITISE_STRING);
    ...
    if ($item_submit) {

    What are those other problems if they had been left where they were?

    One of those other problem is that superglobals such as $_SESSION and $_POST are used within that database section. Those should have been handled earlier on, so that you don't have superglobals scattered all throughout your code.

    Getting that done before performing the database work is one of those invisible demarcation barriers. It's a "separation of duties" that helps to simplify your code, and make it easier to modify and understand later on.

    SQL Statement

    Next we get to the $sql statement.

    A fundamental flaw in how the mysqli_real_escape_string was used before is that whenever you assign the escaped value to variables, those variables have a flow-on effect long after the database part of the code has been done.

    While in this particular case it doesn't have a direct impact, if you had also escaped $username to send to your database, you would then find when outputting it in the HTML section that the $username variable is already escaped.

    The solution to this is to ensure that escaped variables do not persist beyond the point where they are used.

    One common way to do this is to encapsulate things within a function, so that the escaped values do not persist once the function is complete.
    Another way that you could immediately use here, is to escape only when creating the sql string.

    But first, let's look at the SQL string.

    Code:
    $sql = mysqli_query($link, "INSERT INTO items (user_id, item_name) VALUES ('$user_id', '$item_name')");
        if (!$sql) {
    Instead of $sql being the SQL string, it is instead the result of the database call. Most commonly the $result variable is used instead to store the result of the database call, so let's separate out the SQL statement from the result.

    Code:
    $sql = "INSERT INTO items (user_id, item_name) VALUES ('$user_id', '$item_name')";
    $result = mysqli_query($link, $sql);
        if (!$result) {
    Now we just need to ensure that the variables being entered in to the SQL statement are properly escaped. Commonly sprintf() is used to help reduce the complexities of that.

    Code php:
    $sql = sprintf('INSERT INTO items (user_id, item_name) VALUES (%d, "%s")',
        intval($userid),
        mysql_real_escape_string($item_name)
    );

    Notice how $userid is converted to an integer? In the SQL statement, %d is always a number (a decimal value) so you don't need to look any further to know that the code is correct. Further on, we use intval to ensure that whatever $userid happens to be, that it can now be nothing but an integer number. Even though we sanitised earlier on, other things can happen to that variable throughout the code, so it's a good practice to always escape (or cast) in some way the SQL variables. It's a final safety net.

    It's either that or use prepared statements, which many people don't want to deal with.

    The End Result

    So what are we left with after all that?

    There are some other minor issues remaining such as variable name consistency, but I don't want to go tweaking things too much until you can see a clearly defined reason to do so.

    For the sake of succinctness I have left off the HTML code below the PHP script as there's no change to that part. Even though the script still looks mostly similar to what you had, hopefully you can see the benefits now of the improvements that have been made.

    Code php:
    <?php
    session_start();
    $username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
    if (empty($username)) {
        header('Location: /website/login.php');
    }
     
    $item_submit = isset($_POST['item_submit']);
    $userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
    $item_name = filter_input(INPUT_POST, 'item_name', FILTER_SANITISE_STRING);
     
    require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/database_connection.php';
    require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/functions.php';
     
    if ($item_submit) {
        $sql = sprintf('INSERT INTO items (user_id, item_name) VALUES (%d, "%s")',
            intval($userid),
            mysql_real_escape_string($item_name)
        );
        $result = mysqli_query($link, $sql);
            if (!$result) {
                $error = 'DATABASE ERROR.';
                include 'error.php';
                exit();
            } else {
                header('Location: /website/items.php?user=' . urlencode($username));
            }
        }
    }
    ?>

    Or, if you take the function path:

    Code php:
    <?php
    session_start();
     
    function insertItem($link, $userid, $item_name)
    {
        $userid = intval($userid);
        $item_name = mysql_real_escape_string($item_name);
     
        $sql = 'INSERT INTO items (user_id, item_name) VALUES (' . $userid . ', "' . $item_name . '")';
        return mysqli_query($link, $sql);
    }
     
    $username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
    if (empty($username)) {
        header('Location: /website/login.php');
    }
     
    $item_submit = isset($_POST['item_submit']);
    $userid = filter_var($_SESSION['userid'], FILTER_SANITIZE_NUMBER_INT);
    $item_name = filter_input(INPUT_POST, 'item_name', FILTER_SANITISE_STRING);
     
    require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/database_connection.php';
    require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/functions.php';
     
    if ($item_submit) {
        if (insertItem($link, $userid, $item_name)) {
            header('Location: /website/items.php?user=' . urlencode($username));
        } else {
            $error = 'DATABASE ERROR.';
            include 'error.php';
            exit();
        }
    }
    ?>
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  8. #33
    SitePoint Addict
    Join Date
    Dec 2008
    Location
    Brussels
    Posts
    377
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Do you guys use the Hungarian notation for variables?

  9. #34
    SitePoint Wizard silver trophybronze trophy Stormrider's Avatar
    Join Date
    Sep 2006
    Location
    Nottingham, UK
    Posts
    3,133
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    I do use a simple form of it - prefixing variable names with 3 letters depending on its type (str, int, dec, arr, obj etc). I know it's not a popular practice and others on here may disagree, but I find it quite beneficial and have found no downsides to it so far.

  10. #35
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    I think I actually get the hang of it now, and I re-did my code in the style of your example. Not just copy paste, but did it piece by piece while reading your post, which I have probably read near fifty times already last night and today.. no exaggeration.

    I am very eager to learn not only the RIGHT way to do things, but to know why.

    I do have two more things I am wondering about. And these will be the last things for now. I feel privileged enough to have the amazingly detailed help you have given me, but anything further and I feel like I should start paying you.

    Firstly, I am wondering (and you have probably said so but it flew over my head) what the point is of the first set of filtering, when those variables aren't in use at that time and you end up having to escape them differently in the query anyway?

    My guess: The initial filtering is simply an extra line of defense. And the filters that are applied stick with those variables, and mysqli_real_escape_string is an ADDITIONAL measure of safety on TOP of that filtering. Would this be right?

    Secondly, I have never used sprintf before so that query format was/is rather foreign to me, but I think (due to your explanations) that I understand it.

    If I do understand it, this:

    PHP Code:
    (%d"%s"
    Just means that the variables that are going to be in those positions, are going to be of the designated %type. And then AFTER that, you state what those variables are and escape them, in the same order as the %values are listed.

    I am very used to just setting my variables with escapes and then inserting them directly, like in my final example before your epic post. I am guessing from what you have told me, that is when the variables 'persist' and is not a good thing.

    I thank you deeply, your explanations are worded absolutely brilliantly, clearly, and with great detail. Even if I don't entirely get all of it, your wording suits the way that my brain works.

    You should write a book or do a video series - I would purchase in an instant.

  11. #36
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    And for finality sake, this is what I ended up with. Added a few variables to make sure I am doing it right, and am unsure about the $item_date filter/escape. But everything else should be right now...

    PHP Code:
    <?php
    session_start
    ();
    $username filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
    if (empty(
    $username)) {
        
    header('Location: /website/login.php');
    }
     
    $item_submit = isset($_POST['item_submit']);
    $user_id filter_var($_SESSION['user_id'], FILTER_SANITIZE_NUMBER_INT);
    $item_name filter_input(INPUT_POST'item_name'FILTER_SANITISE_STRING);
    $item_category filter_input(INPUT_POST'item_category'FILTER_SANITISE_STRING);
    $item_subcategory filter_input(INPUT_POST'item_subcategory'FILTER_SANITISE_STRING);
    $item_date filter_input(INPUT_POST'item_date'FILTER_SANITIZE_NUMBER_INT); // is this right for DATE?
     
    require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/database_connection.php';
    require_once 
    $_SERVER['DOCUMENT_ROOT'] . '/includes/functions.php';
     
    if (
    $item_submit) {
        
    $sql sprintf('INSERT INTO items (user_id, item_name, item_category, item_subcategory, item_date) VALUES (%d, "%s", "%s", %d)',
            
    intval($user_id),
            
    mysql_real_escape_string($item_name),
            
    mysql_real_escape_string($item_category),
            
    mysql_real_escape_string($item_subcategory),
            
    intval($item_date// likewise, is this right for date?
        
    );
        
    $result mysqli_query($link$sql);
            if (!
    $result) {
                
    $error 'DATABASE ERROR.';
                include 
    'error.php';
                exit();
            } else {
                
    header('Location: /website/items.php?user=' urlencode($username));
            }
        }
    }
    ?>

  12. #37
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by oknow View Post
    Firstly, I am wondering (and you have probably said so but it flew over my head) what the point is of the first set of filtering, when those variables aren't in use at that time and you end up having to escape them differently in the query anyway?

    My guess: The initial filtering is simply an extra line of defense. And the filters that are applied stick with those variables, and mysqli_real_escape_string is an ADDITIONAL measure of safety on TOP of that filtering. Would this be right?
    Let's start from mysqli_real_escape_string. The purpose of that function is to protect the database from many things that can cause it trouble. The mysqli_real_escape_string documentation page says that it includes strange characters such as null or Ctrl-Z, but also normal string-based characters such as newlines, quotes, double quotes, and slashes.

    Those string-based characters are not filtered or escaped in any way, so it's important to use mysqli_real_escape_string to protect the database.

    Let us now go back to the top and look at the initial filtering. That is the place where all of the inputs come in, so that they can then be used appropriately by the rest of the code. Some of them are used by the database, and others aren't, so we have three different possible scenarios that can take place here.

    1. Process all of the inputs
    2. Process some of the inputs
    3. Process none of the inputs

    Clearly the last option is not suitable, because then the non-database parts of your code will still be at risk.

    So let's look at processing some of the inputs. If we do that, with some of the inputs being processed and others being left alone, we end up with something like this:

    Code php:
    $username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
    ...
    $item_submit = isset($_POST['item_submit']);
    $userid = $_SESSION['userid'];
    $item_name = $_POST['item_name'];

    The first thing that stands out to someone newly looking at this code should be the question: why is username being filtered but the others aren't? Perhaps with enough investigation they can find out that the other variables aren't being used where they would cause a problem, but that can take quite some time.

    What happens now if someone decides that they want to display the item that was added, and they update the HTML part of the code? If you're lucky they will use htmlspecialchars and you might see tags on the screen if they're included in the string. If you're not lucky and they forget to use htmlspecialchars, Boom! You are now open to XSS scripting attacks. One of the beneficial things that FILTER_SANITIZE_STRING performs is to strip tags from the string.

    So, in order to protect from such an easily made mistake (I've done it several times myself) that's why we go with option 1. Sanity check all of your inputs, as a mandatory protection measure for all of your code.

    Quote Originally Posted by oknow View Post
    Secondly, I have never used sprintf before so that query format was/is rather foreign to me, but I think (due to your explanations) that I understand it.

    If I do understand it, this:

    PHP Code:
    (%d"%s"
    Just means that the variables that are going to be in those positions, are going to be of the designated %type. And then AFTER that, you state what those variables are and escape them, in the same order as the %values are listed.
    It's even more flexible than that. If you give it a number and a string in the wrong order, it will place them in the right order for you. You can see it being used quite liberally on the mysql_query page, with full details about it at the sprintf page.

    One of the nice features is that you can duplicate values in the string too. For example:

    Code php:
    $format = 'The %2$s contains %1$d monkeys.
               That\'s a nice %2$s full of %1$d monkeys.';
    printf($format, $num, $location);

    Quote Originally Posted by oknow View Post
    I am very used to just setting my variables with escapes and then inserting them directly, like in my final example before your epic post. I am guessing from what you have told me, that is when the variables 'persist' and is not a good thing.
    It's not a good thing when coding in a procedural manner. When the $item_name variable is escaped in the database part, what happens if you later on want to show it in the HTML section? If you don't escape it for the HTML page you will have problems. If you try and HTML escape the database escaped value, you will have problems. If you retrieve it freshly again you will either duplicate the sanitizing, or forget to do so, and have more problems. All because the variable was escaped for the database, which then remains escaped long after the database part is complete.

    So, either escape it inside of a function so that it only remains escaped inside of it, or use a different variable to escape it (messy), or escape it inline with the string.

    Instead of using sprintf it's possible to just escape it inline with the text, but then you have readability problems. It can then be difficult to find and fix any SQL syntax problems, if you have variables messing up your ability to interpret things.

    About the cleanest way that I have found to escape variables inline, is something like this:

    Code php:
    $sql = 'INSERT INTO items (user_id, item_name)' . 
        ' VALUES (' . intval($userid) . ', ' .
        '"' . mysql_real_escape_string($item_name) . '")';

    That makes it clearer what is happening and why, but it still makes the SQL code harder to read. Did you notice the syntax error in above SQL statement above?

    If it's in a separate function , you can then escape the variables and use inline variable expansion:

    Code php:
    function addItem($link, $userid, $item_name)
    {
        $userid = intval($userid);
        $item_name = mysqli_real_escape_string($item_name);
        $sql = 'INSERT INTO items (user_id, item_name) ' .
            "VALUES ($userid, '$item_name')";
        ...
    }

    but that then disguises and hides the variables, plus there a small performance cost as PHP searches for and expands them.

    So that's why I prefer to use sprintf. Not only because it's in the documentation for when working with SQL strings, but because it makes the SQL statements very clear, you can easily escape the variables inline, and you can easily tell that all of the appropriate variables have been properly escaped.

    Code php:
    $sql = sprintf('INSERT INTO items (user_id, item_name) VALUES (%d, "%s")',
        intval($userid),
        mysql_real_escape_string($item_name)
    );

    Quote Originally Posted by oknow View Post
    I thank you deeply, your explanations are worded absolutely brilliantly, clearly, and with great detail. Even if I don't entirely get all of it, your wording suits the way that my brain works.

    You should write a book or do a video series - I would purchase in an instant.
    Wow thank you. I don't quite know what to say, but that's a very nice recommendation indeed. Thanks.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  13. #38
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by oknow View Post
    and am unsure about the $item_date filter/escape.
    Okay, well just to clarify, the filter helps to remove tags and other nasties that might come in (XSS attacks), and the escape helps to protect the database from SQL injection attacks.

    Without the filter, your database can then contain strings with those XSS attacks, which are then time-bombs waiting to be read and inappropriately displayed.
    Without the database escape, SQL injection attacks can easily get through. Read SQL Injection Attacks by Example for good examples of what you're protecting against.

    Protecting both the input and the output leads to the best safety results with a minimum of thinking required. Whenever something is coming in to your code you defend, and when it's leaving your code you protect. That way, it's much easier for you to recognise when it needs to be done, and it's harder for mistakes to occur.

    Does that make sense?
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  14. #39
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    That all makes sense, thank you. I've taken to liking sprintf, especially since I will be converting my code to this far prettier/more readable layout, sprintf just suits that flow better I think.

    re the $date issue

    if my form had three POST options

    $_POST['year'], $_POST['month'], $_POST['day']

    is it possible to combine them in the initial filtering?

    PHP Code:
    $date filter_input(INPUT_POST'year . - . month . - . day'FILTER_SANITIZE_NUMBER_INT); 
    or something similar? can't seem to find anything about multiples in filter_input.

    I eventually want $date to contain the following exactly when it is inserted in the database:

    PHP Code:
    $_POST['year'] . '-' $_POST['month'] . '-' $_POST['day'
    I think I'm good to go, at least for now. I have a lot of code to clean up!

  15. #40
    Non-Member bronze trophy
    Join Date
    Nov 2009
    Location
    Keene, NH
    Posts
    3,760
    Mentioned
    23 Post(s)
    Tagged
    0 Thread(s)
    It often seems to me that a lot of people starting out using php seem to love to open and close wrappers for no good reason -- be it <?php and ?>, or doing double quotes with single echo's per line... though personally I'm in favor of getting rid of <?php and ?> altogether and making it behave like a REAL programming language... this popping in and out of code nonsense might be the wet dream of ASP programmers, but it's hardly something I'd consider good coding practice...

    Even simple things like oknow's example:
    Code:
     echo 'Welcome'; 
         echo htmlspecialchars($username); 
         echo '.';
    leaves me going "what the?" -- though I've seen far, far worse:
    Code:
    <?php echo 'Welcome'; ?>
    <?php echo htmlspecialchars($username); ?>
    <?php echo '.'; ?>
    Which is treading into Carlos Mencia grey territory.

    I mean is:
    Code:
     echo 'Welcome',htmlspecialchars($username),'.';
    Really so hard to work with?!?

    Though it gets worse with the folks using double quotes for EVERYTHING, and then not even bothering to use ANY of the reasons to use double quotes -- inlined vars or escape codes. That one just leaves me shaking my head thinking "what the devil?"

    Though I suspect I've been at this stuff for too long... after three and a half decades of coding in everything from ADA to ZPL, a lot of what I consider common sense and good practice most coders today have no chance of ever even learning.

    Probably why so many poo-poo over simple easy to practice things like consistent code formatting or verbose element names.

  16. #41
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by oknow View Post
    if my form had three POST options

    $_POST['year'], $_POST['month'], $_POST['day']

    is it possible to combine them in the initial filtering?
    There is filter_input_array, which helps to consolidate some parts of it.

    For example:

    Code php:
    $args = array(
        'year' => array(
            'filter' => FILTER_VALIDATE_INT,
            'options'   => array('min_range' => 1900, 'max_range' => 2100)
        ),
        'month' => array(
            'filter' => FILTER_VALIDATE_INT,
            'options'   => array('min_range' => 1, 'max_range' => 12)
        ),
        'day' => array(
            'filter' => FILTER_VALIDATE_INT,
            'options'   => array('min_range' => 1, 'max_range' => 31)
        )
    );
    $myInputs = filter_input_array(INPUT_POST, $args);

    You will then end up with the following array:

    Code:
    array
      'year' => int 2011
      'month' => int 12
      'day' => int 3
    And yes, you can also incorporate other POST data in that array too. For example:

    Code php:
    $args = array(
        ...,
        'item_name' => FILTER_SANITIZE_STRING,
        ...,
    );

    You can then safely assign those from there to other variables:

    Code php:
    $year = $myInputs['year'];
    $month = $myInputs['month'];
    $day = $myInputs['day'];

    If some parts of it don't match the validation, you end up with:

    Code:
    array
      'year' => int 2011
      'month' => boolean false
      'day' => int 3
    So, after assigning the values, checking if any of those variables are invalid can then be usefully performed. You can then throw a suitable exception:

    In some cases it can be very useful to throw an exception instead of hiding or disguising the error.

    Code php:
    if (empty($year) || empty($month) || empty($day)) {
        throw new OutOfRangeException('Invalid date argument.');
    }
    // carry on with date stuff

    Code execution will then stop at that exception, so there's no need to wrap the rest of the code in an else condition.

    Quote Originally Posted by oknow View Post
    I eventually want $date to contain the following exactly when it is inserted in the database:

    PHP Code:
    $_POST['year'] . '-' $_POST['month'] . '-' $_POST['day'
    You do not want to use just $year + '-' + $month + '-' + $day as the date, because you then end up with formatting problems such as:

    2011-10-5
    2011-11-25
    2011-5-11
    2011-6-5

    If anything makes the mistake of treating those as strings, sorting them then becomes a nightmare.

    So instead, ensure that your dates are all formatted in a manner that guarantees that they are all consistant:

    2011-05-11
    2011-06-05
    2011-10-05
    2011-11-25

    So, once you have the appropriate year month and day values, you want to put them together as a date with leading zeros in their proper place, so this looks like a job for the date function. You would use Y for the full year, m for a numeric month with leading zeros, and d for the day with leading zeros.

    That is, 'Y-m-d'

    But we need a timestamp to create that date from. Fortunately strtotime is very good at that job, and can accept many different types of dates, even things like "+1 week 2 days" or "next Thursday"

    So we have the following:

    Code php:
    $timestamp = strtotime($year + '-' + $month + '-' + $day);
    $sqlDate = date('Y-m-d', $timestamp);

    Now $sqlDate contains a nicely formatted version of '2011-06-05' which you can make use of from there.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  17. #42
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Sorry for my late response, but I ran into a small problem and I have been trying to fix it myself so that I wouldn't have to ask, but my brain is done with this one now If this requires a lengthy explanation, please don't worry about it.

    I wanted to change what we had from:

    PHP Code:
    session_start();
    $username filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
    if (empty(
    $username)) {
        
    header('Location: /website/login.php');
    }
    /*
    code continues
    */ 
    to do the following:


    PHP Code:
    session_start();
    if (!isset(
    $_SESSION['username'])) {
         require_once 
    $_SERVER['DOCUMENT_ROOT'] . '/includes/cookie.php';
    }
    else {
         
    $username filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
    }
    /*
    code continues
    */ 
    It works just fine, but I know that it is probably not right. If it is, woohoo. But I'm not that lucky. heheh.

    cookie.php simply checks if $_COOKIE['username'] is set, and if it is set, the $_SESSION['username'] is created (after database checks to confirm the cookie is legit etc) and the script continues. If it is not set, the script just ends, having done nothing, and the rest of the page continues as normal.

    This page can be accessed by people who are not registered, that is why if the person has no cookies/session I want the page to just continue. So if they aren't logged in, I don't need $username filtered at all (it won't be used).

    Sorry to ask another question, I feel bad about it, but I figured that it is directly related to the code we ended up with. Everything else I have gotten to work fine and I am learning well how to use sprintf, filters (except for this part! ) and correct escaping in my queries etc.

    And thank you for the date explanation and examples, that actually pretty much made sense to me and I have been working on implementing it

  18. #43
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by oknow View Post
    PHP Code:
    session_start();
    if (!isset(
    $_SESSION['username'])) {
         require_once 
    $_SERVER['DOCUMENT_ROOT'] . '/includes/cookie.php';
    }
    else {
         
    $username filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
    }
    /*
    code continues
    */ 
    It works just fine, but I know that it is probably not right.
    The only thing to fault there is that $username ends up being undefined when the else part isn't run, which can lead to warnings from your code later on.

    Is there a useful reason why the else part should not always be done?

    Code php:
    session_start();
    if (!isset($_SESSION['username'])) {
         require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/cookie.php';
    }
    $username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);

    filter_var is well behaved when working with non-existent variables. In the example above, $username would end up being an empty string is the session username doesn't exist.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  19. #44
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Oh of course, I was forgetting about filter_var returning an empty string, duh.

    I was just thinking that I never set $username at all unless the user viewing the page was is logged in - (isset($_SESSION['username'])).

    But yeah, smarter to filter it anyway just in case for sure!

    Thanks again Paul. You're the best.

  20. #45
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Yeah, doing it that way is definitely smarter, especially after testing just then.

    When I was doing it like this:

    PHP Code:
    session_start();
    if (!isset(
    $_SESSION['username'])) {
        require_once 
    $_SERVER['DOCUMENT_ROOT'] . '/includes/cookie.php';
    }
    else {
        
    $username filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);

    If the session had to be set by the included cookie.php, the else wasn't recognizing that a session was created, thus $username was not being defined.

    Your change fixes that.

    Woo!

  21. #46
    Hibernator YuriKolovsky's Avatar
    Join Date
    Nov 2007
    Location
    Malaga, Spain
    Posts
    1,072
    Mentioned
    4 Post(s)
    Tagged
    0 Thread(s)
    Sorry if this has been said before, I didn't bother reading all the posts.

    @OP
    regarding the first part of the question.
    Opening and closing the <?php tags is going to affect performance, but not significantly, but it is still considered best to open and close them only if necessary, instead use comments or line-breaks to separate content.


    regarding your second point
    Code:
    $username = htmlspecialchars($_SESSION['username']);
    yes, the $username variable will be affected in the includes that follow.
    but
    Code:
    echo '$username';
    is not going to work, because your using single quotes.
    use this
    Code:
    echo "$username";
    instead.


    But you must keep in mind that this type of variable sanitization is only going to help for content within html tags like this:
    Code:
    <span><?php echo $username;?></span>
    if you were to use that variable in html attributes for example
    Code:
    <a href="<?php echo $username;?>"><?php echo $username;?></a>
    you would be immediately vulnerable to XSS attacks, because someone can just put
    Code:
    fred" onclick="alert(document.cookie)
    as their username, you can enable ENT_QUOTES
    Code:
    $username = htmlspecialchars($username, ENT_QUOTES);
    and it will be somewhat helpfull, even though you have to be religious about your quotes around html attributes.

    What I'm trying to say is, every type of content needs it's own validation and safeguards, you can't use one function fits all approach.

    I suggest you read this XSS prevention sheet (yes, all of it...)

  22. #47
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Thanks Yuri. I have since eliminated any opening and closing of php tags and started commenting when necessary - it just looks nicer anyway, I've found.

    Code:
    echo '$username';
    is not going to work, because your using single quotes.
    use this
    Code:
    echo "$username";
    instead.
    I've started only escaping variables when necessary, thanks to the help I received in this post, so my new method for $username (which has only been used in two places so far - welcome and navigation) has been this:

    Code php:
    <li><?php echo '<a href="items.php?user=' . htmlspecialchars($username, ENT_QUOTES) . '">' ?><strong>Items</strong></a></li>

    I'm guessing that's ok since the entire thing is in "".

    I might have to change this though:

    Code php:
    echo 'Welcome, ' . htmlspecialchars($username, ENT_QUOTES) . '!';

    I suggest you read this XSS prevention sheet (yes, all of it...)
    Reading right now. All of it.

  23. #48
    Hibernator YuriKolovsky's Avatar
    Join Date
    Nov 2007
    Location
    Malaga, Spain
    Posts
    1,072
    Mentioned
    4 Post(s)
    Tagged
    0 Thread(s)
    Yeah, reading most of the advice given, and your lucky Paul replied with such excellent examples

    regarding XSS, I think your fine because you have it as a query, if it was the full href, like this for example
    Code:
    <a href="<?php echo htmlspecialchars($url, ENT_QUOTES);?>">Some link</a>
    Then as far as I know, it is possible to XSS it, but in not sure how, all I know is that it's not going to prevent an alert box appearing in some browsers if someone uses "JavaScript:alert(document.cookie)" as the $url variable, but I might be just over-paranoid.

    There is an addon for Firefox called XSSme, I'm not sure how good it is, but I think it's worth a try.

  24. #49
    Non-Member bronze trophy
    Join Date
    Nov 2009
    Location
    Keene, NH
    Posts
    3,760
    Mentioned
    23 Post(s)
    Tagged
    0 Thread(s)
    Thing is, even this:

    Code:
    <li><?php echo '<a href="items.php?user=' . htmlspecialchars($username, ENT_QUOTES) . '">' ?><strong>Items</strong></a></li>
    Is still asking "WHY?!?" as you are probably wasting time entering and exiting PHP too much -- but as a rule of thumb I consider more than ONE instance of <?php and ?> in a file to be bad practice... <?php at the start of the file, ?> after, and anything that resembles output inside ECHO.
    Code:
    <?php
    echo '
    <li><a href="items.php?user=',htmlspecialchars($username,ENT_QUOTES),'">Items</a></li>';
    ?>
    Though I'd expand that to the entire document -- I think it's become I come from a real programming language background; the very notion of ANY output not being wrapped in a function and explicitly said "echo" or "writeln" or "print" just rubs me the wrong way.

    Which honestly is why I'd REALLY like to see <?php and ?> go the way of the dodo as there's no legitimate reason for them to even exist in the language.

  25. #50
    SitePoint Zealot
    Join Date
    Jan 2010
    Posts
    140
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Going to have to bump this.

    This works:

    Code php:
    session_start();
    if (!isset($_SESSION['username'])) {
    	require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/cookie.php';
    }
    $username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);

    But I didn't think to test it when I am not logged in and do not have a cookie set.

    If that is the case, I'm getting an 'Undefined Index: username' error.

    I can fix that by changing to this:
    Code php:
    session_start();
    if (!isset($_SESSION['username'])) {
    	require_once $_SERVER['DOCUMENT_ROOT'] . '/includes/cookie.php';
    }
    else if (isset($_SESSION['username'])) {
    $username = filter_var($_SESSION['username'], FILTER_SANITIZE_STRING);
    }

    But 1) Based on testing with the original code, I don't think that will work if the session is set by the cookie, and 2) I thought that filter_var shouldn't give me an error if the variable ends up being empty anyway.. I don't see why the original code should be giving an error if the user is not logged in.

    What am I doing wrong?


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •