Dit al gecheckt?

https://stackoverflow.com/questions/4365…-value/43651836

Leuk dat je iets met osFighter bezig bent. Het is inmiddels zes jaar geleden dat ik daar aan begonnen ben. Ik weet het niet meer zeker, maar kan je het veld `groups` niet gewoon nullable maken? Of een lege string meesturen? Mogelijk dat dat het probleem oplost.

In de develop branch op de Github repo zit ook een Laravel versie. Die ziet technisch iets 'beter' in elkaar. Tussen aanhalingstekens aangezien beide projecten al jaren niet zijn aangeraakt en daardoor outdated en een verhoogd risico op security issues hebben. Maar hé, wat is daar nieuw aan met dat soort spellen ?

Een default waarde in de database zal inderdaad al helpen.

Verder moet $groups als variable ook worden gevuld anders krijg je daar ook een error op.
Daarnaast moet hij dan ook wel in de query voorkomen. Dus dan zou het zoiets moeten zijn:

$groups = 1;
$query = "INSERT INTO ".TBL_USERS." SET username = :username, password = :password, usersalt = :usersalt, userid = 0, userlevel = $ulevel, email = :email, timestamp = $time, actkey = :token, ip = '$userip', regdate = $time, groups=:groups";
 $stmt = $this->connection->prepare($query);
 if ($stmt->execute(array(':username' => $username, ':password' => $password, ':usersalt' => $usersalt, ':email' => $email, ':token' => $token, ':groups' => $groups))) {
 $items = array(':user' => $this->getLastUserRegisteredId());
 $this->query("INSERT INTO ".TBL_INFO." SET uid = :user", $items);
 $this->query("INSERT INTO ".TBL_TIME." SET uid = :user", $items);
 return true;

NB

Bovenstaande query maakt (waarschijnlijk) gebruik van prepared statements. Een van de doelen hiervan is om indirect DATA in de SQL op te nemen, op een zodanige wijze dat deze niet als SQL geïnterpreteerd kan worden. Dit maakt queries veilig(er) en beschermt ze op deze manier tegen SQL-injectie.

Echter, op het moment dat je rechtstreeks variabelen gaat invoegen in de SQL-string ontstaat het gevaar van SQL-injectie. Je schendt hiermee in wezen de spelregels van prepared statements waarbij je alle DATA apart zou moeten verwerken.

Nu zou je zeggen "ja maar $ulevel, $time en $userip zijn onschadelijk dus hier kan niets mee misgaan." Dat kan misschien wel zo zijn, maar elke keer dat je deze code ziet dat zou je misschien de neiging hebben om dat (nogmaals) te verifiëren. Oftewel, deze code zet je elke keer aan tot denken (of zou dit moeten doen).

Ook zie je nog een ander aspect in het bovenstaande fragment: wanneer gebruik je quotes, wanneer niet? Houdt de afwezigheid van quotes in dat deze bewust zijn weggelaten of toch per ongeluk zijn vergeten? Wat voor implicatie kan dit in sommige gevallen hebben? Kan de query daardoor in bepaalde randgevallen mislopen?

Al deze bovenstaande problemen heb je niet als je simpelweg de spelregels van prepared statements zou volgen. Het lijkt mij een goed ontwerpprincipe om op een zodanige manier code te schrijven dat deze vanwege een bepaalde aanpak al ondubbelzinnig is, maar dan moet je dus wel de regels van deze aanpak aanhouden.

Mijn voorstel zou dan ook zijn om alle variabelen uitsluitend via placeholders in de SQL in te voegen, en niet in een soort van hybride vorm zoals hierboven gebeurt. Dit resulteert in hoofdpijncode die potentieel voor problemen kan zorgen.

En dan nog het volgende: bovenstaand fragment bevat drie queries. Als het de bedoeling is dat deze query-batch in het geheel, of in het geheel niet in de database ingevoegd dient te worden, dan wordt het tijd om (database-)transacties te gaan gebruiken.

Vooral als het uitgebreide "administratieve" systemen betreft, dan is het belangrijk dat de informatie onderling consistent blijft. Transacties garanderen dit.

Als er in een batch queries (bijvoorbeeld A, B, C, D) er in B iets misgaat, dan worden C en D normaal gesproken (zonder gebruikmaking van transacties) mogelijk alsnog uitgevoerd. Dit kan resulteren in ontkoppelde, foutieve of tegenstrijdige informatie in de database.

Als deze serie echter in een transactie zou zitten, en er gaat iets mis in B, dan zou A worden teruggedraaid, en C en D zouden dan nooit worden uitgevoerd.

Uiteraard hangt het er natuurlijk ook vanaf hoe $this->query() intern werkt, mogelijk wordt verdere verwerking dan al afgebroken, maar A zit dan waarschijnlijk permanent in je database.

Misschien mislukt dan als resultaat van het mislukken van B een operatie in het systeem (waarvan een terugkoppeling en melding gegeven zou moeten worden), maar dit heeft dan in ieder geval niet tot gevolg dat je database vervuild raakt met niet-kloppende gegevens.

Dit soort (simpele?) voorzieningen (een juist gebruik van prepared statements, transacties) zou je gewoon moeten volgen. Maar misschien moet dit eerst een keer vreselijk misgaan zodat je echt met de neus op de feiten wordt gedrukt, een gebrande hand is immers de beste leermeester.