I used in clause but give type conversion error

Question

I used a dynamic sql query and prevent sql injection i want used ( @clientIds ) instead of ('+@clientIds+' )

GO
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
--exec test '1,2'
Alter PROCEDURE [dbo].[test]
    @clientIds varchar(Max)
AS
BEGIN
    DECLARE @sSQL  nvarchar(max);
    DECLARE @params NVARCHAR(MAX);
    Select @sSQL=N'select * from tblClient WITH (NOLOCK) Where clientID in ( @clientIds )'
    SET @params = N'@clientIds NVARCHAR(50)';
    EXECUTE sp_executesql @sSQL, @params, @clientIds;
END

`NOLOCK` means "read dirty data while taking excessive locks". It won't fix any problems you may have. In this case, probably because `ClientID` isn't covered by an index — Panagiotis Kanavos, Jun 11 '19 at 12:24
I won't use a dynamic SQL since I can. Why you need so? Why not just pass a table valued parameter? — Ilyes, Jun 11 '19 at 12:27
Which SQL Server version are you targeting? You can't pass multiple values in a parameter in any case, nor use `IN @clientIds` and have it treated as a list of values. You can pass a table-valued parameter and join with it. SQL Server 2016 and later has `STRING_SPLIT` which allows you to split the values and join with the results — Panagiotis Kanavos, Jun 11 '19 at 12:27
@AshishMaurya no there isn't. It's the *exact* opposite. It's your current code that's vulnerable to SQL Injection. In fact, that's what caused the error in the first place — Panagiotis Kanavos, Jun 11 '19 at 12:28
@AshishMaurya in that case the only way to avoid SQL injection is to use a table-valued parameter, or use the XML-based string splitting technique to convert the list of IDs to a result set of IDs. — Panagiotis Kanavos, Jun 11 '19 at 12:31
A better idea is to *NOT* use a stored procedure and use an ORM like EF or Dapper create a safe query. There would be no SQL injection risk if the list of IDs was created from a `List` on the client — Panagiotis Kanavos, Jun 11 '19 at 12:33
if run like this exec test '1,2);drop table tblclient--' then it delete table so iwant used like (@cliendid) not ('+@clientid+') — Ashish Maurya, Jun 11 '19 at 12:37

Ilyes · Answer 1 · 2019-06-11T12:43:55.280

0

Your SP can be like

Alter PROCEDURE [dbo].[test]
    @clientIds varchar(Max)
AS
    SET NOCOUNT ON;

    DECLARE @SQL  NVARCH(max) = N'SELECT * FROM [tblClient] WHERE [clientID] IN(' + @clientIds + N');';

    EXECUTE sp_executesql @SQL;

But, that won't prevent SQL Injection as it may be in the parameter '1,2;DELETE * FROM tblClient;.

I would use a Table-Valued Parameter instead as

CREATE TYPE MyType AS TABLE (Id INT);

Alter PROCEDURE [dbo].[test]
  @IDs MyType READONLY
AS
    SET NOCOUNT ON;

    SELECT *
    FROM [tblClient] 
    WHERE [clientID] IN(SELECT Id FROM @IDs);

OR

Alter PROCEDURE [dbo].[test]
  @IDs MyType READONLY
AS
    SET NOCOUNT ON;

    SELECT C.*
    FROM [tblClient] C INNER JOIN @IDs Ids
    ON C.clientID = Ids.Id;

Also, check out my answer here for a situation like yours.

edited Jun 11 '19 at 12:43

answered Jun 11 '19 at 12:30

Ilyes

14,640
4
29
55

That's what the OP wants to avoid as it's wide open to SQL injection – Panagiotis Kanavos Jun 11 '19 at 12:31
@PanagiotisKanavos Under construction :) – Ilyes Jun 11 '19 at 12:32
the OP edited the question multiple times. `IN(' + @clientIds + N')` was the original query – Panagiotis Kanavos Jun 11 '19 at 12:33

I used in clause but give type conversion error

1 Answers1