[PHP MYSQL] Czy ten skrypt jest bezpieczny? Opcje

[julek12]

Witam,
Napisałem skrypt na VIP sms i chciałbym wiedzieć czy jest on bezpieczny oraz czy jest dobrze napisany

[PHP] pobierz, plaintext 
<p><b>Krok po kroku:</b><br />
1. Czytasz regulamin usługi<br />
2. Wysyłasz sms o treści xxx pod numer xxx<br />
3. Wpisujesz swoją nazwę użytkownika z forum (wielkość liter brana pod uwagę)<br />
4. Wpisujesz kod, jaki dostałeś w smsie zwrotnym<br />
5. Klikasz "Zrob ze mnie przyjaciela!"<br />
6. Sprawdzasz czy masz nową grupę</p>
 
<p><form action="vip.php" method="post">
        <p>
        <label>Twój nick</label>
        <input type="text" name="nick" value="" />
        <label>Kod</label>
        <input type="text" name="check" maxlenght="8">
        <input class="button" type="submit" value="Zamawiam Vip" />
        </p>
        </form></p>
 
<?php
mysql_connect('xxxx', 'xxxx', 'xxxx') or
die ('Niepoprawne polaczenie z baza danych ');
mysql_select_db('xxx_mybb') or die ('Zla baza danych');
 
$t = time();
 
$id = "xx";
$code = "xx";
$type = "sms";
$grupa = '8';
 
$nick = $_POST['nick'];
$check = $_POST['check'];
$del=1;
 
