Mon code est il bon ?

**Sheiya** · 13/05/2009, 16h06

Bonjour alors j'aimerais vous exposer mon code ici pour voir ce que vous en pensiez. Failles, propretés etc...
Avant tout, j'aimerais que vous m'éclaircissiez sur un point.
Le :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

mysql_query("SET NAMES 'utf8'");

Est il obligatoire pour prévenir le serveur MYSQL que la transaction des données ce fait en UTF8 ?

Pas page enregistrement (pour ce qui est des protections htmlspecialschars, je le fais à l'affichage) :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
<?php
include ("connect.php");
$sql = "SELECT * FROM quetes WHERE titre='".$_POST['titre']."'";
$result = mysql_query($sql);
if(mysql_num_rows($result) > 0) // Vérification si il existe déjà.
{
echo 'La quêtes existe déjà';
}
else // Sinon, on enregistre
{
if (!empty ($_POST['titre']) && is_numeric($_POST['type']) && is_numeric($_POST['faction']) && is_numeric($_POST['classe']) && !empty ($_POST['de']) && is_numeric($_POST['lvl']) && !empty ($_POST['objectif']) && !empty ($_POST['aide']) && is_numeric($_POST['xp']) && is_numeric($_POST['kinah']))
{
include ("connect.php");
$sql = "INSERT INTO quetes VALUES ('', '".mysql_real_escape_string($_POST['titre'])."', '".($_POST['type'])."', '".($_POST['faction'])."', '".($_POST['classe'])."', '".mysql_real_escape_string($_POST['de'])."', '".($_POST['lvl'])."', '".mysql_real_escape_string($_POST['objectif'])."', '".mysql_real_escape_string($_POST['aide'])."', '".($_POST['xp'])."', '".($_POST['kinah'])."')";
mysql_query ($sql);
echo $sql;
}
else
{
echo 'Vous n\'avez pas remplis tout les champs, ou vous avez fait une erreur...';
}
}
?>

Maintenant l'affichage :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
<?php
// Affichage des quêtes de la faction ELYSEENS !
if (isset ($_GET['Faction']) AND isset ($_GET['Classe']))
{
	if (($_GET['Faction'] == 'Elyseens') OR ($_GET['Faction'] == 'Asmodiens') OR ($_GET['Faction'] == 'E/A') OR ($_GET['Classe'] == 'Tout') OR ($_GET['Classe'] == 'Mage') OR ($_GET['Classe'] == 'Prêtre') OR ($_GET['Classe'] == 'Guerrier') OR ($_GET['Classe'] == 'Éclaireur'))
	{
			if ($_GET['Faction'] == 'E/A')
			{
			$faction = 0;
			}
			if ($_GET['Faction'] == 'Elyseens')
			{
			$faction = 1;
			}
			if ($_GET['Faction'] == 'Asmodiens')
			{
			$faction = 2;
			}
			if ($_GET['Classe'] == 'Tout')
			{
			$classe = 0;
			}
			if ($_GET['Classe'] == 'Mage')
			{
			$classe = 1;
			}
			if ($_GET['Classe'] == 'Prêtre')
			{
			$classe = 2;
			}
			if ($_GET['Classe'] == 'Guerrier')
			{
			$classe = 3;
			}
			if ($_GET['Classe'] == 'Éclaireur')
			{
			$classe = 4;
			}
		include ("connect.php");
		$sql = "SELECT * FROM quetes WHERE faction=".$faction." AND classe=".$classe;
		$result = mysql_query($sql);
		while ($rep = mysql_fetch_array($result))
		{
		echo 'Titre : '.$rep['titre'].'<br />';
		}
	}
}
// Affichage => FIN
 
else
{
?>
<a href="quetes.php?Faction=Elyseens&Classe=Tout">Toutes les quêtes Elyseens</a><br />
<a href="quetes.php?Faction=Elyseens&Classe=Mage">Elyseens/Mage</a><br />
<a href="quetes.php?Faction=Elyseens&Classe=Prêtre">Elyseens/Prêtre</a><br />
<a href="quetes.php?Faction=Elyseens&Classe=Guerrier">Elyseens/Guerrier</a><br />
<a href="quetes.php?Faction=Elyseens&Classe=Éclaireur">Elyseens/Éclaireur</a><br /><br />
<a href="quetes.php?Faction=Asmodiens&Classe=Tout">Toutes les quêtes Asmodiens</a><br />
<a href="quetes.php?Faction=Asmodiens&Classe=Mage">Asmodiens/Mage</a><br />
<a href="quetes.php?Faction=Asmodiens&Classe=Prêtre">Asmodiens/Prêtre</a><br />
<a href="quetes.php?Faction=Asmodiens&Classe=Guerrier">Asmodiens/Guerrier</a><br />
<a href="quetes.php?Faction=Asmodiens&Classe=Éclaireur">Asmodiens/Éclaireur</a><br />
<?php
}
?>

Qu'en pensez vous ? Merci

**sabotage** · 13/05/2009, 16h23

Tu pourrais un peu alléger le deuxieme script :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
<?php
if (isset ($_GET['Faction']) AND isset ($_GET['Classe'])) {
	switch ($_GET['Faction']) {
		case 'E/A' : $faction = 0; break;
		case 'Elyseens' : $faction = 1; break;
		...
	}
 
	switch ($_GET['Classe']) {
		case 'Tout' : $classe = 0; break;
		case 'Mage' : $classe = 1; break;
		....
	}
 
	if (isset($classe) && isset($faction)) {
		include ("connect.php");
		$sql = "SELECT * FROM quetes WHERE faction=".$faction." AND classe=".$classe;
		$result = mysql_query($sql);
		while ($rep = mysql_fetch_array($result)) {
			echo 'Titre : '.$rep['titre'].'<br />';
		}
	}
}

Ou utiliser des elseif{}

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

$sql = "INSERT INTO quetes VALUES ('', '"

Il vaut mieux indiquer le nom des champs et ne pas mettre une chaine vide pour l'autoincrementation, certains configurations de mysql ne l'accepte pas.

**Séb.** · 13/05/2009, 16h41

Je mets en commentaire les critiques que je peux apporter au script...

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
<?php
include ("connect.php"); // Pas besoin de " autour d'un littéral, des ' suffisent
 
// Pas besoin de " autour d'un littéral, utilise " ou immerge $_POST dans la chaîne
// $_POST n'est pas échappée, magic-quotes ?
$sql = "SELECT * FROM quetes WHERE titre='".$_POST['titre']."'";
 
$result = mysql_query($sql);
 
// Si la requête sert à tester l'existence d'un enregistrement
// Alors inutile de faire un SELECT *, un SELECT COUNT( ) suffit
if(mysql_num_rows($result) > 0)
{
echo 'La quêtes existe déjà';
}
else
{
// J'aurais d'abord tester la validité des champs avant de faire appel au serveur MySQL
// empty( ) n'est pas adapté pour tester si un champ a été rempli ou non
// is_numeric( ) n'est pas adapté pour tester si une valeur est un entier ou non
if (!empty ($_POST['titre']) && is_numeric($_POST['type']) && is_numeric($_POST['faction']) && is_numeric($_POST['classe']) && !empty ($_POST['de']) && is_numeric($_POST['lvl']) && !empty ($_POST['objectif']) && !empty ($_POST['aide']) && is_numeric($_POST['xp']) && is_numeric($_POST['kinah']))
{
// Tu rappelles une 2nde fois connect.php
include ("connect.php");
// Tu n'échappes pas toutes les valeurs envoyées à MySQL
$sql = "INSERT INTO quetes VALUES ('', '".mysql_real_escape_string($_POST['titre'])."', '".($_POST['type'])."', '".($_POST['faction'])."', '".($_POST['classe'])."', '".mysql_real_escape_string($_POST['de'])."', '".($_POST['lvl'])."', '".mysql_real_escape_string($_POST['objectif'])."', '".mysql_real_escape_string($_POST['aide'])."', '".($_POST['xp'])."', '".($_POST['kinah'])."')";
mysql_query ($sql);
// Bien pour le débugage uniquement
echo $sql;
}
else
{
echo 'Vous n\'avez pas remplis tout les champs, ou vous avez fait une erreur...';
}
}
?>

+ Code non indenté = difficilement lisible

Pour le 2nd script tu peux remplacer la 1re partie par :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
if ( isset($_GET['Faction'], $_GET['Classe']) )
{
        $factions = array(1 => 'Elyseens', 2 => 'Asmodiens', 0 => 'E/A') ;
        $classes = array(0 => 'Tout', 1 => 'Mage', 2 => 'Prêtre', 3 => 'Guerrier', 4 => 'Éclaireur') ;
 
        $faction = array_search($_GET['Faction'], $factions) ;
        $classe = array_search($_GET['Faction'], $classes) ;
 
        if ( $faction !== FALSE && $classe !== FALSE ) {
 
            include ("connect.php");
 
            // Eviter les SELECT * si on n'utilise pas tous les champs
            $sql = "SELECT titre FROM quetes WHERE faction=".$faction." AND classe=".$classe;
            $result = mysql_query($sql);
 
            while ($rep = mysql_fetch_array($result)) {
                echo 'Titre : '.$rep['titre'].'<br />';
            }
        }
}
else ...

**sabotage** · 13/05/2009, 17h11

J'ai reflechi (et oui) à ce is_numeric() :

si l'utilisateur passe autre chose que des chiffres, ca ne passera pas ; si l'utilisateur passe un decimal ou un chiffre avec des zeros au début, ca passera mais mysql ne gardera que la valeur entiere (sous reserve d'utiliser les guillemets).

Donc finalement n'est ce pas suffisant comme test ? Et du coup inutile d'echapper la chaine.

**Sheiya** · 13/05/2009, 17h23

Envoyé par sabotage

Donc finalement n'est ce pas suffisant comme test ? Et du coup inutile d'echapper la chaine.

J'ai pas compris ce que tu essaye de me dire la :p

Ensuite

// empty( ) n'est pas adapté pour tester si un champ a été rempli ou non

Alors je met quoi ? ^^

// Tu n'échappes pas toutes les valeurs envoyées à MySQL

Ce qui veut dire ? (si c'est is_numeric, unitule de faire le test non , )

Sinon,

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

mysql_query("SET NAMES 'utf8'");

Est il obligatoire pour prévenir le serveur MYSQL que la transaction des données ce fait en UTF8 ?

**sabotage** · 13/05/2009, 17h34

Est il obligatoire pour prévenir le serveur MYSQL que la transaction des données ce fait en UTF8 ?

Est-ce que cela te coute de le mettre ?
Cela t'assure que la connexion se fait dans l'encodage que tu souhaites independemment de la configuration du serveur.

**Séb.** · 13/05/2009, 17h45

// empty( ) n'est pas adapté pour tester si un champ a été rempli ou non

Alors je met quoi ? ^^

empty('0') => TRUE alors que la chaîne n'est pas vide http://fr.php.net/empty

Amha mieux vaux faire :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

if ( $tonChamp === '' ) { // Si rien n'a été saisi

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

if ( $tonChamp !== '' ) { // Si qquechose a été saisi

Ensuite :

Citation:
// Tu n'échappes pas toutes les valeurs envoyées à MySQL

Ce qui veut dire ? (si c'est is_numeric, unitule de faire le test non , )

Certes, mais il faudrait que tu aies 100% confiance en is_numeric( ) (en restant raisonnable).
La sécurité est affaire de non-confiance

si l'utilisateur passe autre chose que des chiffres, ca ne passera pas ;

Si si ça passera car is_numeric( ) accepte les notations scientifique et hexadécimale, dixit la doc.

**Sheiya** · 13/05/2009, 19h31

Si si ça passera car is_numeric( ) accepte les notations scientifique et hexadécimale, dixit la doc.

Ma table est en Tynint je risque logiquement rien.

**Séb.** · 13/05/2009, 19h52

Tout dépend de ta définition du risque.

Mon code est il bon ?

Langage PHP

Discussions similaires

Partager

Partager