Prosta strona - ocena kodu

Siemka
Od jakiegoś czasu uczę się php. Postanowiłem uczyć się tworząc bloga. Nie tworzyłem go z myślą że będę z niego korzystał. Nie jest on zbyt poprawny.. ma na pewno dużo błędów. W dodatku jest on nieskończony ale brak mi już sił by go ukończyć. Nie ma panelu admina i paru innych "pierdółek".
Prosił bym o ocenę kodu.. jestem przyszykowany na krytykę. Wiem że kod nie jest zbyt czytelny.. z niektórych błędów zdaje sobie sprawę ale stosuje je po prostu z lenistwa ;p Oczekuje jakiś wskazówek.. być może przyzwyczajam się do jakiś "złych praktyk".

link do strony http://kotnakeyboardzie.cba.pl/ (wiem.. ten pasek z menu Od razu pod nagłówkiem jest po prostu do wywalenia.. i np opcja rejestracja widoczna nawet gdy ktoś jest zalogowany)

kod strony:
http://www.sendspace.pl/file/22162488daec65f9849629a

O ile przed xss się zabezpieczyłeś (na moim poziomie, więc to i tak może być nie kompletne, ale filtruj_w powinno spełnić swoje zadanie), to o SQL injection aż się prosi:

              if(empty($poletext))
              {
                  $message='Uzupelnij wszystkie pola!';
              }
              elseif(!preg_match('/(?:[0-9A-Za-z\-\_]+){3,}/', $poletext))
              {
                  $message="tresc komentarza może zawierać tylko duże i małe litery, cyfry, myślniki i znaki podkreślenia. Musi mieć przynajmniej 3 znaki";
              }elseif(ereg('<script>',$poletext))
              {
                  $message="tresc komentarza nie moze zawierac slow takich jak &lt;script&gt;";
              }else
              {
                    $data_dodania=time();
                    $ip_dodajacego=$_SESSION['ip'];
                    $autor_id=$_SESSION['id'];
                    mysql_query("INSERT INTO comments VALUES('','$id_wpisu','','$autor_id','$data_dodania','$poletext','$ip_dodajacego')");
                    $message='komentarz dodany poprawnie';    
                    header("Location: index.php?file=wpis&id_wpisu=$id_wpisu"); 
              }

Źle używasz preg_match. Ogólnie to nigdy nie powinieneś dawać danych, którym nie ufasz prosto do zapytania, bo możesz np. pomylić się w regeksie. Używaj prepered statements, które masz w PDO. Jakby co ja nie jestem ekspertem w tej dziedzinie, na OWASP (https://www.owasp.org/index.php/Main_Page) znajdziesz wszystkie niezbędne informację jak zrobić twoją stronę bezpieczną.

Moje uwagi:
1.Nazwa pliku adminopcjegeneralne.php... Cóż - chyba nie potrzeba głębokiego komentarza.
2.sha1('trolololo22117'.md5($text).'bksa48432');. Samo sha1($text) jest już niemodne, mniej bezpieczne czy co? :]
3.

    if($modd==1) return true;
    else return false;

Na:

return $modd==1

if(check_mod()==true) $pozwolenie=true;
if(check_mod()==false)$pozwolenie=false;

Na:

$pozwolenie = $check_mod();

Poza tym takie coś:

if ($x==true) {}

To to samo co:

if ($x) {}

Tylko druga wersja jest bardziej czytelna (jak dla mnie).
5.W pliku edytujkomentarz.php masz takie coś: if($pozwolenie==true) (== if(check_mod())) po to, aby i tak w środku tej instrukcji robić if (check_mod()==true)
6.edytujwpis.php: $id_wpisu = mysql_real_escape_string($_GET['id_wpisu']);. Ale nigdzie wcześniej nie sprawdzasz nawet, czy $_GET['id_wpisu'] istnieje.
7.settype($id_wpisu,'integer'); na $id_wpisu = (int)$id_wpisu bądź $id_wpisu = intval($id_wpisu)
8.if($_GET['usun']==1) ... if($_GET['usun']==2) na switch
9.logowanie.php:

$haslo=mysql_real_escape_string($_POST['password']);

Skoro hasło i tak jest hashowane, to po co je escapujesz?
10.

$query = mysql_query("SELECT `user_name` , `user_password` FROM `users` WHERE `user_name`='$nick'");

No tak - po co dodać jeden dodatkowy warunek (and password=$password, ale wcześniej oczywiście wykonać sha1 na haśle), skoro można pobrać wszystkich użytkowników o danym user_name (co prawda i tak musi być to jeden user, ale jednak) i iterować po nich?
11.rejestracja.php:

