Is deze code netjes/ veilig?

**gast10494** · 18-04-2013, 08:49

Ik wil met een scripter in zee gaan voor het maken van een uitgebreid systeem. Ik zelf heb geen verstand van scripten. Nu mijn vraag is het stukje code hieronder veilig en netjes geschreven? Zo ja dan ga ik verder in onderhandeling met deze scripter. Zo nee dan uiteraard niet. Hij heeft hier nog wel de volgende opmerkingen bij:

Nogmaals sommige dingen kunnen anders en we gebruiken het ........ (..... naam bedrijf) Framework als basis, helaas ga ik daar geen code van vrij geven omdat dit te complex is.

<?php
class Validation
{
public $validationErrors = array();
public $aDefvals = array();

private $validationPost;
private $validationRules = array();

private $savedValidationRules = array();

public function __construct($validationPost = array())
{
$this->validationPost = $validationPost;
}

public function setValidation($field, $rules = array())
{
if(count($rules) == 0 && array_key_exists($field, $this->savedValidationRules))
{
$this->validationRules[$field] = $this->savedValidationRules[$field];
}
else
{
$this->validationRules[$field] = $rules;
}
}

public function returnDefvals()
{
return $this->aDefvals;
}

public function runValidation()
{

foreach($this->validationRules as $field => $rules)
{
if(!isset($this->validationPost[$field]))
{
$this->validationPost[$field] = '';
}
foreach($rules as $rule => $ruleValue)
{
$required = false;
if(array_key_exists('required', $rules))
{
if($rules['required'] === true)
{
$required = true;
}
}
if(!$required && empty($this->validationPost[$field]))
{
break;
}

if(method_exists($this, $rule))
{
if(!$this->$rule($field, $this->validationPost[$field], $ruleValue))
{
$this->validationErrors[$field] = true;
break;
}
}
}

if(!array_key_exists($field, $this->validationErrors))
{
$this->aDefvals[$field] = $this->validationPost[$field];
}
}
}

private function alpha($key, $value, $ruleValue)
{
if(preg_match('/^([a-z])+$/i', $value))
{
return true;
}

return false;
}

private function alphaNumeric($key, $value, $ruleValue)
{
if(preg_match('/^([a-z0-9])+$/i', $value))
{
return true;
}

return false;
}

private function exactLength($key, $value, $ruleValue)
{
if(strlen($value) == $ruleValue)
{
return true;
}

return false;
}

private function greaterThan($key, $value, $ruleValue)
{
if(preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $value))
{
if($value > $ruleValue)
{
return true;
}
}

return false;
}

private function lessThan($key, $value, $ruleValue)
{
if(preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $value))
{
if($value < $ruleValue)
{
return true;
}
}

return false;
}

private function match($key, $value, $ruleValue)
{
if($value == $ruleValue)
{
return true;
}

return false;
}

private function maxLength($key, $value, $ruleValue)
{
if(strlen($value) <= $ruleValue)
{
return true;
}

return false;
}

private function minLength($key, $value, $ruleValue)
{
if(strlen($value) >= $ruleValue)
{
return true;
}

return false;
}

private function numeric($key, $value, $ruleValue)
{
if(preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $value))
{
return true;
}

return false;
}

private function required($key, $value, $ruleValue)
{
if(!empty($value))
{
return true;
}
return false;
}

private function validEmail($key, $value, $ruleValue)
{
if(preg_match("/^([a-z0-9\+_\-]+)(\.[a-z0-9\+_\-]+)*@([a-z0-9\-]+\.)+[a-z]{2,6}$/ix", $value))
{
return true;
}

return false;
}

private function validURL($key, $value, $ruleValue)
{
if(preg_match('/^(https?:\/\/)?([\da-z\.-]+)\.([a-z\.]{2,6})([\/\w \.-]*)*\/?$/', $value))
{
return true;
}

return false;
}

}
?>

**gast31394** · 18-04-2013, 08:56

Het is niet helemaal netjes, aangezien je bij een protected of een private variable een _ voor de naam zet.

Dus:

PHP Code:


<?
  public $error;
  private $_password;
?>

Voor de rest is het vrij netjes.

**gast10494** · 18-04-2013, 09:04

Kan dit gevolgen hebben?

**Rob Quist** · 18-04-2013, 09:11

Origineel gepost door gast10494

Kan dit gevolgen hebben?

Nee, helemaal niet, heeft er niets mee te maken. Het blijft gewoon een variabel en is alleen om de developer aan te geven dat het om een protected variabel gaat (voor de makkelijkheid) en zodat je niet zomaar probeert een protected var te overschrijven.

Even een tip; kun je in je eerste post even [ code ] en [/code] bloks neerzetten? Dat leest wat makkelijker. Dan check ik m ff voor je.

