Code layout and variable questions

There is filter_input_array, which helps to consolidate some parts of it.

For example:


$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:


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:


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

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


$year = $myInputs['year'];
$month = $myInputs['month'];
$day = $myInputs['day'];

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


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.


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.

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:


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

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?


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.

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 :sick: If this requires a lengthy explanation, please don’t worry about it.

I wanted to change what we had from:


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

to do the following:


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! :D) 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 :slight_smile:

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.

Yeah, doing it that way is definitely smarter, especially after testing just then.

When I was doing it like this:


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!

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


$username = htmlspecialchars($_SESSION['username']);

yes, the $username variable will be affected in the includes that follow.
but

echo '$username';

is not going to work, because your using single quotes.
use this

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:

<span><?php echo $username;?></span>

if you were to use that variable in html attributes for example

<a href="<?php echo $username;?>"><?php echo $username;?></a>

you would be immediately vulnerable to XSS attacks, because someone can just put

fred" onclick="alert(document.cookie)

as their username, you can enable ENT_QUOTES

$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…)

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. :slight_smile:

echo '$username';

is not going to work, because your using single quotes.
use this

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:


<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:


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

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

Reading right now. All of it. :slight_smile:

Yeah, reading most of the advice given, and your lucky Paul replied with such excellent examples :slight_smile:

regarding XSS, I think your fine because you have it as a query, if it was the full href, like this for example

<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. :stuck_out_tongue:

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

Thing is, even this:


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


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

Going to have to bump this.

This works:


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:


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? :confused:

It seems that filter_input doesn’t support $_SESSION yet, so we’ll have to do this more-like the old-fashion way, where an empty variable is first assigned, and then only if the superglobal exists, do you assign it then.

Ahh the memories.

I’ve also moved the cookie part afterwards, which seems to flow better due to the cookies relying on the lack of a username.


session_start();

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

filter_var I’m guessing you meant? Same issue with both I’m sure, though. No wonder I was lost. Thanks for clearing it up for me.

Seems to work without $username = ‘’; too. But I think I understand the reason for including the line. :slight_smile:

I need to re-arrange it so that the cookie check is done first if necessary, so that if the sessions are set by cookie.php they are sanitized, which won’t happen if cookie.php sets the sessions last.

I’m pretty sure I can figure that out though. :slight_smile:

Thanks again Paul!

Nope, filter_input, which handles get, post, cookie, server, and env globals.

Ahh, so cookie.php updates the session variable, that makes sense. If you had the cookie code consistently set the session variable, even with just an empty value, you will not then receive an undefined insex with the later code that retrieves the session.

That being something like this:


$_SESSION['username'] = filter_input(INPUT_COOKIE, 'username', FILTER_SANITIZE_STRING);

I also think that once you have ensured the safety of what you are putting in to the session variable (as done above) that you won’t need to explicitly filter it, since it has already been implicitly done when it was set.

Then you could get away with the following:


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

That looks great. I’m unfortunately in the middle of changing my designated session/cookie variables, but I think I’m set on the way I’m doing it, so I’m about to try implement this improvement you suggested…

can I use a filter on a $row; ?

ie

	
$userid = filter_var($row['id'], FILTER_SANITIZE_NUMBER_INT);
$username = filter_var($row['username'], FILTER_SANITIZE_STRING);
$_SESSION['userid'] = $userid;
$_SESSION['username'] = $username;
$_SESSION['logged_in'] = TRUE;

I ask because the new cookie I am using, a single cookie, is a hashed combination of variables that I won’t be using in sessions anymore - it is only to check if the user is allowed to log in and have the sessions set. Hope that makes sense! heh

edit: wait, I could just do this

	
$_SESSION['userid'] = filter_var($row['id'], FILTER_SANITIZE_NUMBER_INT);
$_SESSION['username'] = filter_var($row['username'], FILTER_SANITIZE_STRING);
$_SESSION['logged_in'] = TRUE;

Right? (Assuming rows are ok to filter!)

Yes, filter_var can be used on any variables, but when dealing with arrays it can be better to use [url=“http://www.php.net/manual/en/function.filter-var-array.php”]filter_var_array instead…

If it’s from one of the superglobal arrays though, filter_input should be used for them instead.

Gotchya! Thanks.

I’ve been teaching myself filter_var_array on and off to sort out my date issue from earlier, took a break from it admittedly.

This is going to work. And much better than I had it set up.

I just have to figure out how to completely re-code my cookie.php file to fix these issues :eek:… I think I can do it though. I will have to come up with something other than the $row thing. $username = $_SESSION[‘username’]; won’t work to check on every page either, unless cookie.php always runs, because if the user is already logged in, cookie.php setting+filtering the sessions will be skipped… I think that is right. My brain is having a hard time with this tonight. Hah. I will rack my brain over this I am sure I can get it.

Thanks again Paul, you’re a legend.

There are three different things that are involved here.
$_POST, $_SESSION and $_COOKIE

That gives a total combination of 8 different possibilities when the page loads.

[list=8][]none of them are set
[
]only $_POST
[]only $_SESSION
[
]both $_SESSION and $_POST
[]only $_COOKIE
[
]both $_COOKIE and $_POST
[]both $_COOKIE and $_SESSION
[
]all three, $_COOKIE, $_SESSION and $_POST
[/list]

Create a test for each of those 8 cases, which provides bad data in each of the areas. You can use simple test or [url=“http://phpunit.sourceforge.net/”]phpunit to easily automate the tests.

When all of the tests result in good data instead of bad, that’s when you know you have a fully working solution.

i believe option 7 should be:
7. both $_COOKIE and $_SESSION

Ta, updated.

Writing tests that check for each of those situations, and then writing code that solves each test, results in the following code:


<?php
session_start();
$username = filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING);
if (empty($username) && isset($_SESSION['username'])) {
    $username = $_SESSION['username'];
}
if (empty($username)) {
    $username = filter_input(INPUT_COOKIE, 'username', FILTER_SANITIZE_STRING);
}

$_SESSION['username'] = $username;
setcookie('username', $username);

echo '<p>Username: ' . htmlentities($username) . '</p>';
?>