Escape closing brackets in T-SQL identifier quoting#400
Open
sdebruyn wants to merge 1 commit into
Open
Conversation
Bracket-quoted T-SQL identifiers require a literal ] to be doubled (]]) to avoid prematurely closing the bracket. Without escaping, any identifier containing ] either generates invalid T-SQL (silent breakage for reserved-word column names that happen to contain ]) or, more importantly, lets a crafted identifier terminate the bracket and inject arbitrary T-SQL. Apply the escape to every bracket-quoting site: - FabricAdapter.quote() - FabricColumn.quoted - FabricRelation.quoted - fabric__alter_column_type (columns.sql) - fabric__alter_relation_add_remove_columns (columns.sql) - fabric__create_table_as listColumns block (create_table_as.sql) - fabric__create_columns (snapshots/helpers.sql)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #399.
FabricAdapter.quote(),FabricColumn.quoted,FabricRelation.quoted, and five Jinja macro sites all compose bracket-quoted T-SQL identifiers without escaping]. Any identifier containing]produces invalid T-SQL, and a crafted identifier of the formfoo];<sql>--lets a name terminate the bracket and inject arbitrary T-SQL into whatever statement the macro is composing. The dbt-fabric attack surface is small (identifiers are usually static), but projects that derive names from external systems are exposed, and silent breakage on reserved-word columns containing]is a real bug regardless.Changes
Doubling
]to]]inside the brackets is the standard T-SQL escape (Delimited Identifiers). Applied at every bracket-quoting site:FabricAdapter.quote()—identifier.replace("]", "]]")before the format.FabricColumn.quoted— same onself.column.FabricRelation.quoted()— same onidentifier.fabric__alter_column_type(columns.sql) — both theCASTsource and the alias.fabric__alter_relation_add_remove_columns(columns.sql) —addanddrop columnbranches.fabric__create_table_as(create_table_as.sql) — thelistColumnsblock used forcontract.enforcedmodels.fabric__create_columns(snapshothelpers.sql) — column addition during snapshot column expansion.The Jinja escape uses the standard
| replace(']', ']]')filter inside the existing[ ... ]wrapper.Out of scope
This PR only adds the missing escape. Two related quoting inconsistencies were noticed while writing the patch but kept out to keep the diff reviewable:
fabric__alter_column_type's metadata-query branch composes column names with double quotes ('"' + ... + '"') while the rest of the file uses brackets.fabric__get_use_database_sqlcurrently strips[/]/"from the database name rather than escaping them. That is safe against injection but silently rewrites identifiers that legitimately contain those characters.Happy to follow up on either in separate PRs if useful.