propre ou pas propre ?

Invité2 · 25/01/2011, 17h15

Bonjour à tous,

J'ai besoin de conseils pour une fonction dans laquelle je fais plusieurs requêtes Mysql. Je ne trouve pas ma façon de faire très propre et voudrais avoir vos conseils pour faire quelque chose "dans les règles de l'art".

Merci.

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
public function GetTemplateVar($ContId,$TplId){ 
		$tab = $this->GetDb()->select("SELECT template_name,css_id FROM ".$this->__get('db_prefix')."_templates WHERE template_id =".$TplId);
		$this->__set('css_id', $tab->css_id);
		$tab = parent::GetDb()->select("SELECT content_id,content_name,last_modified_by,create_date,modified_date FROM ".parent::__get('db_prefix')."_content WHERE content_id =".$ContId);
		$this->smarty->content_id = $tab->content_id;
		$this->smarty->title = $tab->content_name;
		$tab = parent::GetDb()->select("SELECT type,text_name,content,create_date,modified_date FROM ".parent::__get('db_prefix')."_content_text WHERE content_id = ".$ContId." and text_name like 'content_en'");
		$this->smarty->content=$tab->content;
		$tab=parent::GetDb()->select("SELECT css_text,media_type FROM ".$this->__get('db_prefix')."_css WHERE css_id = ".$this->__get('css_id'));
		$this->smarty->css_text = $tab->css_text;
		$this->smarty->css_media_type = $tab->media_type;
		$this->GetTemplate($TplId);
	}

**Benjamin Delespierre** · 26/01/2011, 11h05

Hello

Déjà commence par aérer un peu, ça facilitera la lecture même pour toi (tu verra quand tu devra reprendre le code que tu as écrit il y a 6 mois par exemple). ça vaut également pour les commentaire.
Pense surtout à ne pas dépasser les 140 caractères (affichage intégral sur 19 pouces - les puristes C vont te dire 80 caractère mais je trouve que c'est trop peu pour PHP), cela t'évitera d'avoir l'ascenseur vertical qui rends la lecture du code pénible.

Premier point: je ne vois nulle part la sécurisation de tes requêtes SQL. Qu'est ce qu'il se passe dans ta fonction quand une requête échoue ? ça lance des warnings ? ça part en fatal error ? Ou pire: ça fait n'importe quoi !

Second point: tu mets des variables directement dans les requêtes. Ce n'est pas vraiment un problème si tu as le contrôle total de ces données, en revanche si elles arrivent de l'utilisateur, il vaut mieux les sécuriser pour éviter les injections.
Cette sécurité contre les injection à été introduite par l'interface mysqli et reprise par PDO (que tout bon programmeur PHP se devrait d'utiliser sous peine d'être fouetté publiquement

) en utilisant les requêtes préparées.

Troisième point: A aucun moment tu ne vérifie les retours de tes fonction, tu es à ce point sûr que tout marche comme sur des roulettes huilés dans le beurre ? Une légère vérification du retour de tes fonctions / méthodes serait de bon ton.
Un exemple tout bête:

Code :

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

1
2
 
MaClasse::methodeDeClasse()->methodeInstance()->autreMethode();

Il suffit qu'une seule de ses fonctions renvoie null, false ou void (ou n'importe quel autre type scalaire d'ailleurs) au lieu d'un objet et tout part en FATAL ERROR.

Mais bon, ne dramatise pas, dans l'ensemble ton code est bien (surtout en comparaison de toutes les horreurs qu'on peut voir sur ce forum.)
Donc pou résumer tout ça je dirais: blinde ton code et protèges-le !

Invité2 · 27/01/2011, 15h23

Merci.
En fait, la fonction "select()" est est une méthode d'une autre classe. Les requêtes y sont préparées.

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
public function select($query) {
		try
        {
            $requete = $this->PDOInstance->prepare($query);
            $requete->execute();
			$result = $requete->fetch(PDO::FETCH_OBJ);
			return $result;
        }
        catch (Exception $e) 
        {
            //On indique par email que la requête n'a pas fonctionné.
             error_log(date('D/m/y').' à '.date("H:i:s").' : '.$e->getMessage(), 1, 'XX@XXXXX.com');
        }
	}

Je récupère les données avec __get().Mais je viens d'avoir un problème assez bizarre : il a fallu que j'ajoute dans chaque constructeur de classe dérivée "parrent::__construct()" pour que cela fonctionne. Pourtant, avant que je test les possibilité de gestion des messages d'erreur php, cela fonctionnait très bien

Code :

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

1
2
3
function __get($data){
			return $this->param[$data];
	}

