Company Test Question for PHP Programmers

[mayafishing]

Hi everyone, I got this question when applying for a PHP programmer position.

 

Question:

 

Check out the following code and make suggestions on how to improve it based on concerns from Security, Compatibility and Efficiency:

 

<?

echo("<p>The characters you have input are: " .$_GET['q'] . ".</p>");

?>

 

 

Anyone up to the challenge?

 

 

[rameshfaj]

<?

echo("<p>The characters you have input are: " .$_GET['q'] . ".</p>");

?>

 

 

 

Theres nothing to challenge but I think this will work:

 

echo" \n The characters you have input are: .$_GET['q']";

[per1os]

lol you are expecting to get a programming job and you do not know how to fix the statement? Kind of funny.

 

<?php
$q = isset($_GET['q'])?stripslashes($_GET['q']):'';
echo '<p>The characters you have input are: ' . $q . '.</p>';
?> 

 

Explanation:

 

Check for the isset of the get variable with the ternary operator (? and : ) to improve efficiency in that it will not throw a notice error of an undefined index. Single quotes are also proven to increase efficiency during bench tests. You remove the ( ) from the statement because it is just 2 extra characters that are un-needed. As far as security is concerned there is no need because we are not evaluating the statement or using the statement in a MySQL Database query. We stripslashes on the data because in most builds of php magic_quotes_gpc is turned on, which automatically adds slashes to get/post data submitted.  Finally we use <?php because in later version <? has been depreciated.

 

My statement still stands as above, you will be way over your head if you get hired for this job and you cannot answer that simple question =)

 

Edit:

Thinking about the security aspect, I guess this would make it not vunerable to XSS:

 

<?php
$q = isset($_GET['q'])?strip_tags(stripslashes($_GET['q'])):'';
echo '<p>The characters you have input are: ' . $q . '.</p>';
?> 

 

Using the strip_tags would remove any possible XSS exploit tag.

[mayafishing]

Thanks guys, I learned something new.

 

Here is my original answers to the company:

 

 

<p>

<?php

echo "The characters you have input are:".validate_input($_POST['q']);

?>

</p>

 

Note: validate_input() is used for removing the tags which might be used for attacking database.

(Or maybe validate_input() should be called clean_up() or htmlentity(stripslashes(trim())). )

 

Explained:

 

1. no need () after echo.

2. <p></p> should be saperated from the php script itself.(might improve efficiency).

3. use POST instead of GET to inprove security, because the data appended to GET might be stealed on the way to target page. (if the data is sensitive)

4. ANY input to database should be checked and cleaned to make sure it does not contain any harmful tags.

5. use GET to transmit data has limitations on the length of the characters.

 

 

 

 

 

 

 

[per1os]

Explained:

 

1. no need () after echo.

2. <p></p> should be saperated from the php script itself.(might improve efficiency).

3. use POST instead of GET to inprove security, because the data appended to GET might be stealed on the way to target page. (if the data is sensitive)

4. ANY input to database should be checked and cleaned to make sure it does not contain any harmful tags.

5. use GET to transmit data has limitations on the length of the characters.

 

1, you are correct.

2. Highly doubt that removing the <p> would improve efficiency at all, maybe this would be better though:

 

<?php
$output = "";
$q = isset($_GET['q'])?strip_tags(stripslashes($_GET['q'])):'';
$output .= '<p>The characters you have input are: ' . $q . '.</p>';

echo $output;
?> 

 

Reasoning is that storing the output into a variable will allow you to decide where the data should be echoed and will not break the script from a needed header redirect or setcookie function. It is always better to echo out data using the above method to have full control of your output.

 

3, Using POST over GET is better, but does not improve security unless you are passing password or sensitive information.

4, You are correct if it is being entered into a DB you should use the type of database escape function to clean it up.

5, GET does limit the transportation of characters, as you should not be passing massive amounts of data via GET. POST is much better to do that type of thing. GET should really only be used in circumstances you want people to be able to link back to a webpage such as a search page etc.

[mayafishing]

and will not break the script from a needed header redirect or setcookie function.

 

Do you have examples to illustrate this point?

 

Thanks a lot

[per1os]

and will not break the script from a needed header redirect or setcookie function.

 

Do you have examples to illustrate this point?

 

Thanks a lot

 

<p>
<?php
if (isset($_POST['login'])) {
if ($_POST['username'] == "user" && $_POST['password'] == "password") {
	setcookie('username', $_POST['username'], time()+3600, '/', 'domain.com');
                 echo 'Hi! ' . $_POST['username'];
}
}
?>
</p>

 

That will throw a header after output error. Now if the <p> was stored inside and $output same with the hi part we would not have that issue:

 

