• (nodebb)

    $where = "where comment = 'frist'";

  • Foo AKA Fooo (unregistered)

    Well, string interpolation is just a more sophisticated form of string concatenation, so actually nothing to see here, (bad) business as usual.

  • Prime Mover (unregistered)

    Not to mention how when it is created it's "$fields" and when it's used it's "$field" ...

  • Alex (unregistered)

    It feels like the author of this code is inches away from reinventing Backus-Naur form.

    I'm fairly sure I've produced similar code previously, but that was because there was a specific business need. As an example and/or mea culpa, there was one project where I had to build the data pipeline for an insurer's actuarial team. This sounds pretty straightforward, but (a) the pipeline needed to incorporate a dozen different checks, at four different points in the process, for three qualitatively different kinds of output, with the exact conditions being different across each of 50 products, and (b) the actuarial team had insisted I use Excel/VBA. Bozhe moi.

    I did eventually come up with something that worked and was easily reconfigurable for new products / tests / boundary conditions. However, the process was a bit... interesting. Some parts, such as extracting ages from the source data, I just had to hand-code for each product - some products used age last birthday, some used age next birthday, some used age nearest birthday, and some used an indecipherable morass of rounding bugs. Some parts, such as the data checks, I could set up a user-editable configuration table (in Excel), use that to generate the SQL code for individual data tests (in Excel, God help me), and iterate over that to generate text files (in VBA). At this point I know I'm going to Developer Hell when I die.

    I then had to build a (fairly elegant IMO) recursive templating system in VBA to stitch all of these components together in different ways depending on whether you were generating point-in-time model points or analysing decrements over the period between two extracts. Yay.

    It all worked, it all met the brief, it all minimised the chances of things falling over the second I walked out the door, and it was all documented to death. But if you excerpted any one section from the resulting ~200k lines of SQL, it would make for one heck of a WTF.

  • Brian (unregistered)

    “Why is the WHERE keyword part of the $where variable instead of inline in the query, but that isn’t the case for SELECT or FROM?”

    So the WHERE can be empty. Like when querying a list of states or other values for a list.

  • RLB (unregistered) in reply to Prime Mover

    Not to mention how when it is created it's "$fields" and when it's used it's "$field"

    Which is a great way to get bugs when, in the real code, that variable really is created as $fields just that one time, and the query is done with the fields from the previous query. Very elegant.

  • Jaloopa (unregistered) in reply to Brian

    The default should be WHERE 1 = 1

  • (nodebb) in reply to Jaloopa

    I am of the school of thought that if something does not need to be there, don't put it in. When faced with this when programming in VBA in MS-Access (thus string interpolation is not really available), I tend to resort to conditions to concatenate the Where clause.

    WhereClause = ""
' All sorts of conditions which concatenate **" AND " condition** to the end each time.
If WhereClause > "" Then
    ' Because the WhereClause starts with " AND ", skip the first one.
    strSQL = strSQL & vbNewLine & "WHERE " & Trim(Mid(WhereClause, 4))
    ' Using vbNewLine for easier debugging.
