Hi @iwcoetzer,
Araq is correct, your properQuote doesn't do anything. To get a big portion of
SQL injection safety, always use parameterised queries. Never combine strings
with SQL. As soon as you start combining text for queries, you're in trouble.
For example, this is a bad idea:
proc `%` (frmt: SQL, values: openarray[string]): SQL =
let v = values.mapIt(properQuote(it))
type StrSeq = seq[string]
result = SQL(string(frmt) % StrSeq(v))
Run
Ideally the query should be able to be run without being changed at all, and
only parameters changed. As well as better sanitation, this also means your
queries don't need to be prepared each time they're executed and a whole host
of other performance bonuses. For example, each preparation requires tokenising
and sending the query to the server, processing the queries (without running
them), then waiting for a response back! Additionally, parameterised queries
will tell you if there is some type issue relating to parameters and the table
way earlier than if you're combining text.
The obdc library has support for handling parameterised queries natively and
therefore avoid injection. What I would do personally is update the query with
paramters directly:
#[ prepare a SQL script ]#
query.statement = "SELECT TOP 100 * FROM $1 (NOLOCK) WHERE EffectivePeriod
>= ?effectivePeriod" % ["dbo.DimContract"]
query.params["effectivePeriod"] = "201901" # Or maybe this wants to be an
actual date rather than a string?
Run
Note that there is still a string concatination for the table name. In this
case you have two options:
1. Make damn sure your table names can't be entered/edited or corrupted by
users. I would be very cautious when taking this approach.
2. Use dynamic SQL To select from a table, and **pass the table name as a
parameter** as above to the dynamic SQL. This is, as I understand it, the
'preferred' approach, though it does seem a tad unweildy, it is very powerful.
A quick tutorial on how to do this is here:
[https://www.sqlservertutorial.net/sql-server-stored-procedures/sql-server-dynamic-sql](https://www.sqlservertutorial.net/sql-server-stored-procedures/sql-server-dynamic-sql)/
So, in their example the line
SET @table = N'production.products';
Run
would be more like
SET @table = ?myTableName;
Run
You still need to be careful to only use parameterised data in dynamic SQL
though, so don't pass in strings to be combined into dynamic SQL unless there
is no way there's user input.
To make things a bit easier, I've recently added fire-and-forget queries to
odbc so you can use type checked parameters for instant queries if you don't
want to create a query object but want type safe parameter checks.
let results = connection.dbq("SELECT ?value1, ?value2", [("value1", 17),
("value2", 24)]
Run
@Araq: I wrote the odbc library for a few reasons:
1. The standard db_* libraries just handle all data as strings, which is fine
for quick queries but not viable (IMHO) for lots of data work. I put in a fair
bit of effort to make sure you could directly use native Nim types both for
parameters and all the results from the server and they are type checked
against the ODBC types.
2. Odbc allows named parameters such as ?myParam rather than just ?.
3. Finally - and most damningly as I've detailed above, db_odbc doesn't use
odbc parameterisation and instead [just uses string
concatination]([https://github.com/nim-lang/Nim/blob/b6924383df63c91f0ad6baf63d0b1aa84f9329b7/lib/impure/db_odbc.nim#L194)](https://github.com/nim-lang/Nim/blob/b6924383df63c91f0ad6baf63d0b1aa84f9329b7/lib/impure/db_odbc.nim#L194\)).
Yes some effort is made to escape strings, but this is to me wholey worse than
the guaranteed parameter isolation that native odbc parameters give you. There
is absolutely no way to escape parameterised queries when used properly.