$m = "SELECT * FROM `mybb_users` WHERE `username` = '".$nick."'";
$wykonanie = mysql_query($m) or die ("Nie mogę wykonać zapytania.");
$n = mysql_num_rows($wykonanie);
if ($n == 0 && $check != NULL)
{
echo "Niepoprawny nick.<br />";
}
else
{
 
$zgroup = "SELECT `usergroup` FROM `mybb_users` WHERE `username` = '".$nick."'";
$wgroup = mysql_query($zgroup) or die ("Nie mogę wykonać zapytania.");
$group = mysql_fetch_array($wgroup);
 
if ($group[usergroup] == 3)
{
echo "Jesteś super moderatorem, a chcesz zostać VIPem? TO NIEMOŻLIWE.";
}
else if ($group[usergroup] == 4)
{
echo "Jesteś administratorem, a chcesz zostać VIPem? TO NIEMOŻLIWE.";
}
else if ($group[usergroup] == 5)
{
echo "Twoje konto jest zawieszone. Dokonaj pełnej aktywacji i będziesz mógł kupić konto VIP.";
}
else if ($group[usergroup] == 6)
{
echo "Jesteś moderatorem, a chcesz zostać VIPem? TO NIEMOŻLIWE.";
}
else if ($group[usergroup] == 7)
{
echo "Twoje konto zostało zbanowane. Dopiero, gdy minie Ci ban będziesz mógł zakupić konto VIP.";
}
else if ($group[usergroup] == 12)
{
echo "Jesteś redaktorem, a chcesz zostać VIPem? TO NIEMOŻLIWE.";
}
else
{
if($check == NULL)
echo 'Proszę wpisać kod';
 
$handle = fopen("http://dotpay.pl/check_code.php?id=".$id."&code=".$code."&check=".$check."&type=".$type."&del=".$del, 'r');
    $status = fgets($handle, 8);
    $czas_zycia = fgets($handle, 24);
    fclose($handle);
    $czas_zycia = rtrim($czas_zycia);
 
    if ($status == 0 && $check != NULL)
    {
    echo "Kod niepoprawny.";
    }
    else
    {
    if (!isset($_COOKIE['ActiveCode']) && $check != NULL)
    {
mysql_query("UPDATE `xxx_mybb`.`mybb_users` SET `usergroup` = '".$grupa."',
`vip` = '".$t."' WHERE `mybb_users`.`username` ='".$nick."'");
echo "<font color=\"red\"><b>Konto dostało VIPa! Teraz nie spamuj i się nie podniecaj..  !</b></font>";
    }
}
}
}
?>
[PHP] pobierz, plaintext 

Ten post edytował julek12 6.03.2009, 19:04:05

[Mephistofeles]

Skrypt raczej podatny na SQL Injection - nie filtrujesz $nick. A czy dobrze napisany? U mnie by to nie przeszło, ale jak działa, to chyba dobrze .
Aa, i jeszcze jedno, jeżeli nie korzystasz ze zmiennych wewnątrz łańcucha, to zamykaj go apostrofami a nie cudzysłowami, podobno szybciej, ale przynajmniej nie trzeba wtedy slashować cudzysłowów w HTMLu.

Ten post edytował Mephistofeles 6.03.2009, 19:26:32

[julek12]

@up
dzięki. o co ci chodzi z tymi apostrofami i cudzysłowami??

[PHP] pobierz, plaintext 
<?php
$nick = mysql_real_escape_string($_POST['nick']);
?>
[PHP] pobierz, plaintext 

lepiej?

Ten post edytował julek12 6.03.2009, 21:53:28

[Mephistofeles]

Np. o to:

[PHP] pobierz, plaintext 
<?php
echo "<font color=\"red\"><b>Konto dostało VIPa! Teraz nie spamuj i się nie podniecaj.. tongue.gif !</b></font>";
?>
[PHP] pobierz, plaintext 

Jeśli łańcuch rozpoczynasz cudzysłowem, to musisz slashować każdy cudzysłów wewnątrz, a skoro i tak nie wykorzystujesz czegoś takiego:
"coś tam $jakaszmienna" to ograniczaj lepiej łańcuchy apostrofami. 'coś tam' (+ ewentualnie) . $jakaśzmienna

[julek12]

Aha dzieki.

Teraz kod wygląda tak:

[PHP] pobierz, plaintext 
Kup konto VIP, aby zakupić konto VIP wyślij SMSa:
<br><b>* Na numer <font color="red">xx</font> o tresci <font color="red"><b>xx</b></font> za <font color="red"><b>3.66 zl brutto</b></font>, a za kod dostaniesz <font color="red"><b>100</b></font> punktow premium.</b><br />
 
    Serwis SMS obsługiwany przez <a href="http://www.dotpay.pl" target="_blank">Dotpay.pl</a><br>
    Regulamin: <a href="http://www.dotpay.pl/regulaminsms" target="_blank">http://www.dotpay.pl/regulaminsms</a><br>
    Usługa dostępna w sieciach: Era, Plus GSM, Orange, Play (+ Sami Swoi, Heyah, Tak Tak, Simplus, Orange GO, POP).<br>
    <b>TibiaServ</b> nie odpowieda za źle wpisane treści SMS.<br /><br />
 
        <form action="vip/" method="post">
        Twój nick:<br />
        <input type="text" name="nick" value="" /><br /><br />
        Kod:<br />
        <input type="text" name="check" maxlenght="8"><br />
        <input class="button" type="submit" value="Zamawiam Vip" /><br />
        </form><br />
 
<?php
mysql_connect('localhost', 'xx', 'xx') or
die ('Niepoprawne polaczenie z baza danych ');
mysql_select_db('xx_mybb') or die ('Zla baza danych');
 
$t = time();
 
$id = "xx";
$code = "xx";
$type = "sms";
$grupa = '8';
 
$nick = mysql_real_escape_string($_POST['nick']);
$check = $_POST['check'];
$del=1;
 
$m = "SELECT * FROM `mybb_users` WHERE `username` = '".$nick."'";
$wykonanie = mysql_query($m) or die ("Nie mogę wykonać zapytania.");
$n = mysql_num_rows($wykonanie);
if ($n == 0 && $check != NULL)
{
echo '<b>Niepoprawny nick.</b><br />';
}
else
{
 
$zgroup = "SELECT `usergroup` FROM `mybb_users` WHERE `username` = '".$nick."'";
$wgroup = mysql_query($zgroup) or die ("Nie mogę wykonać zapytania.");
$group = mysql_fetch_array($wgroup);
 
if ($group[usergroup] == 3)
{
echo '<b>Jesteś super moderatorem, a chcesz zostać VIPem? TO NIEMOŻLIWE.</b>';
}
else if ($group[usergroup] == 4)
{
echo '<b>Jesteś administratorem, a chcesz zostać VIPem? TO NIEMOŻLIWE.</b>';
}
else if ($group[usergroup] == 5)
{
echo '<b>Twoje konto jest zawieszone. Dokonaj pełnej aktywacji i będziesz mógł kupić konto VIP.</b>';
}
else if ($group[usergroup] == 6)
{
echo '<b>Jesteś moderatorem, a chcesz zostać VIPem? TO NIEMOŻLIWE.</b>';
}
else if ($group[usergroup] == 7)
{
echo '<b>Twoje konto zostało zbanowane. Dopiero, gdy minie Ci ban będziesz mógł zakupić konto VIP.</b>';
}
else if ($group[usergroup] == 12)
{
echo '<b>Jesteś redaktorem, a chcesz zostać VIPem? TO NIEMOŻLIWE.</b>';
}
else
{
if($check == NULL)
echo '<b>Proszę wpisać kod.</b>';
 
$handle = fopen("http://dotpay.pl/check_code.php?id=".$id."&code=".$code."&check=".$check."&type=".$type."&del=".$del, 'r');
    $status = fgets($handle, 8);
    $czas_zycia = fgets($handle, 24);
    fclose($handle);
    $czas_zycia = rtrim($czas_zycia);
 
    if ($status == 0 && $check != NULL)
    {
    echo '<b>Niepoprawny kod.</b>';
    }
    else
    {
    if (!isset($_COOKIE['ActiveCode']) && $check != NULL)
    {
 
$z8 = "SELECT `vip` FROM `mybb_users` WHERE `username` = '".$nick."'";
$w8 = mysql_query($z8) or die ("Nie mogę wykonać zapytania.");
$o8 = mysql_fetch_array($w8);
$o = $o8[vip] + (60*60*24*30);
$t1 = $t + (60*60*24*30);
 
        if ($group[usergroup] == 8)
        {
mysql_query("UPDATE `julek12_mybb`.`mybb_users` SET `vip` = '".$o."' WHERE `mybb_users`.`username` ='".$nick."'");
echo '<font color=\"red\"><b>Twoje konto VIP zostało przedłużone o 30 dni.</b></font>';
        }
        else
        {
mysql_query("UPDATE `julek12_mybb`.`mybb_users` SET `usergroup` = '".$grupa."',
`vip` = '".$t1."' WHERE `mybb_users`.`username` ='".$nick."'");
echo '<font color=\"red\"><b>Twoje konto zyskało VIPa.</b></font>';
        }
    }
}
}
}
?>
[PHP] pobierz, plaintext 

Ten post edytował julek12 6.03.2009, 22:30:51

[Mephistofeles]

Nom. Chociaż oczywiście można lepiej, np. użyć PDO . Ale po co w tak prostym skrypcie .

[julek12]

PDO - co to??

Napisał byś mi twoją wersje? Lepszą:)

Ten post edytował julek12 6.03.2009, 22:32:47

[Mephistofeles]

PDO to biblioteka obsługi MySQLa, ale warunek: PHP co najmniej 5. Główne zalety to szybkość, odporność na SQL Injection, i łatwość obsługi. Poczytaj na WebCity.pl
Nie mam czasu zagłębiać się w działanie twojego skryptu, ale jak masz dostęp do PHP 5 to polecam użycie PDO.

[julek12]

Oczywiście, że mam php 5 a kto teraz nie ma:)

