Stored procedure to query custom data tables as dynamic SQL

There's a lot going on here, but the background for why this is necessary is that there is a set schema, or 'core' set of tables that are prefixed with 'bu', and any core table can have a custom table of the same base name but prefixed with xb and has the same primary Key. I have no idea if a customer has created an extension table, but assume they know its structure and the data model.

This sproc is called via a web API endpoint that the customer will manage, so there may be cases where it is an unsecured call. Does this open the door to destructive injection attacks? Also, there is some duplicated functionality that could be moved into a function or CTE, but I ran out of time and SQL prowess.

```
SET QUOTED_IDENTIFIER OFF
GO
SET ANSI_NULLS OFF
GO

/
-- CHANGE LOG
-- *

-- Date Changed Who Desc/Comments
-- ------------ -------- ----------------------------------------------------------
-- 12/10/2015 This Guy Created - Gets any Core data by Xtend table name
and specially formatted where clause.

Parameters:
@tableName is the name of the xtend
table. It must begin with 'xb' or the sproc
returns 0.
@returnColumns is a comma delimited list of columns
in the Core (bu) table that are returned. '*' can
be passed in to retrieve all the columns in the
Core table (None of the columns in the joined tables will be returned).
@whereClauses (@xbWhereClause - required, @coreWhereClause -optional)
are comma delimited wher

Does this open the door to destructive injection attacks?

It depends, but potentially, yes it can, especially with dynamic SQL, which you should try to avoid unless it is absolutely necessary. You should be able to sanitize the input before passing it to the database server, how to do it really depends on what technologies you are using.

NOTE: DO NOT TRY THIS ON A PRODUCTION DATABASE

What would happen if the stored procedure was called with @xbWhereClause = 'Vendor_ID:=:123456; DELETE FROM xbSpecial_Request;--'? If you are lucky it will throw a syntax error. But the SQL procedure is not "smart" to know whether it should execute arbitrary code or not, it just does it usually. Just keep that in mind and make sure to sanitize database inputs before it even reaches the database server.

Another disadvantage of dynamic SQL is that it can be quite slow, as SQL engine has to calculate a new execution plan whenever you call EXEC sp_executesql @SQL. It is also more prone to errors due to the amount of string manipulation that is needed to make it work, which in your case is quite a lot.

PRINT statements

PRINT statements don't really belong in production SQL code. They are slow and often not very useful (how often do you go check the SQL Server console log for statements like "Core WHERE Exists"?)

If you need to log this information somewhere, log it either in a table, or some external file system that is searchable.

INFORMATION_SCHEMA

Here is an article about: The case against INFORMATION_SCHEMA views by DBA extraordinaire Aaron Bertrand. For the reasons cited in the article, it is better (more informative) to query the sys schema rather than INFORMATION_SCHEMA.

For example, this statement:

SELECT 'bu.' + c.COLUMN_NAME
    FROM INFORMATION_SCHEMA.COLUMNS c
    WHERE c.TABLE_NAME = @coreTableName

Could be rewritten as such:

SELECT 'bu.' + c.name
    FROM sys.columns AS c
      JOIN sys.tables AS t on c.object_id = t.object_id
    WHERE t.name = @coreTableName

If you need to you can also join sys.schemas to sys.tables on their common schema_id key, in case you need to use the schema names for something.

Too much

I think you should really re-evaluate if this is a good idea. Perhaps have a DBA at your organization look over the logic to point out how it could be improved.

Just looking at the way this is called, it really looks like there is something problematic with this complicated "string builder" that finally executes a dynamic SQL statement... Most SQL stored procedure calls aren't supposed to look like that, and I think if you're going to do this you may as well have the calling application build the query instead of having a database procedure for it.

EXEC [API].[GetCoreFieldsByXtendColumnValues] 'xbSpecial_Request', '*','Latest_Special_Status:=:New', 'Vendor_ID:=:123456','Foo_Detail_Line_ID:buFoo_Detail_Line:Foo_Detail_Line_ID,Foo_ID:buFoo_Header:Auth_ID'

StackExchange Code Review Q#119662, answer score: 7