Groetjes
Aanvullend bericht:

Origineel gepost door gast31394

Het is niet helemaal netjes, aangezien je bij een protected of een private variable een _ voor de naam zet.

Dus:

PHP Code:


<?

  public $error;

  private $_password;

?>

Voor de rest is het vrij netjes.

Overigens opent men tegenwoordig PHP met <?php - <? = phased out

**gast10494** · 18-04-2013, 09:22

Even een tip; kun je in je eerste post even [ code ] en [/code] bloks neerzetten? Dat leest wat makkelijker. Dan check ik m ff voor je.

hoe doe ik dit? heb de code via pb toegestuurd gekregen.

**Rob Quist** · 18-04-2013, 09:29

Huh? Kan tegenwoordig niet eens meer een bericht aanpassen op SD? Ik zie zelf ook geen edit knop... Naja dan post ik de code wel ff;

PHP Code:


class Validation
{
  public $validationErrors = array();
  public $aDefvals = array(); 
  private $validationPost;
  private $validationRules = array(); 
  private $savedValidationRules = array();  
  public function __construct($validationPost = array())
  {
    $this->validationPost = $validationPost;
  } 
  public function setValidation($field, $rules = array())
  {
    if(count($rules) == 0 && array_key_exists($field, $this->savedValidationRules))
    {
      $this->validationRules[$field] = $this->savedValidationRules[$field];
    }
    else 
    {
      $this->validationRules[$field] = $rules;
    }
  } 
  public function returnDefvals()
  {
    return $this->aDefvals;
  } 
  public function runValidation()
  { 
    foreach($this->validationRules as $field => $rules)
    {
      if(!isset($this->validationPost[$field]))
      {
        $this->validationPost[$field] = '';
      }
      foreach($rules as $rule => $ruleValue)
      {
        $required = false;
        if(array_key_exists('required', $rules))
        {
          if($rules['required'] === true)
          {
            $required = true;
          }
        }  
        if(!$required && empty($this->validationPost[$field]))
        {
          break;
        } 
        if(method_exists($this, $rule))
        {
          if(!$this->$rule($field, $this->validationPost[$field], $ruleValue))
          {
            $this->validationErrors[$field] = true;
            break;
          }
        }
      } 
      if(!array_key_exists($field, $this->validationErrors))
      {
        $this->aDefvals[$field] = $this->validationPost[$field];
      }
    }
  } 
  private function alpha($key, $value, $ruleValue)
  {
    if(preg_match('/^([a-z])+$/i', $value))
    {
      return true;
    } 
    return false;
  } 
  private function alphaNumeric($key, $value, $ruleValue)
  {
    if(preg_match('/^([a-z0-9])+$/i', $value))
    {
      return true;
    } 
    return false;
  } 
  private function exactLength($key, $value, $ruleValue)
  {
    if(strlen($value) == $ruleValue)
    {
      return true;
    } 
    return false;
  } 
  private function greaterThan($key, $value, $ruleValue)
  {
    if(preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $value))
    {
      if($value > $ruleValue)
      {
        return true;
      }
    } 
    return false;
  } 
  private function lessThan($key, $value, $ruleValue)
  {
    if(preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $value))
    {
      if($value < $ruleValue)
      {
        return true;
      }
    } 
    return false;
  } 
  private function match($key, $value, $ruleValue)
  {
    if($value == $ruleValue)
    {
      return true;
    } 
    return false;
  } 
  private function maxLength($key, $value, $ruleValue)
  {
    if(strlen($value) <= $ruleValue)
    {
      return true;
    } 
    return false;
  } 
  private function minLength($key, $value, $ruleValue)
  {
    if(strlen($value) >= $ruleValue)
    {
      return true;
    } 
    return false;
  } 
  private function numeric($key, $value, $ruleValue)
  {
    if(preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $value))
    {
      return true;
    } 
    return false;
  } 
  private function required($key, $value, $ruleValue)
  {
    if(!empty($value))
    {
      return true;
    }
    return false;
  } 
  private function validEmail($key, $value, $ruleValue)
  {
    if(preg_match("/^([a-z0-9\+_\-]+)(\.[a-z0-9\+_\-]+)*@([a-z0-9\-]+\.)+[a-z]{2,6}$/ix", $value))
    {
      return true;
    } 
    return false;
  } 
  private function validURL($key, $value, $ruleValue)
  {
    if(preg_match('/^(https?:\/\/)?([\da-z\.-]+)\.([a-z\.]{2,6})([\/\w \.-]*)*\/?$/', $value))
    {
      return true;
    } 
    return false;
  } 
}

Aanvullend bericht:
Redenen om met hem in zee te gaan;

