From 608b6f5fa32375eacedf368880a07f91c9c26d5e Mon Sep 17 00:00:00 2001 From: Laurent Destailleur Date: Thu, 30 Sep 2021 15:24:57 +0200 Subject: [PATCH] Clean code --- htdocs/accountancy/admin/card.php | 6 +-- htdocs/accountancy/journal/bankjournal.php | 4 +- htdocs/admin/multicurrency.php | 2 +- .../compta/paiement/class/paiement.class.php | 4 +- htdocs/core/class/commoninvoice.class.php | 2 +- htdocs/core/class/commonobject.class.php | 10 ++-- .../webservices/server_supplier_invoice.php | 10 ++-- test/phpunit/CodingPhpTest.php | 49 +++++++++++++++++-- 8 files changed, 61 insertions(+), 26 deletions(-) diff --git a/htdocs/accountancy/admin/card.php b/htdocs/accountancy/admin/card.php index cd48526e62b..4d69eae7db8 100644 --- a/htdocs/accountancy/admin/card.php +++ b/htdocs/accountancy/admin/card.php @@ -75,7 +75,7 @@ if ($action == 'add' && $user->rights->accounting->chartofaccount) { setEventMessages($langs->trans("ErrorFieldRequired", $langs->transnoentities("Label")), null, 'errors'); $action = 'create'; } else { - $sql = 'SELECT pcg_version FROM ' . MAIN_DB_PREFIX . 'accounting_system WHERE rowid='.((int) $conf->global->CHARTOFACCOUNTS); + $sql = "SELECT pcg_version FROM " . MAIN_DB_PREFIX . "accounting_system WHERE rowid = ".((int) $conf->global->CHARTOFACCOUNTS); dol_syslog('accountancy/admin/card.php:: $sql=' . $sql); $result = $db->query($sql); @@ -138,7 +138,7 @@ if ($action == 'add' && $user->rights->accounting->chartofaccount) { } else { $result = $object->fetch($id); - $sql = 'SELECT pcg_version FROM '.MAIN_DB_PREFIX.'accounting_system WHERE rowid='.((int) $conf->global->CHARTOFACCOUNTS); + $sql = "SELECT pcg_version FROM ".MAIN_DB_PREFIX."accounting_system WHERE rowid=".((int) $conf->global->CHARTOFACCOUNTS); dol_syslog('accountancy/admin/card.php:: $sql=' . $sql); $result2 = $db->query($sql); @@ -260,7 +260,7 @@ if ($action == 'create') { print ''; // autosuggest from existing account types if found print ''; - $sql = 'SELECT DISTINCT pcg_type FROM ' . MAIN_DB_PREFIX . 'accounting_account'; + $sql = "SELECT DISTINCT pcg_type FROM " . MAIN_DB_PREFIX . "accounting_account"; $sql .= " WHERE fk_pcg_version = '" . $db->escape($accountsystem->ref) . "'"; $sql .= ' AND entity in ('.getEntity('accounting_account', 0).')'; // Always limit to current entity. No sharing in accountancy. $sql .= ' LIMIT 50000'; // just as a sanity check diff --git a/htdocs/accountancy/journal/bankjournal.php b/htdocs/accountancy/journal/bankjournal.php index d05135e7a67..4367241dbd7 100644 --- a/htdocs/accountancy/journal/bankjournal.php +++ b/htdocs/accountancy/journal/bankjournal.php @@ -362,7 +362,7 @@ if ($result) { // Retrieve the accounting code of the social contribution of the payment from link of payment. // Note: We have the social contribution id, it can be faster to get accounting code from social contribution id. - $sqlmid = 'SELECT cchgsoc.accountancy_code'; + $sqlmid = "SELECT cchgsoc.accountancy_code"; $sqlmid .= " FROM ".MAIN_DB_PREFIX."c_chargesociales cchgsoc"; $sqlmid .= " INNER JOIN ".MAIN_DB_PREFIX."chargesociales as chgsoc ON chgsoc.fk_type = cchgsoc.id"; $sqlmid .= " INNER JOIN ".MAIN_DB_PREFIX."paiementcharge as paycharg ON paycharg.fk_charge = chgsoc.rowid"; @@ -1019,7 +1019,7 @@ if (empty($action) || $action == 'view') { // Test that setup is complete (we are in accounting, so test on entity is always on $conf->entity only, no sharing allowed) - $sql = 'SELECT COUNT(rowid) as nb FROM '.MAIN_DB_PREFIX.'bank_account WHERE entity = '.$conf->entity.' AND fk_accountancy_journal IS NULL AND clos=0'; + $sql = "SELECT COUNT(rowid) as nb FROM ".MAIN_DB_PREFIX."bank_account WHERE entity = ".((int) $conf->entity)." AND fk_accountancy_journal IS NULL AND clos=0"; $resql = $db->query($sql); if ($resql) { $obj = $db->fetch_object($resql); diff --git a/htdocs/admin/multicurrency.php b/htdocs/admin/multicurrency.php index 95b418a75f0..e8619219471 100644 --- a/htdocs/admin/multicurrency.php +++ b/htdocs/admin/multicurrency.php @@ -138,7 +138,7 @@ if ($action == 'add_currency') { $TCurrency = array(); -$sql = 'SELECT rowid FROM '.MAIN_DB_PREFIX.'multicurrency WHERE entity = '.$conf->entity; +$sql = "SELECT rowid FROM ".MAIN_DB_PREFIX."multicurrency WHERE entity = ".((int) $conf->entity); $resql = $db->query($sql); if ($resql) { while ($obj = $db->fetch_object($resql)) { diff --git a/htdocs/compta/paiement/class/paiement.class.php b/htdocs/compta/paiement/class/paiement.class.php index 3e1dc59a52e..aab54e4ef81 100644 --- a/htdocs/compta/paiement/class/paiement.class.php +++ b/htdocs/compta/paiement/class/paiement.class.php @@ -304,8 +304,8 @@ class Paiement extends CommonObject $facid = $key; if (is_numeric($amount) && $amount <> 0) { $amount = price2num($amount); - $sql = 'INSERT INTO '.MAIN_DB_PREFIX.'paiement_facture (fk_facture, fk_paiement, amount, multicurrency_amount)'; - $sql .= ' VALUES ('.((int) $facid).', '.((int) $this->id).", ".((float) $amount).", ".((float) $this->multicurrency_amounts[$key]).')'; + $sql = "INSERT INTO ".MAIN_DB_PREFIX."paiement_facture (fk_facture, fk_paiement, amount, multicurrency_amount)"; + $sql .= " VALUES (".((int) $facid).", ".((int) $this->id).", ".((float) $amount).", ".((float) $this->multicurrency_amounts[$key]).")"; dol_syslog(get_class($this).'::create Amount line '.$key.' insert paiement_facture', LOG_DEBUG); $resql = $this->db->query($sql); diff --git a/htdocs/core/class/commoninvoice.class.php b/htdocs/core/class/commoninvoice.class.php index dfabfd24ebe..9bd17c77ea5 100644 --- a/htdocs/core/class/commoninvoice.class.php +++ b/htdocs/core/class/commoninvoice.class.php @@ -734,7 +734,7 @@ abstract class CommonInvoice extends CommonObject $sql .= 'fk_facture, '; } $sql .= ' amount, date_demande, fk_user_demande, code_banque, code_guichet, number, cle_rib, sourcetype, entity)'; - $sql .= ' VALUES ('.((int) $this->id); + $sql .= " VALUES (".((int) $this->id); $sql .= ", ".((float) price2num($amount)); $sql .= ", '".$this->db->idate($now)."'"; $sql .= ", ".((int) $fuser->id); diff --git a/htdocs/core/class/commonobject.class.php b/htdocs/core/class/commonobject.class.php index 3c95d9c26a0..ca3cc7ad39d 100644 --- a/htdocs/core/class/commonobject.class.php +++ b/htdocs/core/class/commonobject.class.php @@ -8682,7 +8682,7 @@ abstract class CommonObject // If field is an implicit foreign key field if (preg_match('/^integer:/i', $this->fields[$key]['type']) && empty($values[$key])) { if (isset($this->fields[$key]['default'])) { - $values[$key] = $this->fields[$key]['default']; + $values[$key] = ((int) $this->fields[$key]['default']); } else { $values[$key] = 'null'; } @@ -8699,9 +8699,9 @@ abstract class CommonObject $this->db->begin(); if (!$error) { - $sql = 'INSERT INTO '.MAIN_DB_PREFIX.$this->table_element; - $sql .= ' ('.implode(", ", $keys).')'; - $sql .= ' VALUES ('.implode(", ", $values).')'; + $sql = "INSERT INTO ".MAIN_DB_PREFIX.$this->table_element; + $sql .= " (".implode(", ", $keys).')'; + $sql .= " VALUES (".implode(", ", $values).")"; // $values can contains 'abc' or 123 $res = $this->db->query($sql); if ($res === false) { @@ -8717,7 +8717,7 @@ abstract class CommonObject // If we have a field ref with a default value of (PROV) if (!$error) { if (key_exists('ref', $this->fields) && $this->fields['ref']['notnull'] > 0 && key_exists('default', $this->fields['ref']) && $this->fields['ref']['default'] == '(PROV)') { - $sql = "UPDATE ".MAIN_DB_PREFIX.$this->table_element." SET ref = '(PROV".$this->id.")' WHERE (ref = '(PROV)' OR ref = '') AND rowid = ".((int) $this->id); + $sql = "UPDATE ".MAIN_DB_PREFIX.$this->table_element." SET ref = '(PROV".((int) $this->id).")' WHERE (ref = '(PROV)' OR ref = '') AND rowid = ".((int) $this->id); $resqlupdate = $this->db->query($sql); if ($resqlupdate === false) { diff --git a/htdocs/webservices/server_supplier_invoice.php b/htdocs/webservices/server_supplier_invoice.php index 6803133d527..926fa8f6418 100644 --- a/htdocs/webservices/server_supplier_invoice.php +++ b/htdocs/webservices/server_supplier_invoice.php @@ -350,13 +350,9 @@ function getSupplierInvoicesForThirdParty($authentication, $idthirdparty) if (!$error) { $linesinvoice = array(); - $sql .= 'SELECT f.rowid as facid'; - $sql .= ' FROM '.MAIN_DB_PREFIX.'facture_fourn as f'; - //$sql.=', '.MAIN_DB_PREFIX.'societe as s'; - //$sql.= ' LEFT JOIN '.MAIN_DB_PREFIX.'product as p ON pt.fk_product = p.rowid'; - //$sql.=" WHERE f.fk_soc = s.rowid AND nom = '".$db->escape($idthirdparty)."'"; - //$sql.=" WHERE f.fk_soc = s.rowid AND nom = '".$db->escape($idthirdparty)."'"; - $sql .= " WHERE f.entity = ".$conf->entity; + $sql .= "SELECT f.rowid as facid"; + $sql .= " FROM '.MAIN_DB_PREFIX.'facture_fourn as f"; + $sql .= " WHERE f.entity = ".((int) $conf->entity); if ($idthirdparty != 'all') { $sql .= " AND f.fk_soc = ".((int) $idthirdparty); } diff --git a/test/phpunit/CodingPhpTest.php b/test/phpunit/CodingPhpTest.php index 21a58f34159..414daa21632 100644 --- a/test/phpunit/CodingPhpTest.php +++ b/test/phpunit/CodingPhpTest.php @@ -250,7 +250,6 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase $ok=true; $matches=array(); - // Check string get_class... preg_match_all('/'.preg_quote('get_class($this)."::".__METHOD__', '/').'/', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { $ok=false; @@ -260,9 +259,9 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase $this->assertTrue($ok, 'Found string get_class($this)."::".__METHOD__ that must be replaced with __METHOD__ only in '.$file['relativename']); //exit; + // Check string $this->db->idate without quotes $ok=true; $matches=array(); - // Check string $this->db->idate without quotes preg_match_all('/(..)\s*\.\s*\$this->db->idate\(/', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { if ($val[1] != '\'"' && $val[1] != '\'\'') { @@ -276,11 +275,10 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase //exit; - $ok=true; - $matches=array(); - // Check sql string DELETE|OR|AND|WHERE|INSERT ... yyy = ".$xxx // with xxx that is not 'thi' (for $this->db->sanitize) and 'db-' (for $db->sanitize). It means we forget a ' if string, or an (int) if int, when forging sql request. + $ok=true; + $matches=array(); preg_match_all('/(DELETE|OR|AND|WHERE|INSERT)\s.*([^\s][^\s][^\s])\s*=\s*"\s*\.\s*\$(...)/', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { if ($val[2] == 'ity' && $val[3] == 'con') { // exclude entity = ".$conf->entity @@ -300,8 +298,39 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase $this->assertTrue($ok, 'Found non quoted or not casted var into sql request '.$file['relativename'].' - Bad.'); //exit; + // Check that forged sql string is using " as string PHP quotes + $ok=true; + $matches=array(); + preg_match_all('/\$sql \.= \'\s*VALUES.*\$/', $filecontent, $matches, PREG_SET_ORDER); + foreach ($matches as $key => $val) { + //if ($val[1] != '\'"' && $val[1] != '\'\'') { + var_dump($matches); + $ok=false; + break; + //} + //if ($reg[0] != 'db') $ok=false; + } + //print __METHOD__." Result for checking we don't have non escaped string in sql requests for file ".$file."\n"; + $this->assertTrue($ok, 'Found a forged SQL string that mix on same line the use of \' for PHP string and PHP variables into file '.$file['relativename'].' Use " to forge PHP string like this: $sql = "SELET ".$myvar...'); + //exit; + + // Check that forged sql string is using " as string PHP quotes + /* + $ok=true; + $matches=array(); + preg_match_all('/\$sql \.*= \'SELECT.*\$/', $filecontent, $matches, PREG_SET_ORDER); + foreach ($matches as $key => $val) { + var_dump($matches); + $ok=false; + break; + } + $this->assertTrue($ok, 'Found a forged SQL string that mix on same line the use of \' for PHP string and PHP variables into file '.$file['relativename'].' Use " to forge PHP string like this: $sql = "SELET ".$myvar...'); + */ + // Check sql string VALUES ... , ".$xxx // with xxx that is not 'db-' (for $db->escape). It means we forget a ' if string, or an (int) if int, when forging sql request. + $ok=true; + $matches=array(); preg_match_all('/(VALUES).*,\s*"\s*\.\s*\$(...)/', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { if ($val[1] == 'VALUES' && $val[2] == 'db-') { // exclude $db->escape( @@ -321,6 +350,8 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase // Check '".$xxx non escaped // Check string ='".$this->xxx with xxx that is not 'escape'. It means we forget a db->escape when forging sql request. + $ok=true; + $matches=array(); preg_match_all('/=\s*\'"\s*\.\s*\$this->(....)/', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { if ($val[1] != 'db->' && $val[1] != 'esca') { @@ -332,6 +363,8 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase $this->assertTrue($ok, 'Found non escaped string in building of a sql request (case 1) in '.$file['relativename'].' - Bad.'); // Check string sql|set...'".$yyy->xxx with xxx that is not 'escape', 'idate', .... It means we forget a db->escape when forging sql request. + $ok=true; + $matches=array(); preg_match_all('/(sql|SET|WHERE|INSERT|VALUES).+\s*\'"\s*\.\s*\$(.........)/', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { if (! in_array($val[2], array('this->db-', 'this->esc', 'db->escap', 'dbs->esca', 'mydb->esc', 'dbsession', 'db->idate', 'escapedli', 'excludeGr', 'includeGr'))) { @@ -345,6 +378,8 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase //exit; // Check string sql|set...'.$yyy->xxx with xxx that is not 'escape', 'idate', .... It means we forget a db->escape when forging sql request. + $ok=true; + $matches=array(); preg_match_all('/(\$sql|SET\s|WHERE\s|INSERT\s|VALUES\s|VALUES\().+\s*\'\s*\.\s*\$(.........)/', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { if (! in_array($val[2], array('this->db-', 'db->sanit', 'conf->ent', 'key : \'\')', 'key])."\')', 'excludefi', 'regexstri', ''))) { @@ -361,6 +396,8 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase // Checks with IN // Check string 'IN (".xxx' or 'IN (\'.xxx' with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request. + $ok=true; + $matches=array(); preg_match_all('/ IN \([\'"]\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { if (!in_array($val[1], array('$db->sani', '$this->db', 'getEntity', 'WON\',\'L', 'self::STA', 'Commande:', 'CommandeF', 'Entrepot:', 'Facture::', 'FactureFo', 'ExpenseRe', 'Societe::', 'Ticket::S'))) { @@ -374,6 +411,8 @@ class CodingPhpTest extends PHPUnit\Framework\TestCase //exit; // Check string 'IN (\'".xxx' with xxx that is not '$this->db->sanitize' and not '$db->sanitize'. It means we forget a db->sanitize when forging sql request. + $ok=true; + $matches=array(); preg_match_all('/ IN \(\'"\s*\.\s*(.........)/i', $filecontent, $matches, PREG_SET_ORDER); foreach ($matches as $key => $val) { if (!in_array($val[1], array('$db->sani', '$this->db', 'getEntity', 'WON\',\'L', 'self::STA', 'Commande:', 'CommandeF', 'Entrepot:', 'Facture::', 'FactureFo', 'ExpenseRe', 'Societe::', 'Ticket::S'))) {