Why is string_literal() not safe?

[smilingthax]

string_literal(str) add database-adapter-specific quotes (usually '), but does not escape any occurrences of those in str.

This is unexpected (does not match what most database libraries do) and could even lead to sql-injection vulnerabilities. And the documentation does not remind you, either.

The workaround seems to be:

{%- macro safe_string_literal(str) -%}
  {{string_literal(escape_single_quotes(str))}}
{%- endmacro -%}

There is a problem with this approach, though: You can't be sure that string_literal is dispatched to an (database-adapter-specific) implementation that actually wraps the string in single quotes – it might use double quotes, or triple-single-quotes (there is a PR for bigquery), ... – and then escape_single_quotes would not be the right function to use!

Thus, safe_string_literal(str) would have to use dispatch, too.
To me, this clearly sounds like something that should already be provided out-of-the-box by dbt... either as separate function (backward compat?), or as new function.

[smilingthax]

BTW: Some frameworks have a special case for NULL / nil / None: their_safe_string_literal(none) would result in SQL NULL instead of '...some test...'. I'm not sure, whether dbt should do something similar...
(The more general version would be quoteValue(...none / number / string / boolean ...) which gives the appropriate sql... – or: queries with placeholders that are bound from a separate object...)

[dbeatty10]

It looks like dbt-labs/dbt-utils#82 originally introduced string_literal into the dbt_utils package, and then we moved it from dbt-utils to dbt-core in dbt-labs/dbt-utils#597, et al.

We can see an example of the gymnastics you are talking about here.

Can also see the triple quotes PR you mentioned: dbt-labs/dbt-bigquery#1089

[dbeatty10]

Overall, the current behavior that omits escaping looks like an oversight rather than a design decision, and I agree that it would be most helpful if there were a "safe" macro that is dispatched to a database-adapter-specific implementation as-needed.

The key activities to support that would be:

1. Robust test cases that can be inherited by each database adapter, including all the anticipated edge cases
2. Determining how to approach backwards-compatibility

[dbeatty10]

Added links to this issue: dbt-labs/dbt-core#2986 (comment)