werkte eerst wel nu niet ????
ik heb een upload script voor foto-albums
deze werkt helemaal top, tenminste totdat mijn klant mij gister opbelde en zegt dat de foto's niet werken
dus ik gaan checken en inderdaad het werkt niet
de upload werkt wel
echter zet hij de gegevens niet meer door naar de database
en naar mijn weten is hier niks aan veranderd
het deleten van foto's uit de database enzo werkt wel via zelfde script
upload deel
Code (php)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
protected function handle_file_upload($uploaded_file, $name, $size, $type, $error,
$index = null, $content_range = null) {
$file = parent::handle_file_upload(
$uploaded_file, $name, $size, $type, $error, $index, $content_range
);
if (empty($file->error)) {
$sql = 'INSERT INTO `'.$this->options['db_table']
.'` (`account`, `users__id`, `filename`, `mainimage`)'
.' VALUES ('.$this->options['account'].','.$this->options['id'].', ?, ?)';
$query = $this->db->prepare($sql);
$query->bind_param(
'si',
$file->name,
$file->mainimage
);
$query->execute();
$file->id = $this->db->insert_id;
}
return $file;
}
$index = null, $content_range = null) {
$file = parent::handle_file_upload(
$uploaded_file, $name, $size, $type, $error, $index, $content_range
);
if (empty($file->error)) {
$sql = 'INSERT INTO `'.$this->options['db_table']
.'` (`account`, `users__id`, `filename`, `mainimage`)'
.' VALUES ('.$this->options['account'].','.$this->options['id'].', ?, ?)';
$query = $this->db->prepare($sql);
$query->bind_param(
'si',
$file->name,
$file->mainimage
);
$query->execute();
$file->id = $this->db->insert_id;
}
return $file;
}
delete deel
Code (php)
1
2
3
4
5
6
7
8
9
10
11
12
2
3
4
5
6
7
8
9
10
11
12
public function delete($print_response = true) {
$response = parent::delete(false);
foreach ($response as $name => $deleted) {
if ($deleted) {
$sql = 'DELETE FROM '.$this->options['db_table'].' WHERE users__id='.$this->options['id'].' AND account='.$this->options['account'].' AND filename=?';
$query = $this->db->prepare($sql);
$query->bind_param('s', $name);
$query->execute();
}
}
return $this->generate_response($response, $print_response);
}
$response = parent::delete(false);
foreach ($response as $name => $deleted) {
if ($deleted) {
$sql = 'DELETE FROM '.$this->options['db_table'].' WHERE users__id='.$this->options['id'].' AND account='.$this->options['account'].' AND filename=?';
$query = $this->db->prepare($sql);
$query->bind_param('s', $name);
$query->execute();
}
}
return $this->generate_response($response, $print_response);
}
Eerste gedachte: de query die de insert doet, mislukt.
Wat is de foutmelding die er komt bij de insert.
Als geen: wordt de query wel uitgevoerd? Dat is een uitgebreidere zoektocht, dus ik zou eerst eens foutafhandeling rond $query->execute() toevoegen.
Reden zou kunnen zijn dat de auto-increment aan zijn einde zit (bijvoorbeeld als je een tinyint voor een id-kolom hanteert)
ik heb deze er tijdelijk maar even uitgehaald nu werkt het wel weer
Daarnaast, je maakt hier wel een hele rare spagaat door de helft van je DATA gewoon te concateneren in je SQL-string.
Nu zou je zeggen "ja maar dat zijn opties die uit het systeem komen, en dus zijn ze veilig" ofzo. Dat klopt misschien wel, maar iedereen die ooit die code ziet zal zich daarvan moeten vergewissen. Je laat mensen dus onnodig nadenken over query-veiligheid, terwijl prepared statements hier juist voor bedoeld zijn!
En wat als je straks een twijfelgevalletje hebt: een waarde die van een externe bron afkomstig is, maar gevalideerd is en dus veilig is (en daarbij, hoe verschilt dat dan precies van de bovenstaande $this->options)? Neem je deze gewoon in de SQL op? Of toch via een parameter? Maar wat houdt dat dan in als je deze gewoon concateneert, maakt dit dan prepared statements niet helemaal overbodig als je alles gewoon fatsoenlijk vantevoren valideert?
Op dit moment zit er sowieso geen lijn in wat je doet. Ik zou je dringend aanraden om dit te fixen en hierbij gewoon af te stappen van prepared statements in MySQLi. Bij gebrek aan eigen inspiratie doe een van de twee dingen:
- stap over naar PDO en gebruik daar prepared statements (maar dan dus consequent op de goede manier)
- gebruik MySQLi en escape de DATA-delen met real_escape_string() in combinatie met quotes
En wat @Ivo dus zegt, je hebt een manier nodig om foutmeldingen met queries te detecteren en/of af te handelen. Je zou dus sowieso veel profijt hebben van een soort van database-wrapper. Je kunt op die manier veel sneller fouten (met queries) localiseren.
Nu ben je waarschijnlijk nog geen biet wijzer, want in wezen zeg je "het ging fout, en ik weet niet waarom, en nu gaat het weer goed, en ik weet nog steeds niet waarom". Het is jouw taak als programmeur om dit uit te zoeken om herhaling te voorkomen. Code besluit niet zomaar om op het ene moment wel te werken en het andere moment niet. Wat dat betreft is code heel makkelijk: het doet *precies* wat het is voorgeschreven om te doen.
nee maar je hebt wel gelijk
die prepared statements saten al in het oorspronkelijke login script waar ik 10 jaar mee ben begonnen
voor mysqli gebruikte ik dit niet maar ik kon geen goede werkende manier vinden die voor all mijn paginas werkte en dus heb ik dit gebruikt
ik moet ook een keer het geheel op de schop gooien en nieuw maken
echter is dit niet zo makkelijk gedaan als gezegd
mijn script is om het even heel bot te zeggen jarenlange ideeen scripten en aanpassingen in een werkende bedrijfs omgeving
het zijn serieus misschien wel meer dan 2000 files
ik zou het op dit moment ook niet echt weten
hele boekhoudingen worden met mijn gehele systeem gedaan
POS systemen
verschillende dynamische websites die allemaal met elkaar samenwerken
het is misschien verouderde code maar het werkt en ik verbeter elke dag
en het meeste werkt perfect hoor maar het heeft zeker wel verbetering nodig of eigelijk
geheel nieuw
maar ik kan niet zomaar even een nieuwe maken omdat ik daarvoor een werkende omgeving nodig hebt
ik test veel voordat ik iets online zet maar als scripter kan je ook niet altijd alles voorzien
daar huur ik soms beta testers voor in omdat ik het zelf in mijn 1tje gewoon niet altijd alleen kan doen
bedoeling is dat ik over een jaar een geheel nieuw script ga maken
het bestaande script moet dan eerst helemaal zijn hoe ik het wil
en daarna ga ik het gehele script herschrijven met moderne code
waarschijnlijk moet ik dan een heel team inhuren
want het is echt heeeeeel veel werk
en het moet allemaal dus werken op de oude database zonder ook maar 1 foutje
1 foutje betekent direct faliet voor 10+ bedrijven
1 uur offline kan duizenden euros kosten
om die reden wacht ik dus nog even tot ik zeker weet dat alles perfect is in het oude systeem
en ik denk dat ik bijna zover ben maar soms maak ik snelle aanpassingen in niet noodzakelijke delen en dan krijg ik dus dit soort errors
ach het werkt allemaal weer gelukkig daar gaat het om
Sylvester vader op 11/09/2019 19:28:15:
ik moet ook een keer het geheel op de schop gooien en nieuw maken
Mja, maar met een database-wrapper of als je dit met een uitgebreidere class nog verder uit code abstraheert is dat dus niet nodig. Je probleem op dit moment wordt grotendeels veroorzaakt door hard coding. Beschouw het INSERT-statement uit je oorspronkelijke bericht.
Voorheen gebruikte je waarschijnlijk:
Code (php)
1
mysql_query(...)
Toen schakelde je over op MySQLi:
Code (php)
1
mysqli_query(...)
En toen had je waarschijnlijk gehoord dat prepared statements de bom was en deed je dit:
Code (php)
1
2
3
2
3
$st = $this->db->prepare(...);
$st->bind_param(...);
$st->execute(...);
$st->bind_param(...);
$st->execute(...);
NB: de naamgeving die jij gebruikt ($query) is niet echt fantastisch. prepare() retourneert een statement object of false. Geef variabelen zelfdocumenterende namen.
Anyhow, volgende keer is het waarschijnlijk weer een andere flavor of the month.
Wat je tot nu toe functioneel hebt gedaan is echter nooit veranderd: je INSERT nog steeds een record in een tabel. Dan je beklag dat er zoveel bestanden zouden zijn waarin je dit soort dingen zou moeten aanpassen. Sja, als je nooit nadenkt hoe je dit op een indirecte manier voor elkaar krijgt dan zul je dus inderdaad elke keer alle bestanden in moeten duiken om deze hard coded meuk aan te passen.
Maar wat nu als je eens gaat nadenken over hoe je dit wel op een indirecte manier regelt waarbij je dus hard coding vermijdt? Je zou bijvoorbeeld een soort van INSERT-methode kunnen maken voor je database/informatiedrager (wat deze ook moge zijn, dit abstraheer je uit je code). Het daadwerkelijk uitvoeren zit dan in de implementatie van deze code, en niet meer in de lopende code van deze "scripts".
Super simpel voorbeeld:
Code (php)
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
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
<?php
class MyDB {
protected $db;
public function __construct($options) {
$this->db = new mysqli(...); // use $options
$this->db->set_charset(...); // preferred charset
}
public function insert($table, $args) {
$sql = 'INSERT INTO '.$table.' (';
$sql .= implode(', ', array_keys($args));
$sql .= ') VALUES (';
$sql .= implode(', ', $this->quote($args));
$sql .= ')';
// echo '<pre>'.$sql.'</pre>';
$this->db->query($sql);
}
public function insertId() {
return $this->db->insert_id;
}
protected function quote($args, $escape=true) {
$out = array();
foreach ($args as $v) {
$out[] = "'".($escape ? $this->escape($v) : $v)."'";
}
return $out;
}
public function escape($arg) {
return $this->db->real_escape_string($arg);
}
}
?>
class MyDB {
protected $db;
public function __construct($options) {
$this->db = new mysqli(...); // use $options
$this->db->set_charset(...); // preferred charset
}
public function insert($table, $args) {
$sql = 'INSERT INTO '.$table.' (';
$sql .= implode(', ', array_keys($args));
$sql .= ') VALUES (';
$sql .= implode(', ', $this->quote($args));
$sql .= ')';
// echo '<pre>'.$sql.'</pre>';
$this->db->query($sql);
}
public function insertId() {
return $this->db->insert_id;
}
protected function quote($args, $escape=true) {
$out = array();
foreach ($args as $v) {
$out[] = "'".($escape ? $this->escape($v) : $v)."'";
}
return $out;
}
public function escape($arg) {
return $this->db->real_escape_string($arg);
}
}
?>
Nu, het enige wat in je "lopende code" staat is:
Code (php)
Als er nu iets in database-land verandert hoef je enkel de IMPLEMENTATIE van de insert() methode aan te passen, en hoef je echt helemaal 0,0 te veranderen in al die 2000 bestanden zelf.
Laat dit ff bezinken.
Ik denk dat hiermee al vrij duidelijk is dat het gebruik van een wrapper gigantische voordelen heeft ten opzichte van alles maar rauw in je code duwen...
Sylvester vader op 11/09/2019 19:28:15:
ach het werkt allemaal weer gelukkig daar gaat het om
Simpelweg omdat iets werkt maakt het nog niet juist.
en inderdaad eerst gebruikte ik mysql_query(...)
echter toen had ik dus al die prepared ergens in het toen login script die ik toen had (wist toen nog niet eens dat dat kon op die manier
toen ik over moest naar mysqli_query(...)
kwam ik erachter dat ik er een db connectie bij moest doen en dit was vroeger met
mysql_query(...) niet nodig
echter omdat ik dus zoveel code had in heel veel paginas lukte het mij niet om dit op normale manier aan te passen doormiddel van een bulk edit
toen vond ik dus die manier van prepared en daarmee kon het wel
probleem is dus dat ik zelf alles moet leren en dus helaas nogsteeds niet alles kan
jij hebt mij nu dit geleerd met dat ene stukje en het klinkt eigenlijk heel simpel (weer iets geleerd) dus in nieuwe versie zal ik het inderdaad waarschijnlijk zo doen :)
dus bij deze bedankt voor de kleine maar wijze les
Thomas van den Heuvel op 11/09/2019 16:11:51:
Nu zou je zeggen "ja maar dat zijn opties die uit het systeem komen, en dus zijn ze veilig" ofzo. Dat klopt misschien wel, maar iedereen die ooit die code ziet zal zich daarvan moeten vergewissen. Je laat mensen dus onnodig nadenken over query-veiligheid, terwijl prepared statements hier juist voor bedoeld zijn!
Nu zou je zeggen "ja maar dat zijn opties die uit het systeem komen, en dus zijn ze veilig" ofzo. Dat klopt misschien wel, maar iedereen die ooit die code ziet zal zich daarvan moeten vergewissen. Je laat mensen dus onnodig nadenken over query-veiligheid, terwijl prepared statements hier juist voor bedoeld zijn!
Ik heb nog wel een voorbeeldje waarom "de waarde komt uit het systeem en is dus veilig" niet opgaat:
Ik heb ooit een bug moeten vinden waarom 1 medewerker nooit een order kon afhandelen, terwijl de rest dat prima kon doen.
Uiteindelijk bleek, dat in de logging de naam van de medewerker geplaatst werd. Was mooier geweest als dat het gebruikers_id was geweest, maar a la.
Deze naam was "veilig" volgens bovenstaande redenatie, maar omdat meneer iets van D'Artagnan heette, ging de query grandioos mis.
altijd leuk in text delen in een script met een soort wysiwyg script
als je dan in de text een quote zette dan voerde hij dus de query niet uit :P
dat heb ik toen gelijk aangepast want dat hielt ook in dat er een inject mogelijk was
maar in het begin snapte ik er niks van :P
Leuke die ik in het verleden tegen kwam:
Code (php)
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
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
if((function_exists("get_magic_quotes_gpc") && get_magic_quotes_gpc()) || (ini_get('magic_quotes_sybase') && (strtolower(ini_get('magic_quotes_sybase'))!="off")) ){
stripslashes_deep($_GET);
stripslashes_deep($_POST);
stripslashes_deep($_COOKIE);
}
$locatie = $_SERVER['REQUEST_URI'];
$array = Array();
$array[] = "mysql";
$array[] = "query";
$array[] = ";";
$array[] = "}";
$array[] = "INSERT";
$array[] = "%40";
$array[] = "DROPTABLE";
$array[] = "TRUNCATE";
$array[] = "UPDATE";
$array[] = "DELETE";
$array[] = "WHERE";
foreach($array As $foutbezig)
{
if(eregi($foutbezig,$locatie))
{
echo "Waarschijnlijke SQL injectie! Probeer opnieuw.";
exit();
}
}
stripslashes_deep($_GET);
stripslashes_deep($_POST);
stripslashes_deep($_COOKIE);
}
$locatie = $_SERVER['REQUEST_URI'];
$array = Array();
$array[] = "mysql";
$array[] = "query";
$array[] = ";";
$array[] = "}";
$array[] = "INSERT";
$array[] = "%40";
$array[] = "DROPTABLE";
$array[] = "TRUNCATE";
$array[] = "UPDATE";
$array[] = "DELETE";
$array[] = "WHERE";
foreach($array As $foutbezig)
{
if(eregi($foutbezig,$locatie))
{
echo "Waarschijnlijke SQL injectie! Probeer opnieuw.";
exit();
}
}
Dit was bedoeld voor een stationsdatabase van iemand. Laat er toevallig ook een station bestaan wat Purmerend OverWHERE heet ;).. Daar hang je dan.
Gewijzigd op 13/09/2019 17:28:44 door - Ariën -
Quote:
echter omdat ik dus zoveel code had in heel veel paginas lukte het mij niet om dit op normale manier aan te passen doormiddel van een bulk edit
toen vond ik dus die manier van prepared en daarmee kon het wel
toen vond ik dus die manier van prepared en daarmee kon het wel
Ah, de aloude vertrouwde aardappelstempel. Als je dat op die manier moet doen, is dat vast niet de laatste keer dat je dat op die manier moet doen. Verander dus van aanpak.
Quote:
"de waarde komt uit het systeem en is dus veilig"
Even voor de duidelijkheid: die redenatie klopt dus niet! Je mag er nooit vanuit gaan dat als DATA eenmaal in je systeem zit (die nog steeds afkomstig was van gebruikers) ineens veilig zou zijn. Je zult deze dus altijd zonder uitzondering als "onbetrouwbaar" moeten behandelen. Alle DATA zou dus altijd en overal ge-escaped moeten worden, tenzij je een (gedocumenteerde) reden hebt om dit niet te doen.
Dit werkt ook een stuk eenvoudiger: je hoeft niet langer na te denken of iets veilig is of niet (met bijbehorend gevalsonderscheid voor elke individuele case) als je het gewoon altijd als "onveilig" behandelt.
filter input, escape output
overal, altijd.
Gewijzigd op 13/09/2019 17:54:39 door Thomas van den Heuvel