Correct usage of Dynamic SQL in SQL Server

Question

I’m using static SQL for 99% of the time, but a recent scenario led me to write a dynamic SQL and I want to make sure I didn’t miss anything before this SQL is released to production.

The tables’ names are a combination of a prefix, a 2 letters variable and a suffix and column name is a prefix + 2 letters variable.

First I’ve checked that @p_param is 2 letters length and is “whitelisted”:

IF (LEN(@p_param) = 2 and (@p_param = ‘aa’ or @p_param = ‘bb’ or @p_param = ‘cc’ or @p_param = ‘dd’ or @p_param = ‘aa’)

    BEGIN
        set @p_table_name = 'table_' + @p_param + '_suffix';
        set @sql = 'update ' + QUOTENAME(@p_table_name) + ' set column_name = 2 where id in (1,2,3,4);';
        EXEC sp_executesql @sql;

--Here I’m checking the second parameter that I will create the column name with
        IF (LEN(@p_column) = 2 and (@p_column = 'ce' or @p_column = 'pt')
           BEGIN
               Set @column_name = 'column_name_' + @p_column_param; 
               set @second_sql = 'update ' + QUOTENAME(@p_table_name) + ' set ' + 
                                  QUOTENAME(@column_name) + ' = 2 where id in (@p_some_param);';

               EXEC sp_executesql @second_sql, N'@p_some_param NVARCHAR(200)', @p_some_param = @p_some_param;
           END

    END

Is this use case safe? Are there any pitfalls I should be a ware of?

I presume you're confident that column_name exists in @p_table_name? Similarly, you're confident that the data type of @column_name is capable of having an integer inserted into it, and that you don't need to check its data type? I'd also add an 'else' statement: "insert into failed_queries(table, column) values (@p_param, @p_column);" — Jeff Breadner
– Jeff Breadner, Commented Feb 26, 2018 at 1:27
Yes, all of these conditions should met, if they aren't then I can assume a client sent something it wasn't suppose to. — SuperFrog
– SuperFrog, Commented Feb 26, 2018 at 1:30
I think checking the length and ensuring that the arguments are in a whitelist is redundant, but better safe than sorry. I think your issues are likely to be around the long-term supportability of this solution, rather than your specific implementation; it would be better (I think) to have a single table where p_param and p_column are values in fields. In other words, I think this will work, but I think it should be re-worked to be done in a more standard way -- that should make it more performant, and easier for someone else to support in a year or two? — Jeff Breadner
– Jeff Breadner, Commented Feb 26, 2018 at 1:36
I think you're safe for SQL injection. That database seems like it would be annoying to work with in other respects, though, with table names and column names that are essentially meaningless. Something to note is that if @p_some_param is a list, that query won't work the way you expect it to. — ZLK
– ZLK, Commented Feb 26, 2018 at 1:51
Please note that column names and tables do have meaning, I've changed them for the sake of this question. I'm aware that @p_some_param won't work as is, and I'm using a function to handle this. — SuperFrog
– SuperFrog, Commented Feb 26, 2018 at 1:54

Zohar Peled · Accepted Answer · 2018-02-26 06:52:42Z

Seems like you've lost some things in the translation to meaningless names to prepare your query to post here, so it's kinda hard to tell. However, the overall approach seems OK to me.

Using a whitelist with QUOTENAME for the identifiers will protect you from SQL injections using the identifiers parameters, and passing the value parameters as a parameter to sp_executeSql will protect you from SQL injections using the value parameters, so I would say you are doing fine on that front.

There are a couple of things I would change, though.
In addition to testing your tables and columns names against a hard coded white list, I would also test then against information_schema.columns, just to make sure that the procedure will not raise an error in case a table or column is missing.
Also, Your whitelist conditions can be improved - Instead of:

IF (LEN(@p_param) = 2 and (@p_param = ‘aa’ or @p_param = ‘bb’ or @p_param = ‘cc’ or @p_param = ‘dd’ or @p_param = ‘aa’)

You can simply write:

IF @p_param IN('aa', 'bb', 'cc','dd')

Collectives™ on Stack Overflow

Correct usage of Dynamic SQL in SQL Server

1 Answer 1

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Related