fix: improve sql table name validation

[terriko]

• Related: fix: improve bandit config #3830 #3885

@harshittiwariii has re-enabled bandit rule B608 in our bandit config. We've got a few places it's being triggered because we're constructing a query with a table name as variable, and there's no correct way to handle this construction in sqlite3. For now, they've been marked as # nosec and we reviewed them manually, but it would be nice to have a function that validates table names before they're used in this way, since we know in advance what our valid table names are.

I'm opening this issue specifically for @harshittiwariii to finish the work begun in #3885 . It's not available to anyone else unless @harshittiwariii gets busy and wants to put it back in the general pool.

[harshittiwariii]

Thank you for opening this issue for me, @terriko . I appreciate the opportunity to continue the work begun in #3885. I understand the need for a function to validate table names before they're used in queries, especially considering the challenges with sqlite3. I'll work on implementing the necessary function to ensure the correct handling of table names.

[harshittiwariii]

Hey @terriko , just giving you a heads-up on an update or preliminary approach.
To address the security concerns raised by bandit and ensure that only valid table names are used in constructing queries, we can implement a function that validates table names before they are used. Here is some proposed approach and some sample code:

Validate Table Names Function: This function will check if the provided table name is valid based on predefined criteria, such as only containing letters (a-z) and underscores (_). [coz we know our table naming patterns]

import re

def is_valid_table_name(table_name):
    """Check if the provided table name is valid."""
    # Define regex pattern for valid table names (letters a-z and underscore)
    pattern = re.compile(r'^[a-z_]+$')
    return bool(pattern.match(table_name))

Usage in Query Construction: Before constructing queries with table names provided by users or variables, we'll validate the table name using the is_valid_table_name function. If the table name is not valid, we will raise an exception or handle it accordingly.

def construct_query(table_name, condition):
    """Construct a SQL query with a validated table name."""
    if not is_valid_table_name(table_name):
        raise ValueError("Invalid table name provided.")
    
    # Construct the query
    query = f"SELECT * FROM {table_name} WHERE {condition};"
    return query

Handling Exceptions: If an invalid table name is provided, we can handle it by raising a ValueError or any other appropriate exception.

try:
    query = construct_query("invalid_table", "id = 1")
except ValueError as e:
    print(f"Error: {e}")

By implementing this approach, we are ensuring that only valid table names are used in constructing queries, mitigating the risk of SQL injection attacks.

Also, Please let me know if you have any other expectations or if there's anything else we need to consider.

[terriko]

I think this will work, but now that I'm thinking about it I'm thinking we should take the other approach we discussed. Given that we have less than a dozen table names, I think it might be minimally more secure to check against a list of them rather than using a regex. Mostly this is to avoid https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS (which probably isn't a huge consideration here over all the other places we use regexes, but why make the problem worse if we don't have to?)

[harshittiwariii]

I think this will work, but now that I'm thinking about it I'm thinking we should take the other approach we discussed. Given that we have less than a dozen table names, I think it might be minimally more secure to check against a list of them rather than using a regex. Mostly this is to avoid https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS (which probably isn't a huge consideration here over all the other places we use regexes, but why make the problem worse if we don't have to?)

@terriko Thanks for the review. I will proceed with the earlier discussed approach.