.
That guestbook script, annotated - Grin with cat attached
Previous Entry Next Entry
That guestbook script, annotated Apr. 14th, 2006 06:39 pm
http://www.phase.org/files/guestbookannotated.html

From: babysimon
Date: April 14th, 2006 - 07:13 pm (Link)
That's a really interesting way to interview.

Two idle questions from a non-PHP programmer:

1) What do you mean by "But far more subtly, this probably won't set the "From:" header correctly without setting "-frobomail@mydomain.com" as the 5th parameter to mail()."? A quick look at the PHP documentation suggests the 5th arg to mail() allows you to add arbitrary headers to the mail, with interesting security issues, but I don't see what you mean.

2) Why do you say "Use of "select *" here is valid, as we can be assumed (from other evidence in this script) to be fetching all fields from each row."? The only evidence I can see is the insert statement, and that doesn't prove anything if the table has additional nullable columns.

Oh, and thank you for mentioning CSRF. I never heard of / thought of that before, and I probably have sonme work to do on Tuesday as a result.
From: wechsler
Date: April 15th, 2006 - 12:24 am (Link)
1) Headers is param 4; command-line switches to mail is param 5, and various sendmail-alikes will need the -f switch to properly assert the sender. Otherwise you fall into lots of spamtraps.

2) For the purposes of this exercise you can probably assume you're being told all you need to know. If you were unsure about the fields in the table you could just ask the hypothetical newbie, ie the interviewer. I'd expect candidates to check the advice they were giving was valid. This is therefore as much a personality check as a technical one.

CSRF: is actually fairly new to me, too, and would be handily defanged in this case by fixing the more obvious security issues. I don't expect candidates to get everything in this test (the -f issue is frankly me taking the piss and no-one spotted it).

I'd have missed a few of these points myself until recently and there may be more I'll learn about soon. I'm finding I learn a *lot* from PHP-London about this sort of thing.

However anyone who can't spot the blatant SQL injection or XSS attacks is not going to get very far. PHP is very much a shoot-yourself-in-the-foot language and requires coders to take real care, and I'd rather this were the case than use a language or security model that tries to protect me from myself. Again this may be because I started as a C coder.
From: wechsler
Date: April 15th, 2006 - 09:52 am (Link)
2) On more sober reflection I've clarified the annotation on that line.
From: meico
Date: April 14th, 2006 - 07:38 pm (Link)
My very first critique would be that there was only one fairly useless comment. Basically the script is wholly un-commented.

Regardless of any of the other mistakes this is cardinal sin #1. I wouldn't hire anyone who didn't point this out... (even if they caught everything else)
From: wechsler
Date: April 15th, 2006 - 12:11 am (Link)
Personally I use PHPDOC comments in the appropriate places, and beyond that comments are required only at unsually unclear points. My take is that clear, uncommented (or minimally commented) code is far preferable to unclear, commented code in which the comments will invariably get out of sync. PHP lends itself to code readability.

I would not be worried about PHPDOC-only commenting in a basic but clearly written script. YMMV, particularly depending on the language used (if I'd have seen something like this in C code for example, I'd be pissed).
From: meico
Date: April 18th, 2006 - 02:42 am (Link)
comments are required only at unsually unclear points

The trouble is that a person writing code is usually unable to get into the mindset of someone who is not down in the trenches and thus if they use that rule the comments can often be far too sparse. :(

Of course, I agree that clean code is preferable to bad code with good comments. I always advise that people fix bad code instead of just commenting it.

Also, I generally advise that comments tell why a particular line is there, not how it works or what it is doing- unless the how it works or what it is doing is non-standard or complicated in the given context... That way the comments compliment the code instead of just being redundant chaff that gets in the way. (and it also means you can often change the specifics of the code without having to change the comments)

particularly depending on the language used (if I'd have seen something like this in C code for example, I'd be pissed).

I too have found that language choice can make a difference to the amount of comments that should be used- assmebly being an extreme example where practically every statement should have its own comment.
From: wechsler
Date: April 15th, 2006 - 10:01 am (Link)
Actually I had the choice of no, or misleading, comments for the purpose of this test. It wouldn't have been so much of a test otherwise...
From: meico
Date: April 18th, 2006 - 02:27 am (Link)
For a test I definitely think you made the right choice. :)