Am I protect against SQL Injection?


I have read several articles on MSDN, here at Experts-Exchange, etc. to better understand SQL injection and prevention standards.  Below is a result of this research.  However, I would appreciate it if someone could confirmed that this is the path I need to take.

Thank you in advance.
CREATE PROCEDURE [dbo].[sp_addNavElement]
	@navID varchar(30), 
	@navURL varchar(30)
DECLARE @cmd nvarchar(max)
DECLARE @parameters nvarchar(max)
SET @cmd = N'INSERT INTO navSystemA (navID, navURL, navOrder) Values (@navID, @navURL, (select count(*) from navSystemA)+ 1 )'
SET @parameters ='@navID varchar(30), @navURL varchar(30)'
EXEC sp_executesql @cmd, @parameters, @navID, @navURL

Open in new window

Who is Participating?
dportasConnect With a Mentor Commented:
CREATE PROCEDURE [dbo].[prc_addNavElement]
      @navID VARCHAR(30),
      @navURL VARCHAR(30)
      INSERT INTO navSystemA (navID, navURL, navOrder)
      VALUES (@navID, @navURL,(SELECT COUNT(*) FROM navSystemA)+ 1 );
Éric MoreauSenior .Net ConsultantCommented:
it would be a lot safer to use a Stored Procedure instead of a dynamic query. But since you are limiting the lenght to 30 characters, not much can be done.
trumpmanAuthor Commented:

Perhaps I am missing something?  This is a Stored Procedure.
Free Tool: IP Lookup

Get more info about an IP address or domain name, such as organization, abuse contacts and geolocation.

One of a set of tools we are providing to everyone as a way of saying thank you for being a part of the community.

Éric MoreauSenior .Net ConsultantCommented:
No your not missing anything. I was! As long as you are calling this SP using Command objects, you should not have problems.
This is a poor example because the dynamic SQL is completely unnecessary - it would be better just to put the INSERT in the proc without EXEC or sp_executesql. Either way, it isn't vulnerable to SQL injection - it's just inefficient and poor practice.

PS. Never use the "sp_" prefix for user procs. sp_ is reserved for system procs and will adversely affect your code.
trumpmanAuthor Commented:

Could you elaborate on your suggested method?  .."better just to put the INSERT in the proc without EXEC or sp_executesql"...

Thank you.  How would the desired result look?

- Trumpman
devshbConnect With a Mentor Commented:
In theory I think you're still open to xss attacks, so by using the logic you've got I think you're pretty much covered against direct injection, but people could still use script tags etc, so you might also want to clean the value of any < or > characters etc during the insert, although it depends on the nature of the data.

See also the "resources" section on the site:
which explains the difference between xss attacks and direct injection
(and that site also has a free data scanner)
Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.

All Courses

From novice to tech pro — start learning today.