Return to Answer

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'

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'

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'

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'

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'

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'