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.

--
Matthias
/"\
\ / ASCII RIBBON CAMPAIGN - AGAINST HTML MAIL
X - AGAINST M$ ATTACHMENTS
/ \