- Object Georienteerd PHP = altijd goed
- Netjes inspringen c.q. nette code
- Ziet er veilig uit

Redenen om het niet te doen;
- Niet gedocumenteerd
- Geen docblocks
- geen typehinting / typecasting
- Manier van controleren is enigsinds exotisch

Als ik kijk naar het gemiddelde site-dealer heb je hier een van de betere site-deal programmeurs :)

**gast10494** · 18-04-2013, 09:59

Rob,

Bedankt voor je reactie en uitleg. Kreeg net nog een pb van hem binnen met volgende opmerkingen:

Nogmaals sommige dingen doen wij anders het, is vrij oude code, bijv $_gebruiken wij niet meer of
underscores in variable etc. Dit is puur een ruwe code!

Maar zo te lezen kan ik hier in ieder geval zeker op verder borduren. Gaat om een project van in totaal 1250 euro dus behoorlijke uitgave.

**Rob Quist** · 18-04-2013, 10:06

Mwa, die $_ komt uit PHP4, maar is soms nog wel handig als je bepaalde probleempjes wilt voorkomen. Niks mis mee. 1250 euro.. Valt mee hoor, ik ken projecten van 400.000 euro :) Maar inderdaad zeker de tijd waard om te kijken met wie je in zee gaat ;)

**N. Kaag** · 18-04-2013, 10:33

Ik ben het volledig oneens met de stelling dat een private of protected variable met een underscore moet beginnen.
Om aan de programmeur aan te geven dat het een private of protected variable is, moet de documentatie gewoon op orde zijn.
Tegenwoordig werken de meeste programmeurs met een IDE die automatisch code aanvult. Als je een beetje goede software hebt, laat hij de private variablen al nieteens zien, en de protected variablen zijn er vaak om overschreven te worden.

Het is een trucje dat vroeger werd gebruikt omdat PHP nog helemaal niet gericht was op private, public of protected variablen, en programmeurs toch de behoefte hadden om aan elkaar aan te geven welke variablen overschreven mogen worden.

Voor de rest ben ik geen fan van typecasting, omdat ik van mening ben dat controleren of iets een juiste waarde heeft beter is dan hopen dat het na een typecast wel goed komt. En PHP is al helemaal niet de taal om in te typecasten ;-)
Typehinting is leuk, maar omdat het alleen bruikbaar is voor objecten, arrays en callables, nog niet heel erg bruikbaar in PHP.

Voor de rest ben ik van mening dat 1 class nog geen OOP maakt.
Dit zal eerder onder een helper class vallen, wat juist een beetje los staat van het OOP principe.

Voor de rest ziet de code er wel redelijk uit. De meeste variable en functie namen zijn redelijk duidelijk, alleen jammer dat de documentatie ontbreekt.
Ik kan verder uit deze code weinig nuttigs zeggen of het een goede programmeur is of niet. Maar dat kan je vrijwel nooit aan de hand van 1 class of bestand.

**Joshua de Gier** · 18-04-2013, 11:12

Origineel gepost door N. Kaag

Ik ben het volledig oneens met de stelling dat een private of protected variable met een underscore moet beginnen.

Ben het met alles eens, echter bovenstaande: is maar net welke code convention je gebruikt. Persoonlijk vind ik het ook prettig om privates met een underscore te laten beginnen, al is het alleen maar voor de herkenbaarheid in de rest van je applicatie. Maar goed: benamingen van variabelen/functies blijft een persoonlijke voorkeur.

**Mark Puts** · 18-04-2013, 12:02

Zijn daarnaast ook nog wel wat dingetjes die beter kunnen. Zoals het gebruik van ctype_ en filter_var ipv pregmatches etc. Niet heel grote issues maar naar mijn mening net wat netter.

**gast10494** · 18-04-2013, 17:00

Bedankt voor de reacties. Eventjes voor Rob hier maar opnieuw:

Code:

<?php
class Validation
{
                public $validationErrors = array();
        public $aDefvals = array();
 
        private $validationPost;
        private $validationRules = array();
 
                private $savedValidationRules = array(); 
 
                public function __construct($validationPost = array())
        {
            $this->validationPost = $validationPost;
        }
 
        public function setValidation($field, $rules = array())
        {
            if(count($rules) == 0 && array_key_exists($field, $this->savedValidationRules))
            {
                $this->validationRules[$field] = $this->savedValidationRules[$field];
            }
            else 
            {
                $this->validationRules[$field] = $rules;
            }
        }
 
        public function returnDefvals()
        {
            return $this->aDefvals;
        }
 
