Ensure that we always use prepared SQL queries

[QuantumDancer]

Currently, we are using query.push(format!(..., input)) to build the SQL query for the advanced record filtering. See. e.g.:

AUDITOR/auditor/src/routes/advanced_record_filters.rs

Lines 135 to 141 in db13f0d

	query.push(" WHERE ".to_string());
	if let Some(record_id) = &filters.record_id {
	let is_valid_record_id = ValidName::parse(record_id.clone().to_string());
	if is_valid_record_id.is_ok() {
	query.push(format!("a.record_id = '{}' and ", &record_id));
	}
	}

Even though we check that this is a ValidName, we probably should not use .query() but push_bind when inserting user data into the SQL query to ensure we are not susceptible to SQL injections.

The documentation of QueryBuilder.push() states:

You should not use this to insert input directly into the query from an untrusted user as this can be used by an attacker to extract sensitive data or take over your database.

This method does not perform sanitization. Instead, you should use .push_bind() which inserts a placeholder into the query and then sends the possibly untrustworthy value separately (called a “bind argument”) so that it cannot be misinterpreted by the database server.

Note that you should still at least have some sort of sanity checks on the values you’re sending as that’s just good practice and prevent other types of attacks against your system, e.g. check that strings aren’t too long, numbers are within expected ranges, etc.

In the case mentioned above, this would look like this (since ValidName::parse() could potentially do some input sanitation, I think it's nicer to use the result of that and not the original user input):

 query.push(" WHERE ".to_string()); 
 if let Some(record_id) = &filters.record_id { 
     if let Ok(valid_record_id) = ValidName::parse(record_id.clone().to_string()) {
        query.push("a.record_id = ");
        query.push_bind(valid_record_id);
        query.push(" and ");
    }
 } 

Edit: As discussed below: instead of what I've written above, we should use ValidName and ValidAmount instead of normal strings and number types in the Filter struct.

[stefan-k]

[...] since ValidName::parse() could potentially do some input sanitation [...]

"could potentially" is a bit of an understatement, because that's exactly what ValidName is designed for ;)

[QuantumDancer]

AFAIK we currently only check for certain criteria in ValidName::parse() but don't change the input. So right now there should not be a difference between using the output of ValidName::parse() or the original input after checking if ValidName::parse() returned an Ok(...). What I meant is that if we expand the parsing to also change the input in the future, then it makes a difference.

[stefan-k]

What I meant is that if we expand the parsing to also change the input in the future, then it makes a difference.

I'd rather Err on invalid input because it is unlikely that a sanitized input represents the users intention. From the point of view of a user, an error is easier to debug than confusing output.

Ideally your Filter struct only holds valid elements.

[QuantumDancer]

Implemented in #648