<?php
$output = "<p>";
if (isset($_POST['login'])) {
if ($_POST['username'] == "user" && $_POST['password'] == "password") {
	setcookie('username', $_POST['username'], time()+3600, '/', 'domain.com');
                 $output .= 'Hi! ' . $_POST['username'];
}
}else {
    $output .= "Please Login!";
}
$output .= "</p>";

echo $output;
?>

[mayafishing]

I think it can also be solved by using output buffering.

 

<?php
ob_start();
?>

<p>

<?php
if (isset($_POST['login'])) {
if ($_POST['username'] == "user" && $_POST['password'] == "password") {
	setcookie('username', $_POST['username'], time()+3600, '/', 'domain.com');
                 echo 'Hi! ' . $_POST['username'];
}
}
?>

</p>

<?php
ob_flush();
?>

But your code is clearly more clean.

 

[per1os]

I think it can also be solved by using output buffering.

 

<?php
ob_start();
?>

<p>

<?php
if (isset($_POST['login'])) {
if ($_POST['username'] == "user" && $_POST['password'] == "password") {
	setcookie('username', $_POST['username'], time()+3600, '/', 'domain.com');
                 echo 'Hi! ' . $_POST['username'];
}
}
?>

</p>

<?php
ob_flush();
?>

But your code is clearly more clean.

 

 

We are going for efficiency right? Output buffering is not very efficient and in this case is the wrong code to use. Output buffering is only for certain circumstances, this is not one of them.

 

If you want proof simply do a bench test of 5,000 times:

 

<?php
function microtime_float()
{
    list($usec, $sec) = explode(" ", microtime());
    return ((float)$usec + (float)$sec);
}

$start = microtime_float();

for ($i=0;$i<5000;$i++) {
ob_start();
?>

<p>

<?php
echo 'Hi!';
?>

</p>

<?php
ob_flush();
}
$end = microtime_float();
$time = $end - $start;

echo "It took $time seconds using output buffering.\n";



$start = microtime_float();
$output = ""; //initialize variable
for ($i=0;$i<5000;$i++) {
    $output .= '<p>Hi!</p>';
}

echo $output;

$end = microtime_float();
$time = $end - $start;

echo "It took $time seconds <b>NOT</b> using output buffering.\n";

?>

 

Let me know how much slower output buffering is than the code I posted.

 

 

It took 0.097039937973022 seconds using output buffering.

 

It took 0.0049819946289063 seconds NOT using output buffering.

 

As you can see with just 5000 times there is already a significant difference, try 50,000 and it should seconds difference. =) Efficiency.

[Psycho]

I'll throw in my 2 cents.

 

There are many assumptions being made with regard to this "problem". That is fine as long as those assumptions are clearly stated in the answer.

 

For my answer I would supply this:

Since the question states "...how would you improve it..." I will assume that it is working code and there are no such issues as header redirects that would require putting the text into a variable (although it is a valid argument). I will also assume that the variable needs to be a GET since I have no information to make a qualified evaluation otherwise (perhaps it is passed via a link).

<?php
if (isset($_GET['q']) && $_GET['q']!=='') {
  echo '<p>The characters you have input are: ' . htmlentities(stripslashes($q)) . '.</p>';
} else {
  echo '<p>You did not input any characters.</p>';
}
?> 

 

Security:

Security is improved by the use of stripslashes and htmlentities. Without these a user could inject code into a page and cause any number of undesired concequences.

 

Compatibility:

The page no longer "expects" that a value will be received and will display an appropriate response if none is received.

 

Efficiency:

None really except the use of single quotes. But, I consider that to be almost insignificant. The code is actually less efficient than it originally was, but Security & Compatibility must be met first.

[roopurt18]

I can't believe this thread made it as far as it did before someone pointed out watching for embedded HTML or Javascript in the $_GET variable.

 

Good job mj 

[per1os]

I can't believe this thread made it as far as it did before someone pointed out watching for embedded HTML or Javascript in the $_GET variable.

 

Good job mj 

 

What are you talking about?

 

Edit:

Thinking about the security aspect, I guess this would make it not vunerable to XSS:

 

<?php
$q = isset($_GET['q'])?strip_tags(stripslashes($_GET['q'])):'';
echo '<p>The characters you have input are: ' . $q . '.</p>';
?> 

 

Using the strip_tags would remove any possible XSS exploit tag.

 

That was taken care of with that statement right there bud. That would of eliminated the XSS exploits.

[KevinM1]

Question about security -- I currently use the following escape function for things to be inserted into a database (which I think I got from Frost in my thread about how escaping characters works):

<?php

