Escape apostrophes in generated queries

[NocturnalSolutions]

As discovered in #141, it appears Kuery is not escaping apostrophes inside strings in generated queries. For example, the following code:

        s = Select(from: t)
            .where(t.a == "Grocer's apostrophe's")

Will build a query like:

SELECT * FROM "tableSelect" WHERE "tableSelect"."a" = 'Grocer's apostrophe's'

Which will cause a failure with the SQL engine. The proper query, with apostrophes escaped, should appear as:

SELECT * FROM "tableSelect" WHERE "tableSelect"."a" = 'Grocer''s apostrophe''s'

The changes in this PR add a new static method to the Utils struct called escapeApos() which escapes apostrophes. It adds tweaks to other functions in the Utils struct associated with building values for SQL queries to escape apostrophes in the resulting strings. Two test cases have been added; one with an Insert query and one with a Select query.

Impact: Probably a minuscule increase in execution time due to the additional code. Also, anyone using Kuery who had implemented their own code to escape apostrophes to work around the bug might have double-escaping problems now.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (next@d1f891a). Click here to learn what that means.
The diff coverage is 77.77%.

@@           Coverage Diff           @@
##             next     #142   +/-   ##
=======================================
  Coverage        ?   84.33%           
=======================================
  Files           ?       44           
  Lines           ?     3651           
  Branches        ?        0           
=======================================
  Hits            ?     3079           
  Misses          ?      572           
  Partials        ?        0

Flag	Coverage Δ
#SwiftKuery	84.33% <77.77%> (?)

Impacted Files	Coverage Δ
Sources/SwiftKuery/Utils.swift	54.54% <77.77%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1f891a...38b827d. Read the comment docs.

[kilnerm]

Looks good barring the method name. We avoid using truncated names elsewhere so it would be good if we can do the same here.

[kilnerm]

I think this would be better without truncating the method name, eg, escapeApostrophes.

[Trundle]

I would recommend not following this approach, as it gives a false sense of security. There are more ways to generate broken queries. For example, what happens if the input string is something like foo \' bar? It is also a bit dependent on the SQL dialect what and how to escape.

The README already states that injection is possible and that parameters should be used instead (see the section SQL Injection Prevention using Parameterization). So, even if surprising, the observed behaviour is somewhat expected.

If this behaviour is surprising (and I think it is), then I would suggest that instead of escaping some problematic values, Kuery should rather follow the route of other SQL DSLs (such as sqlalchemy in Python for example) and translate a Select(…).where(some.key == "Foobar") into a parametrized query and pass "Foobar" separately to the query input as parameter.

[NocturnalSolutions]

I would recommend not following this approach, as it gives a false sense of security.

This is more about functionality than security, though it should also increase security.

For example, what happens if the input string is something like foo ' bar?

It would become foo \'' bar and then work as expected if that's what someone was trying to insert. AFAIK, the backslash isn't an interesting character in SQL.

It is also a bit dependent on the SQL dialect what and how to escape.

I considered this, but after doing a bit of research, I couldn't find a major SQL dialect that didn't escape apostrophes by doubling them.

The README already states that injection is possible and that parameters should be used instead

I don't disagree with you that using parameters is ideal, but Kuery lets you build queries without using parameters, so we should ideally get them working the best way we can.

If this behaviour is surprising (and I think it is), then I would suggest that instead of escaping some problematic values, Kuery should rather follow the route of other SQL DSLs (such as sqlalchemy in Python for example) and translate a Select(…).where(some.key == "Foobar") into a parametrized query and pass "Foobar" separately to the query input as parameter.

That's an interesting idea. Get to work on that PR. :)

(But seriously, if you want to take the lead on that, I'll pledge to help out with testing and such.)

[groue]

I considered this, but after doing a bit of research, I couldn't find a major SQL dialect that didn't escape apostrophes by doubling them.

If only it were true. Look, for example, at mysql_escape_string and mysql_real_escape_string in PHP (both deprecated now with parameter-based apis). You can't really be naive about this subject. When it hits, it hits hard.

That's why parameters were invented.

You can think about it both in terms of functionality and security. But you may consider granting security first and foremost. This usually involves making unsafe APIs harder to find, and type.

In GRDB, for example, SQL injection bugs are visible, because unsafe strings can only trigger injection when they feed an api explicitly named with sql in its name:

let dangerousString = "Grocer's apostrophe's"

// All identical queries, all safely use parameters
let request = Player.filter(nameColumn == dangerousString)
let request = Player.filter(sql: "name = ?", arguments: [dangerousString])
let request = Player.filter(sql: "name = :name", arguments: ["name": dangerousString])

// SQL injection bugs:
let request = Player.filter(sql: "name = \(dangerousString)")
let request = Player.filter(nameColumn == SQLExpressionLiteral(dangerousString))

It is possible to inject raw SQL in the query, as you can see. Some users need that. Some will trigger an injection bug on the way.

But there is no public or internal api that escapes a string. It's useless. No one needs escaping a string. When you think you need to escape a string, the answer is parameters. Don't repeat the history of PHP.

Edit: For the sake of precision: SQLite sometimes absolutely needs SQL literals and won't accept parameters. You can't use a parameter, for example, when you need to specify the default value of a column: CREATE TABLE t (a TEXT DEFAULT 'foo'). This is a very rare exception. When this happens, GRDB doesn't try to escape values, because it's so full of traps. Instead, it asks SQLite to do it.

[NocturnalSolutions]

I think we're talking past each other. I'm not trying to make Kuery impermeable here. I'm just trying to make it possible to use apostrophes in a query. Kuery users will still have to be aware of the same security issues as before.

[Trundle]

I get the intention, but in my opinion the suggested solution makes some queries work (but not all) and breaks other legitimate queries.

One example is MySQL. In MySQL, \ is an escape character (except if the NO_BACKSLASH_ESCAPE mode is active). That means that your example query of .where(t.a == "Grocer's apostrophe's") will work, while a query such as .where(t.path == "C:\some\file.txt") will not do the right thing. On the other hand, a query such as .where('t.f == "Grocer\'s apostrophe\'s") that worked before will now break. I find that not very obvious.

[kilnerm]

Having had a brief chat about this I am going to take a quick look at what the implications of using parameters beneath the api are.

[kilnerm]

Having had a brief look at the possibility of using parameters beneath the API it certainly seems a feasible and desirable direction to move in.

I am in the process of finalizing version 3.0 of SwiftKuery and associated plugin updates. Once released I will take a look at getting changes to use parameters into a minor release ASAP.

[mbarnach]

I think this PR is good now? I've recently run into the problem again, and it would be nice to have it integrated.
Can we merged it?

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ NocturnalSolutions
❌ kilnerm
_{You have signed the CLA already but the status is still pending? Let us recheck it.}