propre ou pas propre ?

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 :

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]

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 :

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 !

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

Code :

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 :

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 :

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]

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]

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 :

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]

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]

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 :

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]

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]

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]

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

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 :

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 :

parent::__construct();

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

[stealth35]

y'a pas besoin de faire de quote, si les paramètres sont dans la requête préparé

Code :

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();

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 :

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);
			}
	}

[stealth35]

t'es plus sous PDO ?

Je suis toujours sous PDO, pourquoi ?

[stealth35]

Envoyé par phpdeveloppeur

Je suis toujours sous PDO, pourquoi ?

non non c'est moi c'est le "select2" qui ma perturbé

lol c'est vrai que j'aurais pu choisir un autre nom.