Query API with raw string is dangerously designed

[Joannis]

Right now, the query function has two overloads. One for PostgresQuery, which handles parameter binding in interpolation. The other one accepts a raw string. Because the PostgresQuery uses a great transparent designs, those not extremely intimately familiar with Swift may be tricked into believing any String is safe.

This is amplified by the fact you can pass String into the query APIs the same way you can a PostgresQuery. Thus leads many users to use the wrong API, leaving them vulnerable to SQL injection.

I'd recommend deprecating the query aPI for string, and if strictly necessary reintroducing it while marking it explicitly unsafe/unchecked in the public signature.

[Jerry-Carter]

This issue caught my eye. While I respect Little Bobby Tables, the README clearly sets an expectation which I read as 'Don't worry, we've got this for you.' If this is violated, I agree there is a real problem:

While this looks at first glance like a classic case of SQL injection 😱, PostgresNIO's API ensures that this usage is safe. The first parameter of the query(_:logger:) method is not a plain String, but a PostgresQuery, which implements Swift's ExpressibleByStringInterpolation protocol. PostgresNIO uses the literal parts of the provided string as the SQL query and replaces each interpolated value with a parameter binding.

The code in the string based query function appears to handle the bindings, at least based on a cursory review. So I'm not sure this is necessarily a problem.

       var bindings = PostgresBindings(capacity: binds.count)
        binds.forEach { bindings.append($0) }
        let query = PostgresQuery(unsafeSQL: string, binds: bindings)

But even if the string based query function is okay, the reality is that SQL Injection appears to be entirely possible using Postgres-NIO. Suppose the user decided to build up a raw query as a string - perhaps to reuse it multiple times. They might very well end up with something like this:

    let unsafeQuery = "INSERT INTO students (name, email) VALUES (\(student.name), \(student.email) RETURNING id"
    ...
    try await connection.query(unsafeQuery, logger: logger)

where a client might cause problems when specifying a name such as 'Robert', 'bob@example.com'); DROP TABLE students;--

As usual, the moral of the story is that libraries may be helpful, but the wise programmer trusts no one.

Edit / Addition: I don't see any way to prevent a determined programmer from shooting himself in the foot. One could block the automatic conversion of String into PostgresQuery, but then the programmer could always create let myQuery = PostgresQuery(unsafeQuery) and thereby bypass the protection. This matter would seem best handled as a warning in the README.md file.