Proposal: Improvements to `sqlx::query()` for Query Builders, and Explicit Injection Safety

[abonander]

Status Quo

At the core of the sqlx::query() family of functions lies a contentious design choice that goes all the way back to the initial prototypes of SQLx:

pub fn query<'q>(query: &'q str) -> Query<'q, ...> { ... }

The query string must be a string slice, usually 'static but can also be borrowed. This was a deliberate choice, because allowing String to be passed directly makes it extremely tempting to simply format!() user input into the query string instead of using bind parameters, which would easily introduce SQL injection vulnerabilities into the application. For example, to co-opt XKCD #327:

async fn insert_student(conn: &mut PgConnection, student_name: &str) -> sqlx::Result<StudentId> {
    sqlx::query_scalar(format!("INSERT INTO students(name) VALUES('{}') RETURNING student_id", student_name))
        .fetch_one(conn)
        .await
}

// imagine this is user input, and wasn't properly sanitized
let student_name = "Robert'); DROP TABLE students; --";

let student_id = insert_student(&mut conn, &student_name).await?;

Then suddenly, you've lost all student data for the year because the query that got executed looked like this:

INSERT INTO students(name) VALUES('Robert'); DROP TABLE students; -- ') RETURNING student_id

(Postgres would reject the attempt to prepare a query string with more than one statement, but MySQL and SQLite wouldn't.)

The idea is that by requiring dereffing to a string slice, it will at least make the code start to smell a bit and hopefully lead to the user re-evaluating their implementation:

sqlx::query_scalar(&format!("INSERT INTO students(name) VALUES('{}') RETURNING student_id", student_name))

The "easier" path is intended to be using bind parameters, which by design do not introduce injection vulnerabilities because the database knows not to interpret bound values as part of the SQL. By comparison, this implementation is completely injection-safe, and is the approach that the API design was intended to push users towards:

async fn insert_student(conn: &mut PgConnection, student_name: &str) -> sqlx::Result<StudentId> {
    sqlx::query_scalar("INSERT INTO students(name) VALUES(?) RETURNING student_id")
        .bind(student_name)
        .fetch_one(conn)
        .await
}

However, it does have its downsides.

The intention is not at all clear in the design.

Time and again, users have opened issues or questions on the discussion board or asked on Discord about why the API is designed this way, and it gets very tedious to explain the reasoning (I admittedly have copy-pasted large chunks of this first part from a Q&A so I didn't have to write it again). This isn't a complaint about people asking questions, of course, it's more an expression of annoyance that the question needs to be asked in the first place.

Of course, we could just explain it better in the docs, which we should be doing anyways. But it's still an issue that it's even necessary.

The design doesn't give off very obvious signals when the user is doing something wrong.

All you have to do for the function to accept a String is to add a & and let auto-deref do the rest. Yeah, it's a code smell, but it reeks more of poor API design than injection vulnerability. I personally would complain about an API so inflexible that all I could pass is &str; what if I want to pass String or Arc<str>?

And if all a user has to do is add a &, is there really any opportunity for them to gain insight into why they shouldn't be doing that?

There's legitimate use-cases for passing a constructed String if the user knows what they're doing.

This design decision is notorious for making it very difficult to implement query builders on top of SQLx, because of the lifetime issues that it introduces: #1428

pub fn sql_insert_with_return_id(....) -> QueryAs<'q, sqlx::Postgres, (i64,), PgArguments> {
   let mut sql = String::from(....);
   // .. continue to build string
   let mut query = sqlx::query_as::<sqlx::Postgres, (i64,)>(&sql);

  query // <-- Error here since query_as require sql to have same lifetime as the Query
}

Proposal

I don't want to just allow String to be passed directly, because I think it's really important for the design of an API to avoid straight-up handing the user a footgun. We can warn about injection vulnerabilities in the docs all we want, but as long as it's the path of least resistance, some people will still end up shooting themselves. And allowing that to happen just isn't the Rust way.

I think we could copy a page from std's notebook here and introduce something similar to UnwindSafe: https://doc.rust-lang.org/stable/std/panic/trait.UnwindSafe.html

/// A query string that is safe to execute on a live database.
pub trait InjectionSafe {
    fn sql(&self) -> &str;
}

/// Note: *only* implemented for static strings as those are highly unlikely to contain injection vulnerabilities,
/// barring compile-time shenanigans or deliberately leaking string allocations.
impl InjectionSafe for &'static str {
    ...
}

