Jump to content

Recommended Posts

No it does not work. A value of "<body>", for example, would pass the test when it should fail. Be sure you understand the order of operations. Same concept as when you have a mathematical expression - if you do things out of order you will get different results. In this case, the expressions are run from the inside-out.

 

So, first you are executing empty($username). That function ONLY returns a Boolean TRUE or FALSE.

 

Then you are executing strip_tags() on the result of the above. Well, strip_tags() won't do anything on a Boolean value.

 

Plus, if you don't want to allow tags in the username, then you should strip them from the value that you end up saving as well.

$username = strip_tags(trim($_POST["username"]));
if(empty($username))
{
    echo "username is required";
}
else
{
    //Continue

However, there is no technical reason you can't support tags in the username (or most values). You should be properly escaping the values when you output to HTML anyway. For example, there are plenty of people on this forum that have HTML tags in their names.

Edited by Psycho

Hi,

 

please forget about this strip_tags() garbage..

 

Not sure where you dug this function out, but it never made any sense whatsoever. What it does is mangle the input and delete everything which somehow looks like an HTML tag. For example, if a user chooses the name “I <3 PHP”, you end up with “I ”. Why would you want that?

 

The strip_tags() function is one of those infamous brainfarts of the PHP core developers. They look at a problem (in this case cross-site scripting vulnerabilities), fail to understand it and consequently add some completely useless “feature” to the PHP core. And then generations of PHP newbies run around with this crap instead of using an appropriate solution.

 

To prevent cross-site scripting attacks, use htmlspecialchars() right when you insert the value into the HTML document. Do not mangle the user input. This is not only pointless, it also kills usability. Would you want to work with an application which randomly breaks your input? I wouldn't.

While my initial response could have been more detailed, your latest proposed code (after Psycho's wonderful response) is still lacking.  You can't run an empty call on the strip_tags result.  As my off the cuff answer tried to make clear - run the empty test first as a true response from that answers one half of your question with the least use of resources ie,  "is it empty?".  Once you determine that it is not empty, then do your other tests to validate it or sanitize it.

The strip_tags() function is one of those infamous brainfarts of the PHP core developers. They look at a problem (in this case cross-site scripting vulnerabilities), fail to understand it and consequently add some completely useless “feature” to the PHP core. And then generations of PHP newbies run around with this crap instead of using an appropriate solution.

 

I agree that strip_tags() should not be used to prevent cross-site scripting, and removing tags from user input is typically a stupid idea. In fact,it always irks me when I see people trying to prevent certain characters in fields such as name and end up disallowing characters that people use inthier names (dash, apostrophe, accented characters, etc.)

 

But, strip_tags() is a useful function. There are many scenarios where you might have content that is used for multiple purposes - browser output being only one. For example, you might have content for a page which includes hyperlinks and formatting code. If you wanted to re-purpose the content for output to a text file or PDF that doesn't support those tags you would want to remove them.

 

 

htmlspecialchars() may be fine for user names but not for user messages? because if the user post a link it would break the link? 

 

You don't want users creating their own links. Let them put in a URL in plain text then programatically make it a hyperlink when you display it. That's what this site does.

If you want to allow limited use of HTML, you need much more sophisticated tools and a lot of knowledge about different attacks. Unless you really, really want people to post clickable links and are willing to invest a lot of time in this particular feature, you should keep away from this. It's not worth the trouble. Just have people post their links as plaintext.

 

Dealing with HTML in a secure way is already a major task and requires a fully-featured library like HTML Purifier. In addition to this, links are particularly nasty, because they can be used for all kinds of attacks.

 

For example, links can execute JavaScript code:

<a href="javascript:alert('XSS')">Click me!</a>

And links can render complete pages which execute JavaScript:

<a href="data:text/html;charset=utf-8;base64,PHNjcmlwdD5hbGVydCgnWFNTJyk8L3NjcmlwdD4=">Click me!</a>

And of course links can point to malicious websites.

 

It's not worth the trouble.

You need to sloooooooow down. I'm sure there are plenty of things you want your application to have. But, you have to learn how to differentiate things you MUST have, things you really WANT to have and things that would be NICE to have. First, build the ability for the user to post comments using htmlentities() or htmlspecialchars() to prevent any malicious content from making it's way into the output. Then, once you have that working and have implemented any other high value features you can go back and figure out how to properly allow clickable hyperlinks.

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.