Συνέντευξη PHP developer: η επιστροφή

defacer · 3 Σεπτεμβρίου 2014

Πριν από λίγους μήνες είχα δημιουργήσει topic σχετικό με την ύπαρξη θέσης εργασίας για έμπειρο PHP developer στην εταιρία όπου εργαζόμουν. Σε ένα από τα αρχικά posts παρέθεσα 4 ερωτήσεις που επιλέχθηκαν για αρχικό screening των υποψηφίων χωρίς βέβαια να δώσω τις απαντήσεις. Υποσχέθηκα όμως πως θα το κάνω αργότερα, και το αργότερα έφτασε.

Σ' αυτό το topic σκοπεύω να ποστάρω:

Τις 4 αρχικές ερωτήσεις μαζί με τις "τέλειες" απαντήσεις τους (εδώ), καθώς και κάποια γενικά σχόλια σχετικά με τις απαντήσεις που πήρα στην πράξη.
Άλλες 6 παρόμοιες ερωτήσεις μαζί με τις απαντήσεις τους (αρχικά συγκέντρωσα 10 και απ' αυτές επιλέχθηκαν μόνο 4 για να μη γίνουμε υπερβολικοί).
Μια σειρά από λιγότερο "στενές" ερωτήσεις μαζί με σύντομες αναφορές στα points που θα έπρεπε να περιέχουν οι απαντήσεις τους τις οποίες χρησιμοποίησα στις συνεντέυξεις. Αυτές είναι χοντρικά χωρισμένες σε γνωστικές κατηγορίες και η φύση τους είναι τέτοια που επιτρέπουν (ενδεχομένως επιβάλλουν) αμφίδρομη συζήτηση στα πλαίσια του σχηματισμού της "σωστής" απάντησης.
Την εκφώνηση για ένα hands-on coding test που ζητήθηκε από κάποιους υποψήφιους να κάνουν, καθώς και τη δική μου "official" λύση του.

Τα παραπάνω θα χρειαστούν αρκετή ενασχόληση, οπότε η δουλειά θα γίνει τμηματικά όταν βρίσκω κατάλληλη ευκαιρία (θα κάνω update αυτό το αρχικό post σχετικά κάθε φορά).

Οι στόχοι μου είναι:

Να ανοίξω ένα παράθυρο σ' αυτό τον τομέα (της διαδικασίας εύρεσης εργασίας ως developer) δίνοντας ποιοτικά real world παραδείγματα, προκειμένου να αποκτήσει όποιος ενδιαφέρεται μια ιδέα για το τι ενδέχεται να βρει μπροστά του.
Να δώσω πραγματικά παραδείγματα για το πώς ακόμα και σε φαινομενικά απλούστατα πράγματα υπάρχει χώρος για να ξεχωρίσει ο αφρός και για το πώς πρέπει να προσεγγίζει κανείς τέτοια πρακτικά θέματα αν θέλει να ξεχωρίσει.
Να δημιουργήσω προϋποθέσεις για ενδιαφέρουσα και χρήσιμη συζήτηση σχετική τόσο με τη διαδικασία της αξιολόγησης υποψηφίων όσο και με τις τεχνολογίες που συνδέονται με τη συγκεκριμένη περίπτωση.

Θα έπρεπε να εννοείται αλλά παρόλα αυτά το λέω ότι το υλικό που ακολουθεί δεν είναι βέβαια στο επίπεδο του έμπειρου senior developer στον οποίο αναφέρεται η θέση. Δεν είναι αυτή η δουλειά του. Η δουλειά του είναι να ελέγξει ότι πράγματα τα οποία οποιοσδήποτε θέλει να αποκαλεί τον εαυτό του senior developer θα έπρεπε να τα έχει με τα μάτια κλειστά, όντως τα έχει με τα μάτια κλειστά. Ο πήχης είναι αρκετά χαμηλότερα προκειμένου απλά να κοπεί όποιος ζορίζεται για να τον περάσει, αλλά αυτό είναι θετικό για τις ανάγκες του topic μιας και έτσι το υλικό θα είναι προσβάσιμο σε πολύ περισσότερα μέλη.

Enjoy!

Επεξ/σία 3 Σεπτεμβρίου 2014 από defacer

defacer · 3 Σεπτεμβρίου 2014

Οι αρχικές (fantastic four)

Q: Does this code have any actual or potential problems? Do you see any possibilities for improvement worth exploring? Make appropriate suggestions.

if(!strpos($foreignCustomerName, $suffix)) {
    // ...
}

Το σημαντικότερο που πρέπει να ειπωθεί εδώ (για να πιάσουμε τη βάση) είναι ότι η χρήση της strpos μ' αυτό τον τρόπο είναι λάθος επειδή δύο από τις τιμές που ενδέχεται να επιστραφούν είναι το boolean false (δεν βρέθηκε το suffix) και το integer 0 (βρέθηκε στην αρχή του customer name). Ο έλεγχος με not δε μπορεί να ξεχωρίσει ανάμεσα σ' αυτές τις δύο δραματικά διαφορετικές περιπτώσεις και είναι απλά bug που περιμένει την κατάλληλη στιγμή να εμφανιστεί. Αυτή ακριβώς η πληροφορία αναφέρεται και στο manual page με bold και κόκκινο χρώμα, οπότε δεν υπάρχουν δικαιολογίες.

Ένα άλλο σημείο ενδιαφέροντος είναι πως ενώ ψάχνουμε για κάτι που ονομάζεται suffix (κατάληξη, επίθεμα), στην πράξη κάνουμε έναν έλεγχο που θα επιστρέψει true ακόμα κι αν αυτό εμφανίζεται σε κάποιο σημείο στη μέση του customer name. Εδώ θα έπρεπε η αραχνοαίσθηση να προειδοποιήσει πως πιθανότατα κάτι γίνεται στραβά -- σωστό θα ήταν είτε ο έλεγχος να γίνεται με τρόπο που όντως να εξασφαλίζει ότι μόνο αν βρεθεί ως κατάληξη θα έχουμε επιτυχία, είτε να αλλάξει το όνομα της μεταβλητής σε κάτι καταλληλότερο. Ακόμα και στην περίπτωση που για οποιοδήποτε λόγο ο οποίος δε φαίνεται σ' αυτή τη μία γραμμή είμαστε εξασφαλισμένοι πως το suffix δε μπορεί να εμφανιστεί παρα μόνο στο τέλος, και πάλι η χρήση της strrpos αντί για την strpos θα ήταν προτιμότερη -- απαραίτητη, αν θέλεις να λέγεσαι senior.

Τέλος, είναι ενδιαφέρον πως ενώ ψάχνουμε σε κάτι που ονομάζεται "foreign customer name" και άρα είναι πιθανότατο ότι περιέχει περίεργους χαρακτήρες -- π.χ. για ελληνικά ονόματα τονισμένα φωνήεντα κλπ -- η έρευνα γίνεται με μια function η οποία βασικά αντιμετωπίζει τα strings σα χαζές σειρές από bytes. Αυτό σημαίνει πως για να δουλέψει σωστά με 100% βεβαιότητα θα πρέπει να ισχύουν όλα τα παρακάτω:

Θα πρέπει τα περιεχόμενα των δύο μεταβλητών να έχουν το ίδιο character encoding (π.χ. utf-8).
Ακόμα κι αν έχουν το ίδιο encoding, όλα τα Unicode encodings μπορεί θεωρητικά να βρίσκονται σε οποιοδήποτε από 4 διαφορετικά normal forms. Strings που βρίσκονται σε διαφορετικό normal form φαίνονται ίδια στο μάτι αλλά διαφέρουν σε επίπεδο χαζών bytes, οπότε η strpos που δεν ξέρει απ' αυτά ενδέχεται να επιστρέψει αρνητικό ενώ δε θα έπρεπε.
Το όποιο encoding έχει επιλεγεί θα πρέπει να είναι τέτοιο ούτως ώστε να μην υπάρχει το ενδεχόμενο να εμφανίζεται μέσα στο byte stream κάποια ακολουθία 1+ bytes η οποία να μη μπορεί να ερμηνευτεί αυτοτελώς. Για παράδειγμα, αν ο χαρακτήρας Α αντιστοιχεί σε 0x10 (ένα byte) και ο B σε 0x20 0x10 (δύο bytes), τότε μπορεί να βάλεις την strpos να ψάξει για Α και να "το βρει" ενώ στην πραγματικότητα βρήκε το δεύτερο μισό ενός Β. Αυτό το πρόβλημα είναι αρκετά θεωρητικό μιας και όλα τα unicode encodings είναι σχεδιασμένα έτσι που να το αποκλείουν ενώ στα single byte encodings δεν υφίσταται εξ ορισμού, αλλά το γεγονός ότι κάποιος το γνωρίζει και το αναφέρει κάτι μας λέει.

Τις περισσότερες φορές όταν έχουμε να κάνουμε με international κείμενο η σωστή λύση είναι κάποια encoding-aware συνάρτηση αντί για τις χαζές strpos και σία.

Q: What is wrong with this code? Suggest appropriate solutions/improvements.

final class Multiplier
{
    protected $unit;
 
    public function Multiplier($unit = 1)
    {
         $this->unit = $unit;
    }
 
    public static function multiply($times)
    {
        return $this->unit * $times;
    }
}

Εδώ η βάση είναι πως απλά δε γίνεται να χρησιμοποιείται το $this μέσα σε μια static function. Είναι φανερό (όσο επιτρέπεται να το πούμε αυτό στον προγραμματισμό) με μια γρήγορη ματιά πως πρέπει να αφαιρεθεί το static γιατί διαφορετικά η class απλά δε δουλεύει.

Ένα άλλο περίεργο στην class είναι ότι η $unit property είναι δηλωμένη protected (δηλαδή προσβάσιμη από την class Multiplier και τους απογόνους της) ενώ η ίδια η class είναι δηλωμένη final (δε μπορεί να έχει απογόνους). Αυτό δεν είναι πρόβλημα, αλλά είναι ένδειξη πως για κάποιο άγνωστο λόγο έχουμε στα χέρια μας κώδικα που δεν πολυβγάζει νόημα. Γενικά θα έπρεπε το κάθε τι να γίνεται για συγκεκριμένο λόγο, οπότε με το που βλέπεις final θα έπρεπε να σκέφτεσαι "χμ ενδιαφέρον, γιατί είναι final?" και με το που βλέπεις protected "γιατί είναι protected?". Εδώ ο συνδυασμός απλά μπερδεύει, οπότε είναι σκόπιμο να σταματήσει το μπέρδεμα (π.χ. βγάζοντας το final ή κάνοντας το protected, private).

Τέλος, η class αυτή δε θα λειτουργήσει ως έχει αν βρίσκεται (ή αν βρεθεί στο μέλλον) μέσα σε κάποιο namespace και τρέχουμε PHP >= 5.3.3 (που είναι ψιλοσίγουρο, η σειρά 5.3 έχει ήδη γίνει end of life). Ο λόγος είναι πως στην PHP 5 οι constructors πρέπει να δηλώνονται με το όνομα __construct και όχι με το όνομα της class (όπως σε Java/C++/C#, κλπ), αλλά για λόγους συμβατότητας η PHP δέχεται ακόμα και την παρωχημένη εκδοχή.

Aside: Σε ένα κλασικό παράδειγμα PHP fail, λίγο μετά την εισαγωγή των namespaces στη γλώσσα (version 5.3.0) αποφασίστηκε πως αν μια class βρίσκεται μέσα σε namespace δε μπορεί βέβαια να είναι παλιός κώδικας οπότε σ' αυτή την περίπτωση δεν κάνουμε παραχωρήσεις και δεν αναγνωρίζουμε τέτοιους constructors. Θα μπορούσαν να το είχαν σκεφτεί λίγο νωρίτερα.

Q: What is the best way to escape user input so that security problems are prevented? Answer in less than 100 words.

Ερώτηση-παγίδα, ο σκοπός της οποίας είναι όχι να διαπιστωθεί αν ο ερωτώμενος έχει δει στη ζωή του τη mysql_escape_string (μακριά!!!) αλλά αν πραγματικά έχει καταλάβει πώς πρέπει να προσεγγίζεται το θέμα της ασφάλειας παρουσία κακόβουλης εισόδου (και στο 100% σχεδόν των εφαρμογών, οποιαδήποτε είσοδος επιβάλλεται να θεωρείται a priori κακόβουλη).

Δεν υφίσταται σαν έννοια η έκφραση "escape input". Αυτό που κάνουμε escape είναι το output, και δεν υπάρχει "καλύτερος τρόπος" να το κάνουμε αυτό -- εξαρτάται από το τι είδους output είναι. SQL, HTML, XML, plain text, JSON, shell commands κλπ έχουν το κάθε ένα διαφορετικές μεθόδους που πρέπει να ακολουθήσουμε.

Q: What is wrong with this code? Find as many actual or potential problems as possible and suggest appropriate solutions/improvements.

$sql = "SELECT * FROM dwidget WHERE id = ".$_POST[id];
$results = mysql_query($sql);
while ($row = mysql_fetch_row($results)) {
    // ...
}

Πάρα πολλά πράγματα είναι wrong with this code. Αυτό που βγάζει μάτι (η βάση) είναι ότι υπάρχει πανεύκολη δυνατότητα SQL injection, και αυτό είναι πρόβλημα που επιβάλλεται να διορθωθεί χθες κάνοντας quote και escape την τιμή της μεταβλητής με κατάλληλο τρόπο. Κανονικά αυτό θα ήταν βάζοντάς το ανάμεσα σε single quotes και χρησιμοποιώντας τη mysql_real_escape_string, αλλά επειδή όλη η mysql extension δεν πρέπει να χρησιμοποιείται πλέον στην πράξη θα γίνει διαφορετικά. Η ενδεδειγμένη λύση είναι χρήση prepared statement, στην οποία περίπτωση ο συνδυασμός quote/escape γίνεται αυτόματα.

Ένα άλλο σημαντικό πρόβλημα είναι πως εδώ χρησιμοποιείται η παρωχημένη mysql extension, κατά τη χρήση της οποίας προειδοποιεί το ίδιο το manual. Not much to say, just don't do it.

Περνώντας στα λιγότερο σημαντικά, έχουμε μια σειρά από μικροπροβλήματα τα οποία δε θέλουν πολλά σχόλια:

Δεν υπάρχουν quotes γύρω από το id στο $_POST[id]. Αυτό σημαίνει ότι θα αντιμετωπιστεί σαν constant με την τιμή "id" (string) και η PHP θα δώσει μήνυμα E_NOTICE. Αν γράφεις κώδικα που δίνει E_NOTICE δε μπορεί σε κανένα σύμπαν να λέγεσαι senior.
Δεν είναι απαραίτητο ότι υπάρχει το $_POST["id"]. Αν δεν υπάρχει η PHP θα δώσει E_NOTICE (βλέπε παραπάνω) και το query θα αποτύχει λόγω συντακτικού σφάλματος (βλέπε παρακάτω).
Δε γίνεται κανένας έλεγχος αν το query πέτυχε ή όχι -- έτσι πως είναι γραμμένο, λάθος στο query συνεπάγεται λάθος όρισμα στη mysql_fetch_row, που συνεπάγεται E_WARNING κλπ κλπ.

Tέλος, το SELECT * είναι κακή πρακτική και πρέπει να αποφεύγεται όπως το λιβάνι από το διάολο. Ο βασικότερος λόγος είναι πως δεν ξέρεις τι ακριβώς κάνεις select. Έρχεται αύριο κάποιος και προσθέτει στον πίνακα ένα BLOB column με 500ΜΒ data σε κάθε γραμμή; Συλλυπητήρια, γιατί δεν πρόκειται βέβαια και ούτε είναι πρακτικό να γίνει full review όλου του κώδικα μπας και κάπου παίζει ρόλο η προσθήκη του νέου column.

Τα πράγματα γίνονται ακόμα χειρότερα λόγω της χρήσης της mysql_fetch_row η οποία επιστρέφει πίνακα όπου τα keys είναι integers, άρα μέσα στο while περιμένουμε να δούμε πράγματα όπως echo $row[5] κλπ. Το χειρότερο εδώ είναι ότι αν προστεθεί κάπου ένα άσχετο column τότε ενδέχεται να αλλάξουν τα keys, και απο κει που πριν έλεγες echo $row[5] και έβγαινε το username τώρα να βγαίνει η διεύθυνση κατοικίας. Oops! Το ίδιο πράγμα μπορεί να γίνει αν αφαιρεθεί κάποιο column ή αν απλά αλλάξει η σειρά των columns στη database.

Update: Hat tip στον kercyn για το αετίσιο μάτι και το feedback -- άλλαξα λίγο το wording στην 4η απάντηση για να μη δίνει την εντύπωση ότι φάσκω και αντιφάσκω. Επίσης βάσει των παρατηρήσεών του βλέπω πως η 3η ερώτηση θα έπρεπε να έχει διατυπωθεί διαφορετικά προκειμένου να έχει σαν "σωστή απάντηση" αυτή που δίνω παραπάνω. Στις συνεντεύξεις συζητήσαμε με κάθε υποψήφιο τις απαντήσεις του οπότε δεν τίθεται θέμα παρεξήγησης, εδώ όμως είναι διαφορετικά. ΠΙθανότατα θα επανέλθω σε κείνο το σημείο.

Επεξ/σία 3 Σεπτεμβρίου 2014 από defacer

defacer · 3 Σεπτεμβρίου 2014