/// Assert that dynamic string data is safe from SQL injection.
///
/// The user assumes responsibility if constructing query strings based on untrustworthy input.
pub struct AssertInjectionSafe<T>(pub T);

impl<T: AsRef<str>> InjectionSafe for AssertInjectionSafe<T> {
    ...
}

pub fn query<Q: InjectionSafe>(query: Q) -> Query<Q, ...> {
   ...
}

Existing usage with &'static str would remain unchanged:

let foo: i32 = sqlx::query_scalar("SELECT 1").fetch_one(&mut conn).await?;

However, usage of dynamic query strings would now be forced to assert that they do not contain SQL injection vulnerabilities, and in exchange would be freed of lifetime constraints:

// Of course, this is a blatant violation of the API contract if `table_name` cannot be trusted.
// A closed set of names like an enum would be better but that would pad out the example.
// Either way, the user has assumed responsibility here, and is aware that any vulnerabilities are on them.
pub fn build_query(table_name: &str) -> Query<AssertInjectionSafe<String>, ...> {
    sqlx::query(AssertInjectionSafe(format!("SELECT * FROM {}", table_name)))
}

Of course, it is still possible to bypass using AssertInjectionSafe, but it's a more significant code smell (and involves deliberately introducing a memory leak):

pub fn build_query(table_name: &str) -> Query<&'static str, ...> {
    let query: Box<str> = format!("SELECT * FROM {}", table_name)).into();
    // Memory leak!
    sqlx::query(Box::leak(query))
}

And ideally, on the way the compiler would have informed them that their query string needs to implement InjectionSafe which would have prompted them to check the docs and learn about SQL injection and why what they're trying to do is deliberately annoying.

[abonander]

A thought I had was to not have a sql() method on InjectionSafe and treat it like a true marker trait, then have Q: AsRef<str> + InjectionSafe as the bound on query().

The main idea with that would be to make it more obvious that the parameter is still supposed to be "some sort of string" but with an extra marker trait attached.

[autarch]

I'm stumbling across this right now, so let me add another use case. It's pretty common to want to sort a query based on user preferences or request parameters. The easiest way to do this is to dynamically build the ORDER BY clause. But that becomes basically impossible with the current API.

I had to construct a truly grotesque SQL abomination to try to work around this, where the user's preferred sort order is expressed as a three-element array of an enum type:

            ORDER BY
                CASE $2[0]
                    WHEN 'original' THEN COALESCE( a.sortable_name, a.name )
                    WHEN 'transcripted' THEN COALESCE( a.transcripted_sortable_name, a.transcripted_name )
                    WHEN 'translated' THEN COALESCE( a.translated_sortable_name, a.translated_name )
                END ASC,
                CASE $2[1]
                    WHEN 'original' THEN COALESCE( a.sortable_name, a.name )
                    WHEN 'transcripted' THEN COALESCE( a.transcripted_sortable_name, a.transcripted_name )
                    WHEN 'translated' THEN COALESCE( a.translated_sortable_name, a.translated_name )
                END ASC,
                CASE $2[2]
                    WHEN 'original' THEN COALESCE( a.sortable_name, a.name )
                    WHEN 'transcripted' THEN COALESCE( a.transcripted_sortable_name, a.transcripted_name )
                    WHEN 'translated' THEN COALESCE( a.translated_sortable_name, a.translated_name )
                END ASC

I'm not even sure if this works, because now I have other lifetime issues. Notably, the parameter passed to .bind() also have to live for as long as the query itself, which is also a pain. It'd be really nice to be able to pass in cloned bind params in this case.

