CodeSOD: Stored Procedures are Better

We all know that building SQL queries via string concatenation, and then sending them to the database, is just begging for fragile code and SQL injection attacks. But, what if the bad part is the “sending them to the database” part? Has anyone ever thought about that?

Kris‘s predecessor has.

CREATE PROCEDURE [dbo].[usp_LossMit_GetCDCMappingInfo]
@PropertyNameString NVARCHAR(4000),
@Environment CHAR(1)
AS
BEGIN
DECLARE @TICK CHAR (1) SET @TICK = CHAR(39)
DECLARE @SQLSelect NVARCHAR (4000)
DECLARE @SQLWHERE NVARCHAR (4000)
DECLARE @SQLSelectII NVARCHAR (4000)
DECLARE @SQLWHEREII NVARCHAR (4000)

SET @SQLSelect = ‘
SELECT
CDCID As PropertyValue,
CDCName AS EntityName,
ISNULL(RTRIM(PropertyName), ‘+ @TICK + @TICK + ‘) AS PropertyName
FROM dbo.LossMitCDCIDMapping’
SET @SQLWHERE = ‘
WHERE PropertyName IN (‘ + @PropertyNameString + ‘)
AND Environment = ‘ + @TICK + @Environment + @TICK +
‘AND IsActive = 1’

SET @SQLSelectII = ‘
UNION
SELECT
lccm.CDCControlID AS PropertyValue,
lccm.CDCControlName AS EntityName,
ISNULL(RTRIM(lccm.PropertyName), ‘+ @TICK + @TICK + ‘) AS PropertyName
FROM dbo.LossMitCDCIDMapping lcm
INNER JOIN dbo.LossMitCDCControlIDMapping lccm
ON lcm.CDCID = lccm.CDCID’
SET @SQLWHEREII = ‘
AND lcm.PropertyName IN ( ‘+ @PropertyNameString + ‘)
AND lcm.Environment = ‘ + @TICK + @Environment + @TICK + ‘
AND lccm.Environment = ‘ + @TICK + @Environment + @TICK + ‘
AND lcm.IsActive = 1
AND lccm.IsActive = 1′

PRINT (@SQLSelect + @SQLWHERE + @SQLSelectII + @SQLWHEREII)
EXEC (@SQLSelect + @SQLWHERE + @SQLSelectII + @SQLWHEREII)
END

/*****usp_LossMit_GetAutoIndex******/

GO

Now, just one little, itsy-bitsy thing about T-SQL: it handles variables in SQL statements just fine. They could have written AND Environment = @Environment without wrapping it up in string concatenation. This entire function could have been written without a single string concatenation in it, and the code would be simpler and easier to read, and not be begging for SQL injection attacks.

And I have no idea what’s going on with @TICK- it’s a one character string that they set equal to an empty 39 character string, so I assume it’s just “”- why are we spamming it everywhere?

And not to be the person that harps on capitalization, but why @SQLSelect and @SQLWHERE? It’s next-level inconsistency.

My only hypothesis is that this code was originally in ASP or something similar, and someone said, “Performance is bad, we should turn it into a stored procedure,” and so someone did- without changing one iota about how the code was structured or worked.

Kris has this to say:

Just started at a new job–it’s going to be interesting…

Interesting is certainly one word for it.

Source: Read More