Functie veilig?

Jannick

Haihai,
Ik heb hier een functie voor m'n website.. En ik vraag me af of ie veilig genoeg is. Het word gebruikt om geld van contant naar bank te overzetten. Ergens denk ik dat ik wat vergeten ben... Maar wat. Wat zou ik hier nog aan kunnen verbeteren? De meegegeven $id word al gecontroleerd voordat de methode word aangeroepen.

PHP

function cnb($hoeveel, $id)
{
    $sql = mysql_query("SELECT contant FROM users WHERE id = '" . $id ."'");
    $fetch = mysql_fetch_assoc($sql);
    if ($fetch['contant'] < $hoeveel)
    {
        return false;
    }
    else {
        $query = mysql_query("UPDATE users SET bank = bank + $hoeveel WHERE id = '" . $id ."'") or die(mysql_error());
        if ($query)
        {
        $query2 = mysql_query("UPDATE users SET contant = contant - $hoeveel WHERE id = '" . $id ."'") or die(mysql_error());
            if($query2)
            {
                return true;
            }
        }
        else { return false;
        }
    }
}

Meer tonen

Jannick

Edit: Iets in code gefixed

RiiCk

Hallo Jannick,

Na mijn weten is die wel beveiligd genoeg. Ik neem aan dat je er boven over er onder nog wel hebt staan dat ze niet meer van de bank kunnen halen dan dat ze hebben?

Groetjes

EDIT:
Ik zie dat dit er al in staat sorry, Dan lijkt die mij veilig genoeg.

Jannick

Jup, in een ander script word er een variabele aangeroepen die gekoppeld staat aan deze functie. Als die true is dan krijg je dat te zien, als ie false is een melding dat je niet genoeg geld hebt.

Niels

$id is niet beveiligd. Maak er (int) $id van of voer mysql escape real string uit.

RiiCk

Citaat van Niels

$id is niet beveiligd. Maak er (int) $id van of voer mysql escape real string uit.

Zoals jannick zegt:
De meegegeven $id word al gecontroleerd voordat de methode word aangeroepen.

Niels

Dan nog betweter. Waarom zou je extra controles plaatsen als het zo normaal toch echt moet gebeuren. met (int) weet je tenminste zeker dat het een integer is en geen decimaal getal.

Tevens zie je ook niet hoe $id 'gecontroleerd' wordt wel dus hoe kun je dan een klein scriptje bekijken op veiligheid.

Stefan.J

$hoeveel = '0, level = 255';

Denk daar maar eens over na.

Jannick

Heel leuk, maar voordat de functie word aangeroepen worden zowel de ID als valuta gecontroleerd of het numeriek is, anders roept ie 'm nieteens aan... Zou het beter zijn om het binnen de functie te controleren?

Edit:
Wat ik nu heb:

PHP

function cnb((int)$hoeveel, (int)$id)
{
    if(is_numeric($hoeveel)) {
    $sql = mysql_query("SELECT contant FROM users WHERE id = '" . $id ."'");
    $fetch = mysql_fetch_assoc($sql);
    if ($fetch['contant'] < $hoeveel)
    {
        return false;
    }
    else {
        $query = mysql_query("UPDATE users SET bank = bank + $hoeveel WHERE id = '" . $id ."'") or die(mysql_error());
        if ($query)
        {
        $query2 = mysql_query("UPDATE users SET contant = contant - $hoeveel WHERE id = '" . $id ."'") or die(mysql_error());
            if($query2)
            {
                return true;
            }
        }
        else { return false;
        }
    }
    }
}

Meer tonen

het is wel dubbel nu volgens mij, maar goed...

Edit 2: Ok, dat werkte dus niet. *zoekt verder*

Stefan.J

Die verantwoordelijkheid kun je beter binnen de functie leggen ja. Anders zit je als nog alles eerst te valideren, en dan de functie aan te roepen.

Jannick

Jup, heb 't aangepast Die (int) hoort voor de variabelen voordat ze gebruikt worden?, dus niet in bijv. function cnb($hoeveel, $id) maar in if ($fetch['contant'] < (int)$hoeveel) ?

Niels

Citaat van Jannick

Heel leuk, maar voordat de functie word aangeroepen worden zowel de ID als valuta gecontroleerd of het numeriek is, anders roept ie 'm nieteens aan... Zou het beter zijn om het binnen de functie te controleren?

Maar dan nog. Door een FAIL van php kan het altijd mis gaan. filter_var heeft bijvoorbeeld ook nog veel fouten waardoor bijvoorbeeld niet legitieme e-mail adressen toch als TRUE returnen.

Jannick

De controle voor e-mailadressen is volgens mij maximaal, er komen explodes, ereg's etc in voor, maar dat terzijde. Voor de geinteresseerden, ik heb 'm hiervan gebruikt: Klik.
Dit is het script wat ik nu heb:

PHP

