View Single Post
Limited edition
Moff's Avatar
Årsaken til at dette feiler er en syntaksfeil i SQL-spørringen din. Dette ville du fått beskjed om hvis du hadde brukt tilkoblingskoden jeg postet ovenfor. Du setter ikke error mode for PDO, og dermed blir feilmeldingen suppresset, slik at du ikke ser hvor det går galt. Det er en litt pussig feil også, for du har skrevet det korrekt alle andre steder i denne tråden:

Kode

// Feil:
SELECT 'id', 'email' FROM 'login' WHERE 'email' = :email

// Korrekt:
SELECT id, email FROM login WHERE email = :email
Databasenavn og tabellnavn skal ikke skrives med 'anførselstegn', det er kun for variabler. Når du jobber med PDO så bør du sette error mode til exception, slik at det genereres en exception ved denne typen feil:

Kode

$db = new PDO("mysql:host=$host; charset=utf8; dbname=$database", $username, $password, array(
	PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, // Sett error-mode til exception
	PDO::ATTR_EMULATE_PREPARES => false, // Ikke bruk emulate prepares
	PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC // (Valgfritt) Sett default fetch mode
));
Du kan angi ganske mange ulike innstillinger for PDO på denne måten. Her har jeg tatt med 3 stk. Error mode til exception og emulate prepares til false bør alltid være med. Error mode er som sagt innstillingen som velger hva som skal skje hvis det oppstår feil; det ryddigste er exceptions (kilde: jeg sier at det er sånn). Det betyr at du kan bruke try/catch-blokker for å fange opp feil og handle ut i fra det. Emulate prepares er en feature som handler om hvordan PDO oppfører seg med bruk av prepared statements. Det finnes noen gamle database-drivere der ute som ikke støtter prepared statements, og PDO vil da forsøke å emulere prepared statements. Ikke la deg lure av navnet på innstillingen; hvis denne er satt til true, så vil PDO alltid emulere prepared statements, selv om systemet ditt støtter prepared statements. Hvis du setter denne til false (som er min anbefaling), så vil PDO kun emulere på systemer som ikke støtter prepared statements. Du kan sjekke hvorvidt denne allerede er satt til false, og hvis den er, unnlate å si noe om det - men jeg synes det er best practice å alltid være eksplisitt og sette den til false. Better safe than sorry.
Den tredje innstillingen jeg har tatt med er default fetch mode. Du har skrevet denne linjen:

Kode

$userInfo = $statement->fetch(PDO::FETCH_ASSOC);
Hvis default fetch mode er satt til PDO::FETCH_ASSOC, så ville du kunne skrevet dette i stedet:

Kode

$userInfo = $statement->fetch();
Om du ønsker å bruke denne funksjonen er litt smak og behag. Jeg bruker den som regel ikke, fordi jeg mikser og matcher litt mellom ulike modes når jeg jobber. Hvis du jobber med et prosjekt hvor du kun bruker FETCH_ASSOC, så er det likevel noe du kan vurdere. (For meg så handler det om lesbarhet i kode, det er lettere for meg å se hva som skjer hvis fetch mode alltid er angitt, så jeg slipper å gå til en annen fil for å se hva som er default for prosjektet).

En annen sikkerhets-ting jeg kan tipse om er bruken av openssl_random_pseudo_bytes(). Jeg ville brukt random_bytes() i stedet. Årsaken er først og fremst at random_bytes() er en native PHP-funksjon som ikke krever at du har installert og konfigurert OpenSSL-extensionen. Den andre fordelen er at random_bytes() er kryptografisk sikker (cryptographically secure), i motsetning til openssl_random_pseudo_bytes(). Når du jobber med ting som har med sikkerhet å gjøre, for eksempel nonce-verdier (som dette tokenet er), så bør du bruke en sikker random-funksjon som ikke er forutsigbar. Som du sikkert er klar over så er ikke randomness noe en datamaskin er i stand til å gjøre. Alle verdier som genereres "tilfeldig" er det vi kaller psuedo-random ("liksom-tilfeldig"). Det er likevel noen algoritmer som er så gode at de ikke regnes for å være forutsigbare, og det er disse vi betegner som cryptographically secure. Du har sikkert spilt dataspill som genererer "tilfeldige" kart, og slike spill har gjerne en funksjon for at du kan legge inn en seed til kartet. Hvis to ulike spillere starter spillet med samme seed, så vil de få nøyaktig samme kart, selv om kartet er "tilfeldig" generert. Dette demonstrerer hvordan noe kan være tilfeldig, men likevel 100% forutsigbart.

I likhet med hash-algoritmer, som vi har diskutert tidligere, så er det viktig å finne en balanse mellom hva som er trygt, og hva som tar kortest mulig tid for datamaskinen å regne ut. Derfor har vi algoritmer som er trygge (og trege) og algoritmer som er utrygge og lynraske. For å generere kartfiler i et spill trenger du ikke noe som er trygt; det spiller ingen rolle. For nonce-verdier, passord og slike ting, da er sikkerhet veldig viktig.

$insertSql-en din er forresten et eksempel på en situasjon hvor du ikke trenger å bruke prepared statements, hvis du ikke vil. Du bør uansett være konsekvent i koden din; her bruker du execute() med et array, i stedet for å bruke bind-funksjonen slik du gjør lengre opp i koden. Det spiller ikke så voldsomt stor rolle hva du gjør, men du bør i alle fall gjøre det samme hver gang.

