[PHP-MySql] gegevens verwijderen uit database

[Verwijderd]

Hoi, ik ben bezig met een cms voor een voetbalsite. Nu ben ik al zover dat de admin zelf berichten toe kan voegen, ik wil echter ook berichten laten verwijderen. Ik heb de volgende code hiervoor:

PHP:

$Id = $_GET["Id"];
if ($GET["actie"] == "wis" && $Id) {
$sql = "DELETE FROM dames_nieuws WHERE Id=$Id";
if (!mysql_query($sql)) {
echo "Verwijderen nieuws mislukt!";
exit;
} 
echo "OK, uw nieuwsbericht is verwijderd.";
} else {
?>
<p><b>Nieuws wissen</b><br>
Wis het volgende nieuwsbericht:<br>
<?php
$sql = "SELECT * FROM dames_nieuws";
$resultaat=mysql_query($sql);
while ($rij=mysql_fetch_array($resultaat)) {
$Id = $rij["Id"];
echo $rij["Bericht"]." <a href=\"".$_SERVER["PHP_SELF"]."?actie=wis&Id=$Id\" onClick=\"return Confirm(1)\">wis</a><br>\n";
}
}
?>

Id en Bericht zijn onder andere velden in de tabel dames_nieuws (naast Datum en Afbeelding).

Als ik nu verwijs naar niews-wissen.php, bijv. http://.../nieuws-wissen.php?actie=wis&Id=5, dan krijg ik gelijk alle nieuwsberichten met daarachter Wis. Ook als ik dan op Wis klik gebeurt er niks.
Waar gaat het fout?

[ Voor 0% gewijzigd door dusty op 23-08-2007 03:07 . Reden: php tags toegevoegd ]

[Verwijderd]

Die ene $GET moet zoiezo $_GET zijn.

[GlowMouse]

Verwijderd schreef op woensdag 22 augustus 2007 @ 13:27:
Id en Bericht zijn onder andere velden in de tabel dames_nieuws (naast Datum en Afbeelding).

Als ik nu verwijs naar niews-wissen.php, bijv. http://.../nieuws-wissen.php?actie=wis&Id=5, dan krijg ik gelijk alle nieuwsberichten met daarachter Wis. Ook als ik dan op Wis klik gebeurt er niks.
Waar gaat het fout?

