Should I tell my coworker his code is WTF-worthy?

[presidentender]

The lead developer on my project wrote a stored proc using the 'exec' function with a string built from parameters, a la The Daily WTF. I asked if I could just change it instead of doing SQL injection filtering (since a proper parameterized query would make the filtering unnecessary) and he said no. I'm the new guy here and don't want to make waves, but I'd really rather not deal with this type of thing again. It makes matters either better or worse that he's a good developer and estimator in most other areas.

 

[Titan]

Depends entirely on structure and leadership on the team, it's your call, not a 100% stupid thing to do.

I draw the line when the office won't let me write my code correctly. I put my foot down and defend good practices or it's new job time.

 

[Markbnj]

I think the fact that he said 'no' probably answers the question for you. If he's a reasonable guy, and good in other areas, then he should both see the sense of your argument, and be willing to explain his thinking to you. More so when you're raising the possibility of serious security implications. Since he won't do either, there's something else going on. You can either leave it alone until you have some seniority and team respect, or take a hard line like Titan suggests. But if you go that route be willing to sacrifice the relationship, and perhaps the job, for your principles.

 

[sourceninja]

Or you could make your code inject bad sql to screw with him. Drop a table or two.

 

[Ken g6]

Originally posted by: sourceninja
Or you could make your code inject bad sql to screw with him. Drop a table or two.

I wouldn't do that as a programmer. Especially if there was any chance it might happen in production.

But you could leak word of the issue to the testing team, if there is one. 🙂 (Remember, the contents of emails are on the record, but phone calls usually aren't.)

 

[degibson]

If your coworker writes bad code, by all means just tell them directly, assuming they're not incompetent/@$$hole/both. However, if your superior writes bad code (as it appears to be in this situation), then its a real sticky situation.

As much as I hate to stroke Ken's ego by agreeing with him, I like his idea of leaking it to the testers. You can make good contacts there, too. Testers are handy people to have on your side on a rainy day.

 

[KB]

I would send the lead developer an email demonstrating the flaw in the SP and discussing the risks. If he says NO to that then leave it at that, but archive the email and be prepared to bring it back to show your superiors when your app is SQL injected and the customers want some blood.

 

[presidentender]

Thanks everyone for the replies. I've got some filtering in place already to avoid SQL injection, so this one will go unnoticed, and I'm too passive to want to make waves. Unfortunately, the test team consists of us, so there'll be nothing there either.

 

[SilentZero]

You did the right thing. It's your lead developer, you put out your suggestion/idea, he said no, so case closed. I wouldn't continue pushing it especially if you are new there. If a problem occurs in production because of it, then he will have to answer for it, and might end up listening to your suggestions in the future.