Et sinon, voilà la fameuse classe qui me pose problème après ajout de quelques verifications. Je ne vois pas quoi faire de plus... ?

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
public function GetTemplateVar($ContId,$TplId){ 
		// Recuperation des differentes variables necessaires a l affichage du template.
		$tab = $this->GetDb()->select("SELECT template_name,css_id FROM ".$this->__get('db_prefix')."_templates WHERE template_id =".$TplId);
		if($tab)
			$this->__set('css_id', $tab->css_id);
		$tab = parent::GetDb()->select("SELECT content_id,content_name,last_modified_by,create_date,modified_date FROM ".parent::__get('db_prefix')."_content WHERE content_id =".$ContId);
		if($tab) {
			$this->smarty->content_id = $tab->content_id;
			$this->smarty->title = $tab->content_name;
			}
		$tab = parent::GetDb()->select("SELECT type,text_name,content,create_date,modified_date FROM ".parent::__get('db_prefix')."_content_text WHERE content_id = ".$ContId." and text_name like 'content_en'");
		if($tab)
			$this->smarty->content=$tab->content;
		$tab=parent::GetDb()->select("SELECT css_text,media_type FROM ".$this->__get('db_prefix')."_css WHERE css_id = ".$this->__get('css_id'));
		if($tab){
			$this->smarty->css_text = $tab->css_text;
			$this->smarty->css_media_type = $tab->media_type;
			$this->GetTemplate($TplId);
			}
	}

Merci.

**Benjamin Delespierre** · 28/01/2011, 09h51

Vu que je ne connais pas ton code en détail, je ne sais pas non plus ou ça coince; pars à la chasse aux erreurs, fait des traces un peut partout, utilise un débugger (www.xdebug.org).

C'est quel genre de problème pour commencer ? Un problème de logique ? La méthode ne renvoie pas les bons résultats ? Il y a une erreur fatale ? Jannet Jackson sors un nouveau disque ?

**pc.bertineau** · 28/01/2011, 10h41

Envoyé par phpdeveloppeur

Merci.
En fait, la fonction "select()" est est une méthode d'une autre classe. Les requêtes y sont préparées.

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
public function select($query) {
		try
        {
            $requete = $this->PDOInstance->prepare($query);
            $requete->execute();
			$result = $requete->fetch(PDO::FETCH_OBJ);
			return $result;
        }
        catch (Exception $e) 
        {
            //On indique par email que la requête n'a pas fonctionné.
             error_log(date('D/m/y').' à '.date("H:i:s").' : '.$e->getMessage(), 1, 'XX@XXXXX.com');
        }
	}

Attention, cette méthode select ne te protège pas des injections SQL !! C'est au moment où tu introduis tes variables venant de l'utilisateur dans la requête qu'il faut les échapper (avec mysql_real_escape_string notamment).

De plus, utiliser des requêtes préparées, c'est bien, mais l'intérêt est réel pour des requêtes exécutées en boucle dans un script avec des valeurs changeantes. Dans ton cas elle ne sert à rien et ne te protège même pas des injections SQL.

Continue sur PDO, c'est effectivement une bonne pratique, et renseigne toi sur les requêtes préparées, comment et pourquoi les utiliser. Idem pour la protection contre les injections !

**Benjamin Delespierre** · 28/01/2011, 11h10

Attention, cette méthode select ne te protège pas des injections SQL !! C'est au moment où tu introduis tes variables venant de l'utilisateur dans la requête qu'il faut les échapper (avec mysql_real_escape_string notamment).

De plus, utiliser des requêtes préparées, c'est bien, mais l'intérêt est réel pour des requêtes exécutées en boucle dans un script avec des valeurs changeantes. Dans ton cas elle ne sert à rien et ne te protège même pas des injections SQL.

En effet, il faut que tu utilise des patterns de remplacement pour tes prepare statement sinon ça ne sert strictement à rien et ça ne te protège pas.
Les requêtes préparées sont surtout utile car elles permettent de ne pas avoir à construire manuellement les requêtes, s'en servir comme tu le fais n'est pas correct.

En revanche, si les requêtes préparées le protège des attaques par injection, elles ne protègent pas les XSS, prudence donc.

**stealth35** · 28/01/2011, 11h42

Envoyé par pc.bertineau

Attention, cette méthode select ne te protège pas des injections SQL !! C'est au moment où tu introduis tes variables venant de l'utilisateur dans la requête qu'il faut les échapper (avec mysql_real_escape_string notamment).

mysql_real_escape_string avec PDO ???

faire ca me semble plus logique, même si je vois pas pourquoi tu récupères qu'un seul résultat :

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
public function select($query, array $input_parameters)
{
    try
    {
        $requete = $this->PDOInstance->prepare($query);
        $requete->execute($input_parameters);
        $result = $requete->fetchObject();
        $result->closeCursor();
 
        return $result;
    }
    catch (Exception $e) 
    {
        //On indique par email que la requête n'a pas fonctionné.
        error_log(date('D/m/y').' à '.date("H:i:s").' : '.$e->getMessage(), 1, 'XX@XXXXX.com');
    }
 
    return false;
}

**pc.bertineau** · 28/01/2011, 14h54

Envoyé par stealth35

mysql_real_escape_string avec PDO ???

PDO sans requête préparée ne dispense pas d'échapper les données extérieures, si ?