I'm happy to open another issue for that if that would be helpful.

[autarch]

Oops, ignore the bit about .bind(). I can pass cloned arguments to it.

[abonander]

@autarch you might also be interested in this proposal #1488

The main idea is to allow adding/removing parts of the query at runtime while still providing compile-time checks.

[autarch]

@autarch you might also be interested in this proposal #1488

The main idea is to allow adding/removing parts of the query at runtime while still providing compile-time checks.

Thanks. This looks like an interesting approach.

FWIW, I initially tried using the compile-time query macros, but I gave up because there's no non-hacky way to support custom types (like CITEXT) with those macros. Obviously that's OT for both of these issues.

[luke-biel]

I'm gonna add a bit of info as question that sparked this issue was asked by me.

Bit that led me to investigation of query and execute apis is:

PgPoolOptions::new()
    .max_connections(self.pool_size)
    .after_connect(move |conn| {
        let schema = schema.clone();
        Box::pin(async move {
            conn.execute(format!("SET search_path TO {}", schema).as_str())
                .await?;
            Ok(())
        })
    })
    .connect_with(
        PgConnectOptions::new()
            .host(&self.host)
            .port(self.port)
            .username(&self.username)
            .password(&self.password)
            .database(&self.database),
    )
    .await

I can guarantee that schema is safe, as it's provided by person deploying application on a machine, not an end user. AssertInjectionSafe still holds my hand, but it's clear in it's intent and I can live with it.

Per my use case, and I failed to clarify that initially, it would be beneficial if I could

sqlx::query("SET search_path TO $1")
    .bind(schema.clone())
    .execute(conn)
    .map(|r| r.map(|_| ()))
    .boxed()

which currently causes postgres connections to hang indefinitely
or

conn.execute(format!("SET search_path TO {}", schema)).map(|r| r.map(|_| ())).boxed()

which can't compile
or
set schema in PgConnectOptions as per jdbc spec (options param in https://jdbc.postgresql.org/documentation/head/connect.html)

This is more of a nitpick about how code looks and how much extras are there rather than a real issue.

[abonander]

We actually just released support for the options parameter, either in the URL or set on PgConnectOptions.

The reason the version using sqlx::query() hangs indefinitely is because Postgres doesn't support SET in prepared statements, so your .after_connect() is kicking back an error which is causing Pool to drop the connection and try again and again.

[luke-biel]

We actually just released support for the options parameter, either in the URL or set on PgConnectOptions.

If that's true I'm in awe 😄

[kevinmehall]

Take a look at the Scala library Relate and its InterpolatedQuery type.

It's basically a string-like type for a known-safe query fragment that also carries a list of values to bind to the parameters. In Rust, maybe something like

struct InterpolatedQuery {
  query: Cow<'static, str>,
  params: Vec<Box<dyn Encode>>,
}

You could have a safe constructor from &'static str, a scarier-named constructor from String, and a format_query!() macro that works a lot like query!() but concatenates InterpolatedQuery args into the literal query and all other types as bound parameters:

let where: InterpolatedQuery = format_query!("id = ?", id); // or maybe `format_query!("id = {id}")` and insert the placeholder
let sort = InterpolatedQuery::sql("foo");
sqlx::query(format_query!("select foo from bar where {where} order by {sort}"))

[autarch]

I found a workaround for the issue I had with wanting to generate a query dynamically and then return a stream from calling fetch(), and I wanted to document it here in case anyone else comes across this issue:

use futures_util::stream::BoxStream;
use sqlx::PgPool;

#[derive(Clone, Debug)]
pub struct DB {
    pool: PgPool,
}

impl DB {
    pub fn get_item<'a>(
       &'a self,
       some_id: &str,
       select: &'a mut String,
    ) -> BoxStream<'a, Result<Item, sqlx::Error>> {
        *select = format!("...", ...);
        sqlx::query_as::<_, Item>(select)
            .bind(some_id)
            .fetch(&self.pool)
    }
}

Then the caller can easily arrange to keep the string used for the select alive as long as the returned stream.