From: <kontakt@costam.pl>

Czy to nie powinno być (tzn.się ten e-mail) zapisany w configu?
12.wpisy.php:

echo"</DiV>";

13.wyloguj.php:

     echo "Zostałeś wylogowany";
     header ("location: index.php");

To w ogóle działa?
Nie można wysłać nagłówków, skoro został już wysłany tekst (inaczej: header nie będzie działać, jeżeli już cokolwiek wyświetliłeś np.za pomocą echo)

1. Skoro już używasz XHTML 1.0 Strict (z nagłówkiem XML-owym), to nie używaj takiego elementu jak <center>, bo on nie istnieje;
2. Zapoznaj się z error_reporting() i handlerami błędów - w przyszłości się przyda;
3. Plik, który nazywa się config.php ma w sobie funkcje checkEmail() i coś tam jeszcze - lekka niekonsekwencja - jak konfiguracja, to konfiguracja, a nie łączenie z bazą i szyfrowanie hasła;

Ogólnie - widać, że prosty skrypt pisany na początek.

po co parsować !preg_match('/(?:[0-9A-Za-z-_]+){3,}/', $poletext) jak można użyć htmlspecialchars i można będzie ozywać " i ' a nie będzie to wywoływać błędu.
Mam sugestię co do układania elementów strony, polecał bym zrobić to w jednym pliku bo dużo łatwiej będzie z późniejsza modyfikacją zobacz jak to jest w phpBB by przemo albo jPortal2. Zauważ również, że można załadować stronę beż index.php teraz to nie robi różnicy ale przy większym projekcie może to wywołać błędy z cennymi info dla hackera ;]

Patryk27 napisał(a)

Samo sha1($text) jest już niemodne, mniej bezpieczne czy co? :]

Rainbow tables... Z drugiej strony nadmierne hashowanie to też przesada, tego MD5 powinno nie być. A w ogóle użyjcie blowfisha czy czegoś sensownego. Te algorytmy (SHA1 i MD5) są zoptymalizowane pod szybkość :|

proqix napisał(a)

po co parsować !preg_match('/(?:[0-9A-Za-z-_]+){3,}/', $poletext) jak można użyć htmlspecialchars i można będzie ozywać " i ' a nie będzie to wywoływać błędu

Może nie każdy chce mieć pełen przekrój śmiesznych znaczków wśród nazw użytkowników? Zauważ że na upartego można upchnąć np. prefiksy znaku multibajtowego których PHP nie obsługuje lub dziwne białe znaki z okolic 1-30 w tabeli ASCII. Pełna kontrola w takich miejscach akurat jest całkiem fair. Mimo wszystko w regeksie bodajże brakuje kotwic i dopasowana zostanie ewentualnie tylko część, dorzuć ^ na początek i $ na koniec by uczynić to dokładnym dopasowaniem.

header( "refresh: 2;url=index.php" );

Cóż to za cudo?

       $query = mysql_query("SELECT `user_name` , `user_password` FROM `users` WHERE `user_name`='$nick'");
       while ($wiersz = mysql_fetch_row($query)) 
        {
            if ($nick==$wiersz[0])              $pop_nick=true;
            if (codepass($haslo)==$wiersz[1])   $pop_haslo=true;
            
        }

Dziwaczna konstrukcja, przecież zakładając unikalność nicku wynikowa ilość rekordów może wynieść albo 0, albo 1. Po co pętla while() i takie dziwne ustawianie zmiennych?

Demonical Monk napisał(a):

header( "refresh: 2;url=index.php" );

Cóż to za cudo?

Przekierowanie po 2 sekundach po otwarciu strony?
O takim słyszał?: <meta http-equiv="Refresh" content="2; url=http://www.example.com/" /> - jak sam atrybut wskazuje - to samo można uzyskać nagłówkami, i to właśnie zrobił autor.

Bodajże to wymysł IE, ale działa przecież w każdej przeglądarce.

edit: http://en.wikipedia.org/wiki/List_of_HTTP_header_fields#Refresh

wiem nazwa adminopcjegeneralne.php beznadziejna.. ale ten akurat plik i tak miał być do wykasowania..

2.sha1('trolololo22117'.md5($text).'bksa48432');. Samo sha1($text) jest już niemodne, mniej bezpieczne czy co?

Znajomy mi mówił że lepiej robić to w ten sposób.. jest bezpieczniej.. ile w tym prawdy jest to nie wiem

7.settype($id_wpisu,'integer'); na $id_wpisu = (int)$id_wpisu bądź $id_wpisu = intval($id_wpisu)

To jest jakaś duża różnica ?

13.wyloguj.php:
echo "Zostałeś wylogowany";
header ("location: index.php");

