• Hans (unregistered)

    That co-worker also didn't understand string comparison: if plantvalue is NOT empty, then ignore it, else (so empty) attempt to add a filter to the query

  • Prime Mover (unregistered) in reply to Hans

    "But 

    !
    doesn't mean not, it means very!"

  • (nodebb) in reply to Hans

    Actually, the else parts are all fine (well apart from the scoping issue described in the main article). For example if partNumber is "", the SQL filter will be PartPlant.PartNum = '' This seems perfectly reasonable if specifying an empty string for one of the fields is meant to filter out all the values that are not empty.

  • Pag (unregistered)

    TRWTF is: how does someone who can't write code that compiles, doesn't even know that it's supposed to compile, become able to check in changes? Something has gone wrong with the recruitment/onboarding process surely?

  • (nodebb) in reply to Pag

    It's only fair: if the managers can be stupid, why can't the associates be stupid, as well?

  • Prime Mover (unregistered) in reply to Pag

    Told the interviewer they were an expert in C#. They got believed and onboarded, and given a simple assignment to write a getter for PartPlant. They were given a how-to doc on how the VCS worked and instructions on this, that and the other, got thrown the ball and asked to run with it. Such a person is not going to admit to not having a clue, the only way to really find out what a new staff member is like is to see where and how they fail.

  • Andy Miller (unregistered)

    I disagree with your ordering of skills.

    Not putting in gaping security holes has to come first. Until your code compiles, it is harmless.

  • Brian Boorman (unregistered)

    It's only vulnerable to injection attacks if the user can control the contents of the variables. We have no context here to say whether that's the case or not. For all we know plant and part numbers come to this code as the result of a previous query to the PDM database and are thus controlled.

  • tbo (unregistered)

    The best part is you can kind of tell that this wasn't the first set of errors they got. I can only assume they added the "string" to each line because they originally didn't have it there and it complained that the variable was undeclared. So you add it to every line, right? Then you get these other compile errors, but you just declared the variable, so the compiler is clearly wrong, and the code will probably work anyway, so just check it in and be done with it.

  • Ulli (unregistered)

    It is not vulnerable against SQL injection at all, because it doesn't compile AND the logic of the if clauses are inverted. Adding an empty string to a filter string is another WtF.

  • ricecake (unregistered)

    It doesn't even pass the "Dude. It compiles." test. https://thedailywtf.com/articles/flawless-compilation

  • jochem (unregistered)

    Yep, this is what juniors do. They shouldn't be writing SQL therfore. Today I had to correct a junior who had put his code in several places, some of which in DEBUG ifdefs which he didn't understand. That gave strange effects in staging...

  • Argle (unregistered)

    I once inherited a project done in C. It behaved badly. I noticed all warnings were turned off in the build batch file. I turned them on. Bad mistake. The warnings scrolled off the top of the screen for several minutes. I know I have ignored some warnings before, but almost all I saw are considered compiler errors now. E.g.,

    foo(int x) { // do stuff, but with no return keyword }

    bar() { if ( ! foo(5) ) { // report horrible error } }

    8051 CPU, return value is in the A register as I recall. Great plan: build your error response based on a random number. And let's play Wheatly and unplug the alarm.

  • Zygo (unregistered)

    Even if we ignore the C# compile issues, the SQL code it generates won't compile either.

  • (nodebb)

    You have to wonder what their source control setup is like. Where I'm working we are using git, bitbucket and jenkins. In order to merge code it needs to compile on jenkins and have one other person perform a code review and accept it.

  • Airdrik (unregistered) in reply to Holywhippet

    ... including that it passed all the tests. Right? there are tests, right? (well, it's probably generous to assume there are tests, or even sane source control management that has such guards against merging code that doesn't build and pass the tests) You at least tried running the code to make sure it worked, right?!? Well of course not, because the stupid IDE that I'm using won't let me run it. :uberfacepalm:

  • (nodebb)

    I suppose mastering the art of writing code that compiles needs to come before writing code that doesn't have gaping security vulnerabilities. After all, code that can't run can't be exploited either.

    So, are you saying TRWTF is languages that need to be compiled, compilers that allow "gaping security vulnerabilities", or any code that runs.....?

  • NULL (unregistered)

    Looks like someone who has never developed in c# before and came from something like Python where the scope logic is different

  • Donald Klopper (google)

    Just building up SQL statements in your program doesn't constitute a security risk.

    ORM tools do that all the time.

    What is risky, is handling "parameters" as plain-text and padding the SQL with input values from users. Those should obviously be handled as parameters.

  • Jay (unregistered) in reply to Zygo

    It's missing the base query, but the SQL should compile fine once appended to the end of a proper base query.. If it actually worked. I also want to point out that his conditions are wrong. He's only filing in the filter string when the condition variable is ""

    "if (plantvalue !="") Then Filter = Filter + "" Else filter= filter + " AND PartPlant.Plant = '" + plantvalue + "'" "

    I took out the declarations. He does this in all 3 conditionals, probably because he wrote the first conditional wrong and just copy-pasted it without testing it.

  • Gnasher729 (unregistered)

    I’d recommend to the developer to declare “string filter” before the if/else sequence and the error goes away. Warnings about unused variables can obviously be ignored. Failing tests can be avoided by not writing tests.

  • Quirkafleeg (unregistered)

    Must have been an ex incompetent JavaScript programmer who once looked at ES6's "let" and "const" and decided to ignore learning about scope.

  • David Mårtensson (unregistered) in reply to Brian Boorman

    That's a dangerous assumption.

    Just because the user cannot do so now does not mean at some time in the future a change gives them the possibility.

    I never ever assume that any variable is safe. Sure today there are much better ways than string concatenation, but even back when that was a common strategy we did not trust any variable no matter where it came from.

    Every time you concatenated it you wrapped it in a function that made all the escaping, even if you used the same variable 3 times in the same line. Just in case someone would copy paste the code and forget the escaping part.

    And it worked.

    The key to security is to strive to never leave a single gap, because the attacker need only one mistake that they find.

Leave a comment on “Filtering Out Mistakes”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article