End If

    Addendum 2020-10-22 08:36: Oops, I guess markdown does not get interpreted inside CODE blocks. Ignore the ** in the first comment.

  • (nodebb) in reply to Nutster

    I hope you realize you are flat out admitting that you still, in 2020, build queries using string concatenation and you are OK with it.

    Please read this: https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html

  • Alex L (unregistered) in reply to Prime Mover

    THIS. The fact that this query will be using whatever the last query stored in the global $field, and not the "carefully constructed" $fields...

  • Randal L. Schwartz (google) in reply to Jaime

    Also see https://bobby-tables.com for the same thing with specific changes and a much easier URL to remember.

  • tbo (unregistered)

    I asked the other people at my company and they assure me they did not submit this. I'm not sure whether it's reassuring or frightening that other companies have this sort of code around. On the plus side, at least in our company, if you come across this nowadays you are probably looking at very old (15-20+ years old) code.

  • (nodebb) in reply to tbo

    if you come across this nowadays you are probably looking at very old (15-20+ years old) code

    Must be nice. I come across things like this in code reviews.

    There's a guy I work with that manages a highly-customizable workflow application. He builds data lookups using a product feature that loads an html file and displays it in an embedded IE control inside the app. He writes database access code in VBScript, and the web pages have javascript that's been cut and pasted from page to page since 1996.

  • (nodebb)

    As long as the things being concatenated aren't derived from user-input, there's no problem. The code is just fine (assuming it is just a typo for field vs fields). There is no chance for Sql injection in this article's code...instead it is just a strange way to put a SQL statement in your code.

  • (nodebb) in reply to Auction_God

    Perhaps you know more about the code than we do. There is obviously code missing, because we can't determine why $val1 is pre-quoted and $val2 is not. It seems reasonable to suspect that the vulnerability is there.

    The point is that SQL injection is so common that the only sensible way forward is to establish a standard way to call database that is injection resistant and reject all code that doesn't follow the standard. Don't allow arguments like "I followed all 906 possible code paths and none of them are vulnerable". Instead, all calls are locally protected and are verified to follow the pattern rigidly, preferably by a code analysis tool. The one or two places where you are doing something completely out of the ordinary and need to code outside the box, serious pen testing is required.

  • Sole Purpose Of Visit (unregistered) in reply to Jaime

    Not to mention that any future state will be unpredictable from the current state.

    In other words, all interpolated or concatenated variables might very well be generated today via entirely internal methods. But who's to say that (by an entirely innocent development in future) none of those variables will have external exposure?

    I mean, seriously. Just use prepared SQL statements and have done with it.

    OTOH I'm with Remy here. There's a certain deranged beauty about the code in the OP. The ideas behind it show a lot of promise, right up until the point where reality blows both your feet off.

  • Naomi (unregistered)

    Y'know, at my old job, they really liked building dynamic queries in SQL. It always seemed WTF-y to me, but I'm not a DBA, so maybe I was missing something.

  • löchleindeluxe (unregistered) in reply to Jaloopa

    People who write code like the submitter's example will argue that "WHERE 1=1" will create a performance bottleneck.

  • (nodebb) in reply to Naomi

    It always seemed WTF-y to me, but I'm not a DBA, so maybe I was missing something.

    It's actually not a DBA thing, SQL Injection vulnerabilities are almost always in the application code.

    Think of SQL Injection like the teenage joke of calling a bar and asking for the bartender to see they can find "Amanda Hugginkiss". Thinking it's a DBA problem is like expecting the phone company to prevent this.

    The real solution is to tell the bartender to say "I'm looking for a customer with the name: XXX" rather than to speak more naturally, but in a way that the sentence structure can be subverted.

  • Naomi (unregistered) in reply to Jaime

    I know what an SQL injection vulnerability is. I meant "maybe there's some reason the practice I described in my original post makes sense that I'm not aware of".

  • (nodebb)

    There is not. The entire will of the security industry is bent on making as many people as possible aware of this fact, but skulls are thick. It's been #1 on the OWASP Top Ten for nearly a decade, but we still get doubters.

    I work in security leadership. I spend almost all of my time "getting people on board"... they really, really, really are reluctant to take an outside expert's guidance when it means they have to change what they've been doing for thirty years.

    Addendum 2020-10-23 09:59: BTW, by "outside expert" I don't mean me. I mean OWASP, Bruce Schneier, the NSA, VeraCode, Rapid7, etc...

  • MiserableOldGit (unregistered)

    I've not come across many people who "doubt" SQL injection exists (I bet that generates some anecdotes), but certainly many who aren't aware of what it really means and/or that their cack-handed methods to mitigate against it ummm ... don't.

    As long as the things being concatenated aren't derived from user-input, there's no problem.

    Whilst this is true, this is where we end up with logic bombs hiding in our code base for decades. I believe in not over engineering things, but I've also been burnt by the twin evils of scope creep and code reuse. What you write works beautifully, and then someone decides to call it from somewhere else, or give it input you never had in mind and it still works beautifully, until the vulnerability "is discovered" and all hell breaks loose ... and guess who gets blamed!

    20 years ago, if you'd been given MS Access 2 and VB4 and no budget for tools, training or testing such shortcuts were understandable. These days, doing it right isn't any extra effort if you familiarise yourself with the libraries you have available. If you are the guy maintaining the original MSACC/VB4 frankenstein complete with a vintage ASP front end you have my condolences.

  • 🤷 (unregistered)

    At my old company we had tons of SQL queries that were vulnurable against injections. Luckily about 90% of those programs where used in-house only, and knowing the kind of people who used the program, the chances of someone actually knowing how to pull off an injection attack (let alone know what SQL even is) was 0. BUT, there were a few queries that where used on the website, ie open to get attacked by anyone. Luckily I could convince my colleagues to parameterize those queries after dropping a table in a test database via an SQL injection.

  • Some Ed (unregistered) in reply to Jaime

    The answer you're looking for is to use prepared statements with all of the variables defined as parameters, and then execute the prepared statement with the necessary parameters.

    It's more awkward than string concatenation or interpolation, and the point about why $join and $where were variables here gets to the heart of it: a prepared statement either has a join or does not. It has a where or does not. You can't have parameters for those features.

    There's other limits. I don't do SQL most of the time, so I don't recall all of the limitations. But when I converted our app from using interpolated strings to prepared statements, I went from a single interpolated string (hauntingly similar to the one above) to about 30 prepared statements.

    It was kind of interesting that just making that change alone caused a 5% improvement in our search performance, which was roughly a 1% improvement of our overall application.

    That having been said, that was over 20 years ago, and I have not really dealt with interpolated string queries since. (I mean, I've certainly rejected them in code reviews, but even that hasn't happened since I last changed roles. I mean, I still review code, but it doesn't include any SQL at all.)

Leave a comment on “Query Elegance”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article