[sodapopcan]

Sounds about right.

Not even a few years ago I worked with people who insisted it was ok to write injection unsafe code if you knew for sure that you owned the injected values. Didn't matter that maybe one day that function would change to accept user-supplied data, that's not their problem! It was a Rails app and they were literally arguing wanting to do:

    .where("id = #{id}")

over:

    .where("id = ?", id)

in those certain situations. So, you know, it takes all kinds, I guess.

[PH95VuimJjqBqy]

This is a case of militancy.

If we're talking about a typed integer there is no chance of that turning into an sql injection attack.

If we're talking about a string, I'd probably insist on parameterizing it even if we completely own it just on the off chance that the future changes.

To draw an analogy, gun safety is important and everyone knows it. But I don't practice gun safety while watching television on my couch because the gun is locked away. I practice gun safety when I'm actually handling the thing that is dangerous.

And yes, I realize it being locked away is technically gun safety, it's an imperfect analogy, please roll with it.

[beart]

Your analogy is not flawed, but your conclusion is.

It is a perfect analogy because you are practicing gun safety by locking the gun away. If someone that you are not expecting wanders into your home while you are sitting on the couch, such as a child, they will not suddenly have access to the firearm. This is exactly why you don't assume that you will never receive unsafe input in this situation.

[PH95VuimJjqBqy]

and as you're sitting on that couch watching television you're also practicing car safety because you're not actively breaking any traffic laws.

IOW, you're free to make that claim and you're not wrong per se, but you're not right and it doesn't refute the point.

[beart]

The equivalent analogy is that you didn't leave the car in neutral on the top of a hill.

The number one rule of firearm safety - Treat every firearm as if it were loaded.

And yet children shoot themselves or others all the time because a gun was not safely stored.

But I digress...

[willsmith72]

to be pedantic, just being "typed" is not enough these days with dynamically-typed server code.

[PH95VuimJjqBqy]

I disagree with you, if it's typed it's safe. The issue is if it's untyped or the type isn't enforced (by the runtime, by the compiler, or by the code itself).

I understand your point, I'm just saying if it's actually typed, it's safe.

[vaylian]

> If we're talking about a typed integer there is no chance of that turning into an sql injection attack.

Unless the database table switches to non-integer ids at some point.

[sodapopcan]

Ruby is a dynamic language.

[prosqlinjector]

I think I agree with your coworkers. If the data is predefined constants, then you don't need to worry about injection. All functions have preconditions which must be met for them to work. As long as that's specified, that's acceptable.

Imagine the internals of a database. An outer layer verifies some data is safe, and then all other functions assume it's safe.

The example you're sharing is a bit of straw man. It's just as easy to use the parameter, so of course that's the right thing. But interpolating a table name into the string from a constant isn't wrong.

[sodapopcan]

I'm not sure if this is a troll or not and I don't really want to debate this kind of thing on HN, but you've baited me. It is not a straw man. As I said, the source of the input could change in the future and it could be missed. The safe version is no more complicated than the unsafe version, so why wouldn't you just do the safe one? There is zero advantage to the unsafe way and it's straight up reckless to defend it.

I'm one of those people who moved from Ruby to Elixir. Ecto, Elixir's defacto database wrapper, will throw and exception if you try and write interpolated code like this, so luckily I don't have to have these insane arguments anymore (well, I work alone now, so there are several reasons I don't have to have them).

EDIT: My bad, I glazed past the last part of your statement.

Ya, I think this is probably where some of the defensiveness comes from: using a library vs rolling your own. If you're rolling your own, of course you're going to need to interpolate table names and whatnot, but it shouldn't even be possible to interpolate values. My example and argument is based of Rails, though, where you never specify a table name or anything like that. So in the specific case of my coworkers, they were wrong.

[greiskul]

Yeah, bad code doesn't stop being bad code just because it is correct. Good code not only is correct, but it is obviously so. There are zero excuses in a case like this to write it in the unsafe way. Just because you know a gun is not loaded, doesn't mean you should play with it.

[nextaccountic]

Yeah if a codebase is full of stuff like this, auditing it is awful. It's like, instead of employing computers to check the details your code, force it to be done manually (in an error prone way)

[prosqlinjector]

This is nonsensical. When you use a function, how do you know what it will do? You guess from its name?

> auditing it is awful.

If a function specifies a requirement, you look at the callers and see if that requirement is met. If it's easy to verify in code, you can assert. Is there an easier way to audit correctness?

[noduerme]

Idk. I have some pieces of production code that need to inject `$tableIdentifier`.`$field` into a query, where both are nominally passed from the client. I don't rely strictly on a list of constants in those cases. I take the user request, check the table name against a constant list, then run a query (every time) to describe the fields in that table and type-check them against what's in the user-submitted variables. Then escape them. Anything mismatched in name or shape at any stage of that is considered malicious.

[prosqlinjector]

The only principle I want to defend is that a function is correct relative to its preconditions. If the caller doesn't meet them, that's on them.

[nextaccountic]

That kind of reasoning only works if the language or ecosystem has some kind of compile time error or linter or comprehensive testing that will catch the error if the preconditions ever change. One way of doing is is encoding the preconditions in the type system. Another is through fuzzing

If you keep the preconditions informal and never check them, the code becomes brittle to modifications and refactoring. For a sufficiently large codebase you almost guarantee that at some point you will have a SQL injection bug.

That said, using prepared statements isn't the only way to guard against SQL injections. You can also use a query builder that will escape properly all data (provided this query builder itself is hardened against bugs). Using dynamic sql is the only way to make some kinds of queries, so a query builder is a must in those cases.

What you shouldn't do is to use string concatenation to build query strings in your business logic. It may or may not contain a bug right now, but it is brittle to changes in the codebase.

[prosqlinjector]

> That kind of reasoning only works if the language or ecosystem has some kind of compile time error or linter or comprehensive testing that will catch the error if the preconditions ever change.

Most requirements can't be verified at compile time, or even at runtime in a feasible amount of time.

If you expect functions to do things that they don't say they do, I don't know what to tell you. Conventions and specs are the best we have.

[noduerme]

I think you were broadly misunderstood. If the defined constants come from or are checked against the ones stored in the database, fair play. If they're floating around in some static consts in a code file, also ok as long as that's extremely well documented and someone knows what's what. If some boss pays to cut corners for it to be written with magical constants like "WHERE life.meaning!=42" and then fires the person who they hired to write that script, they deserve whatever they get.

Just like the meaning of life, it's best not to come to premature conclusions. Could all work out, or it could be a funny joke for aliens in the end.