function myEscape($string){
   return (get_magic_quotes_gpc()) ? mysql_real_escape_string(stripslashes($string)) : mysql_real_escape_string($string);
}

?>

 

Would something like the following be more secure?

<?php

function myEscape($string){
   return (get_magic_quotes_gpc()) : mysql_real_escape_string(strip_tags(stripslashes($string))) : mysql_real_escape_string(strip_tags($string));
}

?>

[roopurt18]

@frost110

 

Stripslashes removes backslashes and is used before inserting data into a database.

 

The code originally pasted isn't inserting anything into a database; it is echo'ing it to the screen.  Therefore, stripslashes will do nothing against the following:

 

http://www.domain.com?p=<script>document.href.location=www.spoofeddomain.com</script>

 

htmlentities will protect against that.

[per1os]

@frost110

 

Stripslashes removes backslashes and is used before inserting data into a database.

 

The code originally pasted isn't inserting anything into a database; it is echo'ing it to the screen.  Therefore, stripslashes will do nothing against the following:

 

http://www.domain.com?p=<script>document.href.location=www.spoofeddomain.com</script>

 

htmlentities will protect against that.

 

Look closer my friend, strip_tags would do something against that.

[roopurt18]

Well, it was your edit that got me.  I was going off the code block at the top of your original post.  I still prefer htmlentities as it will show you the full content of the originally entered characters.  Also, I'm not sure strip_tags will protected against:

 

  ?p=<sc<script>ript>document.href.location=www.spoofeddomain.com</sc</script>ript>

[per1os]

Well, it was your edit that got me.  I was going off the code block at the top of your original post.  I still prefer htmlentities as it will show you the full content of the originally entered characters.  Also, I'm not sure strip_tags will protected against:

 

  ?p=<sc<script>ript>document.href.location=www.spoofeddomain.com</sc</script>ript>

 

Yea htmlentities would be nicer especially if there is html to be displayed. But strip_tags does work either way and an FYI for ya it does work on that query string.

 

At any rate, it bothers me when people think I do not know what I am doing and second guess my tactics =) Either way both methods do work.

[roopurt18]

At any rate, it bothers me when people think I do not know what I am doing and second guess my tactics =)

 

It bothers us all I think!  I've seen enough of your posts to know you know a thing or two about what you're talking about. 

 

Anyways, to answer Nightslyr's question, adding strip_tags or htmlentities to your database cleansing function is not really necessary as it doesn't protect the database.  In most cases, stuff going into the database is going to be re-displayed to other users later, so it is usually appropriate to do something about embedded HTML / Javascript.  But this is not a concrete thing that you do 100% of the time like you do with mysql_real_escape_string().  When you remove HTML from a user's input depends on who that user is and how that input will be used later.

 

A CMS, for example, may not strip html tags from posts coming from the site's administrator because you can assume the site's administrator is not trying to harm their users.

 

However, you would want to remove at least Javascript and iframes from comments posted by anonymous users.

 

A lot of sites combat this in one of two methods.  Either they use BBC markup and remove all tags from the input and replace the BBC markup with actual HTML markup when redisplaying the input.  The other alternative is to allow certain HTML tags and strip all others via the second parameter to strip_tags.

[KevinM1]

At any rate, it bothers me when people think I do not know what I am doing and second guess my tactics =)

 

It bothers us all I think!  I've seen enough of your posts to know you know a thing or two about what you're talking about. 

 

Anyways, to answer Nightslyr's question, adding strip_tags or htmlentities to your database cleansing function is not really necessary as it doesn't protect the database.  In most cases, stuff going into the database is going to be re-displayed to other users later, so it is usually appropriate to do something about embedded HTML / Javascript.  But this is not a concrete thing that you do 100% of the time like you do with mysql_real_escape_string().  When you remove HTML from a user's input depends on who that user is and how that input will be used later.

 

A CMS, for example, may not strip html tags from posts coming from the site's administrator because you can assume the site's administrator is not trying to harm their users.

 

However, you would want to remove at least Javascript and iframes from comments posted by anonymous users.

 

A lot of sites combat this in one of two methods.  Either they use BBC markup and remove all tags from the input and replace the BBC markup with actual HTML markup when redisplaying the input.  The other alternative is to allow certain HTML tags and strip all others via the second parameter to strip_tags.

 

Ah, okay.  Regarding the second option, something like this?

<?php

mysql_real_escape_string(strip_tags(stripslashes($string), '<p><br>'));

?>

[roopurt18]

Assuming that is how the second argument for strip_tags is formatted, yes.  You had it in your original code I think, but remember you only need to stripslashes if the gpc magic quotes are turned on.