To w ogóle działa?
Nie można wysłać nagłówków, skoro został już wysłany tekst (inaczej: header nie będzie działać, jeżeli już cokolwiek wyświetliłeś np.za pomocą echo)

he jakoś działa.. sam się zastanawiałem czemu ;)

header( "refresh: 2;url=index.php" );

Cóż to za cudo?

Gdzieś na jakimś forum wyczytałem że można tego użyc...

Wow.. nie wiedziałem że robie aż tyle błędów.. fakt z tym kodem

 $query = mysql_query("SELECT `user_name` , `user_password` FROM `users` WHERE `user_name`='$nick'");
       while ($wiersz = mysql_fetch_row($query)) 
        {
            if ($nick==$wiersz[0])              $pop_nick=true;
            if (codepass($haslo)==$wiersz[1])   $pop_haslo=true;
 
        }

to dopiero teraz zobaczyłem jak bardzo to jest porąbane..

zresztą coś takiego

if ($x==true) {}

myślałem że jest czytelniejsze od

if ($x) {} 

czy może któryś z tych zapisów po prostu wykonuje się szybciej ?

Ogólnie dzięki za wszystkie uwagi na pewno mi się przydadzą ;)

Rozprawianie nad tym, czy if ($x==true) {} wykona się szybciej od if ($x) {} nie ma sensu. To jest kwestia ułamka milisekundy. Szczątkowa optymalizacja to zło w takich wypadkach.

2.sha1('trolololo22117'.md5($text).'bksa48432');. Samo sha1($text) jest już niemodne, mniej bezpieczne czy co?

Znajomy mi mówił że lepiej robić to w ten sposób.. jest bezpieczniej.. ile w tym prawdy jest to nie wiem

Z solą to akurat dobrze, ale podwójne hashowanie różnymi algorytmami jest złe - stwarzasz większą ilość kolizji. Osobiście bym użył blowfisha. SHA i MD są zoptymalizowane pod szybkość działania, więc do zabezpieczania haseł się po prostu nie nadają.

if ($x) jest szybsze.
10 tysięcy sprawdzeń tego wykonuje się u mnie średnio 0.0017 sek.

if ($x==true) jest wolniejsze.
10 tysięcy sprawdzeń wykonuje się średnio 0.0025 sek.

Testy przeprowadzono ok 10 razy w odstępach czasowych, żeby wyeliminować "zamuły" od innych programów, które mogłyby źle wpływać na wynik.

Podsumowanie - wszystko jedno czego użyjesz, JEDNO sprawdzenie to ok 0,00000008 sekundy różnicy, a przy jednym requeście będziesz miał takich 10-50. Czyli wciąż jakieś 1/10000 milisekundy różnicy na jednym wywołaniu strony.

Ok.. Postanowiłem przepisać ten kod.. spróbuje napisać to obiektowo.. na początek jakiś prosty system szablonów i z nim właśnie mam problem.

Zrobilem go w ten sposób (a właściwie zrobione tak było w jakimś tutorialu)

oto fragment funkcji view.php

public function __set($name, $value)
    {
        $this->data[$name] = $value;
    }
    
    public function __get($name)
    {
        return $this->data[$name];
    }
    
    public function render()
    {
        extract($this->data);
        require($this->dir);
    }
 

i używamy tego tak

 
$tpl->wyswietl_formularz=true;
$tpl->message_typ='x';                                                      //typ wiadomosci funkcji helpers::komunikat
$tpl->message_tresc='';

i w templatce możemy używać zmiennych $wyswietl_formularz , $message_typ itp.

No ale problem jest gdy musze przesłać do templatki tablice ;/
Obiegłem problem używając serialize() na tablicy.. a w templatce unserialize()

No i moje pytanie.. Czy to jest dobry pomysł ? Pewnie to będzie wolne.. jak można zrobić to inaczej ?
Cały kod w załączniku

edit:
Już nieaktualne.. przez jakiś głupi błąd nie działało mi przesyłanie tablic bezpośrednio do templatki a ja Od razu obwiniłem funkcje extract() że nie radzi sobie z tablicami dwuwymiarowymi <facepalm>

Ok napisałem ten blog obiektowo.. a raczej próbowałem. Zrobiłem go na systemie szablonów (wiadomo prościutki). Zrobiłem w nim tylko logowanie i wyswietlanie artykułów i dodawanie komentarzy. Chyba dalej nie ma sensu się w niego zagłębiać.. chciałbym spróbować mvc.
Prosił bym tak samo jak wcześniej o ocenę.. starałem się nie popełniać tych błędów co napisaliście

Kod w załączniku