Sometimes you get busy and delegate small tasks to junior programmers. But if you don't pay close enough attention you find yourself with this kind of code in production:

class DivtoggleController extends Zend_Controller_Action {

    public function closeAction() {
        /* ... code removed for brevity ... */

        $req = $this->getRequest();
        $formData = $req->getPost();

        $d = $formData['div'];
        $i = $formData['id'];

        $dm = new Model_DivtoggleManager();
        $rs = $dm->setDivToggleById($d, $i);



class Model_DivtoggleManager extends Zend_Db_Table {

    public function setDivToggleById($div, $id) {
        $result = $this->getAdapter()->query(
           "update div_toggle set " . $div . "=1 where id=" . $id


So, given that I have removed the authentication / session management logic for brevity, who can tell me what possible problem there might be with this sample?

  • Why is this not stored in a simple cookie?
    – Darknight
    Apr 18, 2011 at 12:47
  • 1
    @Darknight, you are assuming there is a session in which to store the cookie. Whether there is or not is irrelevant to the larger security issue. Apr 18, 2011 at 22:51

7 Answers 7


You can teach them. Everyone does this in the beginning, even you. If this type of code makes it into production, it's the senior folks fault; not the junior.


One of the things that I have done is I personally have taken to pro-actively asking people to review my code (including the juniors) before a release. The code gets reviewed, the junior folks see it as a learning experience, people lose the fear of code review as a punishment, and they start doing the same thing.

  • 2
    +1 Have to agree. You can't blame a (presumably) junior programmer because you've assumed a level of knowledge they may not possess. (That said, you'd like to think such issues are well known in this day and age. Then again, I'd like to think that I was talented and good looking, etc.) :-) Apr 14, 2011 at 20:18
  • A fair enough statement. I guess I should ask a follow up question asking why management would insist on launching code into production that hasn't been approved by the senior programmers. Do I risk my job over a minor security issue? Apr 14, 2011 at 20:27
  • 1
    @Roger Halliburton: Well, if that security issue does somehow get exploited, what's the worst that could happen? And why would pointing out security problems cost you your job? Does management not want to have this dealt with? Apr 14, 2011 at 20:33
  • 1
    @Roger - it's only minor until you get hacked. :) I think that putting code into production without a review (no matter the level of the developer) is a failure. Unfortunately, it is a very common practice in a lot of companies; and, ime, it usually takes a major oh-$4!t before management starts to value things like code reviews.
    – John Kraft
    Apr 14, 2011 at 20:36
  • 1
    @Roger: All code should be reviewed, not just code written by "junior" programmers. Even senior programmers make mistakes. Apr 18, 2011 at 7:53

Hack their code in front of their eyes then show them how to fix it. Over and over until they understand.

  • 4
    Email them every morning: "By the way, I dropped your database again last night via another SQL injection vulnerability you left in your code. I know how much you like restoring database backups! :D"
    – Ant
    Apr 18, 2011 at 7:55
  • 2
    @Ant: Be nice. Do it shortly after the scheduled backup, not shortly before. Apr 18, 2011 at 8:09
  • @Donal: do it shortly after the backup, then tell them the backup failed and let them sweat for the rest of the day before restoring the backup overnight.
    – jwenting
    Apr 18, 2011 at 8:27

You can mandate them to take a class as soon as they join your company, before they have source control access, that introduces them to SQL injections, cross site scripting, cross site request forgery, and other common vulnerabilities. Cover examples face to face, break bad code in front of them, have them break bad code, and point them to the OWASP site for more info once they "graduate".

You can additionally mandate the use of a custom library that handles this for you, but that's only a secondary solution as they'll be sure to run custom queries when that becomes more convenient.

If you have the resources, ensuring more senior members of the team verify their diffs before committing can be useful also.

Knowledge is power!

  • 3
    Weeding them out before they join your company is better. If you're hiring someone to write anything that uses SQL, not asking interview questions about injection borders on incompetence.
    – Wooble
    Apr 14, 2011 at 20:26
  • I'd generally agree, but a very smart and ambitious junior hire is much, much cheaper than a "PHP developer with N years of experience," that isn't guaranteed to be that much better. Smart junior devs can pick this stuff up really quickly and be quite an asset.
    – yan
    Apr 14, 2011 at 20:27

Assuming that it is the insecurity to which others have referred, as a developer of any level, it's easy to forget that getPost() is not securing the data first.

One way around this is to:

  1. Write a class that gets all POST/GET data and writes it as is to a Singleton class called 'insecure_data'. Then clear the POST/GET arrays.
  2. Developers than have to retrieve POST/GET data from the 'insecure_data' array, not POST/GET arrays.

Any developer that retrieves something from an array called 'insecure_data' and doesn't bother to secure it is either ignorant or lazy. If it's the former, provide training, after which it must be the latter - and then you have a disciplinary issue, not a programming one.

  • Wow, that is needlessly convoluted and still doesn't solve the problem. It isn't a matter of scrubbing the input just for fun, but of scrubbing the data that you are putting into the database, and that data could theoretically come from anywhere, from a web form, or an email, or an XML document that someone uploads. In all of these cases you need to scrub when it goes to the database. Apr 14, 2011 at 21:54
  • @RogerHalliburton Of course it doesn't solve everything, but it's a concept (a sort of design pattern, if you will) rather than a fully-fleshed out security measure to make insecure data look insecure.
    – Dan Blows
    Apr 14, 2011 at 22:04
  • Ah, I see. But you are just fighting human nature, if you tell someone "This object is insecure unless it is flagged as secure" and they try to use it for something, most of the time they'll flag it as secure without actually checking. Well, enjoy the reputation boost. Apr 14, 2011 at 22:26
  • From a junior developer's point of view, just seeing $insecure_data raises a flag in a way that just seeing $postdata (or whatever) does not. The idea comes from Joel Spolsky's "Make wrong code look wrong". If the human nature of your developers is such to ignore that red flag, then you either need to provide a lot of training or get some new developers.
    – Dan Blows
    Apr 14, 2011 at 22:32
  • I don't keep Spolsky on a pedestal. These are developers who easily ignore inconsistent tabbing, they aren't going to think twice about using something that is named "insecure". Apr 14, 2011 at 22:45

One of the best guides that I have read on web security is this Ruby on Rails security guide. Although it is Ruby on Rails, a lot of the concepts apply to any web development. I would encourage anyone new to give that guide a read.

  • 2
    Everyone knows Ruby isn't secure. Apr 14, 2011 at 22:27
  • 5
    @Roger: “Everybody knows” all sorts of things that might or might not be true. I advocate a reality-based approach to programming, not a hearsay-based one. Apr 18, 2011 at 8:11

The code you've linked above is susceptible to an SQL injection attack, because the HTTP inputs you're using in the query have not been cleaned with mysql_real_escape_string or any other means.

  • 1
    oh, i thought we were answering this like a test question :]
    – Ryan
    Apr 14, 2011 at 20:06
  • Downvotes are a bit lame, since the wording of the question was changed ex post :P
    – Ryan
    Jun 30, 2011 at 18:38

In terms of your (presumably overriding) "how can I get programmers to stop doing this" question, I'd say that mentoring them on a regular basis, carefully explaining the issue in question (and the potential fallout, etc.) and emphasising the importance of code vulnerabilities (both in terms of SQL injection and cross site scripting, etc.) is probably the most sensible solution.

If they keep messing up despite all of the above (you might want to cast an eye over their commits, etc. rather than finding out "live"), then the problem is either that you're failing them as a mentor, or that they perhaps need to find something more suitable to do for a living.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.