function cnb($hoeveel, $id)
{
    if(is_numeric($hoeveel)) {
    $sql = mysql_query("SELECT contant FROM users WHERE id = '" . $id ."'");
    $fetch = mysql_fetch_assoc($sql);
    if ($fetch['contant'] < (int)$hoeveel)
    {
        return false;
    }
    else {
        $query = mysql_query("UPDATE users SET bank = bank + $hoeveel WHERE id = '" . $id ."'") or die(mysql_error());
        if ($query)
        {
        $query2 = mysql_query("UPDATE users SET contant = contant - $hoeveel WHERE id = '" . $id ."'") or die(mysql_error());
            if($query2)
            {
                return true;
            }
        }
        else { return false;
        }
    }
    }
}

Meer tonen

Darsstar

PHP

function cnb($boeveel, $id)
{
    $id      = (int) $id;
    $hoeveel = abs((int) $hoeveel);


    $query = mysql_query("UPDATE `users` SET `bank` = `bank` + ".$hoeveel.", `contant` = `contant` - ".$hoeveel." WHERE `contant` >= ".$hoeveel." AND `id` = ".$id);
    return (bool) mysql_affected_rows();
}

Waarom zou je PHP dingen laten controleren wanneer MySQL dit ook kan en je ook nog eens een query bespaart?

Jannick

Waar staat die abs() voor?
Nou ja, ben nog redelijk aan het leren en ben nog niet bekend met dat soort dingen
Edit: $query word nergens aangeroepen... Is dat nu ook overbodig geworden?

Darsstar

abs() geeft de absolute waarde van een getal...
-10 wordt 10
10 wordt 10

Jannick

Nice... Dit is nu wat ik heb:

PHP

function cnb($hoeveel, $id)
{
    $id      = (int) $id;
    $hoeveel = abs((int) $hoeveel);


mysql_query("UPDATE `users` SET `bank` = `bank` + ".$hoeveel.", `contant` = `contant` - ".$hoeveel." WHERE `contant` >= ".$hoeveel." AND `id` = ".$id);
    return (bool) mysql_affected_rows();
}

Typfoutje verbeterd ($boeveel). Vraag na aanleiding van je code: Zijn '-haakjes niet nodig in query's? Ik doe altijd '" . $variabele . "', en zie dat jij ze niet gebruikt. Kan je dat overal zo toepassen, en welke voorwaarden zijn er daaraan?

Darsstar

Om strings MOET je quotes zetten...
Als je mysql laat rekenen en het komt een string tegen zal het deze in een integer omzetten... (volgens mij)
Dus waarom zou je er een string van maken als mysql het daarna weer in een integer omzet...

Het "$query = " was inderdaad niet nodig...
Ik dacht eerst even dat ik de query aan mysql_affected_rows() moest geven, dat was niet zo, en toen was ik vergeten dat stukje ook nog wel te halen.

MrMees

Ik denk wel dat je nog is_numeric of ctype_digit kan gebruiken om een fout aan te geven. Dat is wel zo makkelijk voor de gebruiker!

Net zo als met hoeveelheid > contant. Als gebruiker is het wel netjes en handig om z'n fout te laten zien.

Correct me if I am wrong

Darsstar

Citaat van MrMees

Ik denk wel dat je nog is_numeric of ctype_digit kan gebruiken om een fout aan te geven. Dat is wel zo makkelijk voor de gebruiker!

Doe je mooi bij je formulier?

Citaat van MrMees

Net zo als met hoeveelheid > contant. Als gebruiker is het wel netjes en handig om z'n fout te laten zien.

Doe je als cnb() FALSE returned?

MrMees

Citaat van Darsstar

Doe je mooi bij je formulier?

Met javascript of iets dergelijks?

ICTscripters

Dé plek voor IT

Dé plek voor IT

nieuwe game

Criminolz

[Beta] Ottomaan: gratis browserspel met een echte, levende stad. Feedback gezocht!

Port Father: beta van een browsergame in Antwerpen 1931, feedback welkom

Maffia Hotel: een browsergame waarin je van lobbygast naar spil van het hotel groeit

Crime Club volledig herschreven vanaf nul

Malavita (nieuwe maffia game, van 0)

Herkent deze Notion-structuurscan echte problemen?

[Aanbod] Stadskaart voor je browsergame: van menu naar klikbare stad, vanaf €200

[Verkoop] G4G (Gangsters 4 Gangsters): volledige broncode, nieuw gebouwd in PHP 8.3 | € 30 per domein

[TE KOOP] Malavita.be [Maffiaspel, gebaseerd op omicidio.nl, unieke code]

Functie veilig?

Registratie

PHP waarschuwt voor twee ernstige bugs die makkelijk uit te buiten lijken

Functioneel ontwerp

Access Control List implementatie in PHP/MySQL - deel 1/2

Access Control List implementatie in PHP/MySQL - deel 2/2