        public function runValidation()
        {
 
            foreach($this->validationRules as $field => $rules)
            {
                if(!isset($this->validationPost[$field]))
                {
                    $this->validationPost[$field] = '';
                }
                foreach($rules as $rule => $ruleValue)
                {
                    $required = false;
                    if(array_key_exists('required', $rules))
                    {
                        if($rules['required'] === true)
                        {
                            $required = true;
                        }
                    }    
                    if(!$required && empty($this->validationPost[$field]))
                    {
                        break;
                    }
 
                    if(method_exists($this, $rule))
                    {
                        if(!$this->$rule($field, $this->validationPost[$field], $ruleValue))
                        {
                            $this->validationErrors[$field] = true;
                            break;
                        }
                    }
                }
 
                if(!array_key_exists($field, $this->validationErrors))
                {
                    $this->aDefvals[$field] = $this->validationPost[$field];
                }
            }
        }
 
        private function alpha($key, $value, $ruleValue)
        {
            if(preg_match('/^([a-z])+$/i', $value))
            {
                return true;
            }
 
            return false;
        }
 
        private function alphaNumeric($key, $value, $ruleValue)
        {
            if(preg_match('/^([a-z0-9])+$/i', $value))
            {
                return true;
            }
 
            return false;
        }
 
        private function exactLength($key, $value, $ruleValue)
        {
            if(strlen($value) == $ruleValue)
            {
                return true;
            }
 
            return false;
        }
 
        private function greaterThan($key, $value, $ruleValue)
        {
            if(preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $value))
            {
                if($value > $ruleValue)
                {
                    return true;
                }
            }
 
            return false;
        }
 
        private function lessThan($key, $value, $ruleValue)
        {
            if(preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $value))
            {
                if($value < $ruleValue)
                {
                    return true;
                }
            }
 
            return false;
        }
 
        private function match($key, $value, $ruleValue)
        {
            if($value == $ruleValue)
            {
                return true;
            }
 
            return false;
        }
 
        private function maxLength($key, $value, $ruleValue)
        {
            if(strlen($value) <= $ruleValue)
            {
                return true;
            }
 
            return false;
        }
 
        private function minLength($key, $value, $ruleValue)
        {
            if(strlen($value) >= $ruleValue)
            {
                return true;
            }
 
            return false;
        }
 
        private function numeric($key, $value, $ruleValue)
        {
            if(preg_match('/^[\-+]?[0-9]*\.?[0-9]+$/', $value))
            {
                return true;
            }
 
            return false;
        }
 
        private function required($key, $value, $ruleValue)
        {
            if(!empty($value))
            {
                return true;
            }
            return false;
        }
 
        private function validEmail($key, $value, $ruleValue)
        {
            if(preg_match("/^([a-z0-9\+_\-]+)(\.[a-z0-9\+_\-]+)*@([a-z0-9\-]+\.)+[a-z]{2,6}$/ix", $value))
            {
                return true;
            }
 
            return false;
        }
 
        private function validURL($key, $value, $ruleValue)
        {
            if(preg_match('/^(https?:\/\/)?([\da-z\.-]+)\.([a-z\.]{2,6})([\/\w \.-]*)*\/?$/', $value))
            {
                return true;
            }
 
            return false;
        }
 
    }
?>

Zijn daarnaast ook nog wel wat dingetjes die beter kunnen. Zoals het gebruik van ctype_ en filter_var ipv pregmatches etc. Niet heel grote issues maar naar mijn mening net wat netter.

wat is hier het voordeel van?

Zoals jullie zien heb ik er echt totaal geen van verstand van. Wil wel dus een nieuwe website beginnen.

**Raymond Nijland** · 18-04-2013, 17:21

De regex zijn niet veilig zijn te bypassen door %0a te gebruiken.
De regex moet nog een /D optie hebben om het veilig te maken.
En laat %0a nu net de new line wezen zodat je meerdere emailadressen zou kunnen injecteren bij bijvoorbeeld een contact pagina

Bovendien zijn er meer factoren van belang van een veilige webapplicatie

Session fixation
Session riding
Cross site scripting
Sql injections
Enzovoorts.....

Op basis van dat kleine stukje code kan niemand zeggen of een programmeur de applicatie veilig programmeerd

**gast10494** · 18-04-2013, 18:09

Raymond,

Dank je voor je uitleg. Zal het eens voorleggen aan scripter en kijken hoe hij hierop reageert.

**Raymond Nijland** · 18-04-2013, 19:03

Dat is natuurlijk altijd een goed idee.
Programmeurs zijn goed te vinden. Goede programmeurs zijn minder goed te vinden en Goede programmeurs welke veilig programmeren zijn zeldzaam.

Vaak heeft dit te maken met tutorials welke beginnende programmeurs mee starten zonder goed te begrijpen wat de code precies inhoud.
Ben daarom ook van mening dat 8 van de 10 websites welke hier worden aanboden ook lekken bevatten.

Is deze code netjes/ veilig?