Op deze regel: if ($GET["actie"] == "wis" && $Id) {
$GET["actie"] is waarschijnlijk niet gelijk aan 'wis' ($_GET["actie"] waarschijnlijk wel).

Let trouwens even op wat er gebeurt als iemand voor de lol http://.../nieuws-wissen.php?actie=wis&Id=5%20OR%201=1 opent.

[Verwijderd]

Bedankt... dat zal 'm zijn =)
Ga vanavond gelijk testen!

[DJ Buzzz]

Ik zou als ik jou was ook de GET omschrijven naar een POST... Je hebt nu namelijk een state change in een GET wat nog weleens voor problemen wil zorgen... Zie ook b.v. de W3C recommendation hierover:

http://www.w3.org/2001/tag/doc/whenToUseGet.html

[Janoz]

De code wordt een stuk leesbaarder waneer je dit tussen [php] -tags zet. De code wordt dan ook een stuk leesbaarder.

Dat je gelijk je gegevens krijgt betekent dat ($GET["actie"] == "wis" && $Id) niet geldt. Daar zou je je op moeten focussen.

Tot slot nog enkele opmerkingen over je applicatie: NOOIT dergelijke admin acties middels get doen. Het is dan heel makkelijk om de boel te lopen hacken. Verder doet de tabel 'nieuws_dames' vermoeden dat je database ontwerp ook niet optimaal is. Data zou je niet op moeten nemen in je definitie.


---

Edit: Hmmm, in het vervolg eerst posten en dan bellen. Niet andersom.

[ Voor 7% gewijzigd door Janoz op 22-08-2007 13:40 ]

[Verwijderd]

Janoz schreef op woensdag 22 augustus 2007 @ 13:39:
Verder doet de tabel 'nieuws_dames' vermoeden dat je database ontwerp ook niet optimaal is. Data zou je niet op moeten nemen in je definitie.

De database bestaat tot nog toe uit slechts één tabel. Het is ook de bedoeling dat het hierbij blijft (dus dat de admin van de club alleen het nieuws zelf toevoegt en evt. wist).

Na aanpassing van $GET in $_GET werkt het. Ik zal me nu verdiepen in het "vervangen" van $_GET door $_POST.

[Cartman!]

Janoz bedoelt dat nieuws_dames suggereert dat er ook een nieuws_heren is of zal komen en dat dat niet de bedoeling is. Neem gewoon dan een tabel nieuws en geef het type aan in een veld daarin (dus bijv. dames of heren dmv een SET).

Vervangen van $_GET door $_POST zie ik niet helemaal overigens. Ik neem aan dat de pagina zelf ook nog checkt dat de user ingelogd is en $_POST kun je ook gewoon 'hacken' door lokaal een file te draaien en te posten naar die url.

[AndriesLouw]

En dan ook gelijk even kijken of het ID echt wel klopt:

PHP:

<?php
if ($_GET['actie'] == 'wis' && !empty($_GET['id']) && ctype_digit($_GET['id'])){
    //Wissen
}
?>

empty controleert of de string leeg is.
ctype_digit controleert of de string alleen maar uit getallen bestaat.

[FragFrog]

Cartman! schreef op woensdag 22 augustus 2007 @ 22:32:
Vervangen van $_GET door $_POST zie ik niet helemaal overigens. Ik neem aan dat de pagina zelf ook nog checkt dat de user ingelogd is en $_POST kun je ook gewoon 'hacken' door lokaal een file te draaien en te posten naar die url.

Het is gewoon een stukje standaarden waar een goede devver zich aan houdt: POST request zijn er voor statechanges, GET requests om data op te halen. Daar zijn ze voor bedoelt, daar hoor je ze voor te gebruiken.

Anders krijg je bijvoorbeeld dingen als een Google webaccelerator die netjes URL's opvraagt om voor je te cachen waardoor gebruikers van bepaalde websitepaketten ineens spontaan allemaal items deleten, om maar een voorbeeld te geven

Overigens hoef je helemaal niet lokaal een file te draaien om POST waardes aan te passen hoor, FireFox bijvoorbeeld heeft daar gewoon een plugin voor

@hierboven:
Ik zou dan persoonlijk eerder iets als dit doen:

PHP:

$item_id = intval($_GET['id']);
if($item_id > 0 && $_GET['actie'] == 'wis')
  // wis item
else
 // geef error

Stukje persoonlijke voorkeur meer, het houdt imho je if condities overzichtelijker Daarnaast zorgt de intval ervoor dat het bijvoorbeeld niet heel erg is als er bijvoorbeeld een keer een spatie achter staat - het ID is nog steeds een numeriek positief getal. In jou voorbeeld krijg je dan echter wel (semi onterecht) een error.

[ Voor 20% gewijzigd door FragFrog op 23-08-2007 02:19 ]

[Janoz]

Cartman! schreef op woensdag 22 augustus 2007 @ 22:32:
Vervangen van $_GET door $_POST zie ik niet helemaal overigens. Ik neem aan dat de pagina zelf ook nog checkt dat de user ingelogd is en $_POST kun je ook gewoon 'hacken' door lokaal een file te draaien en te posten naar die url.

Ja, jij kunt makkelijk posten naar een url, maar met get kun je iemand anders heel makkelijk wat uit laten voeren. Stel de adminfunctie werkt met get request en ik open op een forum een topic met daarin een plaatje die linkt naar http://site.tld/admin.php...oz&action=makeGlobalAdmin . Ik hoef er dan alleen nog maar voor te zorgen dat een admin daadwerkelijk in dat draadje gaat kijken.


@voutloos: Ik weet wanneer ik GET en POST zou moeten gebruiken (onderdeel van het SCJWCD examen). Wat ik hierboven beschrijf is iets dat Cartman! over het hoofd ziet.

[ Voor 10% gewijzigd door Janoz op 23-08-2007 10:05 ]

[Voutloos]

Janoz, je voorbeeld action voert een wijziging uit en dat is imo de belangrijkste reden om voor POST te kiezen. Als je de keuze tussen GET en POST dus primair van het hebben van side-effects of het al dan idempotent zijn laat afhangen, zul je zien dat het doorgaans ook uit security oogpunt de logische keuze is.

[Verwijderd]

Om het maar even helemaal volledig maken:
Doe een "POST-Redirect-GET". Post dus, zoals duidelijk beschreven, je vars en stuur als response een redirect. Op die manier kun je ook in de browser vrolijk je back en forward knoppen gebruiken zonder dat iemand formulieren opnieuw gaat posten.

[Cartman!]

Janoz schreef op donderdag 23 augustus 2007 @ 09:43:
[...]

Ja, jij kunt makkelijk posten naar een url, maar met get kun je iemand anders heel makkelijk wat uit laten voeren. Stel de adminfunctie werkt met get request en ik open op een forum een topic met daarin een plaatje die linkt naar http://site.tld/admin.php...oz&action=makeGlobalAdmin . Ik hoef er dan alleen nog maar voor te zorgen dat een admin daadwerkelijk in dat draadje gaat kijken.

Ok, dat is een goed punt. Nooit aan gedacht omdat ik het zelf ook nooit op die manier schrijf misschien.
edit: moet je overigens wel exact weten welke variabelen er gebruikt worden in die site...hoe kom je daar dan achter?

[ Voor 8% gewijzigd door Cartman! op 24-08-2007 09:33 ]

[Janoz]

Je komt erachter door de source. Nu is dat bij maatwerk vaak wat lastig (met een beetje slim nadenken en bekijken hoe de rest opgebwoud is kunt je best ver komen), maar bij opensource projecten is dat wat makkelijker. Veel van de UBB hacks maken hiervan gebruik.