Personlig anbefaler jeg at du bruker bindParam(). Du har brukt bindValue(), som er den samme funksjonen, men den binder verdien av variabelen din i stedet for referansen til den. Så, hva er forskjellen på bind-by-value og bind-by-reference? Se her:

Kode

$var = 'laks';
$q->bindParam(':var', $var);
$var = 'bever';
// Bundet verdi er nå 'bever'

$var = 'laks';
$q->bindValue(':var', $var);
$var = 'bever';
// Bundet verdi er 'laks', selv om verdien til $var nå er 'bever'
Igjen, dette er smak og behag, men du bør i alle fall være konsekvent og bevisst på hvilken av dem du bruker. Jeg foretrekker å bruke bindParam(), fordi jeg da har flere muligheter til å strukturere koden min mer effektivt i enkelte situasjoner. Det skal imidlertid sies at det i noen tilfeller kan være greit å bruke bindValue() hvis du prøver å skrive kortest mulig kode, fordi du MÅ ha en referanse for å bruke bindParam(). Du kan med andre ord ikke bruke noen verdier eller uttrykk i bindParam():

Kode

$q->bindParam(':var', 0); // Error: Cannot pass by reference yada yada yada ...
Du lagrer en dato sammen med tokenet ditt; dette kan du droppe til fordel for en TIMESTAMP-kolonne i databasen din. Opprett en kolonne, sett den som en TIMESTAMP-type og sett default til CURRENT_TIMESTAMP. Hver gang du nå gjør en INSERT mot denne tabellen, så vil du få en timestamp rett inn i denne kolonnen. Jeg pleier å kjøre SELECT-statements med UNIX_TIMESTAMP() på denne kolonnen hvis jeg trenger å sjekke tidspunkt:

Kode

SELECT userId FROM tokens WHERE UNIX_TIMESTAMP(expires) > UNIX_TIMESTAMP() LIMIT 1
Hvis vi tenker at kolonnen "expires" inneholder en timestamp for når tokenet utløper (ikke lengre er gyldig), så vil du her bare select-e tokens som ikke har gått ut på dato. Dette fordrer selvsagt at du har satt inn en timestamp i denne kolonnen som er frem i tid, ikke CURRENT_TIMESTAMP:

Kode

$timestamp = time() + 60 * 15; // 15 minutter fra nå
$timestamp = date('Y-m-d H:i:s', $timestamp); // Konverter fra UNIX_TIMESTAMP til MySQL TIMESTAMP

// INSERT ...
Avsluttningsvis så er det 3 ting til jeg legger merke til, som kan være problematisk.

1 - Eksponering av bruker-ID-er til klienten. Du sender en link som inneholder ID-nummer fra databasen, både fra password_reset_request -og fra login-tabellen. Er dette strengt tatt nødvendig? Nei. Nei, det er det ikke. Du har generert et tilfeldig token, og dette tokenet er bundet til en bruker-ID i databasen din. Hvis en bruker sender deg dette tokenet, så vet du med 99,99% sikkerhet at det er korrekt bruker, og du kan gå ut i fra at bruker-ID-en fra databasen er korrekt. Om klienten også må presentere bruker-ID, så gir ikke dette deg noe ekstra sikkerhet uansett; hvis noen har klart å få tak i tokenet, så har de garantert også tilgang til bruker-ID-en som hører til (fordi alt ligger i samme link). Så, designtips: Generer en sikker nonce-verdi med random_bytes. Gjerne noe som er litt mer enn 16 bytes; jo lengre det er, jo vanskeligere er det å gjette. Du trenger ikke 128 bytes, liksom, men kanskje 32. Kanskje 64. Noter deg IP-adressen til klienten som har bedt om reset, og verifiser at det er samme IP som sender tokenet tilbake til deg. Da kan du være 99,9999% sikker på at det er samme klient. IP får du fra $_SERVER['REMOTE_ADDR']. Sørg for at kolonnen du lagrer IP i er minst 45 tegn for å kunne lagre alle IPv6-formater (spesifikt "IPv4-rutete" IPv6-adresser). Ta også å hiv inn en UNIQUE-key på kolonnen for token, så du er helt sikker på at du ikke kan ha to like tokens (for det ville være katastrofalt). Du kan ev. også sjekke om tokenet du har generert eksisterer fra før, og da lage et nytt et hvis du har klart å generere det samme to ganger. Sannsynligheten for at dette skjer er helt hinsides liten, men det kan jo skje. (Not really.)

2 - Bruk HTTPS. Jeg ser at linken din bruker HTTP. I 2019 så er dette litt krise. Du kan ha så trygge algoritmer du vil, men hvis du sender all informasjon over internett uten kryptering, så hjelper det fint lite. Du kan få gratis TLS-sertifikater fra tjenester som Let's Encrypt, så det er bare å kjøre på.

3 - mail()-funksjonen er litt... vel, den fungerer ikke så bra. 85% av all e-post som sendes på nettet er spam akkurat nå. Hvis du sender e-post uten en verifisert SMTP-server, så vil det alltid bli betraktet som spam. Det beste du kan håpe på er at e-posten kommer fram til spam-mappen, men i mange tilfeller vil den nok bare bli blokkert før den i det hele tatt kommer frem til brukeren. Det å sende automatisert e-post er rett og slett vanskelig, du kan aldri garantere at det ikke havner i spam-filter. Det du kan gjøre er å bruke en skikkelig e-posttjeneste hvor du har en SMTP-server med DKIM, SPF og PTR. Det er litt dill-dall å konfigurere, og du bør også da ta i bruk et e-postbibliotek i PHP for å gjøre tilkobling og utsending lettere.