On Mon, 05 May 2008 10:25:06 +0200, Matthias Watermann <lists@mwat.de>
wrote:

>On Mon, 05 May 2008 07:16:35 +0100, Geoff Cox wrote:
>
>> [...]
>> where the user is typing a number into a box
>>
>> var number_check = number_given;
>> if ( (number_check >7) || (number_check < 1) ) { alert("The number must
>> be in the range 1 to 7!"); } else
>> if (isNaN(number_check)) {
>> alert("Please enter a valid number"); } else {
>> sendGroup1Lab1(number_check);
>> }
>
>You must _never_ rely on client-side validation! As far as the
>server-side script is concerned you should at least assume the user
>switched JavaScript off (not to mention intended malformed values).
>
>> I have changed (***) the php to
>>
>> $result1 = $_GET['answer1'];
>> $result2 = $_GET['answer2'];
>> $result3 = $_GET['answer3'];
>> $result4 = $_GET['answer4'];
>
>If those values are supposed to be numbers you should at least
>ensure that:
>
>$result1 = $_GET['answer1'] * 1;
>$result2 = $_GET['answer2'] * 1;
>$result3 = $_GET['answer3'] * 1;
>$result4 = $_GET['answer4'] * 1;
>
>In case not all digits are allowed (as your JavaScript fragment
>suggests) a RegEx based test seems appropriate:
>
>$hits = array();
>$_GET['answer1'] = (isset($_GET['answer1'])
> && preg_match('|([1-7]+)|', $_GET['answer1'], $hits))
> ? $hits[1] * 1
> : 0; // map invalid values to zero
>$_GET['answer2'] = (isset($_GET['answer2'])
> && preg_match('|([1-7]+)|', $_GET['answer2'], $hits))
> ? $hits[1] * 1
> : 0; // map invalid values to zero
>[...]
>
>(There's no need for those "$result" variables: Why keep the same value
>in memory multiple times?) Do _not_ assume all the expected CGI
>arguments are there actually but always check that.
>
>> [...]
>> $result4 = mysql_real_escape_string($_GET['favorite']); ***
>
>What's "favorite" supposed to be? A string? A number (real or integer)?
>How do you validate that?
>
>> mysql_query("INSERT INTO mytable (answer1,answer2,answer3,answer4)
>> VALUES ('$result1','$result2','$result3','$result4')");
>
>The single quote are needed for string values, while those "$resultX"
>variables are integers. Hence you could omit the single quotes.
>
>> I see that mysql_real_escape_string can only be used after connecting
>> to the database?
>
>Yes. However, id you'd validate the user provided values there's not
>much need for such a call anyway. And escaping possibly malicious
>values might avoid some SQL injection problem but it does _not_ avoid
>your tables being filled with useless data.
>
>> Is the above safer?
>
>Further improvement could be gained by not using the tables directly
>but calling an stored procedure which could implement additional
>validation. But that's another topic and probably oversized for
>your application.
>
>However, you should switch from HTTP/GET to HTTP/POST. While that's
>not a safety net as such at least it makes it a _little_ harder to
>fake the CGI arguments.

Many thanks Matthias - quite a bit to get my head round but will start
trying now!

Cheers

Geoff