Pierwszy kodzik w php - logowanie

pytanie zadane 2 stycznia 2017 w PHP przez niezalogowany

wtaim, bylby ktoś chętny aby spojrzec na ten kod i powiedzieć co jest do poprawy? Nie robilem jeszcze walidacji etc.

<?php	include "header.php";  ?>
<?php
	session_start();
	include "forms.php";
	require_once "config.php";
	
	
	echo "<div id=\"formBG\">".$_SESSION['login_form']."</div>";
	
	if(@$_POST['login']){		
		
		
		$connect = @new mysqli($host, $db_user, $db_pass, $db_name);
		
		if($connect->connect_errno!=0)	echo 'Error: '. $connect->connect_errno;
		else	{
			$login = $_POST['login'];
			$pass = $_POST['pass1'];
				
		$sql = "SELECT * FROM acc WHERE login = '$login' AND password = '$pass'";
		
		if($rezultat = @$connect->query($sql)){
			//	
				if($rezultat->num_rows > 0) {				
					unset($_SESSION['err_login']);
					$rezultat->close();				
					header('Location: index.php');
				} else {
					$_SESSION['err_login'] ='<p class="error">Nieprawidłowy login lub hasło.</p>';		
					if(isset($_SESSION['err_login'])){echo $_SESSION['err_login'];}	
				}
			//
			} else {
				echo "Blad zapytania.";
			}		
			$connect->close();
		}
	
	}
	
?>

<?php	include "footer.php";  ?>

1 odpowiedź

odpowiedź 2 stycznia 2017 przez Arkadiusz Waluk Ekspert (287,950 p.)

Nie używaj @ - one ukrywają błędy, a lepiej wiedzieć o problemach i je poprawić.
```
$pass = $_POST['pass1'];
```
A co jak ktoś nie prześle przez POST tego hasła? Skrypt wysypie ostrzeżenie, niby nawet nie błąd, ale należałoby to poprawić. To samo dotyczy się loginu.
```
$sql = "SELECT * FROM acc WHERE login = '$login' AND password = '$pass'";
```
Nie masz żadnego zabezpieczenia przed sql injection. Najlepiej skorzystać z bindowania, wtedy zapytanie i podstawiane wartości idą osobno do bazy, nie ma możliwości, żeby coś złego się stało.
Hasło w bazie powinno być przechowywane w formie zahaszowanej (patrz password_hash i password_verify).
SELECT * to raczej średnia praktyka - niepotrzebnie wyciąga wszystkie dane z tabeli i kod jest mniej czytelny bo bez zajrzenia do bazy nie wiemy co zwróci.
Nazewnictwo zmiennych - czasem robisz po polsku (np. rezultat), a czasem po angielsku. Proponuję zdecydować się na jeden język, najlepiej angielski.
```
<?php    include "header.php";  ?>
<?php
```
Po co zamykać tag php żeby po chwili znów go otworzyć?
```
echo "<div id=\"formBG\">"
```
Zamiast tego powyżej możesz też zrobić:
```
echo '<div id="formBG">'
```
ale to co masz też nie jest źle, tylko taka moja porada, jak dla mnie lepiej wygląda.
```
if(isset($_SESSION['err_login'])){echo $_SESSION['err_login'];} 
```
Coś takiego jest wg mnie mało czytelne. Polecałbym stosować zawsze taki sam zapis dla lepszej czytelności kodu, ja np. lubię dość znany standard PSR-2 i staram się trzymać zawartych w nim zasad.
Mieszanie kodu HTML i PHP to niezbyt dobra praktyka. Na początku może Ci to nie przeszkadza, ale później przy większym kodzie wszystko staje się bardzo nieczytelne. Polecam więc oddzielać PHP od reszty np. przy użyciu systemu szablonów Twig.

Tyle mi przyszło do głowy. Część z tych uwag to bardziej ogólne sugestie, ale np. podatność kodu na ataki sql injection czy trzymanie w bazie normalnie zapisanego hasła to bardzo poważne błędy.

1	komentarz 2 stycznia 2017 przez niezalogowany Od jutra biore sie za większosć rzeczy, o których wspomniales- dziekuje za wypowiedź.

komentarz 2 stycznia 2017 przez maly Nałogowiec (37,190 p.)

Co do mieszania html z php to dodam że przy nie zastosowaniu kontroli bufora wyjściowego z poziomu kodu(ob_start, ob_end, ...) lub nie odpowiednim skonfigurowaniu php, header('Location: index.php') nie zadziała i wywali ostrzeżenie 'Warning: Cannot modify header information - headers already sent ...', podobnie będzie z session_start.