Link Details

The idea is good, but the implementation of it is not really the best.

Thanks for the feedback Sven. I'm aware that its not the best implementation, but I hacked it together in about 5 minutes just to illustrate the basic idea. Got any specific tips on what you would do differently that I can incorporate into it?

Sure, the article text looks nice. What I mainly reacted on is the obvious error in the code and some of the functions/code style you use.

If we consider the error first, in the process.php file, you have this:

if (!array_key_exists('id',$_POST) || !is_numeric($_POST['id'])) {

Now, that actually says to enter the if sentence only if the post key id does not exist, or if it is not a number. So instead it should be:

if (array_key_exists('id',$_POST) && is_numeric($_POST['id'])) {

Or preferable it should just be:

if (!empty($_POST['id'])) {

The reason we can use empty is due to the id should be a unix timestamp, i.e. it can not be '0' (zero (post/get variables are always strings)) and it is casted to int when used in another verification check so even if it contained tainted data it would be set to int 0 (zero).

That takes care of the error, lets move on to the functions.

-Using mktime() without filling in the parameters will give you a strict error, if you want to get the current unix timestamp you should use time() instead.

-You also use array_key_exists() a lot, the only time you should use this function is if the array key can contain the value "null". This means that in most cases you will use isset() or empty() instead (which one depending on the functionality you need). The reason you want to use those is due to array_key_exists() is more expensive to use (speed/resource wise).

-To check if there is a post request you used eregi('post',$_SERVER['REQUEST_METHOD']), first off never use POSIX functions (ereg) they are dead slow, use PCRE (preg) instead. second in this case you should not use regex at all, you can simply use !empty($_POST) instead. Note, if you need to validate if a name etc is in a string and you dont need regex functionality never use a regex, instead use strpos().

For the code style, that is a personal preference so what I use and like to see might not be what other people like. So I will not comment that :)

On a side note, I use a similar but different system for forms myself. The difference is that I use the key in the input fields instead of in a hidden id. Example: instead of $_POST['name'] the script randomly add a md5 hash to the name, so depending on the initiated settings it can be $_POST['[md5]name'], $_POST['name[md5]'], $_POST['n[md5]ame'] etc ([md5] is the actual md5 string, I did not file like typing 32 characters in each variable.)

Sure this approach can just as easily be broken down, I could write a script to abuse it in a few minutes. It would take more code to break this system than the one you describe, but as you mention the fact is that in most cases the persons will just move on looking for a unprotected form instead, after all why spend time on it when there is so many "ripe" forms out the for picking hehe ;)

This process together with of course having a secure script that does not allow injections to the mail header has removed 99.99% of spam attempts using any forms this system is on. What comes through now is manually entered, and those usally stay with attemt and move on when they notice they can not use it to mass email.

With other words, the key is to ensure that the script is not vulnerable for header injections in addition to an id on the form like your article describes.