concern: sqli (on client)

[dcarosone]

The assembly of SQL strings takes unprotected input from the environment / user
context. This is on the client side (where it's less of a concern because I
already own the db file, but still):

src/command/history.rs:

  let session = env::var("ATUIN_SESSION")?;
  let query_session = format!("select * from history where session = {};", session);

I'm worried that even aside from malicious concerns, users could trip over these in normal operation. One of these queries uses cwd and perhaps a user has a semicolon or backtick or similar similar special character in the dir name. (I got a little stuck on the bash integration when trying to test this; will update when I get past that).

On the server side (at a quick glance) where the input comes from the network,
it's ok; using SQLx and prepared statement bindings.

[ellie]

I totally understand where the concern is coming from, although as you say, given the user already owns the database file (and likely has a plaintext history file too), malicious actors really aren't of concern

I think the solution to this is twofold. I've actually been using the SQL injection as... a feature. Bad as it may sound at first, being able to use SQL to query my history is useful!

I do agree though: weird directory names or other inputs might cause undesired results + this should definitely not be the case

Going forward I think the best approach would be to have both an explicit "SQL allowed" mode, and perhaps a "simple" mode where the query is far more literal (and sanitised)

Server side everything uses prepared statements with SQLx, so there shouldn't be any security concerns there

[ellie]

Closing this as the fix can be tracked in #58

Thank you so much for both checking through the code, and taking the time to report this! 😄