Chat with PHP, MySql, Ajax: How to improve it?

Question

I made this simple chat in php. You have suggestions on how I can improve it?

This file configures the connection to the server

This file is a simple form "login" ( only username required )

 <form action="chat.php" method = "post">
 Username: <input type="text" name="nick"/><br />
 <input type="submit" name="button" value="Go"/>
 </form>
 <?php
 include('connessione.php');
 if(isset($_POST['button'])) {
 $Username = $_POST['nick'];
 $Username = mysql_real_escape_string($Username);
 $Query = "INSERT INTO users (NickName) VALUES ($Username)";
 if(!$Query) {
 }
 else {
 "Error performing query!".mysql_error();
 }
 }
 ?>

This file is the chat!

<script type="text/javascript" src="chat.js"></script>
 <style type="text/css">
 .tastiera {
 width:500px;
 height:50px;
 border-radius:5px;
 border:1px solid #999;
 overflow:auto;
 font-size:13px;
 .chat {
 width:500px;
 height:6000px;
 border-radius:5px;
 border:1px solid #999;
 overflow:auto;
 font-size:13px;
 }
 </style>
 <?php
 include('connessione.php');
 session_start ();
 if($_SESSION['nick'] == ""){
 echo "You are not authorized to enter!";
 }
 exit();
 header('Cache-Control: Private');
 ?>
 <div id="CHAT"></div>
 <form action="salvataggio.php" name="inserimento" method="post" onsubmit="javascript:location.reload();">
 <input type="text" name="messaggio" width="500" height="50"/>
 <input type="submit" value="Chat"/>
 </form>
<iframe src="messaggio.php" name="MSG" id="MSG"></iframe>

This Ajax file Update the chat

 function Update()
 {
 return Request();
 }
 window.setInterval("Update()", 3000)
 var XMLHTTP;
 function Request()
 {
 XMLHTTP = GetBrowser(ChangeStatus);
 XMLHTTP.open("GET", "ajax.php", true);
 XMLHTTP.send(null);
 }
 function ChangeStatus()
 {
 if (XMLHTTP.readyState == 4)
 {
 var R = document.getElementById("CHAT");
 R.innerHTML = XMLHTTP.responseText;
 }
 }
 function GetBrowser(FindBrowser)
 {
 if (navigator.userAgent.indexOf("MSIE") != (-1))
 {
 var Class = "Msxml2.XMLHTTP";
 if (navigator.appVersion.indexOf("MSIE 5.5") != (-1));
 {
 Class = "Microsoft.XMLHTTP";
 } 
 try
 {
 ObjXMLHTTP = new ActiveXObject(Class);
 ObjXMLHTTP.onreadystatechange = FindBrowser;
 return ObjXMLHTTP;
 }
 catch(e)
 {
 alert("attenzione: l'ActiveX non sarà eseguito!");
 }
 }
 else if (navigator.userAgent.indexOf("Mozilla") != (-1))
 {
 ObjXMLHTTP = new XMLHttpRequest();
 ObjXMLHTTP.onload = FindBrowser;
 ObjXMLHTTP.onerror = FindBrowser;
 return ObjXMLHTTP;
 }
 else
 {
 alert("L'esempio non funziona con altri browser!");
 }
 }

This file save into database the messages

<?php
@session_start();
if(!isset($_SESSION['nick'])){
 @header('Location:prechat.php');
}else{
 if(isset($_POST['messaggio'])){
 include 'connessione.php';
 $user = $_SESSION['nick'];
 $mex_chat = addslashes($_POST['messaggio']);
 $query = "INSERT INTO utenti (user, mex_chat) 
 VALUES ('$user','$mex_chat')";
 @mysql_query($query)or die (mysql_error());
 @mysql_close();
 @header('Location:chat.php');
 }
}

This file display the messages

$sql = "SELECT user, mex_chat FROM 
utenti ORDER BY id_chat DESC LIMIT 0,10";
$sql_res = @mysql_query($sql)or die (mysql_error());
if(@mysql_num_rows($sql_res)>0)
{
 while ($fetch = @mysql_fetch_array($sql_res))
 { 
 $utente = stripslashes($fetch['user_chat']);
 $mex_utente = stripslashes($fetch['mex_chat']);
 echo '<b>'. $utente .'</b>: '. $mex_utente.'<br />';
 }
}else{
 echo 'Non sono stati ancora inseriti dei messaggi.';
}
?>

What do you think? How can I improve or resolve any bugs ( if there are ) ?

• Joseph: If you aren't sure if this is working 100%, then come back when you have fixed it.

Answer 1

If I'm reading this right, your AJAX grabs the whole chat every 3 seconds and (re)displays it. IMO, it'd be much better to send some info on the last message you have (timestamp or something), and then fetch only newer messages if there are any (via JSON; format them on the client side). That would reduce the load on the network which, given the short update interval, might be significant for both your and your users' bandwidth.

I suggest posting messages via AJAX as well, so that the page never gets reloaded. It's more pleasant for the user (and requires less network load, if done properly).

And, of course, lose the mysql_* functions! These are insecure and depricated, and are to be abandoned soon. I prefer PDO, but you can also go for mysqli_* functions. Your current code is wide open for attacks.

Answer 2

Actually Vedran, mysql functions aren't insecure when done properly. in this particulary case though, he is wide open.

should be like this

 if(isset($_POST['button'])) {
 $Username = mysql_real_escape_string(strip_tags($_POST["Username"]));
 $Query = "INSERT INTO users (NickName) VALUES ($Username)";
 if(!$Query) {
 }
 else {
 "Error performing query!".mysql_error();
 }
 }

• Corbin: There's three major flaws in this code: $Username should be quoted in the query (VALUES ($Username)), and the if (!$Query) isn't actually doing anything. That's checking if the string Query is false. The query isn't actually run anywhere in this code. The other flaw is that the strip tags doesn't make sense. Just use a white list to only allow certain characters. No point in allowing <b>user</b> to silently create the user user.
• r3wt: ah, thanks. i always strip tags just in case. i do realize i'm probably alone in this, but i take the better safe than sorry approach. as far as your other concerns, i was only intending to show him the use of my_sql_real_escape_string, not necessarily moderating the syntax of his actual queries. i like to use @mysql_query for queries and the reserved variable $sql but thats an entirely different debate.