Keskustelu: Nettisivujen teko: Purkkakoodia

Termos
Kirjottelin rekisteröinnintarkistuskoodipätkää ja kysynkin, että miten tämän voisi hoitaa paremmin. Omasta mielestä vaikuttaa purkalta vaikka toimiikin. sA on salasana ja sB on salasana uudelleen input boxit. Eli register.phpssa formin action on register_handler.php ja tiedot napataan POST:illa tähän. Haukut / korjaukset / risut / ruusut kaikki on kotiinpäin.

<?php
include 'admin/config.php';

$email = $_POST['email'];

@mysql_connect($mysq_host, $mysql_user, $mysql_pass);
@mysql_select_db($database);
$haku = @mysql_query("SELECT email FROM users WHERE email = $email");

$virhe = false;

//register.php lähettää tiedot tähän ja POST:illa napataan ne muuttujiin.
$nick = $_POST['nick'];
$nimi = $_POST['nimi'];
$paikkakunta = $_POST['paikkakunta'];
$ika = $_POST['ika'];
$harrastus = $_POST['harrastus'];
$sA = $_POST['s1'];
$sB = $_POST['s2'];

//Napataan muuttujiin salasanojen pituudet
$sAp = strlen($sA);
$sBp = strlen($sB);

//Päivämäärä ja aika muuttujiin
$time = date("HH:mm:ss");
$date = date("j.n.Y");

//IP ja hosti muuttujiin
$ip = getenv("REMOTE_ADDR");
$host = gethostbyaddr($ip);

//Stripataan turhat tagit pois joukosta jos joku niitä koittaa laitella
$nick = strip_tags($nick);
$nimi = strip_tags($nimi);
$paikkakunta = strip_tags($paikkakunta);
$ika = strip_tags($ika);
$harrastus = strip_tags($harrastus);
$email = strip_tags($email);
$sA = strip_tags($sA);


//Tarkistukset alkaa
if (empty($sA) || empty($sB))
 {
  echo 'Virhe: Salasanakenttä tyhjänä.<br>';
  $virhe = true;
 }

if ($sAp <= 5 || $sBp <= 5)
 {
  echo 'Virhe: Salasanakentässä alle 6 merkkiä.<br>';
  $virhe = true;
 }

if ($sA !== $sB)
 {
  echo 'Virhe: Salasanat eivät täsmää.<br>';
  $virhe = true;
 }

if (empty($nick))
 {
  echo 'Virhe: Nick-kenttä on tyhjä. <br>';
  $virhe = true;
 }

if (empty($nimi))
 {
  echo 'Virhe: Nimikenttä on tyhjä! <br>';
  $virhe = true;
 }

if (empty($paikkakunta))
 {
  echo 'Virhe: Paikkakuntakenttä on tyhjä! <br>';
  $virhe = true;
 }

if (!preg_match("/^(\d){2,2}\.(\d){2,2}\.(\d){4,4}$/", $ika))
 {
  echo 'Virhe: Syntymäaikakenttä on tyhjä tai virheellistä muotoa.<br>';
  $virhe = true;
 }

if (empty($harrastus))
 {
  echo 'Virhe: Harrastuskenttä on tyhjä.<br>';
  $virhe = true;
 }

if (@mysql_num_rows($haku) !== 0 or empty($email) or !strstr($email, "@") or !strstr($email, "."))
 {
  echo 'Virhe: Tarkista E-Mail! (E-Mail löytyy jo kannasta tai on virheellinen.)<br>';
  $virhe = true;
 }

if ($virhe == true)
 {
  echo '<br> Paina edellinen (back)...';
 }

