chore: handle new bandit warnings

[mikix]

Description

The latest release of bandit (1.7.5) added a few new SQL checks that we were tripping on.

It thought we might have SQL injection issues from doing string formatting on raw SQL lines. But none of the cases it found were based on user input, but rather were boilerplate generators.

I also moved some code into a test file because it was the only place where the code was used.

Checklist

• Consider if documentation (like in docs/) needs to be updated
• Consider if tests should be added

[mikix]

bandit doesn't seem to love when there's anything else on the line. It stops mentioning that it skipped the line but will also generate a warning that there's a nosec on the line but no failing test. But if you remove it, the failing test appears.

But it all works as expected without any extra text, so 🤷

[dogversioning]

It thought we might have SQL injection issues from doing string formatting on raw SQL lines. But none of the cases it found were based on user input, but rather were boilerplate generators.

I wonder - is this explanation going to be sufficient to pass a security review?

[dogversioning]

I'm approving this for now, but we should maybe create a technical debt ticket for refactor if we need to for security reasons

[mikix]

I wonder - is this explanation going to be sufficient to pass a security review?

I think for these cases, I'd be happy to defend it. Here's one of the four methods where this happens:

def sql_provider() -> str:
    cols_dates = format_date("IMPORT_DATE")
    cols = f"PROVIDER_ID, PROVIDER_PATH, NAME_CHAR, {cols_dates}"
    return f"select {cols} \n from {Table.provider.value}"  # nosec

It's not even taking any function arguments. The string formatting is just for ease of maintenance. One of the four methods does take an argument, but it's always a hardcoded string when called.

Does that make you worried / do you think it's worth refactoring these to be fully hardcoded strings and using bind variables instead? I hadn't thought to do so, because there's no user input here. But it's an option.

[dogversioning]

Does that make you worried / do you think it's worth refactoring these to be fully hardcoded strings and using bind variables instead? I hadn't thought to do so, because there's no user input here. But it's an option.

:I'm: fine with it, but i can see someone doing a security review throwing up a flag on any usage of the #nosec param. Part of me says address it now since a security review is likely to be a multi day process.

[mikix]

OK I filed #186 to document the idea that we should do a pass and remove these at some point, to simplify the security story.