Personnellement je n'utilise pas systématiquement les preparedStatement comme je l'ai expliqué plus haut. L'exemple de phpdeveloppeur montre bien qu'une requête préparée sans séparer les paramètres de la requête ne protège pas non plus des injections...

**stealth35** · 28/01/2011, 15h04

Envoyé par pc.bertineau

PDO sans requête préparée ne dispense pas d'échapper les données extérieures, si ?

oui oui, sauf que tu vas avoir une belle erreur si tu fais un mysql_real_escape_string, l'extension mysql et l'extension PDO c'est 2 choses différentes, si tu veux protégé une valeur avec PDO c'est PDO::quote

**pc.bertineau** · 28/01/2011, 16h09

ok merci, je vais pouvoir me passer de charger mon extension mysql du coup

Invité2 · 31/01/2011, 13h04

Voilà la fonction select() modifiée. Par contre, quote() me pose un tas de problèmes, je l'ai donc mis en commentaire.
Si le tableau contient un 'int', la requête me renvoi un mauvais résultat.
Si le tableau contient un 'string', la requête ne fonctionne pas.

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
public function select2($query,$input)
	{
		try
		{	 
			// if (is_array($input)){
				// foreach ($input as $key => $value){
					// $SecureTab = array($key => $this->PDOInstance->quote($value));
				// }
			// }
			// else {
				// $SecureTab = $input;
			// }
			$SecureTab = $input;
			$requete = $this->PDOInstance->prepare($query);
			$requete->execute($SecureTab);
			$result = $requete->fetch(PDO::FETCH_OBJ);
			$requete->closeCursor();
 
			return $result;
		}
		catch (Exception $e) 
		{
			//On indique par email que la requête n'a pas fonctionné.
			error_log(date('D/m/y').' à '.date("H:i:s").' : '.$e->getMessage(), 1, 'XX@XXXXX.com');
		}
		return false;
	}

Pour mon erreur de constructeur, je ne sais pas d'où elle venait, mais je pensait qu'en php5, lorsqu'on appel une classe dérivée, le constructeur de la classe parent était automatiquement appelé. J'ai donc ajouté dans les constructeurs de classe dérivées

Code :

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

parent::__construct();

J'attaque le problème du quote().

**stealth35** · 31/01/2011, 13h13

y'a pas besoin de faire de quote, si les paramètres sont dans la requête pré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
//pas bien
$id = 12;
PDO::query("SELECT * FROM user WHERE id=$id");
 
//bien
$id = PDO::quote(12);
PDO::query("SELECT * FROM user WHERE id=$id");
 
//pas bien
$id = 12;
PDO::prepare("SELECT * FROM user WHERE id=$id");
PDO::execute();
 
//bien
$id = 12;
PDO::prepare("SELECT * FROM user WHERE id=?");
PDO::execute(array($id));
 
//complètement inutile...
$id = PDO::quote(12);
PDO::prepare("SELECT * FROM user WHERE id=$id");
PDO::execute();

Invité2 · 31/01/2011, 14h19

Merci beaucoup !

Sinon, dans cette fonction, est-ce que cela pourrait être mieux ? J'aimerais coder mes fonctions proprement dès maintenant plutôt qu'à la fin de mon projet...

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
 
public function GetTemplateVar($ContId,$TplId){ 
		// Recuperation des differentes variables necessaires a l affichage du template.
		// parametres requete
		$input=array('TplId'=>$TplId);
		//requete de récupération du nom de template et de l'id du css
		$tab = $this->GetDb()->select2("SELECT template_name,css_id FROM ".$this->__get('db_prefix')."_templates WHERE template_id = :TplId",$input);
		// assignation de la valeur de l'id de css
		if($tab) {
			$this->__set('css_id', $tab->css_id);
			}
		// parametres requete
		$input=array('ContId'=>$ContId);
		// requete de récupération des différents champs de la table du contenu
		$tab = parent::GetDb()->select2("SELECT content_id,content_name,last_modified_by,create_date,modified_date FROM ".parent::__get('db_prefix')."_content WHERE content_id = :ContId",$input);
		// assignation des champs récupére (pas tous pour l'instant)
		if($tab) {
			$this->smarty->content_id = $tab->content_id;
			$this->smarty->title = $tab->content_name;
			}
		// requête de récupération des champs de la table contenu texte
		$tab = parent::GetDb()->select2("SELECT type,text_name,content,create_date,modified_date FROM ".parent::__get('db_prefix')."_content_text WHERE content_id = :ContId and text_name like 'content_en'",$input);
		if($tab) {
			$this->smarty->content=$tab->content;
			// parametres requete
			$input=array('css_id'=>$this->__get('css_id'));
			}
		// requete récupération css
		$tab=parent::GetDb()->select2("SELECT css_text,media_type FROM ".$this->__get('db_prefix')."_css WHERE css_id = :css_id",$input);
		if($tab){
			$this->smarty->css_text = $tab->css_text;
			$this->smarty->css_media_type = $tab->media_type;
			// afichage de la page
			$this->GetTemplate($TplId);
			}
	}