else
 {
  if ($virhe == false)
   {
    $sA = md5($sA);
    @mysql_query("INSERT INTO users SET
     nick = '$nick',
     nimi = '$nimi',
     paikkakunta = '$paikkakunta',
     ika = '$ika',
     harrastus = '$harrastus',
     email = '$email',
     salasana = '$sA',
     regdate = '$date',
     regtime = '$time',
     ip = '$ip',
     host = '$host',
     tarkastettu = 0,
     logged = 0");
     header("Location: ?page=kiitos");
   }
 }
?>

1. "Stripataan turhat tagit pois joukosta jos joku niitä koittaa laitella"
   Miksi? Jos joku on niin tyhmä, että haluaa nimimerkkinsä <h1>-tagien väliin, sallittakoon se hänelle. strip_tags():n sijaan htmlentities ja mielummin vasta tulostusvaiheessa.
2. Salasanaa ei ainakaan tulisi ajaa minkään strip_tagsin läpi, vaan md5() tai muun vastaavan hash-funktion. Salasanoja ei ikinä tallenneta tietokantaan sellaisenaan. Tai ei ainakaan pitäisi tallentaa.
3. Tarkistat käyttäjän syötteet, HYVÄ! Oikea suunta! Tarkistuksillasi ei kuitenkaan estetä SQL-injektionia. Käyttäjän syötteet aina ennen SQL-lausetta jonkun escape-funktion läpi. Tässä tapauksessa käy esim. mysql_escape_string()
4. Aikaa ei kannata tallentaa tietokantaan missään itsekeksityssä formaatissa, vaan tietokannan omassa formaatissa. Miksi, siitä juuri keskusteltiin täällä.

Kokonaisuutena tuo koodi ei minusta yhtään purkkaa ole. Sisältää vain muutamia tietoturva-aukkoja.

Edit: Hupsista, pistetäänhän se salasana sittenkin md5()-funktion läpi, en sitä huomannut...

Minun mielestäni nuo kannattaa ehdottomasti ajaa myös strip_tags():n läpi, oletettaen siis että kaikki noista ovat tietoja jotka tulevat jollain HTML-sivulla näkymään. Tuon ajv:n ehdottaman h1:n sijaanhan sinne voidaan syöttää mitä tahansa, vaikka img-tagi johon latautuu hello.jpg. Käyttäjä voi myös jättää tagin päättämättä, mikä esim. h1:n tapauksessa sotkee sivun varsin pahasti. Nuo strip_tagsit voisi tosin siirtää heti tuonne alkuun, kun muuttujat haetaan $_POSTista. Turhaa uusimista on hyvä välttää.

Pointti olikin se, että sen strip_tagsin ja käyttäjän syötteen muuttamisen sijaan ajetaan tavara htmlentitiesin läpi, jolloin se näkyy juuri niin, kuin käyttäjä oli sen syöttänyt.
Hello.jpg:n tapauksessa näkyisi siis teksti '<img src="hello.jpg" alt="moi" />', eikä itse kuva.

Tuohan on ihan nättiä ja selkeää koodia.

Tuon virheentarkistuksen voisi tosin yhdistää seuraavasti:

if (empty($harrastus))
{
  $virhe .= 'Virhe: Harrastuskenttä on tyhjä.<br>';
}

Ja lopussa sitten tulostaa pelkästään tuo $virhe mikäli sellainen on säädetty. Ei tartte erikseen echottaa joka virhettä kun kasaat ne noin. Samalla saat koodista vielä nätimpää kun et tartte jokaiseen tarkistukseen edes hakasulkeitakaan. Lisäksi:

if ($virhe == true)
{
  echo '<br> Paina edellinen (back)...';
}

else
{
  if ($virhe == false)
   {

Menee viilaukseksi mutta eikös tuo
if ($virhe == false)
ole täysin turha tuossa? Ja lisäksi boolean muuttujille (true/false) ei kait tarvi ehtolauseessa kuin yhden = merkin tuossa käytetyn kahden sijaan? Tyhjänpäiväistä pilkunviilaustahan tämä on mutta kuitenkin...

JTS kirjoitti:

Ja lisäksi boolean muuttujille (true/false) ei kait tarvi ehtolauseessa kuin yhden = merkin tuossa käytetyn kahden sijaan?

Ei pidä paikkaansa.
Yksi =-merkki on edelleen sijoitusoperaattori, vaikka kuinka olisi boolean-muuttuja.

Sijoitus tosin evaluoituu aina todeksi, joten

$muuli = true;
if($muuli = true) {}

voi antaa (väärän) mielikuvan toimimisesta.

Esimerkiksi

$muuli = false;
if($muuli = true) {}

aikaansaa kuitekin sen, että if-lauseeseen liittyvä koodi suoritetaan, mikä ei liene sitä, mitä tuolla koodilla halutaan ajaa takaa.

Niinhän se tietenkin oli. Kiitos korjauksesta.

Ja tuo empty mieltää '0' tyhjäksi, mutta tuskinpa kenelläkään moista harrastusta onkaan. Se on kuitenkin hämäävä funktio nimeltään, ellei tiedä mitä se tekee.

Hyvin pikaisesti tarkasteltuna seuraavat huomiot, joita kukaan ei vielä tainnut mainita.

Pitäisiköhän ensimmäisen SELECT-lauseen email-kentän ympärillä olla heittomerkit, jos kyseessä on SQL-merkkijono?

Miksi pakotat käyttäjän syöttämään "syntymäajan" etunollilla ja neljällä vuoden numerolla? Yleensä syntymäaika on muotoa 010198 tms., kun taas päivämäärä Suomessa ilman etunollia muotoa 1.1.1998. Nyt käyttäjä voi syöttää vaikkapa päivämäärän 00.00.0000 tai 55.55.5555 ja se menee hyvin tarkistuksesta läpi. Funktiot explode ja checkdate saattaisivat auttaa. Ja jätä ne etunollat vaatimatta, koska se ei ole käyttäjälle tavallinen tapa.

Sen sijaan, että käsket käyttäjän painaa Back-painiketta, sinun pitäisi ainakin tuottaa lomake käyttäjän jo täyttämillä tiedoilla, antaa täyttöohjeet kenttiin, kertoa virheet ja niiden korjaukset, jotta käyttäjän olisi helpointa toimia. Nyt voi käydä niin, että välimuistin takia käyttäjä saa eteensä tyhjän lomakkeen, jonka täyttäminen uudelleen on rasittavaa.

Insert-lauseen standardinmukainen muoto on hieman erilainen kuin käyttämäsi. Kannattaa tarkistaa.

Standardi vaatii, että Location-otsakkeen osoitteen pitäisi olla absoluuttinen. Halunnet ehkä tarkistaa senkin.