[Mephistofeles]

Używaj też switcha zamiast tylu ifów. Jutro ci napiszę może coś ładnego .

[julek12]

dzieki:)

[Mephistofeles]

[PHP] pobierz, plaintext 
<p>
   <b>Krok po kroku:</b><br />
   1. Czytasz regulamin usługi<br />
   2. Wysyłasz sms o treści xxx pod numer xxx<br />
   3. Wpisujesz swoją nazwę użytkownika z forum (wielkość liter brana pod uwagę)<br />
   4. Wpisujesz kod, jaki dostałeś w smsie zwrotnym<br />
   5. Klikasz "Zrób ze mnie przyjaciela!"<br />
   6. Sprawdzasz czy masz nową grupę
</p>
 
<form action="test.php" method="post">
  <p>
     <label>Twój nick</label>
     <input type="text" name="nick" maxlenght="32" />
     <label>Kod</label>
     <input type="text" name="check" maxlenght="8" />
     <input class="button" type="submit" value="Zamawiam Vip" />
  </p>
</form>
 
<?php
try
{
   $nick = $_POST['nick'];
   $check = $_POST['check'];
   
   if (!$nick or !$check)
      throw new Exception('Nie wpisałeś wymaganych danych!');
   
   $pdo = new PDO('mysql:host=hostBazyDanych;dbname=nazwaBazy', 'login', 'haslo'); // Inicjacja bazy bez parametrów kodowania
   //$pdo = new PDO('mysql:host=hostBazyDanych;dbname=nazwaBazy', 'login', 'haslo', array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8')); // Inicjacja bazy z kodowaniem UTF-8
   $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); // Ustawienia parametrów wyjątków PDO
   
   $sql = $pdo->prepare('SELECT `usergroup` FROM `mybb_users` WHERE `username` = :nick'); // Przygotowuanie zapytania
   $sql->bindValue(':nick', $nick, PDO::PARAM_STR); // Podpinanie jest odporne na SQL Injection ;], a na dodatek szybsze
   $sql->execute(); // Wykonanie
   $data = $sql->fetch(PDO::FETCH_ASSOC); // Pobranie tablicy asocjacyjnej
   
   if (!$data)
      throw new Exception('Wpisałeś niepoprawny nick!<br />'); // Wyrzucamy wyjątek, jeśli nie pobrano żadnych danych z bazy
   $sql->closeCursor(); // Zamykam uchwyt
   
   switch($data['usergroup'])
   {
       case 3:
          throw new Exception('Jesteś już super moderatorem, a chcesz zostać VIPem? To Ci zupełnie niepotrzebne :).');
          break;
       case 4:
          throw new Exception('Jesteś już administratorem, a chcesz zostać VIPem? To Ci zupełnie niepotrzebne :).');
          break;
       case 5:
          throw new Exception('Twoje konto jest zawieszone. Konto VIP przydzielamy jedynie w pełni aktywowanym użytkownikom!');
          break;       
       case 6:
          throw new Exception('Jesteś już moderatorem, a chcesz zostać VIPem? To Ci zupełnie niepotrzebne :).');
          break;
       case 7:
          throw new Exception('Twoje konto obecnie jest zbanowane. Poczekaj aż minie okres bana!');
          break;
       case 12:
          throw new Exception('Jesteś już redaktorem, a chcesz zostać VIPem? To Ci zupełnie niepotrzebne :).');
          break;
    }
   
   // $del=1; Skoro tego nie zmieniasz, to po co to jest? Wystarczy wpisać na stałe
    // $t = time(); Skoro tego nie zmieniasz, to po co to jest? Wystarczy wpisać na stałe
   // $id = "xx"; Skoro tego nie zmieniasz, to po co to jest? Wystarczy wpisać na stałe
   // $code = "xx"; Skoro tego nie zmieniasz, to po co to jest? Wystarczy wpisać na stałe
   // $type = "sms"; Skoro tego nie zmieniasz, to po co to jest? Wystarczy wpisać na stałe
   // $grupa = '8'; Skoro tego nie zmieniasz, to po co to jest? Wystarczy wpisać na stałe    
 
    $handle = fopen('http://dotpay.pl/check_code.php?id=xx&code=xx&check='.$check.'&type=sms&del=1', 'r');
   $status = fgets($handle, 8);
   // $czas_zycia = fgets($handle, 24); // A to po co? Nie wykorzystujesz tego dalej...
   fclose($handle);
   // $czas_zycia = rtrim($czas_zycia); // Jak wyżej...
 
   if ($status == 0)
      throw new Exception('Kod niepoprawny!');
      
 
   if (!isset($_COOKIE['ActiveCode']))
   {
       if ($data['usergroup'] == 8)   
       {
           $sql = $pdo->prepare('UPDATE `mybb_users` SET `usergroup` = 8, `vip` = `vip` + :vip WHERE `username` = :nick');
          $sql->bindValue(':vip', 25920000, PDO::PARAM_INT);
        }
        else
        {
           $sql = $pdo->prepare('UPDATE `mybb_users` SET `usergroup` = 8, `vip` = :vip WHERE `username` = :nick');
          $sql->bindValue(':vip', time() + 25920000, PDO::PARAM_INT);
      }
       $sql->bindValue(':nick', $nick, PDO::PARAM_STR);
       $sql->execute();
       echo '<font color="red"><b>Konto dostało VIPa! Teraz nie spamuj i się nie podniecaj!</b></font>'; // To strasznie niepoprawne HTMLowo jest, zmień to lepiej na style CSS
    }
 
}
catch(Exception $e) // Łapiemy błędy
{
    echo $e->getMessage(); // Wyświetlamy treść błędu
}
?>
[PHP] pobierz, plaintext 

Powinno działać . Jak coś to pisz.

Ten post edytował Mephistofeles 7.03.2009, 18:25:14